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

Add unbind-default-keys config option #2733

Closed
wants to merge 2 commits into from

Conversation

Lindenk
Copy link

@Lindenk Lindenk commented Jun 10, 2022

Closes #2720.

There were a few changes I made to reduce confusion with some function/method locations and naming:

  • helix_term::keymap::merge_keys was moved to helix_term::config::Config::merge_keys as a method because it only operates on and changes Config, and was modified to take a set of bindings to merge with itself. New functions were also added to handle the new optional behavior.
  • Config::load now is the single source of truth for loading and merging configs, which was previously also handled in helix_term::application::new in a slightly different but mostly redundant manner.
  • Default is removed from Config and replaced with Config::new and Config::with_default_keys. new returns a Config with no bindings while with_default_keys returns a Config with the default bindings (which is the same behavior as the old default). This to prevent confusion between a default rust struct and helix's default keybindings as these terms were previously conflated.
  • Config::keys no longer loads default bindings through serde when no keys are found as this was always redundant.
  • helix_term::keymap::Keymaps::get no longer crashes when a mode isn't found. This will also help with future mode additions (likely through plugins down the line).

Let me know if any of these changes should be handled differently

EDIT: After updating to a much newer version of helix, this PR only adds the new option unbind_default_keys which does not populate the initial keymap at all. This also includes the fix where Helix would crash if a keybind for a non-existent mode is triggered

@the-mikedavis
Copy link
Member

Could you add some notes on using this to the docs? https://github.com/helix-editor/helix/tree/f37ffaa3a1c7754238029390db00148ec8105874/book/src

@Lindenk
Copy link
Author

Lindenk commented Jun 10, 2022

No problem. If you'd like me to include a new subsection or larger blurb, or if there's a better place to provide this specific documentation, let me know

Copy link
Member

@sudormrfbin sudormrfbin left a comment

Choose a reason for hiding this comment

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

How about renaming the config option to unbind_default_keys ?

helix-term/src/config.rs Outdated Show resolved Hide resolved
book/src/remapping.md Outdated Show resolved Hide resolved
helix-term/src/application.rs Outdated Show resolved Hide resolved
helix-term/src/config.rs Outdated Show resolved Hide resolved
helix-term/src/config.rs Outdated Show resolved Hide resolved
helix-term/src/config.rs Outdated Show resolved Hide resolved
helix-term/src/config.rs Outdated Show resolved Hide resolved
@Lindenk Lindenk requested a review from sudormrfbin June 30, 2022 05:02
@kirawi kirawi added A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer. labels Sep 13, 2022
@the-mikedavis the-mikedavis changed the title Added unbind_defaults config option Add unbind-default-keys config option Oct 15, 2022
helix-term/src/keymap.rs 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
book/src/remapping.md Outdated Show resolved Hide resolved
@the-mikedavis the-mikedavis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Oct 16, 2022
@patrick-kidger
Copy link

Is there any movement on this? I'd really like to try out this feature.

@pascalkuthe pascalkuthe added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 1, 2023
@Lindenk
Copy link
Author

Lindenk commented May 4, 2023

I updated the feature to the current version of helix and addressed the above changes. Now it only adds the config option unbind_default_keys

@Lindenk Lindenk requested a review from the-mikedavis May 4, 2023 02:00
@the-mikedavis the-mikedavis added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. labels May 4, 2023
omentic added a commit to omentic/helix that referenced this pull request Jul 16, 2023
omentic added a commit to omentic/helix-ext that referenced this pull request Nov 1, 2023
ref: helix-editor/helix#2720
ref: helix-editor/helix#2733

Co-authored-by: Linden Krouse <ztaticnull@gmail.com>
postsolar added a commit to postsolar/helix that referenced this pull request Jan 19, 2024
postsolar added a commit to postsolar/helix that referenced this pull request Jan 19, 2024
* Add `unbind_default_keys` config option

Based on helix-editor#2733

* Fix lint issues
postsolar added a commit to postsolar/helix that referenced this pull request Apr 4, 2024
postsolar added a commit to postsolar/helix that referenced this pull request Apr 4, 2024
postsolar added a commit to postsolar/helix that referenced this pull request Apr 6, 2024
postsolar added a commit to postsolar/helix that referenced this pull request Apr 11, 2024
Comment on lines +105 to +109
let mut keys;
match config.unbind_default_keys {
true => keys = HashMap::default(),
false => keys = keymap::default(),
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let mut keys;
match config.unbind_default_keys {
true => keys = HashMap::default(),
false => keys = keymap::default(),
}
let mut keys = match config.unbind_default_keys {
true => HashMap::default(),
false => keymap::default(),
};

Comment on lines +71 to +75
let mut keys;
match local.unbind_default_keys {
true => keys = HashMap::default(),
false => keys = keymap::default(),
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let mut keys;
match local.unbind_default_keys {
true => keys = HashMap::default(),
false => keys = keymap::default(),
}
let mut keys = match local.unbind_default_keys {
true => HashMap::default(),
false => keymap::default(),
};

Comment on lines +363 to +366
let keymap = match keymaps.get(&mode) {
Some(keymap) => keymap,
None => return KeymapResult::NotFound,
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let keymap = match keymaps.get(&mode) {
Some(keymap) => keymap,
None => return KeymapResult::NotFound,
};
let Some(keymap) = keymaps.get(&mode) else { return KeymapResult::NotFound; };

This is the style used elsewhere in the code.

@the-mikedavis
Copy link
Member

#9384 (comment), #2720 (comment) - we want to solve this when we switch config away from TOML rather than adding a new config option now

postsolar added a commit to postsolar/helix that referenced this pull request Apr 20, 2024
omentic pushed a commit to omentic/helix-ext that referenced this pull request May 1, 2024
omentic pushed a commit to omentic/helix-ext that referenced this pull request May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unbind all default keybindings config option
7 participants