Validate user config keybindings

[stefanhaller]

• PR Description

This improves the user experience when users try to use an invalid key name in their config, either for one of our standard keybindings or for the key of a custom command.

Fixes half of #4256 (only the keybindings aspect of it, not the context names).

• 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

[codacy-production]

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
Report missing for 52b1c421	✅ 95.65%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (52b1c42)	Report Missing	Report Missing	Report Missing
Head commit (5979b40)	53089	45986	86.62%

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 (#4275)	69	66	95.65%

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. ↩

[stefanhaller]

Unfortunately we can't i18n-ize these, because the config is read very early, before the translation system is initialized. And it is important that we validate the config right after loading, so that all clients of it can just use it without having to worry about valid data.

[ChrisMcD1]

Unrelated to the body of this work, but why are <c-1> and others not considered valid keys? I see them in the comments up above, but don't know what that means.

[stefanhaller]

This comes from commit 2e66d87. The goal of that commit was to ensure that the keys you see in the global ? menu are exactly what you put in your config file. And the approach it took to ensure this was to have two inverse maps for labelByKey and keyByLabel. The problem with this approach is that there are several labels that map to the same key (e.g. backspace and ctrl-h both map to ascii 8), so you can't put them both in the labelByKey map. We also have #2953 about this.

It would be nice to fix this, and I suppose the way to do that is to manually maintain two separate maps for labelToKey and keyToLabel, where labelToKey can have more entries. And then we need to add a field to the types.Binding struct that stores the original string so that we can show it in the menu. This would still not allow to bind one action to backspace and another to ctrl-h, but it would at least allow binding things to ctrl-h at all (or ctrl-3). This should really work since those are the actual keys that a user presses to invoke the action. (Although I'm not sure about ctrl-3; that's the same as Esc, and this could be confusing.)

[ChrisMcD1]

Would adding the failed node, or value to this panic message be helpful? I'm not sure how possible it is for somebody to sneak other value types inside of here via wacky config.

[stefanhaller]

A wacky config can never cause this panic. We're iterating over the compile-time types of the KeybindingConfig fields here, so the panic can only be seen by a developer who adds something to the KeybindingConfig struct that we don't know how to iterate over (e.g. a map).

But adding the path to the error message could still be helpful for that developer, and it's easy, so why not: 8575494

[ChrisMcD1]

I don't believe that we have an equivalent to the path variable that we can use to point them to where in their config they messed up, but maybe using the CustomCommand.Command, or the index in the slice could be helpful to somebody.

Particularly because we do string.ToLower what they send to use, so if someone creates a custom command with key <CC-4>, and then we give them an error message about <cc-4>, and they do a case sensitive search for where that is in their config. Might be confusing.

That example is starting to seem a bit contrived though....

[stefanhaller]

I briefly thought about adding the command to the error message, but commands can sometimes be very long, so this could be unwieldy. Also, I don't follow your concern about the case sensitive search; the tolower happens inside of isValidKeybindingKey, so the error message contains the actual key in its original case. A case sensitive search will find it in the config file, and I think this is good enough.

[ChrisMcD1]

The highlighting of the line of code leads to some confusion on that 2nd part.

We do strings.ToLower(key) before populating the %s.

[ChrisMcD1]

And if a valid concern, also applies higher up in the file on the other error message.

[stefanhaller]

Oh right, I totally missed that. It doesn't make sense to lower-case there, removing in 9fd487b and 0126e72.

(I had copied this from here without thinking; it doesn't make sense there either, but I'm not bothering to change it there, as the fatal should never trigger any more now.)

[jesseduffield]

Code LGTM but I'll let @ChrisMcD1 finish his review

BTW @ChrisMcD1 it's great to have you onboard.

[ChrisMcD1]

Works well when I tested locally!