Re: [Nano-devel] Softwrap navigation overhaul

[Benno Schulenberg]

On Wed, Jan 25, 2017, at 20:13, David Ramsey wrote:
> The attached patch set against current git, commit 4ed3591 (in a zip
> file, because it's rather massive: 66 patches and over 300K),

Okay.  One by one.

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.

02: Discard.  I don't do blank lines after #endifs.

03-07: Should be a single patch.

08, 10: Single patch.

09: Rake with other things into a separate cosmetic patch.

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.

Only the last part of that patch is okay.

12, 13, 17: Single patch.

14: Discard.  Please stop folding comments at 74 columns.
A normal terminal has 80 columns.  Nano's code uses all
of them.  So no:

-    /* Don't bother scrolling zero lines, nor more than the window can hold. */
+    /* Don't bother scrolling zero lines, nor more than the window can
+     * hold. */

15, 16: Fine.

18: Rake together with 09.

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.

21:  Okay... finally it begins.

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.

Now...

+    for (i = nrows; i > 0; i--) {
+#ifndef NANO_TINY
+       /* We're starting.  If we're in softwrap mode, set current_chunk
+        * to the number of the current softwrapped chunk in the current
+        * actual line. */
+       if (ISSET(SOFTWRAP) && i == nrows)
+           current_chunk = (strnlenpt((*line)->data, *line_x) / editwincols);
+#endif

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

+       if (*line == openfile->fileage) {
+#ifndef NANO_TINY
+           if (!ISSET(SOFTWRAP) || (ISSET(SOFTWRAP) && current_chunk == 0))
+#endif
+               break;
+       }

If not isset or isset...?  This is equivalent to:

            if (!ISSET(SOFTWRAP) || current_chunk == 0)

But, since current_chunk will always be zero when not in softwrap
mode, it is equivalent to just if (current_chunk == 0).  And if
you then drop the cluttering #ifdef, it becomes simply:

        if (*line == openfile->fileage && current_chunk == 0)
                break;

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

In the rest of the function, the comments are excessive.  They
are so long that I can't be bothered to read them.  Also, I hate
the style where the first thing after an if is a comment.  Do the
whole explaining before the if.

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 other
#ifdeffing the variable.

22:  Superfluous variable:

+           ssize_t row_count = openfile->filebot->lineno -
+                                       openfile->current->lineno;
+
+           if (row_count < editwinrows / 2) {
+               rows_from_tail = row_count;
+               at_tail = TRUE;
+           }

Just use rows_from_tail straightaway.

23:  Use an && instead of a double if:

+       else if (line->lineno == openfile->current->lineno) {
+           if (line_x > (xplustabs() / editwincols) * editwincols)
+               focusing = FALSE;

And don't use the word "uncut" in comments.  Use "paste".

24: Discard.

25: Patch 01 should come right before this one, or be fused
with it.

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.

29: Discard.

30: Fine.  But not in your patch set.  Separate.

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?

35:  Hrrrm.  No.  I hate that style.  If it firts on one line, it
is acceptable.  But not if it needs line breaks.

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.

37: Okay.  But separate.  Has nothing to do with overhaul.

41: Just say "on the row where the cursor is":

-           /* Whether the click was on the line where the cursor is. */
+           /* Whether the click was on the line (or softwrapped chunk)
+            * where the cursor is. *

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

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?

46:  Instead of:

+    if (openfile->current_x == actual_x(openfile->current->data,
+                               (xplustabs() / editwincols) * editwincols) +
+                               (editwincols - 1))

Why not:

+    if ((xplustabs() % editwincols) == (editwincols - 1))

?

Okay, I'm tired of this.  I will do another review when you
submit a new installment.

Benno

-- 
http://www.fastmail.com - Accessible with your email software
                          or over the web