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

[Benno Schulenberg]

On Sun, May 7, 2017, at 23:12, David Ramsey wrote:
> Agreed.  At the moment, this is more of a proof of concept to verify
> that it can work.  It can certainly be improved performance-wise.

It badly needs to be improved performance-wise.

0001:

+size_t get_softwrap_breakpoint(const char *text, size_t leftedge,
+                               bool *end_of_line)
...
+    while (*text != '\0' && column < leftedge)
+       text += parse_mbchar(text, NULL, &column);

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

+    size_t index = 0;
+       /* Current index in text. */
...
+       index += char_len;

Seems to be an unused variable.

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

0002:
"""
Instead of repeating formulas all over the place, use the new functions
get_chunk_row(), get_chunk_leftedge(), get_last_chunk_row(), and
get_last_chunk_leftedge().

We're about to change the way chunks are calculated, so that they're no
longer of constant length, so prepare for it.
"""

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.

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?

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

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

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.

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.

+#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));

0006:

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

+       if (break_col - leftedge < editwincols)
+           waddch(edit, '>');

Why does spotlighting need to redraw the '>' marker? 

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.

Benno

-- 
http://www.fastmail.com - IMAP accessible web-mail