Fold enhancements for modern Tree-sitter #918
Conversation
* A new predicate called `fold.invalidateOnChange` that can be used when a change should automatically invalidate the fold cache for each row in a node's range. * The ability to use custom predicates in `folds.scm` files. (Previously, captures from `folds.scm` did not consult an instance of `ScopeResolver` in the processing phase.)
|
Hrrm, weren't these changes included in #906, maybe accidentally if this is intended as a separate PR? |
| // This is almost the exact scenario that created the need for this | ||
| // predicate. Since we use `adjustToEndOfPreviousRow`, this fold won't be | ||
| // valid in the below scenario because it'd start and end on row 0. | ||
| buffer.setText(dedent` | ||
| <div | ||
| foo="bar"> | ||
| <span>hello</span> | ||
| <span>world</span> | ||
| </div> | ||
| `); |
There was a problem hiding this comment.
We are apparently missing this code comment in #906, and another code comment in this PR just lower in this file also is missing in #906.
e.g. (no code comment here in #906): https://github.com/pulsar-edit/pulsar/pull/906/files#diff-5b9140812ffc85182b299c6c33ad1e4149bffd4fe33aa22193f98bbad3819cbdR1958-R1965
So in general I'm thinking this PR's stuff may not be 100% verbatim included in #906.
If there's any good stuff still missing with #906 merged, maybe we can get a re-do of just that small diff, either here or in a new PR? And I can try and review it real quick some time today.
There was a problem hiding this comment.
P.S. looks like the diff in other files is 100% included in #906.
Just some comments and some slightly tweaked spec code.
Looks like we're maybe just missing ba650c7 in master branch now?
[EDIT: Yeah, confirmed. This PR's first commit 30eb21c is effectively identical to 9b1e8e5 which ended up being the tip of the branch in #906 perhaps by accident. So only this PR's second and final commit ba650c7 is un-merged to master branch.]
There was a problem hiding this comment.
I can cherry-pick ba650c7 onto a new branch based on master, and post that as its own PR, and just point to this PR and say "we missed a commit" as the description, pretty much? Or you can do so if you like.
Fixes #915.
NOTE: This doesn't need to be part of 1.114, but I'll push to get it reviewed shortly after we ship 1.114 so that I can quickly add some things to the next Tree-sitter PR that will take advantage of these changes.
Description of the Change
This PR adds:
fold.invalidateOnChangethat can be used when a change should automatically invalidate the fold cache for each row in a node's range.folds.scmfiles. (Previously, captures fromfolds.scmdid not consult an instance ofScopeResolverin the processing phase.)I was wrong in #915; the first fold to match for a given start position is the one that wins. (That wrong conclusion was due to my inability to figure out why my edits to the
folds.scminlanguage-htmlwere not having the desired effect, but that turned out to be a combination of (a) not ignoring the@_IGNORE_capture and (b) not having fold caches invalidated properly because of the other issue.)Because we use the first match,
capture.finalandcapture.shyare redundant. They won't do any harm, but there currently aren't any scenarios in which they'd need to be used in order to generate the correct outcome. But it's still a good idea forfolds.scmto be able to use the same custom predicates that all other sorts of query files can use, so I've ensured that we consult aScopeResolverinstance when we perform queries onfolds.scm.The other issue — a fold cache not invalidating the right row in some unusual scenarios — is now possible to fix with a new
fold.invalidateOnChangepredicate. This acts exactly like the existinghighlight.invalidateOnChangepredicate, and should be used wheneverThis is an unusual scenario, just like
highlight.invalidateOnChange, and should therefore rarely be needed. But the new test case depicts the textbook scenario in which we'd need this predicate — hence I'll be adding it to the HTML, JavaScript, and TypeScript TSX grammars once it lands.Alternate Designs
I didn’t consider alternate designs in either case because these problems had already been solved in a specific way for other queries, so there was no reason to reinvent the wheel here.
Possible Drawbacks
fold.invalidateOnChangehas performance implications if used too broadly. Much likehighlight.invalidateOnChange, it should only be used when someone understands exactly what it’ll do and why it needs to be present.Verification Process
Make sure the new specs pass.
Release Notes