-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Configurable keys 2 (Mapping keys to commands) #268
Configurable keys 2 (Mapping keys to commands) #268
Conversation
Seemed like a good temporary solution until we have plugin or more sophisticated approach. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
(Aside from needing to resolve the merge conflict first, of course.)
* Back Tab => "backtab" | ||
* Delete => "del" | ||
* Insert => "ins" | ||
* Null => "null" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not exactly sure what this is, I'm just transcribing the underlying Crossterm KeyEvent
API as well as I can. I think this is a way of expressing "no key".
* Page Up => "pageup" | ||
* Page Down => "pagedown" | ||
* Tab => "tab" | ||
* Back Tab => "backtab" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Back tab?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are valid key codes, originating back to early IBM computers ¯_(ツ)_/¯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about focus in and focus out available in kakoune?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh... I didn't even know focus in/out was information you could get inside a terminal. Relevant link:
https://unix.stackexchange.com/a/480138
That would be pretty cool to be able to bind to commands. Then you could auto-save all files on focus-out, for example.
(Although, long-term a proper auto-save feature will be better for that, of course.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't seem like Crossterm offers primitives for that, but it does sound interesting! Probably outside of the scope of this PR though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I don't think it's widely supported or anything, just interesting trivia about old features :)
pub struct Config { | ||
pub keymaps: Keymaps, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use a custom serde::Deserialize
for this rather than hand-roll our own? We could use a custom keymap
deserializer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides the nits I have, my concern is that we should use a custom serde Deserializer
rather than hand-rolling our own parser and validator after deserializing from serde.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job!
Probably out of scope for this PR, but I wonder if we want to keep the distinction between "typable command" and … "command" in the long term.
It makes sense to be able to manually type some of them, and even move commands wouldn't hurt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me take a stab at custom serde deserializer separately.
@pickfire Happy with that, thanks! |
Great work! |
This time remapping goes from Keys to Commands.
This required some refactoring of commands, so they have more structure (namely, they're a thin struct around the command function, now exposed via an
execute
method, and a string name).Made all the former command functions private, which are now reachable through associated constants in the Command impl itself. The macro that declares these commands also instantiates a static list of them, for cases where iterating over them is desirable.
Naturally, commands from plugins won't be easy to add to this array (unless we bring in something like the
inventory
crate) but it's not a problem as it doesn't have to be comprehensive: We'd just have to update the FromStr and Display methods so they take into account commands that are defined elsewhere.Another difference from the last PR is that now keys are parsed/printed in a way similar to how Kakoune does it, as proposed by @pickfire . There are very slight differences down to the primitives that
crossterm
exposes, but they shouldn't be a big difference in practice.