Implement code folding feature in the editor

[rakasha681]

I had to drop the sun bool imports for it to compile for me, can add them back in if needed.

[thechillman422]

Hi rakasha681, thank you so much for creating this feature. Someone requested this three years ago. I appreciate all the time you put into this. It will be a great addition to RepDev. Last night I took a quick look at it and it looks very nice. I did run into a problem though. I opened a RepGen, pressed Ctrl-Shift - to collapse all sections. Then I pressed Ctrl-Shift + to expand all sections and 3 random blocks of code were moved to the end of my RepGen. Have you ran into that? I was going to create a dummy RepGen so I can simulate it for you, but did not have the time. I'll try to get to that sometime this week. Thanks again for all the time and effort you put into this :)

Bruce

[rakasha681]

I hadn't run into that, but I didn't test the fold/unfold all heavily enough apparently, I was able to reproduce, and its definitely either a race or not quite accounting for line number shifts as its unfolding. I did it bottom up since that seemed deterministic, but it's not necessarily especially when there are nested folds. Let me see what I can do, I have a couple thoughts.

[rakasha681]

I opened a RepGen, pressed Ctrl-Shift - to collapse all sections. Then I pressed Ctrl-Shift + to expand all sections and 3 random blocks of code were moved to the end of my RepGen.

Give that a try now, instead of walking the folds, it now rebuilds the editor in one shot. Also accounts for those line numbers that are folded in the gutter

[thechillman422]

Hi rakasha681, I just tried the new version and I am still getting blocks of code moved to the bottom. My RepGen is 750+ lines with 14 procedures.

Bruce

[rakasha681]

Hrm, I did a bunch of testing on one of mine that's 1700 lines and 30 procs, with a lot of nesting, and can't seem to reproduce now. I did make a few other tweaks, mainly adding line length guides, so just added those in case they help with that somehow, lol. My only other thought is something in how your formatting is that I didn't already catch as an edge case in any of mine. you can send me an example that does it if you have one you don't mind sharing.

[thechillman422]

I just tried again and still the same. I'll try to get some generic code to fail and post it here for you to try, hopefully tomorrow. Thank you very much.

Bruce

[thechillman422]

I was able to get this snippet to fail. Give it a try and let me know if it fails for you too.
CODE.FOLD.TEST.RG.txt

[thechillman422]

oops, I just saw your commit for 04bc8d1. Use my test code to see if you can get it to fail. Thanks.

[rakasha681]

Thanks for all the testing!

I think I tracked it down, along with a few other things that would have eventually popped up.

One difference in a lot of my test files is they have been touched by windows, which helpfully adds a newline at the end of files, *ix flavors don't, so editing things that were created in RepDev/Symitar that never saw windows would be missing that, which could cause line number counts to be off by 1 which would get an additional +1 for every fold, which would explain things ending at the bottom.

Few other minor edge cases that could cause that too.

I also noted it was re-parsing vars with every fold, which isn't too much of an issue with manual clicking one fold at a time, but when you start folding 30+ with fold-all, and it reparses for each one, that was getting out of hand, so now it batches the parse so there's only one done after the folding or unfolding is done.

Using undo-redo could also cause issues because folding wasn't included in the history, so I fixed that, so if you edit something, then fold, then hit undo, it doesn't try to jump straight to undoing the edit which could have been inside that fold, it undoes the fold, then another undo undoes the edit.. makes a little more sense that way :D

Added a couple console prints to surface any remaining failures hopefully! I did just try the test file and seems to be OK, but if you get any more unexpected unfolding, see if there is an error in the console.

[thechillman422]

WOOHOOO, looks like this last fix worked. I will give it some thorough testing and let you know if I find anything else. I usually run the Beta versions for a couple of weeks to make sure there are no other issues before I push it out. Thanks for the quick patching, I appreciate it.

Bruce

[rakasha681]

Awesome, yeah, always better to test as much as possible, especially with a smaller userbase like we have.

