Re: [PATCH] line folding support

[Benno Schulenberg]

Thanks for posting your work.  Interesting.  But... I'm not sure
about the user experience.  (See at the end of this mail.)


Op 01-04-2023 om 23:59 schreef rexy712:
> Line folding is the ability for to hide or collapse a section of a file when you 
> don't need to see it. All IDEs I've used have had this feature and it's the one 
> thing I feel GNU nano has been missing when working from the terminal.

Well... nano is meant to be a simple editor, not an IDE.

> This patch is pretty massive, so I've also split it into multiple patches after 
> finishing.

Thanks for splitting it up.  But when having the parts, sending also the
integrated patch was not needed -- it just doubles the size of your mail.


> +#include <ctype.h>
> +       /* isspace */

A leftover?  It compiles fine without this.

> +void unfold_folded_segment_from(linestruct *line)

This functions is used just once.  Merging it into its caller makes
things clearer.

> +/* Remove a folded segment containing the given line */

"Remove"??  You probably mean "Unfold"?
Also: sentences should end with a period.

> +   for(;line != NULL && line->folded;line = line->next)

Keywords should have a space after them.  Semicolons too.

> +/* Get the last member of the folded segment */

"Member"?  Probably "line" is meant?

> +/* Remove any folded_segments within the range [top, bottom].

There is no 'top' nor 'bottom" among the parameters of the function,
nor even in its body.  These comments are badly out of sync.

> +        * If not, top is unmodified. */
> +       if (!line->folded) {
> +               linestruct *bot;
> +               find_bracketed_region(openfile->current, &line, &bot);
> +       }

Where's 'top'?  And when not using a variable for anything ('bot'),
better call it 'dummy' or something like that.


> @@ -262,9 +267,13 @@ void extract_segment(linestruct *top, size_t top_x, 
> linestruct *bot, size_t bot_

> +       if (top != bot) {
> +               UNFOLD_SEGMENT(top);
> +               for (linestruct *line = top->next; line != bot->next; line = 
> line->next) {
> +                       UNFOLD_SEGMENT(line);
>                         had_anchor |= line->has_anchor;
> +               }
> +       }

This seems superfluous.  Why unfold anything when extracting a bunch of lines?
Wouldn't it be nice if one could cut and paste stuff in a folded state?


> @@ -376,6 +377,8 @@ bool do_next_word(bool after_ends)

> +       UNFOLD_SEGMENT(openfile->current);

Why should jumping to the next word unfold anything?  When in Geany a section
is folded, Ctrl+Right jumps right over it, as if it contains zero words.  Does
that works differently in other editors/IDEs?


> @@ -147,7 +147,8 @@ void do_backspace(void)

> -               do_left();
> +               openfile->current = openfile->current->prev;
> +               openfile->current_x = strlen(openfile->current->data);
>                 do_deletion(BACK);

Why is this change needed?  Because it changes nano's behavior in an
edge case.  Do for example

    nano --ignore NEWS --jumpy

Then type ^V three times, followed by Backspace.  With the above change
nano no longer centers the current line.  Sure, few people will use
--jumpyscrolling, and few will hit Backspace without seeing what they
are backing into, but still.


I haven't looked at the rest of your patches -- the whole thing is
too massive.  Instead I've tried out how it works.

In a C file, I need to be between a pair of braces for M-[ to do anything.
I would have expected it, when not between braces, to find the first brace
after the current position and then fold that section, and put the cursor
after it, so that a subsequent M-[ will fold the next section, and so on.
When the first found brace belongs to a folded section, then unfold it,
of course.  But that's just me -- I have no experience with folding.

When in a text file, like the README, I would expect an M-[ (when nothing
is selected and the cursor is not on a folded section) to simply unfold
the first preceding folded section.  That avoids having to precisely
place the cursor.

Benno

OpenPGP_signature
Description: OpenPGP digital signature