Re: [Nano-devel] Port of variable-length chunks to 2.8.2

[David Ramsey]

On Wed, May 10, 2017 at 3:28 PM, Benno Schulenberg
<address@hidden> wrote:
> It badly needs to be improved performance-wise.

I know that.  However, Mark Majeres' code was the only thing that did
so, and it didn't even work correctly in all cases, so if you have some
ideas to to how, please speak up, because I'm short on inspiration as to
where to cache these things.  (For example, the main() loop won't work
if we're in something like the replace loop.)  And considering that my
ability to guess the variable and/or function names you want is limited,
if you wouldn't mind giving me a hint as to what to call them so I don't
have to rewrite the code quite as often?

> Every time get_sw_breakpoint() is called, it iterates through the
> entire preceding text of the line in order to find the point again
> that corresponds to leftedge.  It would be much better for performance
> if it got passed not the start of the line, but simply the point that
> corresponds to leftedge.  And when you do that, you have no more need
> to pass leftedge.  (Of course, this will require several other
> routines to pass back the pointer in the text that corresponds to the
> relevant chunk.)

This is what I meant when I mentioned the discrepancies between columns
and indexes; I need both, but I'm not sure how to pass both effectively
without doing something drastic like adding a structure containing both,
and/or caching them both.  Again, a bit of feedback as to what to do
would be useful.

> +    size_t index = 0;
> +       /* Current index in text. */
> ...
> +       index += char_len;
>
> Seems to be an unused variable.

Oops.  When I split out the code that broke lines on whitespace into
patch 0007, apparently I forgot that variable (which the code in 0007
*does* use).

>
> +    if (end_of_line != NULL)
> +       *end_of_line = FALSE;
>
> I would simply set this variable to FALSE whenever it is declared,
> saving an 'if' on every call.

Even in the cases where end_of_line isn't used, which would mean
declaring a dummy boolean?

> The commit message needs to be improved.  It is not about avoiding
> repetition, it is about preparing for chunks of variable width  (width,
> not length; chunks can already be of different length).  So that is what
> the subject needs to say and the rest of the message has to elaborate.

Okay.

> 0003:
>
> -    size_t was_column = openfile->placewewant;
> +    size_t was_column = xplustabs();
>
> Another slowdown.  Is it really necessary?  I've put the placewewant
> back and can't observe any adverse effects.  At least for up and down.
> So... why is it needed?

Because placewewant is not always identical to xplustabs(), and because
I figured it was part of one of your ad hoc workarounds for two-column
characters on the edge of the screen, which I have to reverse as part of
these changes because they get in the way of the real fix to break the
line before such characters.

And confusing the two makes me nervous, as differences between the two
might cause subtle bugs.  You say you can't reproduce it with up and
down; what about when the +LINE,COLUMN option is used and the specified
coordinates are outside the line boundaries?

>
> +       if (to_col - from_col < editwincols)
> +           mvwaddch(edit, row - 1, to_col - from_col, '>');
>
> Why the "move"?  It should be enough to do simply:
>            waddch(edit, '>');
> no?

It did exactly that originally, but that caused the bug I mentioned
before: if the mark is on, the ">" would be drawn after the mark,
because the cursor would be placed after the mark in that case, instead
of at the breakpoint as indended.  Doing it this way guarantees that the
">" is always in the right place.

>
> -       size_t current_chunk = get_chunk_row(*line, *leftedge);
> -       size_t last_chunk = get_last_chunk_row(*line);
> +       size_t current_leftedge = get_chunk_leftedge(*line, *leftedge);
>
> Ehm...  "current_leftedge = leftedge"?
>
> I've inserted this:
>
>     if (current_leftedge != *leftedge)
>            statusline(ALERT, "Mismatching edges -- report a bug!");
>
> and haven't been able to trigger it.

Okay.  Moving it into functions before switching what the functions do
is tricky.

> 0004:
>
> I'm not sure I want clicking after the breakpoint to shift the cursor
> back to the last column of the line.  If the user clicks beyond the
> last column, it means they want the cusor /there/, and that means
> that the cursor will in effect appear at the start of the next row.

If the terminal is 80 columns wide, the line is softwrapped at column
74, and you click on column 78, your proposed solution will put the
cursor at column 4 of the next line, which is nowhere near where you
clicked the mouse, because of how actual_x() works.  Even if I change
that to move it to column 0 of the next row, neither approach seems
intuitive to me, and neither approach matches the behavior of any other
editor I've seen that does softwrapping; all other editors I've seen
shift back to the last column of the line.

> 0005:
>
> +       if (openfile->placewewant >= realspan)
> +           *target_column %= editwincols;
>
> placewewant /cannot/ be larger than realspan -- see the code six
> lines earlier.  And if it's equal, does target_column still need
> to be moduloed?  I've removed those two lines and haven't seen
> errant behavior.

Yes it /can/; the COLUMN portion of the +LINE,COLUMN option can specify
a placewewant past the length of the line (realspan), and a bug caused
by that is the reason you added the code for realspan in commit 244a503.
And such a placewewant also shows up as part of bug
#50995.  This code is to handle cases like that.

> +#ifndef NANO_TINY
> +    /* If we're in softwrap mode, make sure the cursor doesn't go beyond 
> where
> +     * the softwrapped line is broken. */
> +    if (ISSET(SOFTWRAP))
> +       target_column = keep_before_softwrap_breakpoint(leftedge,
> +                                                       target_column);
> +#endif
>
> Instead of inserting that piece of code four times, why not integrate
> it into keep_before_breakpoint(), call it actual_last_column(), and then
> instead of this:
>
>      openfile->current_x = actual_x(openfile->current->data,
> -                                       openfile->placewewant);
> +                                       leftedge + target_column
>
> do this:
>
>      openfile->current_x = actual_x(openfile->current->data,
>                                        actual_last_column(leftedge, 
> target_column));

Hmmm.  I'll look into it.

> 0006:
>
> Oof!  A whole new function especially for spotlighting words
> in softwrap mode?  Why?

For the same reason you wanted a separate one for update_line(): it ends
up being too different from the non-softwrap version.  See below.

> +       if (break_col - leftedge < editwincols)
> +           waddch(edit, '>');
>
> Why does spotlighting need to redraw the '>' marker?

If the text you're replacing is spread across more than one softwrapped
line (as it can be if you're dealing with an unbroken line that takes up
more than one row, and the match just so happens to straddle between
rows), the ">" needs to be drawn as part of the highlighted text, just
as it is on the screen originally.  Just drawing the word in the same
place, as the current code does, is no longer enough.

> Also, I can't understand what the routine is doing.  It seems
> to do a wattron and wattroff for every chunk in the line, but
> these don't seem to have any effect until the actual chunk is
> reached...?  I give up.

The wattron is used to highlight matches, the wattroff is used to *not*
highlight the ">" parts of the match, since the ">"'s are not actually
part of the word, and could lead to major confusion in, say, regular
expression search and replaces.