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

[David Ramsey]

I'll respond to the rest of this in detail soon, but first, (mostly) the
big issues.

On Wed, Mar 8, 2017 at 2:12 PM, Benno Schulenberg
<address@hidden> wrote:

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

Well... edittop only really means the top of the edit window when not in
softwrap mode, and not when the line there is partially offscreen.
Although renaming it is outside the scope of this overhaul.

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

No problem.

It's not that I don't want comments about the style, it's that I
sometimes can't figure out what coding style you're looking for, and we
seem to have very different styles.  (Given our discussions here, it
seems we also think in very different ways as well.)

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

If we're at the top or bottom of the screen, we don't move and just
return.  If was_column is initialized to xplustabs(), we just scanned
the entire line for nothing.  Considering we do enough line scanning
just to determine which softwrapped chunk we're in, I figured a bit of
optimization was in order.  Besides, it might become a bottleneck if the
user is holding the Up or Down arrow.

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

I was doing what you said here:

http://lists.gnu.org/archive/html/nano-devel/2017-01/msg00040.html

"O gott, another expansion...  Why not split update_line() into two
functions, which it basically already is, and let the one for
non-softwrap call the other one with index 0 (instead current_x) when
softwrap is on?  All these #ifndef NANO_TINY and if (ISSET(SOFTWRAP))
piss me off."

Or are you referring to some other duplicated bit there?

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

What do you want me to do instead?  Because the status quo won't work if
you want the problems with two-column Unicode fixed.

"(xpt / cols) * cols" and the like only work as long as softwrapped
lines are of constant length.  Breaking the line early to avoid problems
with two-column Unicode, or with whitespace as the new feature,
necessarily mean that the lines are no longer of constant length.  This
means that the formula to determine where a chunk begins will be much
more complex.  With the softwrap breakpoint finder based on
break_line(), this becomes complex enough that it needs to be in a
function, since duplicating the loop calling that function all over the
code would worsen things even more than this does.

Don't get me wrong, I don't like it much either, but I can't think of an
alternative at the moment.

> 0035-0040: This is an additional feature, so it should be a separate
> patch set, not part of the overhaul.

That's what I had to add after you mentioned the breakage with
two-column Unicode; I figured fixing breakage wasn't a new feature.

Now as for breaking on whitespace instead of the screen... that is a new
feature, and I can split it off, but it will of course still be on top
of this.

> (Also, it should not be called softwrapatblanks, because this shares
> too many letters with softwrap.)

Okay, but I don't like wrapatblanks because it makes me think of hard
wrapping.  Breaking on whitespace is something word processors do; maybe
use a name related to that?