Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

theme keys static and documented #3690

Conversation

poliorcetics
Copy link
Contributor

This PR adds a ThemeKey type that contains as much static scopes as possible for Tree Sitter
highlight queries.

It also adds a new generated file for the book which fully documents the highlight scopes and a
new cargo xtask subcommand to ensure all of the default highlight queries are in the static
scopes and will not fall in ThemeKey::OtherQuery.

Closes #3649

@poliorcetics
Copy link
Contributor Author

@the-mikedavis I didn't add the theme checking but having a single source and always up to date source of truth for theme keys should make it much easier to know if a theme is up to date for maintainers, even with a manual workflow for now.

I added all the rest I think, and the resulting documentation is also more searchable IMO since I can now look for keyword.<something> whereas that was not possible before. because it used a manual tree structure.

@poliorcetics poliorcetics force-pushed the theme-keys-static-and-documented branch 2 times, most recently from fd1ad22 to 51a423d Compare September 4, 2022 15:11
@poliorcetics poliorcetics force-pushed the theme-keys-static-and-documented branch 2 times, most recently from eb2aafc to 4a484db Compare September 6, 2022 08:30
@kirawi kirawi added A-documentation Area: Documentation improvements S-waiting-on-review Status: Awaiting review from a maintainer. labels Sep 13, 2022
helix-view/src/theme.rs Outdated Show resolved Hide resolved
@the-mikedavis
Copy link
Member

I preferred seeing keys as "scope.scope.scope" rather than "ScopeScopeScope" - the fallback behavior was more intuitive. Is there a different way of approaching this, maybe by using a macro when getting a key from a theme?

@poliorcetics
Copy link
Contributor Author

I preferred seeing keys as "scope.scope.scope" rather than "ScopeScopeScope" - the fallback behavior was more intuitive. Is there a different way of approaching this, maybe by using a macro when getting a key from a theme?

I could do Ui_Menu instead of UiMenu, that would make it much easier to read with a separator. It's not as Rust-y but it's a generated enum with dozens of variants so 🤷

@kirawi
Copy link
Member

kirawi commented Sep 16, 2022

We could make use of namespacing.

@poliorcetics
Copy link
Contributor Author

We could make use of namespacing.

I can try generating something like ui::Menu, and Ui constants yes, with the enum as backing type, good idea. I'll see what I can do

@poliorcetics
Copy link
Contributor Author

We could make use of namespacing.

Well it's very hard in macro_rules and I'm not willing to introduce such a proc macro for just this, the complexity and compile time would suffer a lot. Nested elements in macros are always hard, both with types and modules. We could maintain a manual mapping of modules to the keys used in helix, or I can make another macro_rules.

@the-mikedavis, what do you think about Ui_Menu ? From an implementation POV, it's very easy, and the separator is there. I can make everything lowercase too, like ui_menu and wrap the whole thing in something like this:

pub type tk = ThemeKeys;

That way the usage point becomes tk::ui_menu, and the import is simply use helix_view::theme::tk;

@poliorcetics poliorcetics force-pushed the theme-keys-static-and-documented branch 2 times, most recently from 5f4e530 to 84729fd Compare September 20, 2022 07:55
@poliorcetics

This comment was marked as resolved.

@poliorcetics
Copy link
Contributor Author

I moved the names to Ui_Linenr_Selected format, to make it easier to read and rebased on most recent master

@poliorcetics poliorcetics force-pushed the theme-keys-static-and-documented branch 2 times, most recently from fda58fb to 2070048 Compare September 20, 2022 21:42
@poliorcetics poliorcetics force-pushed the theme-keys-static-and-documented branch 3 times, most recently from 3e9a55d to 736fee5 Compare October 8, 2022 13:00
@poliorcetics poliorcetics force-pushed the theme-keys-static-and-documented branch from 736fee5 to 2cb8fe4 Compare October 13, 2022 07:21
@poliorcetics poliorcetics force-pushed the theme-keys-static-and-documented branch from 2cb8fe4 to f5623d6 Compare October 20, 2022 20:37
@poliorcetics
Copy link
Contributor Author

I don't think CI failures are related to my changes

