Re: [Nano-devel] indent/unindent overhaul (including undo/redo support)

[Benno Schulenberg]

Op 10-07-2017 om 20:05 schreef David Ramsey:
> One last update: version 1c.  Relative to the last version, tab creation
> is now simpler, a few comments are improved, and it's resynced against
> current git (78bc8b6).

Okay.  First partial review.

0001:

The changes are massive.  I have no idea whether the code still does the
same as before.  :|

-       /* Throw away the undo stack, to prevent making mistakes when
-        * the user tries to undo something in the reindented text. */
+    /* Throw away the undo stack, to prevent making mistakes when the user
+     * tries to undo something in the reindented text. */

Please don't rewrap lines when you don't actually change the text.
Resist, resist, resist making changes that are unnecessary.

0002:

    text: make indenting add to the beginning of the line

    This is simpler than adding to the end of the line's whitespace.

It may be simpler, but that is not the reason for the change.  It
should talk about consistency, and should refer to a bug on Savannah.

0003:

Same thing comment as above.

+       if (tab_indent_length(f->data) == 0) {
+           statusline(HUSH, _("Cannot unindent lines without full indents"));

This message should only be given when /all/ of the marked lines begin
with whitespace and of at least one of the lines the whitespace is less
than a tab's worth.  If there is just one line without leading whitespace,
nano should be silent.

And a better message would be:
"Can only unindent by a full tab size".

+size_t tab_indent_length(const char *line)
+{
+    size_t indent_len = 0, indent_cols = 0;
+
+    while (*line != '\0') {
+       if (*line == '\t' || *line == ' ') {
+           indent_len++;
+
+           if (*line == '\t')
+               indent_cols += tabsize - (indent_cols % tabsize);
+           else
+               indent_cols++;
+
+           if (indent_cols == tabsize)
+               return indent_len;
+
+           line++;
+       } else
+           break;
+    }
+
+    return 0;

This is way more complicated than needed: indent_cols is superfluous,
because as soon as you see a tab, you can return.  I would write it
like this:

    size_t bytes_of_white = 1;

    while (TRUE) {
       if (*text == '\t')
           return bytes_of_white;

       if (*text != ' ')
           return 0;

       if (bytes_of_white == tabsize)
           return tabsize;

       bytes_of_white++
       text++;
    }

0004:

+/* Dynamically allocate space for a tab at the given column.  Return the
+ * character in tab, as well as its length.  [...] */
+size_t create_tab(char **indent, size_t column, bool spaces)

Comment does not seem to match the parameters.  Also, a distinction is needed
between returning something as the value of the function, and passing back
something in one of the parameters.

The commit message says that it reduces code duplication, but it increases
the line count by fifteen...  The create_tab() function should get passed
a value of TRUE or FALSe, it should itself test the TABS_TO_SPACES flag.
That it gets passed xplustabs() for nothing when that flag is FALSE...
it hardly matters: this typing speed, not an operation on hundreds of lines.

0005:

    and remove the ENABLE_COMMENT #ifdefs for both
    it and related code in discard_until().

This retells the diff in words.  Unneeded.

    Furthermore, extend the undo group structure to contain an array of
    strings, one for each line in the group.

Why is this needed?  Why can't the strdata of the undo item itself be
used?  Anyway, I think it's an unneeded complication for now.  I think
it's enough when undo restores the /visual/ indentation, it doesn't need
to restore the actual whitespace characters that were there.  This has
the advantage that by unindenting a piece of text and then undoing it,
one can normalize the whitespace (to use all spaces, or all tabs, for
the full indents).  So... I say: remove the meticulous restoration.

(And if we later decide that we do want it, it should be a separate
patch.)


About the function of the patch set in general: from a quick test, it
works.  It is nice to be able to undo!  :)  But...  When a line is empty
(that is: it has zero length), it should stay empty: if it is selected
during an indent operation, it should not get any whitespace added to it.
A similar thing for unindenting: when an empty line is selected, this
should not prevent the unindenting of the other selected lines.  And
also a selected line that consist /only/ of whitespace should not prevent
unindenting of the other lines.

Benno