Re: [Nano-devel] Softwrap navigation overhaul

[Benno Schulenberg]

On Wed, Jan 25, 2017, at 20:13, David Ramsey wrote:
> I've finally implemented the changes to how nano navigates softwrapped
> lines, described in the thread starting here:
> 
> http://lists.gnu.org/archive/html/nano-devel/2016-12/msg00046.html
> 
> The attached patch set against current git, commit 4ed3591 (in a zip
> file, because it's rather massive: 66 patches and over 300K),

Ooofff...  Nearly seven hundred lines added.  :|

> There are a few cosmetic changes in there as well, but I've separated
> them out as best I can.

Better rake them together into a single patch and send it
to the list separately, get it applied before the overhaul.
Keep such things out of the patch set.

> Also, the changelog entries are not in the
> preferred style yet,

You mean the commit messages.  Okay.

> Finally,
> there are some reverts of full commits (for example,
> ensure_line_is_visible() and everything related to it is gone),

Why call them reverts?  Why do each of them seperately?
If you don't need the ensure...() calls any more, chuck
them all out in a single patch, including the function
itself.  It makes no sense to remove them one by one.

But I find the order in which you do things strange.  First
you remove tens of things, kind of removing half of the
softwrap functionality, and then you start adding back new
functions.  I would have expected it the other way around:
first add the new stuff you need, then transform the movement
functions to use the new stuff, and at the end scrap all the
then dead code.

> There are also no documentation updates, and there should be eventually

Yes.  That should be a separate patch (set).

> bug #47667: when softwrap is on, PageUp + PageDown is not immobile
> 
> (The overhaul fixes the problem properly by allowing edittop to be
> partially scrolled, thus making making single-line scrolling stay on the
> same line whenever possible.)

Yes, that appears to work well.  Nice.

> Bug #49100: with softwrap, a line that takes up more than one screenful
> causes problems
> 
> (The overhaul fixes the problem properly by allowing edittop to be
> partially scrolled, thus making making single-line scrolling stay on the
> same line whenever possible.)

It... works.  But nano becomes unusably slow (when syntax coloring is
active) when a softwrapped line takes up nearly the whole screen: it
takes two seconds to respond to <Up> or <Down> at the top or bottom
of the screen.

> Bug #49298: when there are softwrapped lines, M-- behaves strangely

Yes, that works well too, after the overhaul.

> Bug #49374: with softwrap, a <Right> at end-of-line sometimes scrolls
> too much

Works fine already since 2.7.1.  But true, those were all ad-hoc fixes.

> Bug #49384: with softwrap, holding down <Right> on a very long line is
> annoyingly slow

It's much better after the overhaul: the complete halting and stuttering
in the fourth or later chunk of a softwrapped line is gone.  There is
still a quarter of a second pause each time the cursor reaches the
bottom right corner of the screen (in smooth scrolling mode), but
that is... okay.

> It seems to work so far, but probably needs more testing.  If anyone
> feels like doing so, it'd be much appreciated.

I've found no failures, so yes, it seems to work well.

What I don't like is: when on the bottom row only the first chunk of
a softwrapped line is visible, and you put the cursor on the very end
of that row, pressing <Right> or <Ctrl+Right> or <Down> there, will
scroll just one chunk.  Pressing <End> instead will scroll half a
screen (all in smooth scrolling mode).  That isn't right.  It should
scroll just enough rows to get the cursor back onto the bottom row.
This is especially important when the chunks of the softwrapped line
take up more than half a screen: centering the cursor then after an
<End> would scroll the start of the line off the top of the screen.

The mirror thing goes when doing <Home> at the topleft of the screen
on a tail chunk of a softwrapped line.

In a later mail (maybe tomorrow) I will do a quick review of all the...
66 patches.

Benno

-- 
http://www.fastmail.com - Does exactly what it says on the tin