Re: [Nano-devel] softwrap navigation overhaul, now breaking on whitespace

[Benno Schulenberg]

On Wed, Mar 8, 2017, at 00:55, David Ramsey wrote:
> On Tue, Mar 7, 2017 at 10:48 AM, David Ramsey <address@hidden>
> wrote:
> > All the changes are in the new version 4c.  Attached.
> 
> Make that version 4d, attached.

0001:
"""These functions take a number of softwrapped chunks to move, a
filestruct, and a location (specifically, a starting column of a
softwrapped chunk).  If they move successfully, they will update the
location and filestruct to point to the beginning of the softwrapped
chunk they moved to.  They won't go past the beginning or end of the
filestruct, and their movement is limited to one screenful of chunks at
most."""

Well, they don't take a filestruct itself, but a pointer to it -- or rather,
a pointer to a linestruct, to start to use the better name.  In the second
sentence you then swap location and linestruct around, which confuses me,
plus: the linestruct itself is not changed at all but the pointer to it.
And then in the third sentence you refer to a filestruct when you mean
buffer.  But it is obvious that these functions will not scroll beyond
the beginning or end of the buffer, so that doesn't need mentioning.
Why movement needs to be limited to a screenful at most is a mystery --
when would these functions be called with a number of rows larger than
the screen?

0003 and 0004:  These two patches contain a nearly indentical piece
of code.  This should probably made into a separate function, called
throttle_the_scrolling(), called with was_lineno and was_leftedge or
zero as parameters.

0005:
+           if (line->lineno < openfile->current->lineno ||
+                       (line->lineno == openfile->current->lineno &&
+                       leftedge < (xplustabs() / editwincols) * editwincols))
+               return TRUE;

It could simply be:

       return (line->lineno < openfile->current->lineno || ....

Even the preceding if could be included in the return statement,
but then things probably become a bit too dense.  But the suggested
return better matches the return in non-softwrap case.

0009:
"""...when we can scroll edittop partially off the screen,"

This rancles with me.  Edittop to me means: the top of the edit
window.  But you use it here in the sense of: the line that edittop
points to.

0010:
+    if (ISSET(SOFTWRAP))
+       edittop_leftedge = (xplustabs() / editwincols) * editwincols;
+    else
 #endif
-    }
+       edittop_leftedge = 0;

The else part should not be necessary, because the variable gets
initialized to zero.

0011:
 #ifndef NANO_TINY
        size_t current_x_save = openfile->current_x;
 #endif
+       ssize_t row_count = mouse_row - openfile->current_y;
+       size_t leftedge;
+
+#ifndef NANO_TINY
+       if (ISSET(SOFTWRAP))
+           leftedge = (xplustabs() / editwincols) * editwincols;
+       else
+#endif
+           leftedge = get_page_start(xplustabs());
 
This can be done more economically by declaring row_count and
leftedge before current_x_save, so you don't need an extra #ifndef.

0013:
+    /* Simple home for non-softwrapped lines. */
+    if (!moved)
 #endif
+    {
        openfile->current_x = 0;
+       moved_off_chunk = FALSE;

If the comment is true, then it should not be necessary to set
moved_off_chunk, it's value should be irrelevant in that case.

0014:
     size_t was_column = xplustabs();
+    size_t line_len = strlen(openfile->current->data);
+
+    bool moved_off_chunk = ISSET(SOFTWRAP);
+    filestruct *was_current = openfile->current;
+
+#ifndef NANO_TINY
+    bool moved = FALSE;
+
+    size_t rightedge_x = 0;

All declarations should be in a single block.  Yes, I know,
you don't want comments about the style, but this gets angering.

+    if (ISSET(SOFTWRAP)) {
+       moved = TRUE;
+[...]
+    }
+
+    /* Simple end for non-softwrapped lines. */
+    if (!moved)
+#endif

You don't need 'moved' and 'if (!moved)' at all.  You just need
an else after that closing brace.

0016:
-/* Move left one character. */
+/* Move left one character (or softwrapped chunk). */
 void do_left(void)

The comment is wrong.  do_left() always just moves one character left
-- if it happens to change line or chunk, that is just a side effect.

+    if (ISSET(SOFTWRAP)) {
+       /* If we were on the first line of the edit window, and we only moved
+        * one chunk, we're now above the first line of the edit window, so
+        * scroll the edit window up. */
+       if (openfile->current_y == 0 && openfile->current == was_current) {
+           size_t old_chunk = (was_column / editwincols);
+
+           if ((openfile->placewewant / editwincols) != old_chunk) {
+               edit_scroll(UPWARD, ISSET(SMOOTH_SCROLL) ?
+                               1 : editwinrows / 2 + 1);
+               return;
+           }
+       }
+    }

Those three ifs could be a single one.  And the "we only moved
one chunk" part is incorrect.  It should say: if we changed chunk.

0017:
+#ifndef NANO_TINY
+    if (ISSET(SOFTWRAP)) {
+       leftedge = (xplustabs() / editwincols) * editwincols;
+       leftedge_placewewant = openfile->placewewant % editwincols;
+    } else
+#endif
+    {
+       leftedge = 0;
+       leftedge_placewewant = openfile->placewewant;
+    }
+
+    was_column = xplustabs()

Better initialize leftedge and leftedge_placewewant when they
are declared.  It saves a four-line else.  And rename the latter
variable to target_column or something like that, because all
this repetitious leftedge leftedge placewewant placewewant
gets on my nerves.

And why not initiliaze was_column when it's initialized?  It was
that way -- why change it?

0018:
+    /* If we're inside a softwrapped chunk, stay inside it. */
     openfile->current_x = actual_x(openfile->current->data,
-                                       openfile->placewewant);
+                                       leftedge + leftedge_placewewant);

Why the comment?  There is no if there.

0022...and before.  Please rename edittop_leftedge to firstcolumn
or something like it.

0023: Same comment as for 0005.

0024:
+       if (openfile->edittop_leftedge > 0)
+           row -= (openfile->edittop_leftedge / editwincols);

The if isn't really needed.  Saves on the comment too.

0025: Why?  This duplicates a chunk of code.  What does it gain us?

0028:
"""In uncut_text(), if edittop was partially scrolled off the screen before
we cut text and then uncut it again, cutting it just updated the
viewport and unscrolled edittop, and uncutting it just put all the
original text back without scrolling edittop again.  In order to keep
edittop in the same position as it was, we have to save and restore not
just edittop, but edittop_leftedge as well."""

"...unscrolled edittop..."?  This whole paragraph perplexes me:
we cut text, then uncut it, cutting it, uncutting it...  I can't follow
it.  It first needs to say that copying text (M-6) involves: cutting it
and then pasting it back.  Then it needs to say that cutting it might
change edittop+firstcolumn (when the mark is offscreen), so, to be
able to keep the screen invariant, both edittop and firstcolumn need
to be remembered and restored.

0029: Same comment.  Also, please drop the #ifndefs.  That is just
clutter.  I don't care about keeping tiny as tiny as possible.

0030, 0031:  Same comments.

0033: No!  What "(xpt / cols) * cols" does, I can quickly understand,
what get_chunk_leftedge(..., ...) does is entirely cryptic.  Now, if
it would shorten each statement and reduce the total number of lines,
it might still be acceptable, but it worsens both.


0034: No.

0035-0040: This is an additional feature, so it should be a separate
patch set, not part of the overhaul.  (Also, it should not be called
softwrapatblanks, because this shares too many letters with softwrap.)

Benno

-- 
http://www.fastmail.com - A fast, anti-spam email service.