(GH-1523) Remove duplicate/overlapping folding regions #1525
Conversation
|
Manually tested this works with the script provided in the issue. The unit test went from; Before; After |
glennsarti
force-pushed
the
gh-1523-fix-overlapping-regions
branch
from
5b3e355 to
ffebf75
Compare
src/features/Folding.ts
Outdated
| return (item.startline !== src[index - 1].startline); | ||
| }) | ||
| // Convert the matched token list into a FoldingRange[] | ||
| .forEach((item) => { foldingRanges.push(item.toFoldingRange()); }); |
There was a problem hiding this comment.
Since you're using a forEach, could we just put the filter inside of the forEach?
You can grab the index as the second parameter in the forEach:
.forEach((item, index) => {
if(index === 0 || item.startline !== src[index - 1].startline) {
foldingRanges.push(item.toFoldingRange());
}
});There was a problem hiding this comment.
True. I can fix that.
There was a problem hiding this comment.
Oh! I liked it with the filter
There was a problem hiding this comment.
So did I, but hey, Tyler got in first :-)
There was a problem hiding this comment.
Could probably reduce further
return foldableRegions
.filter((item, index) => index === 0 || item.startline !== src[index - 1].startline)
.map((item) => item.toFoldingRange());There was a problem hiding this comment.
Given the previous PR about "avoiding" functional style and use imperative I'm happy to remove .filter
There was a problem hiding this comment.
I know it sounds capricious, but I like the functional style and I think it applies very well here. I'm also aware that V8 is doing its best to make it all efficient. My concern last time was that I was finding it hard to pick apart the behaviour, but I was looking at that particular code again recently and thinking it might be better implemented as a filter.
There was a problem hiding this comment.
I'm fine with whatever @glennsarti would like to do in response to @rjmholt :)
glennsarti
force-pushed
the
gh-1523-fix-overlapping-regions
branch
from
ffebf75 to
192ca72
Compare
|
@tylerl0706 Updated. |
There was a problem hiding this comment.
@glennsarti on fire as always! Thanks for the quick turnaround. LGTM
Previously the syntax folder returned an ordered list of folding ranges which VS Code would "ignore" overlapping or duplicate ranges. However on manual testing, it showed that duplicate region did exist and could be folded/unfolded using the "Fold All" and "Unfold All" commands, but could NOT be manipulated manually in the editor using the +/- indicator. This commit uses a filter which removes any duplicate or overlapping regions which is easily detected via similar region start lines. This commit also adds a test for this scenario.
glennsarti
force-pushed
the
gh-1523-fix-overlapping-regions
branch
from
192ca72 to
92a5324
Compare
|
Fixed with newer filter code. Ready for merge @tylerl0706 @rjmholt |
PR Summary
Previously the syntax folder returned an ordered list of folding ranges which
VS Code would "ignore" overlapping or duplicate ranges. However on manual
testing, it showed that duplicate region did exist and could be folded/unfolded
using the "Fold All" and "Unfold All" commands, but could NOT be manipulated
manually in the editor using the +/- indicator.
This commit adds a filter which removes any duplicate or overlapping regions
which is easily detected via similar region start lines. This commit also adds
a test for this scenario.
Fixes #1523
PR Checklist
Note: Tick the boxes below that apply to this pull request by putting an
xbetween the square brackets.Please mark anything not applicable to this PR
NA.WIP:to the beginning of the title and remove the prefix when the PR is ready