Re: [Nano-devel] Softwrap navigation overhaul

[Benno Schulenberg]

On Fri, Feb 3, 2017, at 16:24, David Ramsey wrote:
> Version 2b is attached.  It's now 34 patches on the softwrap side [...]

Okay.  0001:

+           /* If we're not on the first line of the file, we may be able to
+            * take a shortcut, or not. */
+ [...]

Please remove the optimization that "steps" multiple chunks at a time.
Just step one chunk at a time.  Things will become straightforward and
won't need any comments at all.

+       for (i = nrows; i > 0; i--) {
+           /* If we're at the top of the file, get out. */
+           if (*line == openfile->fileage)
+               break;
+
+           /* Move back to the previous line. */
+           *line = (*line)->prev;
+       }

The above I would write as:

+       for (i = nrows; i > 0  && (*line)->prev != NULL); i--)
+           *line = (*line)->prev;

It needs no comments.

0002:

+           /* Try to move forward (editwinrows / 2) softwrapped chunks from
+            * current[current_x].  If we can't, it's because we've hit the
+            * bottom of the file, in which case we're close to the tail of the
+            * file. */
+           int rows_left = go_forward_chunks(&leftedge, &line,
+                                               editwinrows / 2);

It doesn't actually try to move forward; it just checks if there
would be remaning lines /if/ we tried to move forward.  The comment
should bring that across.  And instead of saying "(editwinrows / 2)
softwrapped chunks" it should just say something like "half a screen".

I would elide 'at_tail'.  I would just use 'rows_from_tail != 0' to
mean: do stationary scroll.

0003:

+       filestruct *line = fsfromline(was_lineno)

In a large file, that is a somewhat costly operation.  Instead
moving forward from the old line, why not move backward
from the current one?

0004:

+           /* If we can, we're on current, and we're before the softwrapped
+            * chunk that current_x is on, current[current_x] is below the
+            * screen. */

"If we can, we're on current".  I read this as: "If we can, then we're on
current".  But what you mean is "If we can, /and/ we're on current...".
Anyway, comments are far too verbose.  And instead twice 'return TRUE'
I would write:

+           if (line->lineno < openfile->current->lineno ||
+                       (line->lineno == openfile->current->lineno &&
+                       leftedge < (xplustabs() / editwincols) * editwincols))

Enough for today.

Benno

-- 
http://www.fastmail.com - Send your email first class