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

Custom commands menus #4276

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Custom commands menus #4276

wants to merge 4 commits into from

Conversation

stefanhaller
Copy link
Collaborator

@stefanhaller stefanhaller commented Feb 16, 2025

  • PR Description

I want to be able to configure custom commands that I don't need very often; I don't want these to pollute the global keybindings menu, and I don't want to assign keybindings to them (because there are only so many of these available, and also because I wouldn't be able to remember them, because the commands are not used often). However, I still want to invoke them through keybindings somehow.

I find that the perfect solution for this is to configure a menu that contains custom commands. I can pop open the menu using only one key that I need to remember, but I can access the individual custom commands inside using keys that don't need to be unique with the rest of the global keybindings.

Currently sits on top of #4275 because it needs some of the infrastructure that was add there for validating keys.

Potentially closes #3799.

  • Please check if the PR fulfills these requirements
  • Cheatsheets are up-to-date (run go generate ./...)
  • Code has been formatted (see here)
  • Tests have been added/updated (see here for the integration test guide)
  • Text is internationalised (see here)
  • If a new UserConfig entry was added, make sure it can be hot-reloaded (see here)
  • Docs have been updated if necessary
  • You've read through your own file changes for silly mistakes etc

@stefanhaller stefanhaller added the enhancement New feature or request label Feb 16, 2025
Copy link

