savetheclocktower
commented
8 months ago I needed to revisit some changes I’d made to the grammar-selector package and wanted to explain them just to make sure we’re everybody’s cool with them.
When the old Tree-sitter setting (core.useExperimentalTreeSitter) was replaced with the new setting (core.useLegacyTreeSitter), I tried to invert how that logic affected grammar-selector. In effect, though, I made it so that legacy Tree-sitter grammars rarely got shown at all unless the user had them enabled globally.
I’d also been telling users that they could opt into a legacy Tree-sitter grammar on a language-specific basis, but we didn’t have enough testing around this, and this wasn’t working in most cases because we weren’t even trying to load the legacy Tree-sitter grammars unless core.useLegacyTreeSitter was enabled globally. (That’s now fixed in this PR as well.)
So let me show some screenshots to explain the before-and-after here.
Before: showing only one grammar by default
The grammar-selector.hideDuplicateTextMateGrammars setting made it so that the grammar selector showed only one grammar per language, and it was the Tree-sitter grammar if that grammar existed. Otherwise it was the TextMate grammar.
You can see that the Tree-sitter grammars have a badge marking them as such — even though there’s no real reason to show that badge when this setting is enabled, because it’s not needed to distinguish the grammar from anything.
After: showing only one grammar by default
The big change in this PR is that grammar-selector now respects your scope-specific settings. Suppose you’ve still got modern Tree-sitter grammars enabled globally, but you’ve added this block to your config:
".source.c":
core:
useLegacyTreeSitter: trueWhen compiling a list of grammars to show you, grammar-selector will now order them in terms of your stated preferences. If hideDuplicateTextMateGrammars is checked, only the one you’ve opted into will be shown.
So in this list…
- the “C” entry in the list will refer to the legacy Tree-sitter grammar, and
- all other entries in the list will refer to the modern Tree-sitter grammar (if it exists) or the TextMate grammar.
The other big change here is that there are no badges. Since we’re showing only one grammar for each language, the user can infer that it’s the entry for the grammar they’ve opted into, either globally or scope-specifically.
And since we have three types of grammars now instead of just two, I think we’re past due to rename the hideDuplicateTextMateGrammars setting. Except we won’t rename the setting itself — too much hassle! — but will instead give it a simpler name in the settings: “Hide Duplicate Grammars.”
Before: showing all grammars
When hideDuplicateTextMateGrammars is unchecked right now, we’re doing something that I initially thought was a good idea — but I’ve now changed my mind.
You can see here that the grammar selector shows two C grammars. But we have three such grammars, don’t we? Except that the legacy Tree-sitter grammar is not being shown.
I think my rationale was that this flipped the logic we had before — when modern Tree-sitter grammars were opt-in, we didn’t show them to you unless you told us you wanted to use them. So now that legacy Tree-sitter is opt-in, we should hide them unless you tell us you want to use them, right?
Well, no. I no longer feel this way. “Experimental” and “deprecated” are not equivalent in this comparison. And if someone reports a bug with a modern Tree-sitter grammar, I want to be able to tell them to uncheck hideDuplicateTextMateGrammars, choose a different grammar type in the selector, and show me whether the problem exists in that grammar.
After: showing all grammars
In this PR, if a user says they want to see all grammars, we’ll show them all grammars:
Right now, that means two different kinds of Tree-sitter grammars alongside the TextMate grammars, ordered the way the user has indicated through their preferences.
The screenshot above doesn’t show this, but if I had opted into the legacy Tree-sitter grammar for C files with the configuration snippet above, then that grammar would be listed first out of the three C grammars in the list.
The change I made above means grammar-selector hardly cares about the user's global values for core.useTreeSitterParsers and core.useLegacyTreeSitter, but this screenshot illustrates the one place it still matters. If you've got core.useLegacyTreeSitter enabled globally, then the “Legacy Tree-sitter” badge will be green (i.e., your UI theme’s “success” color), and the “Modern Tree-sitter” badge will be yellow/orange (i.e., your UI theme's “warning” color). When it's disabled, the colors are inverted. I felt like it was good to be consistent with this choice across all items in the list rather than change colors based on the user's preference for a particular language.
When we get rid of legacy Tree-sitter grammars for good, this logic will be a bit simpler.
Takeaway
Since the default value for hideDuplicateTextMateGrammars is true, here’s what most users will notice:
- The grammar selector list won’t have any badges that say “Tree-sitter” anymore.
If anyone thinks this is a problem, or has concerns about other decisions I’ve made here, I’m happy to discuss them.
mauricioszabo
A bit late getting this one in! It's not that we're all good and there's nothing to fix — just that I've had a busy couple of weeks. More to come.
Changelog entries
grammar-selectorpackage so that it will show the user's preferred grammar for a specific language.0.20.9ofweb-tree-sitter. (Unsure if this is worth mentioning in the changelog; I'll leave it up to others.)