simplify fold merging

[aeschli]

Simplifying the merge logic that merges the previously folded ranges with the newly computed ranges.

• when a folded range starts at the same location as the new range, the new range is now folded and not manual
• when a folded range is not found in the new ranges, it is kept and marked as manual
• if a new range overlaps with manually folded range, the new range skipped

[PieterBranderhorst]

Hi @aeschli,

I'm not comfortable with some of those changes to the merge routine.

Some of them are just removing unnecessary code given the assumption that the second parameter is always the currently folded ranges. I wrote the code with an eye toward it being more general and eventually replacing the merge in syntaxRangeProvider with this merge. Because their purpose is the same and the merge in syntaxRangeProvider doesn't catch all bad cases. But removing the currently unnecessary bits is harmless for now, they just may be needed again later.

I'm uncomfortable with removing the test "&& nextA.endLineNumber === nextB.endLineNumber" and instead using rangeA whenever the start line number is the same. Two issues:
1. Suppose we have a top of fold line like "if (a) { if (b) {". I.e. there are two fold open markers on one line. I know that's bad code but it is far from the worst things I've seen :)
Fold the region.
Now remove the "if (a) {" part of the line. With the old code it would unfold. With the new code it will stay folded, including whatever code was folded past the end of the "b" section.
2. It is a problem if ever using the merge for more general purposes as above. (The second parameter is supposed to be definitive given any differences.)

I'm also uncomfortable with removing the two pass logic and the "} else if (nextB.isCollapsed && !nextB.isManualSelection && passNumber === 1) {" section. I think that the new code will now incorrectly keep a folded range in some cases. E.g. start with:

    if (something) {
        a = b;
    }

Fold the "if" line.
Delete the "i" in "if". The folded region will be converted to a manual fold with the new code. I think the previous approach did better by unfolding it.

[aeschli]

For 1.) I just want to avoid that a manual folding range to be created at a place where we have a range from the folding provider.
That's how this would look:

So IMO we want to stick to the folding provider range in this case. The question is, do we keep things folded or should we unfold.
The change to favor unfolding would be

				if (nextA && nextA.startLineNumber === nextB.startLineNumber) {
					// same range in both lists, merge the details
					useRange = nextA;
					useRange.isCollapsed = nextA.endLineNumber === nextB.endLineNumber;
					useRange.isManualSelection = false;
					nextA = getA(++indexA); // not necessary, just for speed
				} else { // use nextB
					useRange = nextB;
					useRange.isManualSelection = true;
				}

The generalization just make this one step hard to understand and to tune. Using the same code in the syntaxRangeProvider is no priority for me.

    if (something) {
        a = b;
    }

Note that the folding range starts after the { and ends before }. Modifications on if shouldn't unfold, unless the folding provider removes the folding range.

[PieterBranderhorst]

I still think we should test for both start and end line number like the original code. Here's an example of why using the newest code. In next image I select a bunch of lines and then ctrl-K-. Only the first few get folded. I think this behaviour will generate a lot of user queries:

Regarding the two pass behaviour here's an example of how removing it creates what I think is the wrong behaviour. (This was what I meant with changing "if", sorry, my brain wasn't in gear then.) I think in the following the fold should auto-unfold:

One more thing, something else I noticed today in the newest code which I think used to work. In the following image when the manual fold is unfolded, at that moment the fold icons should reappear on the two auto regions. But they don't reappear now until we do something to get the folding provider invoked, when I type "xx" below:

[aeschli]

I continued to work on the PR:

• now storing is a manual folding range is created by the user ('user defined folding range') or when a folded ranges with preserved ('recovered folding range')
• When merging, user defined folding range are winning over provider ranges, recovered folding ranges are losing against provider ranges.
• Expanded manual ranges are no longer dropped. They get their own folding marker and are also persisted when an editor closes.
• Add a command Remove Manual Folding Ranges to remove manual folding ranges

@PieterBranderhorst Giving user folded ranges precedence addresses the issue you mentioned in the first and third gif of comment . The second issue looks ok to me. As mentioned, the range should only expand when after the {

[aeschli]

Tet plan items: #156163, #156161

[PieterBranderhorst]

@aeschli That looks good to me in principle, making "userdefined" and "recovered" separate things. I'd have preferred an enum "kind" of { provider, userdefined, recovered } instead of the two bit flags, because isUserDefined and isRecovered should never both be set at the same time. A 3-valued "kind" would be clearer and safer to me. But the bit flags should work.

I'm a bit worried about how quickly this is going into release. I don't have time to review it all today. I'll try to later this week. I'm concerned that there may be subtle consequences.

One thing I did notice in a quick look at some of the code. I think that the line "isUserDefined: range.isRecovered," in getMemento in foldingRange.ts will result in flipping saved ranges back and forth between types userdefined and recovered every time a save-apply pair of calls happens.

[aeschli]

Thanks for catching this, @PieterBranderhorst. Good idea to replace the bits with an enum. I created #156273.
I kept the bit fields internally now, but will replace them next milestone.

[tjx666]

I found that I can't using cmd + k, cmd + . to create manual folding.
It's reconized to remove:

[aeschli]

@tjx666 cmd + k, cmd + . should remove, and cmd + k, cmd + , (comma) should add.
You can redefined this shortcuts if they are not easy to reach on your keyboard

[tjx666]

@aeschli

[PieterBranderhorst]

@aeschli I reviewed the rest of the code changes and didn't see any other problem. The code looks good and is nicely readable.

One comment: I see that you re-added a test I had removed. In folding.ts, in restoreViewState, the test for "state.lineCount !== model.getLineCount()". I suggest removing that test again. I removed it because the new checksum tests detect changes with much greater accuracy and I know that some users use folding with log files. Some log files might change between one use and the next by having lines added at the end. With the checksum tests in place we don't need to invalidate saved fold information in that situation and it seems good to keep it.

[aeschli]

Back from vacation.. Thanks @PieterBranderhorst, makes sense, will do, I created #159032

[PieterBranderhorst]

Good stuff, thanks @aeschli. I've been watching issues since these folding changes went live and all seems well, nothing has come up.