patchGNU nano - Patches: patch #10263, Add line folding support

 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #10263: Add line folding support

Submitter:  Sam <rexy712>
Submitted:  Mon 04 Jul 2022 10:46:55 PM UTC
   
 
Priority:  3 - Low Status:  None
Privacy:  Public Assigned to:  None
Open/Closed:  Open Release:  None

Jump to the original submission

Mon 11 Jul 2022 03:16:45 PM UTC, comment #6: 

What I would try when trying to implement default folding is: search backward for a blank line (or folded line), then from there search forward for the first opening brace at end of line.  When found, then find the matching closing brace, and fold from the blank or folded line to the closing brace.  When a blank line is encountered before an opening brace is found, just fold from the first blank or folded line to the found blank line.  (Of course, this kind of folding works only for C-like programs and for plain text where paragraphs are separated by blank lines, but... it's a start.)

Benno Schulenberg <bens>
Group administrator
Mon 11 Jul 2022 01:00:07 AM UTC, comment #5: 

I appreciate you taking the time to go over the patch. I can see a lot of cruft and old bits that I forgot to clean up as I worked on it now that you've pointed some out. And some other cruft that I didn't even think of as such until having someone else mention it.
I'll take some time and try to do some cleanup and also remove the unneccessary bits like coloring.

I've never been very good with making #ifdef segments look clean and minimal, and as I briefly mentioned in a previous post here, I'm a c++ developer so it's hard for me to not be overly verbose with my code :)

I'll keep hacking away at it and try to be more thorough before posting another version on nano-devel.

One question before that though, so I don't spend a bunch of time on this if it's unneccessary:

So I like your idea of being on the start line of a function or if/for/while statement and hitting the fold hotkey from anywhere within that line to fold the block, but it's not easy to implement in a predictable way. For example:

for (int i = 0;i < 10;++i) {
 ...
}


the less-than sign in the loop would count as a bracket and that could try to be matched for folding instead of the curly-brace. And if you went with folding what nano considers a 'block', those rarely seem to align with 'blocks' of code so that doesn't seem to be a good fit to me.

Instead, what do you think of this idea for folding when no text is currently selected:
If the cursor is on a bracket, find the matching bracket and fold all the lines between that bracket pair? That way you can easily collapse a function or struct or other such things.
And if no bracket is under the cursor, just mark the current line folded? The idea behind this is to allow hitting the folding hotkey multiple times to keep successively adding lines to a folded segment.

Sam <rexy712>
Sun 10 Jul 2022 07:35:27 AM UTC, comment #4: 


+                        } else if(index == FOLDED_LINE) {

Coloring things is not an essential part of folding.  It can be added later.  Now it just clutters the patch.

+#ifndef NANO_TINY
+        bool was_folded = openfile->current->folded;
+#endif

Don't use a variable that isn't needed to make things work.  It's just clutter.

+                if (was_folded) {
+                        unfold_segment(openfile->current);
+                        refresh_needed = TRUE;
+                }

Is there any situation where one doesn't need to do a refresh when one folds or unfolds?  I can't think of any.  So setting 'refresh_needed' should just be part of the folding and unfolding routines.  Then the above can be just two lines instead of four.  Yes: nano's style is not to use braces for a single statement.

+        if (top != bot) {

Again: do not add braces where they are not needed.  And certainly don't add braces where they weren't before.

+        add_to_sclist(MMAIN, "^E", 0, do_fold_segment, 0);

^E is already taken.  You have to pick a keystroke that is not ued yet.  I would suggest M-[, which even looks a bit mnemonic.

...


+/* Add a new folded segment holding the range [start,end] */

The comment does not match the code any more.

+        linestruct *l = line;

Ouch.  Why not just use 'line'?

+int fold_segment_length(linestruct *line)

Poor function name.  I read 'fold' as a verb here.

...


+#ifndef NANO_TINY
+                if (line->folded) {
+                        line = get_end_of_folded_segment(line);
+                        line = line->next;
+                } else
+#endif
+                {
+                        line = line->next;
+                }

Ugh.  This could simply be:

+#ifndef NANO_TINY
+                while (line->folded)
+#endif
+                        line = line->next;


Now about how things work.  If, with your patch, I run 'src/nano +1 README.hacking' and then type Ctrl+Shift+Down three times and then type M-[ (I've adjusted the patch to use this keystroke), then... the top line stays highlighted, and it says that 8 lines are folded.  I would have expeted 10 lines to be folded -- that is: all lines that were visibly marked, the same way that M-3 (commenting) works.

Further, if I type M-[ while nothing is marked, nothing happens.  I would expect M-[ to do something by default.  For example, fold the current paragraph or function, so that with multiple M-[s multiple paragraphs or functions can be folded.

If you make an improved patch, please send it to nano-devel.  Other people will see it, and it's much easier to review -- I won't need to copy stuff over.

Benno Schulenberg <bens>
Group administrator
Tue 05 Jul 2022 07:09:27 PM UTC, comment #3: 

I'm attaching an updated patch with the folded_segment struct removed, the pointer ('fold') in linestruct changed to a bool ('folded'), and the function names made less awkward and confusing.

(file #53400)

Sam <rexy712>
Tue 05 Jul 2022 04:34:50 PM UTC, comment #2: 

I know it's a huge patch, I was trying to keep it small but I kept having issues where different functionality wouldn't handle the cursor placement or unfolding when necessary. I apologize for that.

To address your points:
1) Yeah I wasn't happy with adding a whole pointer to linestruct myself. I initially had it as just a linked list in the openfile struct without having the pointer in the linestruct, but then when finding which fold the line was in it would iterate through the list of fold segments checking each one by one. I hadn't thought to just put a bool in the linestruct to make lookup for non-folded lines just return early. That could be changed easily to just be a bool.

2) Fair enough. It made sense to me when making it, but 'unfold_segment' is a way better name looking at it now.

Sam <rexy712>
Tue 05 Jul 2022 10:53:38 AM UTC, comment #1: 

After staring at the huge patch for a while, two things.

1) Why do you need the separate data structure for folded segments?  I would have expected that for folding one would add just one boolean field to the linestruct: 'folded'.  What capability do the segments provide?

2) The naming of 'remove_folded_segment' makes me think the wrong thing every time: that all the lines in the segment will be deleted.  I would have called this: 'unfold_segment'.

Benno Schulenberg <bens>
Group administrator
Mon 04 Jul 2022 10:46:55 PM UTC, original submission:  

Folding lines is the ability 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. It helps to keep the screen less cluttered when trying to focus.
Folding lines is useful when trying to work on two parts of a file at the same time, or when referencing something in a different section of the same file instead of jumping back and forth.

I also found this bug report before I began implementing line folding, which told me I'm not the only one who would like to see this implemented:
bug #58199: [Wish] the ability to temporarily "fold" lines

I've been using this folding feature for a couple days working on C++ files and it seems stable. I didn't add any self-crediting in the patch or update translations. I assume that would be taken care of separately if this is ever accepted.

Sam <rexy712>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #53400:  0001-Add-line-folding-support.patch added by rexy712 (46KiB - text/x-patch - Updated patch without the folded_segment struct)
file #53395:  0001-Add-line-folding-support.patch added by rexy712 (50KiB - text/x-patch - patch as generated by 'git format-patch master')

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by bens (Posted a comment)
  • -email is unavailable- added by rexy712 (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2022-07-05 rexy712 Attached File- Added 0001-Add-line-folding-support.patch, #53400
    2022-07-05 bens Priority5 - Normal 3 - Low
    2022-07-04 rexy712 Attached File- Added 0001-Add-line-folding-support.patch, #53395

    Back to the top

    Powered by Savane 3.13-12ab.
    Corresponding source code