Make hidden fold ranges independent of range provider, add manual fol…

[PieterBranderhorst]

These changes accomplish two things:

1. Ranges of lines which have been collapsed (folded) are remembered and remain folded even when a subsequent call to the folding range provider does not include the collapsed range in its returned ranges. This change prevents spontaneous unfolds of ranges which the user has hidden in two known situations:
   1a. When a folding range provider misbehaves or responds slowly.
   1b. When ranges temporarily disappear due to changes such as removing the end marker of a comment block.

The saved view state for an edited file includes the currently hidden ranges. To prevent these from being incorrectly applied when the file is changed via an external process and then reopened in vscode, a checksum of each folded range has been added to the saved view state.

2. Added a user command to fold an arbitrary group of lines by first selecting the lines and then invoking the command. The default keyboard assignment for this is {ctrl+K, ctrl+.}
   This command wasn't possible before change (1) above. It is added now because it is possible, seems very desirable, and is useful in testing (1) above.

Testing: All previous fold functionality should remain the same. Except when a user manually folds a group of lines which overlap a fold group defined by the range provider. In that situation the code now removes the overlapping region(s) from the range provider until the user unfolds the overlapping manual region. The new functionality can be tested by using the new ctrl+k ctrl_. command and/or creating open-ended comments which surround previously folded regions.

[ghost]

All CLA requirements met.

[PieterBranderhorst]

Martin, a note in case you are concerned about added cpu load for the changes I proposed.

The added cpu time for these changes is very small relative to the total cpu time spent on folding.

Excluding the range provider, i.e. considering only the code which executes after the range provider returns its results, I think we could reduce the cpu time related to folding to be well below 1/5th of what it is now with a bit of work. If that is something you'd like to see from me I'd be happy to propose code changes for that.
Currently the great majority of time is spent I think in the underlying editor's decoration related functions. The key would be to minimize calls to those functions.

[PieterBranderhorst]

Now added to this PR: A configurable folding regions limit and a user notification when the limit on number of folding regions is exceeded.

[aeschli]

Thanks @PieterBranderhorst, that's nice. Only problem: merging multiple features in our PR makes this quite difficult for me to review. Any chance you can separate them?

[PieterBranderhorst]

Hi Martin. No problem, now I know. I've reverted the configurable folding regions limit out of this PR. (Didn't think it was safe to delete commits so I used revert.) And submitted a new PR, #140480, with just the configurable limit in that one.

[PieterBranderhorst]

I merged current main into this to handle conflicts in folding.ts.

[PieterBranderhorst]

Martin, any news on this? I don't want it to get stale. I see there are now merge conflicts. I can deal with those if you are ready to handle this after I check that in.

[aeschli]

Yes, sorry for the wait. I'll review it next week.

[PieterBranderhorst]

Ok, thanks Martin. I have updated the branch to match current main branch.

[aeschli]

I started reviewing and testing. I like the approach.

I investigated the following issues:
Create typescript file:

class A {

    foo(p: A) {
        const foo = 9;
        console.log('fff' + p + foo);
        if (true) {
            console.log('fff');
            console.log('fff');
            console.log('fff');
        }
        console.log('fff');
    }
}

• collapse foo
• set the cursor on the line above 'foo', press enter
• foo expands
• the problem is in _currentHiddenRegions, isBlocked is tested against the old range positions. Should test against the current positions (using getDecorationRange)