codacy-production bot commented Feb 16, 2025

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
+0.03% 93.50%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (0126e72) 52958 45864 86.60%
Head commit (bd085ab) 53075 (+117) 45981 (+117) 86.63% (+0.03%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#4276) 123 115 93.50%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more

Comment on lines +79 to +81
if !enabled {
continue
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we omit commands that are not applicable in the current context. At first I was a bit torn about this, and considered including them with a DisabledReason so that they appear crossed out. However, omitting them is similar to what we do in the global keybindings menu, and actually I do find it important, because it allows us to configure different commands with the same key in different contexts.

@stefanhaller stefanhaller force-pushed the validate-user-config-keybindings branch from e0e726d to 8575494 Compare February 17, 2025 18:32
@stefanhaller stefanhaller force-pushed the validate-user-config-keybindings branch from 8575494 to 0126e72 Compare February 17, 2025 19:53
Copy link
Owner

@jesseduffield jesseduffield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to find a way to do this where we don't need to introduce a new top-level customCommandsMenus key. Often I'll have to look up a custom command by name that I originally gave a key to because I didn't think I'd ever forget what the key was. With the customCommandsMenus approach we're asking the user to know ahead of time whether the key will be frequently used or not and I think that's an unnecessary burden on the user.

Here's what I'm thinking: we have a global keybinding defined to see all custom commands for the current context. Allow the user to omit the key for a custom command (we may actually already do this, I'm not sure) and if the user omits the key for a custom command, then the only way to invoke it is via this menu (or the general keybindings menu)

There's a separate use case we may still want to support, which is having a custom command invoke a menu which itself contains more custom commands. I think this would be cool, but I think we should achieve it by expanding the current interface of custom commands rather than introducing a new top-level thing. This has the additional benefit of supporting arbitrarily long trees of menus.

Thoughts?

@stefanhaller
Copy link
Collaborator Author

Not sure what you mean by "know ahead of time". When I have a command that I find I don't use often, I'll move it into a menu so that it doesn't clutter the global one, and free up its keybinding for better uses; conversely, when there's one in a menu that I find I use so often that it deserves its own keybinding, I'll move it to top level. What's the problem with that?

we have a global keybinding defined to see all custom commands for the current context. Allow the user to omit the key for a custom command (we may actually already do this, I'm not sure) and if the user omits the key for a custom command, then the only way to invoke it is via this menu (or the general keybindings menu)

This could also work, but it doesn't quite address my needs. The commands still clutter up the global menu, which I don't want; and I can't assign local (in-menu) keybindings to those commands, which I find very useful. This is almost "chord" keybindings, which is a great way to make commands efficient to invoke without polluting the global keybinding space. Without that, I would have to press down-arrow lots of times to invoke a command from the menu when there are many.

I find that this example from the PR illustrates this well.

There's a separate use case we may still want to support, which is having a custom command invoke a menu which itself contains more custom commands. I think this would be cool, but I think we should achieve it by expanding the current interface of custom commands rather than introducing a new top-level thing. This has the additional benefit of supporting arbitrarily long trees of menus.

This was my original idea for solving this, instead of adding a new top-level config. Trouble is, I couldn't get it to work with the schema. We would have to make it so that a custom command can either have a key, description, and an array of commands, or a key, description, and all those other keys that custom commands have (command, context, loadingText, stream, etc.). A mixture of those wouldn't be allowed. It's possible to express this in json schema with the right anyOf construct (when crafting a schema by hand), but I don't know how to do it with our method of auto-generating the schema from the go structs.

Also, I don't think the user experience would be great, because if you did mix those keys (e.g. provide a list of commands and a command string), then the error messages from the schema validation would be quite confusing. I think it's clearer for users to provide a separate config for this purpose.

Also, like you I did find the idea attractive that this would allow nested menus, but thinking about this more, I realized that I wouldn't actually use them ever. It's only a conceptual beauty that has no practical relevance.

@stefanhaller
Copy link
Collaborator Author

Alright, I experimented with adding a subCommands field to CustomCommand instead of adding a new top-level key for menus of commands. We wouldn't be able to validate it with the schema properly (as explained above), but we could at least document that users must not use any of the other fields except key and description when they use subCommands, and we could even validate that at load time. I would be fine with this solution if we can get it to work, and it wouldn't be all that different from what I have in this PR.

But I couldn't get this to work either; schema generation fails with a stack overflow, it seems that the jsonschema library can't handle the recursion. Not sure what else to try, I feel the approach taken in this PR is the closest we will get.

@jesseduffield
Copy link
Owner

Interesting. That sounds like a tractable problem: The docs here seem to support recursive references: https://pkg.go.dev/github.com/santhosh-tekuri/jsonschema/v5#section-readme

Also sorry for not giving your prior comment a proper reply yet: I've been thinking about it in the background

@stefanhaller
Copy link
Collaborator Author

That is not the library we are using. We are using https://github.com/karimkhaleel/jsonschema (which is forked from https://github.com/invopop/jsonschema).

Sure, the jsonschema format itself supports recursive references, but the library we are using for generating the schema from the go structs doesn't seem to.

Unless I'm missing something; @karimkhaleel could you have a look? I pushed my experiments in the branch custom-commands-submenus on this repo. It seems to work well and tests are green, but I can't generate the schema; run go generate ./... to see the stack overflow.

@stefanhaller
Copy link
Collaborator Author

Update: the stack overflow when generating the schema is caused by the schema generator trying to inline all sub schemas, which of course doesn't work when there's recursion. This, by itself, can be fixed by setting the DoNotReference option to false here. We only used that option because I found the schema easier to read when everything is inlined, which is not a compelling reason because nobody reads the schema.

However, if we do this, then our custom code to add default values to the schema no longer works, and neither does the script to generate Config.md from the schema. So both of these scripts would need some work, and I have no idea how much effort that would need. I probably won't have time to look into this myself this week.

@karimkhaleel
Copy link
Contributor

I'll take a look.

@stefanhaller stefanhaller force-pushed the validate-user-config-keybindings branch from 0126e72 to 5979b40 Compare February 21, 2025 12:22
Base automatically changed from validate-user-config-keybindings to master February 21, 2025 12:24
Copy link

codacy-production bot commented Feb 21, 2025

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
Report missing for 16f53481 93.50%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (16f5348) Report Missing Report Missing Report Missing
Head commit (835b58c) 53206 46102 86.65%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#4276) 123 115 93.50%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more

Footnotes

  1. Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct.

This was backwards; we renamed Sha to Hash, so Sha is now deprecated, not Hash.
It has been 'e' instead of 'o' for quite a while now.
We'll reuse it in the next commit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom command and custom command
3 participants