@poliorcetics poliorcetics force-pushed the theme-keys-static-and-documented branch 2 times, most recently from cdd7498 to 7375b81 Compare November 3, 2022 08:52
@poliorcetics
Copy link
Contributor Author

What do I have to do for this to move forward ? IMO it would greatly improve documentation for theme keys, and avoid allocating strings in several places

@archseer
Copy link
Member

archseer commented Nov 3, 2022

avoid allocating strings in several places

Are these allocations that costly? From what I can tell, we use static strings everywhere apart from parsing the theme file.


This might make sense for ui. but I'm not a huge fan of hardcoding the syntax scopes (even with the OtherScope escape hatch), it's a huge macro that's essentially a string interner just so we can use the syntax scopes in a couple of spots in the UI via get/try_get. When rendering we use raw indexes anyway.

The query validation could then be done externally: a file could essentially contain a list very similar to the macro input. We'd then use that to generate the documentation tree. A lint script could then cut the first column of known values, rg @\w+ (dropping any that start with @_) and cross reference against that list.

@poliorcetics
Copy link
Contributor Author

poliorcetics commented Nov 5, 2022

The documentation benefits to know what is used in themes and what is not are nice and not to be dismissed.

For example, this PR surfaces several duplications between sshconfig and the rest, e.g. constant.builtin.boolean vs boolean (only used in sshconfig). I have also caught people unnecessarily introducing new scopes when existing ones were already used elsewhere.

Theming is very important to users, having a list of all keys, documented (which they would not be with a parser from the runtime queries), allows trimming down that list on our side (making theme definition easier) and correctly define new themes on the user's side.

Ideally we would even document which queries are used in which language, but that would probably make the table too big for now so I left it for later, once I have a better idea for presentation.

@poliorcetics poliorcetics force-pushed the theme-keys-static-and-documented branch from 7375b81 to 48c6140 Compare November 22, 2022 11:05
@poliorcetics poliorcetics force-pushed the theme-keys-static-and-documented branch 2 times, most recently from 2e6302f to 222e76c Compare December 2, 2022 13:56
@poliorcetics poliorcetics force-pushed the theme-keys-static-and-documented branch 3 times, most recently from 7430701 to 85f73f0 Compare December 17, 2022 21:39
@poliorcetics
Copy link
Contributor Author

poliorcetics commented Dec 17, 2022

@archseer @the-mikedavis I completely changed the way this is done.

  1. The big macro is now only compiled when compiling the xtask crate, which means regular helix compilation won't pay the cost
  2. I removed the big enum, leaving the &str in the code as currently in master.
  3. I kept the check and documentation to ensure languages don't introduce similar-but-not-the-same keys in their highlight queries, if you look at the first commit you'll see that master has several such instances.

@poliorcetics poliorcetics force-pushed the theme-keys-static-and-documented branch 2 times, most recently from 84a658f to 6df75fb Compare December 26, 2022 16:56
@archseer archseer added this to the next milestone Jan 6, 2023
@poliorcetics poliorcetics force-pushed the theme-keys-static-and-documented branch from 6df75fb to a8894f6 Compare January 9, 2023 22:26
xtask/src/themekeyscheck.rs Show resolved Hide resolved
xtask/src/themekeyscheck.rs Outdated Show resolved Hide resolved
xtask/src/themekeyscheck.rs Show resolved Hide resolved
@poliorcetics poliorcetics force-pushed the theme-keys-static-and-documented branch from a8894f6 to 71cc54d Compare February 25, 2023 10:35
@poliorcetics poliorcetics force-pushed the theme-keys-static-and-documented branch from 71cc54d to 096b8b7 Compare March 10, 2023 23:23
@poliorcetics
Copy link
Contributor Author

Updated to latest master, this PR is no fun to maintain 😓

@poliorcetics poliorcetics force-pushed the theme-keys-static-and-documented branch from 096b8b7 to 00093ef Compare March 11, 2023 09:35
@pascalkuthe pascalkuthe modified the milestones: 23.03, next Mar 27, 2023
@archseer
Copy link
Member

archseer commented Apr 7, 2023

Hmm why were these closed? We were going to merge this in the next cycle

@poliorcetics poliorcetics deleted the theme-keys-static-and-documented branch September 14, 2023 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-documentation Area: Documentation improvements S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Keep all theme keys documented in one place
6 participants