• Set the cursor after foo(p: A) { and press enter
• The range stays folded, but no longer covers the last line
• the problem is that for the end line the old range info is used (hiddenRange.endLineNumber - hiddenRange.startLineNumber)

The decoration currently only covers the first line. I think we need to change to cover the full range to correctly preserve the folded range.

[PieterBranderhorst]

Thanks very much Martin.

And thank you for finding those issues. My apologies.

I'm embarrassed by the first one. As you say, the problem is in isBlocked using the old positions.
Now that the code is in a branch in the Microsoft repository I don't know how I should submit a correction. I think the following would be a minimal change to fix that bug. In foldingRange.ts replace:

    if (!isBlocked(hiddenRange.startLineNumber, hiddenRange.endLineNumber)) {
        let decRange = this._textModel.getDecorationRange(this._editorDecorationIds[i]);
        if (decRange
          && decRange.startColumn === Math.max(decRange.endColumn - 1, 1)

by

    let decRange = this._textModel.getDecorationRange(this._editorDecorationIds[i]);
    if (decRange && !isBlocked(decRange.startLineNumber, decRange.startLineNumber + hiddenRange.endLineNumber - hiddenRange.startLineNumber)) {
        if (decRange.startColumn === Math.max(decRange.endColumn - 1, 1)

The second problem is tougher. Typing into a collapsed range seems wrong and I could argue either way for whether the new text should remain visible or not. Maybe a third choice, should the range automatically unfold when a user does that?

It gets nastier. Suppose that the first line of a folded region, the one which remains visible, is something like if (xxx) { // do something. If the user hits enter in front of the { the resulting hidden region should be different than if they use enter after it. And the same is true of the terminating line if it looks like } // the end

I played around with making the decoration region include a larger block of the text and things are still problematic that way. The best overall result seems to come from starting the decoration at the last character + 1 of the region and ending it at the first character of the line following the region. But it isn't perfect, especially when considering the cases mentioned above. And I'm not sure about possible consequences to other parts of the editor from having large decoration ranges hanging around.

I want to mull this over a bit. Maybe there's a simpler way.

By the way, do you recall the reason for the "isBlocked" test? I.e. under what conditions there can end up being an active selection point inside a hidden range when "update" is called?

[PieterBranderhorst]

I've explored the code which handles decorations. It is very nice. I don't think that increasing folding region decorations to cover their full range will trouble it at all. I think you were right about that as the way to go. I've ended up using the from start line's last column to the end line's last column + 1. That seems to work well in many cases. I've tested with adding text at end of first line and start of last line. Destructive backspace from start of end line un-collapses the fold. Global replace of something inside a hidden region which contains an end of line mostly works well. There are cases which misbehave, e.g. global replace which spans the start or end of a hidden region. That could be fixed by adding local (to folding) code which processes onchange events. And that might be desirable as part of an optimization effort. But for now it seems an acceptable oddity.

With this fix, all edits done in the visible line above and below a hidden region remain visible until the hidden region is unfolded by the user. To me that makes the best sense. It could be argued that some typed changes should disappear into the hidden region but I think that's just too disconcerting.

There are three places changed in foldingModel.ts for these fixes. Can you compare and pick them up from this copy? Or is there another way I should post it?

[aeschli]

Thanks @PieterBranderhorst !
I see your commits on the branch. Also the foldingMode.ts matches what you have in this copy.
What's not working?

I haven't yet tried out your latest, so just to reply. What you write is all correct.

• The decorations should not more expansive if they cover a range. BTW, there are also several options to configure them how they behave when editing happens at the start and end offset. We want to test with operations like format that typically create a lot of modifications.
• The first line is officially not part of the folding range and editing in it should not expand the range
• IsBlocked ensures that the current cursor positions never end up in folded regions. The enter just before ... is one example and when you remove a } at the end and the folding provider decides to expand the range.

What would be good to have some unit tests. The very least for the merge function.

[PieterBranderhorst]

I didn't understand how github would handle my push at this point in the process. All is well.

I am working on adding some unit tests. I expect to finish and push them tomorrow.

[aeschli]

Thanks @PieterBranderhorst
The test case where I press enter right before the ... is still not working, the range does not expand. I would say the fix is to change the stickiness to AlwaysGrowsWhenTypingAtEdges. That way the range will expand.

Another test case is to press enter right before the {. The range stays folded. When the new ranges come in from the TS folding provider we end up with two folding ranges. One starting at foo expanded) and one at { (folded). Expand the folded, and the range goes away.
It's the expected behavior with the approach, just a bit surprising.

[PieterBranderhorst]

I have experimented with combinations of where a range starts and ends and using AlwaysGrowsWhenTypingAtEdges instead of the current setting. I don't think any combination works better than the current one.

The underlying problem is that we would like to unambiguously determine whether a region returned by a range provider is the same as a region which was previously collapsed.

Even the previous code was imperfect in this area. I was able to make it misbehave in a few ways: Changing the first line of a hidden region would sometimes cause the region to unfold and at other times would not; changing the first line could cause the hidden region to expand to include more than it did previously; destructive backspacing into a hidden region would delete text invisibly. And it seemed confusing to me in that when it works it can make things I type "auto-disappear". Though that can be a correct result it nonetheless seems undesirable to me when it happens.

I don't think a perfect fix covering all situations is possible without tracking the specific start and end markers for a range. That would be a daunting task even before considering that some markers are "variable", e.g. line indent.

The current result seems an ok compromise to me. It is an attempt to keep all changes made before or after a collapsed region visible; to not change what part is hidden until it is unfolded. It does now automatically unfold if the user destructive backspaces into it.

I can't think of rules which will have a better result. We could add a flag to track regions which are "manually" folded (either by user or due to re-opening a file without a working range provider.) With that flag we could do as well as the previous code did in most cases but we'd still have some problems as mentioned above. We could then fix some of those problems by further refining some code. I think there would always remain some issues which would be more effort to address than they'd ever be worth.

Can you think of a good set of rules to address these issues? If not, would you like me to add code to address as many as possible? (By adding a "manual" flag per region and adding code to handle some specific cases, and then probably give up on other cases and just un-fold regions we can't reconcile from one range provider return to the next.)

[PieterBranderhorst]

Hi Martin. Is this PR stuck because of the failed automatic check? I see that it failed in "Run Monaco Editor Checks". The files it seems to have a problem with aren't related to anything I modified. And when I try running " yarn monaco-compile-check" on my checkout of same thing it runs fine. I don't know what to do next.

[aeschli]

@PieterBranderhorst I was on vacation last week. Sorry that this is not going quicker, but I also in regular work weeks I don't get much time to look at it. Please be patient.

[PieterBranderhorst]

I'm not wild about that approach. Two problems:

1. In the current branch code fold-selected came out as an almost free side-effect. It only needed the command handler added which is simple code. If fold-selected goes first it either gets most or all of that code anyway, or it is implemented with different more specific code which then either ends up with functional overlap or ends up needing to be refactored back to where we are already.
2. Fold-selected isn't what we started out to handle. While it may be a new feature a lot of people would like, we started out to address a problem users are experiencing with the current code. A serious problem for those who experience it.

I would prefer to further modify the current branch to recreate the existing behaviour when people edit the code at either end of a collapsed region (which is approach 1 above) or take approach 2 or 3. Much of those modifications (e.g. identifying the type of each hidden range as manual vs. provided) is code which would be needed anyway if doing only fold-selected.

[aeschli]

No problem for me if you want to continue to modify the current PR code and not breaking the task up.

But are you ok with havening a setting that let's users to choose between the keeping collapsed regions or discard them if they are no longer reported? As said, when I tested I was not a big fan of having collapsed regions that no longer match a language construct. But I'm interested to see more users try this and report feedback.

[PieterBranderhorst]

Ok, I will continue via the current code. Making it match previous behaviour as much as possible. When done I think there may be just two cases where behaviour while the user is editing will be different. Let's see how close I can get it and then decide whether you still want that setting added.

This branch is adding a bunch of unnecessary merge bulk to the repository. When I have the next version ready should I rebase this, create a new branch so that this is killable, or just carry on in this branch?

[aeschli]

Yes, rebase (and squash) would be good at some point. Either create a new branch or force-push to the existing branch. Both work.

[PieterBranderhorst]

I've made some fixes, squashed all commits in this branch, and rebased it on current source.

I haven't added an experimental flag. I am hoping you'll agree that where this version behaves differently from the current code it is an improvement.

I've added use of a different icon in the margin for regions which were manually folded. Please let me know what you think of it.

Situations I know of where this code behaves differently from the current code are:

1. With cursor at start of first line after a collapsed region do a <backspace>.
   Current behaviour: Inconsistent. Depending on range provider and text involved either keeps region folded displaying a concatenated following line, or removes previous character with no visible change, or auto-unfolds the region.
   New behaviour: Auto-unfold

2. With cursor at end of line at start of folded region, insert a character, or to delete a character which is not a folding token, or <del>
   Current behaviour: Auto-unfold
   New behaviour: Keep region folded.

3. Split the line after a folded region (i.e. use <enter>), before the end token (e.g. "}")
   Current behaviour: The newly created line before the split point disappears into the folded region.
   New behaviour: Auto-unfold

4. Add a new start-of-comment with no matching end above one or more folded regions. Or remove a preceding end-of-comment. This is a case where the new behaviour is part of the purpose of the new code.
   Current behaviour: The folded regions auto-unfold.
   New behaviour: If there is just one hidden region affected it auto-unfolds. This is done because the code can't distinguish this situation from one where the opening token of a fold has been removed. If more than one hidden region is affected they remain hidden and are converted to manually folded regions. If the range provider subsequently provides matching ranges again, e.g. because the unmatched start-of-comment has been fixed, the hidden ranges are converted back to normal hidden ranges.

I hope we're getting close to something satisfactory now :)

[aeschli]

Excellent improvement, much better now!
The only 'strangeness' I found when pressing del just before the ellipses. Maybe that should also expand.

The new icon is a good idea but needs a better symbol. I could imagine something like the ellipses ... which would make it clear that this only expands and goes away then.

[PieterBranderhorst]

You're right, it needed a better icon. I didn't want to add one (right chevron with some added glyph seemed ideal) and I hadn't considered ellipsis. I think it works well.

And yes, auto-expanding when using <del> at end of start line auto-expanding makes more sense. It is more consistent with the rest of the behaviour.

I've checked in both of those changes.

[aeschli]

I'm trying to understand of that's intended or not:

export function activate(context: string) {
	try {
		const schemaRequests = {
			getContent(uri: string) {
				return undefined;
			}
		};
	} catch (e) {
		console.log(e);
	}
}

• collapse the block at const schemaRequests
• Add a /* in front of activate
• folded range expands

[PieterBranderhorst]

Yes, that's intended. This is the one deliberate odd behaviour. When the folding provider no longer returns a region which it previously returned and which is collapsed the foldingModel logic can't easily distinguish between three possible reasons:

1. The user modified the text in such a way that the region really is gone. E.g. They deleted the opening "{" of a region. In this case we should unfold it.
2. The user modified the text such that the region is now embedded in a comment or a string. In this case it makes sense to keep the region folded - the user may next close the comment or string before the region. And even if they don't the fold still makes a kind of sense.
3. The folding range provider is misbehaving. Very slow to respond or responding incorrectly. Which we think is the case in the issue which got this whole thing going. In this case we want the hidden region(s) to stay hidden.

Since the foldingModel can't readily distinguish between these cases I decided to treat it as case (1) if only one hidden region is affected and unfold it, and treat it as case (2) or (3) if more than one hidden region is affected. My reasoning is that even if it is wrong in treating a situation as the first case, as it is in your example, only one fold is affected and that isn't a big hardship to the user. I expect it will nearly never be wrong when treating something the other way.

[aeschli]

Thanks @PieterBranderhorst. That took a long time, sorry for that. Let's see how users like it.

[PieterBranderhorst]

@aeschli Hi Martin. I'm glad you got this done, I'd given up on it.

Please ensure that the release notes mention ctrl-K ctrl-.
Though that wasn't what motivated this change I think that's the part that many people will use. It might get used more than the automated fold points :)

By the way, I've given up on my parameterized folding provider. I got it working as an extension but there are too many difficulties with integrating it with patterns provided by other extensions and as a result I don't think it would get used a lot. It really should be built-in to work well. If you ever want to reconsider it I could bring it up to date and resubmit, just let me know. It:

• Works a lot better than you might think given that it uses regular expressions. I like what it does for Typescript folding better than the built-in Typescript folding provider. It doesn't have the shortcomings you might assume with regular expression parsing.
• Has an option to include indent based folding code. So it could replace the existing indent based folding code while adding the new functionality.
• Is very fast. Faster than the existing indent based folding, even in most cases for elaborate languages.

Also, if you become interested in this area again, I have an approach sketched which would highlight things as a user moves the mouse along the gutter containing the folding icons. It would highlight the code to be folded when over a collapse icon and would temporarily expand and highlight the folded code for expand icons. Please send me a note if this becomes something you'd like to pursue.

Ditto if you'd like a cleanup pass over the folding code. I think the hiddenRangeModel.ts code is almost all unnecessary at this point and could be removed, and efficiency could be improved for all of it by restructuring the internals a bit.

[aeschli]

Hi @PieterBranderhorst, yes sorry for letting this lie around so long. The fold selection feature is a major motivation to get it out of the door. Also I wanted to get some user feedback so it's not just my opinions that shape this.
So if you have some resources later on to respond to user feedback and fine tune the implementation, that would be great.

• I'm curious to see if the 'unfold once' approach will work for users for 'Fold from selection'. I anticipate that some users want the range to stay, even if unfolded.
• The current heuristic to find out which folded ranges to preserve is not super clear (also to me). See the example here where a single folded range goes away, but if there are multiple folded ranges, they stay. If you can add more background on how the multi pass is intended to work..

You mention that we want to keep ranges that in a comment or a string literal. We could get that that information (textModel.tokenization.getLineTokens...)

On the other discussion:

• hover over a folding icon would be great to have. Maybe a hover that shows the folded content?
• parameterized folding provider: I'm not keen on adding that as a built-in provider.

[PieterBranderhorst]

Hi @aeschli,

I will watch Issues for feedback on this.

Regarding having manually created folding ranges persist: I do want to see how much that's wanted. It would be a bit tricky and require some thinking about handling detection of edits which should modify or remove it and about manual removal. It might be less wanted if we implement the highlight and temporary unfold when hovering over a fold icon. If we add that then users could quickly view the content of a manually folded range without unfolding it.

One other future feature on my list which I forgot to mention above: Ability to fold/unfold all regions of a specified type. This already exists but is minimal. It works for comments and "regions". But the folding provider interface, the settings related to "regions", and the "folding imports by default" weren't thought out as a generalizable subject so it would take a bit of rework. To allow language specific concepts like "functions" or "chapters" to be defined and used.

I understand you're not keen on the parameterized folding thing. But I think it would be used a lot. There are a huge number of syntax highlighting extensions for vscode. For many (most I think) of those languages it isn't worth the effort to create language providers nor folding providers. And some people would write custom ones for things like event logs. I think that even for Python a folding range provider was only added recently, and it had teething problems. If I continue working in this area this is the part I'd really like to do next.

[aeschli]

@PieterBranderhorst I started working on #155302 to simplify the merging rules.
Would you be ok with that simplification, or do you see a problem?