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

Configurable keys 2 (Mapping keys to commands) #268

Merged
merged 24 commits into from
Jun 17, 2021

Conversation

PabloMansanet
Copy link
Contributor

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.

@PabloMansanet PabloMansanet changed the title Configurable keys 2: Electric Boogaloo (Mapping keys to commands) Configurable keys 2 (Mapping keys to commands) Jun 14, 2021
book/src/remapping.md Outdated Show resolved Hide resolved
@pickfire
Copy link
Contributor

Seemed like a good temporary solution until we have plugin or more sophisticated approach.

@cessen cessen self-requested a review June 15, 2021 05:14
Copy link
Contributor

@cessen cessen left a 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.)

book/src/remapping.md Outdated Show resolved Hide resolved
* Back Tab => "backtab"
* Delete => "del"
* Insert => "ins"
* Null => "null"
Copy link
Contributor

Choose a reason for hiding this comment

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

Null?

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"
Copy link
Contributor

Choose a reason for hiding this comment

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

Back tab?

Copy link
Member

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 ¯_(ツ)_/¯

Copy link
Contributor

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?

Copy link
Contributor

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

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.

Copy link
Member

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 :)

helix-term/src/keymap.rs Outdated Show resolved Hide resolved
Comment on lines +9 to +11
pub struct Config {
pub keymaps: Keymaps,
}
Copy link
Contributor

@pickfire pickfire Jun 15, 2021

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.

Copy link
Contributor

@pickfire pickfire left a 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.

Copy link
Member

@CBenoit CBenoit left a 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.

book/src/remapping.md Outdated Show resolved Hide resolved
helix-term/src/keymap.rs Outdated Show resolved Hide resolved
helix-term/src/config.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@pickfire pickfire left a 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.

@PabloMansanet
Copy link
Contributor Author

@pickfire Happy with that, thanks!

@archseer archseer merged commit f7e00cf into helix-editor:master Jun 17, 2021
@archseer
Copy link
Member

Great work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants