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

[David Ramsey]

Attached version 2 of the indent overhaul, synced with git e4a69f8.
Thanks for putting in the initial refactor.  Commentary on your last
review is as follows:

0002:

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

Done.

0003:

> Same thing comment as above.

Done.

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

Done.

> If there is just one line without leading whitespace, nano should be
> silent.

Counting lines like that seems overly complex, and won't give any
indication of what's wrong if it's only the one line without leading
whitespace.  The latter seems to be a bigger problem.

Right now, the new version skips over lines that are blank or consist
only of whitespace, as you mention in the last paragraph (which means it
doesn't give warnings for them).  For lines that can be indented, it
gives the warning if it finds at least one line that has insufficient
indentation.

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

Done.

<snip tab_indent_length() version>

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

Incorporated.

0004:

Dropped, as after some thought, it seems like overkill to do that to
save a little bit of space elsewhere.

0005:

> This retells the diff in words.  Unneeded.

Dropped.

> Why is this needed?  Why can't the strdata of the undo item itself be
> used?

Because it would require tacking together the indents of all the lines.
However, I've figured out a simpler way to do that using strdata.

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

As for this... I've accommodated you on everything else as best I can,
but the entire point of having undo is to put the text back *exactly* as
it was.  If it doesn't, it's not undo.

You're seriously saying that I should implement only partial undo, pass
it off as enough because it's less code to implement (even though less
code here would lead to incorrect functionality).  And then, *maybe*
later, implement full undo, and effectively pass it off as though it's a
new feature instead of what undo should have been doing all along?

No.  It's a misuse of the undo code to do that, not to mention false
advertising.  The proper way to normalize the whitespace that you talk
about above is to press Meta-{ to unindent (removing whitespace), and
then press Meta-} to indent the same region (adding normalized
whitespace).  If that needs to be better documented, so be it; I'll help
with that.

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

Done, as stated above.

indent-overhaul-2.zip
Description: Zip archive