Happy to help, I've actually been sitting on these a while and had them implemented in a personal clone but its such a huge QoL I wanted to share the love, lol.

My personal version also has a full update to SWT 3.133 and all the theme/ui improvements it brings among other things. I can publish it if you're interested. It's likely not something you would want to merge to main though since UI updates tend to be hit or miss with users, and it's also migrated to maven for dependencies/packaging and no longer has 32bit support. Since it was for personal use originally, I didn't want to spend any time keeping that stuff backward compatible/optional but might make nice alternate branch that some folks may appreciate.

[thechillman422]

Yup, I actually run my own special build too, that has some functions that other users will probably not use so I keep it to my self as well.

A few months ago, someone had offered to migrate RepDev to Maven for me. There aren't many code contributors these days. I also do not like the idea of things being updated without me knowing about it (me being paranoid). My builds are primarily for users that are not able to build the binary themselves. They trust RepDev so I need to do my best to keep things simple and transparent. I know there are a lot of users out there that keeps their own build, too, and may pull in some updates/enhancements from here occasionally. I prefer not to create a separate branch because that would be more things to maintain. Thank you for the offer tho :)

Just a quick update, I did a bit more testing and still looks pretty good. I do see one issue tho, and I don't think you can do much about it. If you have the option selected to show unused variables, it will give false hits when the sections are collapsed and you click Save. At some point, I will turn this into an "option" in settings so users can enable or disable. And within the Options screen I will alert the users that these two options should not be enabled at the same time due to the false hits. Or at the very lease let them know if the issue so they are aware.

Next week, I will do more testing with the expand and collapse. Once I'm comfortable, I will merge it into my own build to use/test it daily.

[rakasha681]

My first venture into computer programming was in the 80s, with a hand-me-down TRS-80 and '101 BASIC Computer Games' hardcover, which is infamous for typo's in the earlier editions as well as mixing multiple dialect without explanation, so often, even if you typed in the program exactly as it was on the page, it wouldn't run... so my first lesson in programming was "everything seems impossible until you figure out that everything is possible." :D So, much like SWT having no direct support for folding, anything is possible if you create it... the question is, is the effort worth the outcome?

There are actually a few things it's affecting that I've been looking at ways to fix. Find/Replace, and Goto Definition also ignore folded text. There is basically three ways I've come up with to fix it.

1. Full virtual-line model: This is the more 'correct' fix, but it's expensive. It would keep the full text in a model, and then hide lines at the view level, but since SWT has no line hiding API, so in addition to the folding manager i added, we would need a custom line provider and would need to re-write offset usage as a model<->view translation, which is a fairly large refactor overall and adds another layer to maintain.
2. Simplest code change: when the var check, find, or GOTO features run, temporarily un-fold all, but the folds will blink, and the re-collapse logic has to also account for edits from find/replace and stale ranges, not to mention the performance cost with the 'always-on' variable check.
3. Per-call-site shims. Lowest-risk fix that keeps the existing "physically remove" architecture intact. If any folds are active, run an extra one-shot tokenization against folding.getUnfoldedText() into a transient token list + var list, and use those for the analysis pass only. Live highlighting still tracks the visible buffer. For goto definition, if the visible-buffer search misses, scan each FoldRegion.hiddenText for a DEFINE name / PROCEDURE name match, call expandAtLineSilent(headerLine) to reveal it, then retry the existing gotoSection() / matchVarAndGoto flow. For Find/Replace, add an "Include folded sections" checkbox. After the visible search returns no hit, walk folded and String.indexOf each hiddenText; on hit, expand that fold and continue normally. Replace then operates on the now-visible buffer with no special casing. That actually gives a nice additional QoL feature, to be able to fold-all, and then expand one section, and have a find/replace only operate inside what's visible. The tradeoff there is, the unused-vars pass effectively double-tokenizes when folds are active. That should be OK performance-wise since the unused-vars check already runs on a background thread.

