Re: [Nano-devel] Softwrap navigation overhaul

[David Ramsey]

Replying to all.  The new version of the overhaul, version 3, is
attached.

Benno Schulenberg:
> I've applied and pushed several of the cosmetic patches, fused some
> of them together.

Thank you.  I've put the patch set back in sync with current git.

> <Up> and <Down> and ^V and ^Y stop working.  And holding down <Right>
> just runs from left to right over the same line again and again.  A
> <Ctrl+Right> does move to the next line.
>
> (Of course it is silly to use a terminal consisting of just one line,
> but current nano functions fine in that situation.  It should continue
> to do so.  And... it is a good test that most of the logic is correct.)

The problem is in the chunk iterators; when they cap the number of lines
to scroll at (editwinrows - 1), they don't account for editwinrows'
being 1.  The loop then goes zero times, so nothing moves.  Fixed now.

(Incidentally, I usually have constant cursor position display turned
on, and when there's only one row in the terminal, it covers all the
text, so I had to turn it off temporarily to fix this.  Should constant
cursor position display be automatically disabled on such small
terminals?)

----------

> 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.

Done.

> The above I would write as:

<snip>

> It needs no comments.

Done, for both back and forward movement.

> 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".

Attempted to fix the comment.

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

Elided.

As for the other change, the condition is now rows_from_tail != -1 (with
its being initialized to -1), since rows_from_tail == 0 means we're
exactly on the tail and should still move specially, just as we do in
non-softwrap mode.  The only way row counting would get rows_from_tail
to -1 would be if current was one line below filebot, which shouldn't
happen.

> 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?

Done.

> "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:

Attempted to fix the comments, and done.

----------

> You swapped the line and leftedge parameters?  Why?  What I wished
> to see was nrows as first parameter, as that is what "chunks" is
> synonymous with.

Misread the original email.  Fixed now.

> Optimize for the most common case: non-softwrap, not column zero:

Done.

> Applied and pushed.  They are independent from the overhaul.

Thank you.

softwrap-navigation-overhaul-3.zip
Description: Zip archive