Re: [Nano-devel] Softwrap navigation overhaul

[David Ramsey]

Replying to both pieces...

On Sat, Jan 28, 2017 at 6:12 AM, Benno Schulenberg
<address@hidden> wrote:
> 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.

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.

I figured if someone was trying to trace what happened to particular
functions, revert was the way to go; I guess not.

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

When implementing it, it was easier to remove the ad hoc bits that only
applied to the current softwrap implementation, and then implenet the
new bits with the ad hoc bits removed.

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

So when it comes to stuff like do_down(), instead of removing code and
then adding new code, you'd prefer to combine those into one code
replacement?  Because the two sets of code definitely conflict in that
case.

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

Okay,

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

It may be partially the effects of having to process such a long line.
(I tried it in KWrite for comparison, and that refuses to softwrap
anything past 1024 characters.)

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

I know why: they're using edit_redraw(), so they're using flowing
scrolling mode.  They should be using edit_refresh() (or, more
specifically, refresh_needed = TRUE; focusing = FALSE).  I'll fix that.

----------

On Sun, Jan 29, 2017 at 9:06 AM, Benno Schulenberg
<address@hidden> wrote:
> 01: I think those replacements should come after you have started
> using go_back() and go_forward(), after you have begun to transform
> the behavior.  This goes for most of the early patches.

Okay.

> 11: Why unwrap the assert?  And why this:
>
> -    /* Move to the next line in the file. */
> +    /* Move the current line of the edit window down. */
>
> We're scrolling down, which means that all the text moves up.
> The comment now seems to say the opposite.

The comment in the same place in do_up() (move.c, line 418) says "Move
the current line of the edit window up.", and the unwrapped assert is
because the similar assert in the same place in do_down() (move.c, line
416) is unwrapped, despite being overly long.  Why are those things okay
in do_down(), but not here, since the functions basically mirror each
other?  Or am I missing something?

> 19, 20: Don't call them reverts.  Because it makes it sound as
> if those changes were wrong.  But you're changing the whole
> way that softwrap behaves, so you're not reverting anything,
> you are changing the entire logic.

Since those were ad hoc fixes that papered over the problems in bugs
they referenced, I thought those *were* wrong in a sense.  But I suppose
that's more philosophical.  If you just want them removed, fine.

> First, better use the plural: go_back_chunks() and
> go_forward_chunks(), because mostly they get called with a number
> greater than one.  Also, better put the nrows argument first, so that
> it is clearer what "chunks" refers to -- in the descriptions of the
> functions, nrows comes first too.

Okay,

> Why is that if within the for loop?  Put it outside and drop the
> i==nrows.

I was trying to avoid calling strnlenpt() unless absolutely necessary
(and if i is zero or less, it won't be), since it's an expensive
operation.

> (Yes, that will include one superfluous zero test in the tiny version.
> Doesn't matter.  And if the compiler is smart, it will see that
> current_chunk is set to zero at the start and is never assigned to
> again, so it will drop that test.)

Okay.

> In the rest of the function, the comments are excessive.  They are so
> long that I can't be bothered to read them.

I'll pare down their wording.

> Also, I hate the style where the first thing after an if is a comment.
> Do the whole explaining before the if.

Okay.

> Also, all this #ifndef NANO_TINY and if ISSET(SOFTWRAP) is making
> things into a jumble.  Better split each function into two blocks: one
> for the non-softwrap case, and one for the softwrapping one.  So there
> will be just one check for isset softwrap and just one #ifndef per
> function.  Don't bother #ifdeffing the variable.

Okay.  Do I do this even if code the two paths have in common gets
duplicated between #ifdefs?

> Just use rows_from_tail straightaway.

I figured there was no way to know how many lines there were between
filebot and current, and if the file were huge, it could be out of int
range.  But, okay.

> 26: Now you've lost me.  maxchunk?  Max chunk at the bottom of the
> edit window should always be editwinrows.... Ah, maxchunk refers to a
> /line/, not a chunk -- to the line in which the chunk on the bottom
> row falls.  line-on-bottomrow, bottomchunk_x.
>
> Further, the only time that this maxchunk is used is in
> current_is_below_screen().  So, instead of computing this maxchunk in
> three different places, just compute it when it's needed.  The call of
> current_is_below_screen() in do_right()... it should be possible to do
> that in a cheaper way.

I actually was waffling over how to do that, and went with maxchunk
because I figured if you objected to maxlines-like logic, you would have
removed it yourself already.  I guess not.  I did have an alternate
solution in mind that wouldn't require maxchunk or related bits at all;
I'll go with that in the updated version.

> 31 - 34: ...  Instead of expanding bunches of lines, why not rename
> and redefine need_horizontal_scroll() so that it gives the proper
> answer both in softwrap and non-softwrap mode?

In other words, if we're in softwrap mode, don't call get_page_start()
at all, but still check if the mark is set?  Okay.

However, this leads to one more potential comment fix: get_page_start()
refers to nano's scrolling "horizontally within a line in chunks", but
obviously not in the same kind of chunks as in softwrap mode.  What
terminology would you use for that instead?

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

Would you object to the softwrap-specific version's still being called
update_softwrapped_line(), or not?

> Superfluous variable: line.  And line_x isn't an x, it's a column
> position -- it would be better called leftedge or something.  (Yes,
> mouse_x should have been called mouse_col too.)

This is getting muddled.  It's line_x because it's the starting
x-coordinate of the current chunk; if it were the column position of
that chunk, then the overhauled do_up() and do_down() wouldn't have to
call xplustabs() on its value to get the column position when adjusting
for placewewant.  In other words, current[line_x] should point to the
character at line_x, which it *does* in the overhaul.

> 43, 44: ...  Now we have /three/ functions taking up 150 lines just to
> move the cursor home.  When all that is needed is:
>
>     get indent;
>     if (x==0)
>         x = indent;
>     else if (softwrap) {
>         get leftedge_x;
>         if (x==leftedge_x)
>             x = 0;
>         else
>             x = leftedge_x;
>     } else
>         x = 0;
>
> And then the first and third if need a 'clever &&' added. What am I
> missing?

The fact that it was getting too complex when I had all modes in one
function, so I split it up, and tried to be clever enough to filter out
the more sophisticated modes when necessary.  It probably could be
simpler, but...

Your pseudocode version isn't quite right.  It doesn't properly
duplicate the smart home behavior in non-softwrap mode, before or after
the overhaul, and it doesn't account for smart home in softwrap mode
when the indent is long enough to take up more than one chunk.  In
separate bits of pseudocode, they should be as follows:

Non-softwrap-mode home:

     x = 0;

Softwrap-mode home:

     if (softwrap) {
         get leftedge_x;
         if (x==leftedge_x)
             x = 0;
         else
             x = leftedge_x;
     }

Non-softwrap-mode smart home:

     get indent;
     if (x==indent)
         x = 0;
     else
         x = indent;

Softwrap-mode smart home:

     get indent;
     if (softwrap) {
         get leftedge_x;
         if (leftedge_x < indent)
             x = indent;
         else
             x = leftedge_x;
     }

Merging all these cases into one function is the tricky part.

> Why not:
>
> +    if ((xplustabs() % editwincols) == (editwincols - 1))
>
> ?

I think that should work, yes.