I think #3 is the best fit, because #1 is too much debt for the payoff, and #2 is too flaky. Shouldn't take too long to knock that out.

[thechillman422]

I started in the 80's as well. I spent my own $100 (a lot of money at that time for a HS kid) and bought a Timex Sinclair 1000. To my dismay, it went on sale a month or so later for like $50. I think it was around the time that they were discontinuing it and the IBM Clone were starting to come out.

Even option 3 sounds like an expensive proposition. With RepDev the way it is, it may fubar everything. I think the first step is to get this rolled out as is and let the users know what to expect.

BTW, I was reviewing the code and noticed your 3rd commit (428427b) is not related to this feature. When you get the chance, would you roll that back. I like to have 1 pull 1 feature to keep things clear. Thank you very much, I appreciate all you've done.

Bruce

[rakasha681]

Its actually not too bad, more comment than code changes. Shims are nice for isolating a behavior change without re-writing core flows.. basically controlled duplication (the double tokenization), which enables all existing user experience to remain the same, outside the additions of the code folding feature, everything else still works exactly as the current main does :)

Did a bit of testing and looking good on all previous functionality with the new commit.

I rebased and dropped the line guides, easy enough to PR a branch for that later if wanted.

[thechillman422]

Thank you for the quick update. In general that works great. I did find scenarios where it moved the block of code. Here are the steps;
search for the code IF READONLY=FALSE AND SLUPDATEALLOWED(I)=TRUE THEN

Fold the DO above it
Fold the [TEST: UNCOMMENT above that
Fold the PROCEDURE OUTPUTHTMLBODY
Go to Find/Replace and check the box for "Include folded sections" and search for the string
Click Find

The PROCEDURE OUTPUTHTMLBODY does not expand, but the code found is below it.

BUT, if you collapse the whole PROCEDURE OUTPUTHTMLBODY, the Find/Replace does not move the code around. only if you leave some uncollapsed code within the collapsed code.

Let me know if you have any questions. Thanks.

[rakasha681]

thanks for the detailed step by step! Made it easy to reproduce and track down.

[thechillman422]

Great. fix looks good. I will continue testing. Thanks.

[thechillman422]

Just a quick update; things are looking pretty good so far. I've not experienced any more code moves so far. Two things I noticed.

When the DEFINE section is collapsed, IntelliSense does not pick up the variables. Not sure if you can do anything about that. Maybe, at the very least, if the users selects Ctrl-Shift - (collapse all), collapse all except the DEFINE section.

Second thing is that the syntax check does not show errors when the sections are collapsed. This is a bit of hit and miss tho. I've not been able to find a pattern. I press Ctrl-S to save the RepGen and make sure there are no errors, then go into a procedure and delete a character from one of the variables, collapse that procedure, sometimes I have to collapse all, then press Ctrl-S for save and I do not see any errors displayed. But if I press Ctrl-Shift +, expand all, and press Ctrl-S for save, then the error message shows up. I will keep testing. Thanks.

[rakasha681]

Hey, yeah, it's because collapsed text is actually physically removed from the editor and stored as a snapshot in the buffer. The current approach physically mutates the StyledText buffer, which makes folding pervasive, every component that touches text has to learn about it independently, and each one has its own way of be fixed, so its been a game of whack-a-mole. I'm looking at finishing the model/view split, which after all the misc little fixes, is actually almost fully implemented with a little repurposing here and there. With that change, the model (full source) is canonical, view (StyledText) is a projection. Everything non-UI, like the parser, var table, error positions, IntelliSense, etc, will all read from the model. The FoldingManager owns a bidirectional offset mapping (modelOffset ↔ viewOffset) and a "visible?" predicate. With that change, the "folding broke feature X" bug class is gone completely because features can't accidentally inherit fold-awareness because they'd have to opt into view coordinates to even see folds at all, so no more whack-a-mole. Shouldnt take long, its mostly just callsite updated.

[thechillman422]

Thank you for all the work you've done. I was able to build it. I will continue testing.

Bruce.