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

Keymap presentation refinements and underlying restructuring #5635

Closed
wants to merge 113 commits into from

Conversation

gibbz00
Copy link
Contributor

@gibbz00 gibbz00 commented Jan 22, 2023

Implemented the feedback I got from #5565.

This PR only concerns the keymap. It cleans up most of the keymap code in order to make it more readable and maintainable. (Some bugs were discovered and fixed from this alone.) The refactor also made it possible for clean implementations of a myriad of keymap feature requests.

Keymap infobox:

Sorting

  • Rewrote the predefined infobox order such that no sort is required for each time infobox is opened.
    (KeytrieNodes are stored in a Vec rather than a HashMap.)

  • Keymap infobox can optionally be sorted with a sorted-infobox editor config option.
    Sorting is done by modifier, then by KeyCode category, then by each KeyEvent.
    Single character alphas are placed before the rest (ex. "space") in KeyCode::Char category.

  • Key events per command are sorted by length, unless it's a "C-" key event. (Should not be a regression of Implement key ordering for info box #952)

Standard config

Before After After with sorted-infobox = true
image 2023-02-14-173700_screenshot 2023-02-14-173731_screenshot

Personal

Before After After with sorted-infobox = true
2023-02-14-173910_screenshot 2023-02-14-180244_screenshot 2023-02-14-174003_screenshot

Custom labels

Extends the the work of #5203, #3958, and #4164.

[keys.normal.space.c]
# Infobox title
description = "Sort menu"
sticky = true
# Description of command sequence (multiple commands)
C-s = { description = "Sort selection", exec = ["split_selection_on_newline", ":sort", "collapse_selection", "keep_primary_selection"]}
# When no description is supplied
A-s = ["split_selection_on_newline", ":sort", "collapse_selection", "keep_primary_selection"]

2023-02-14-175143_screenshot

One of the limitations to the custom descriptions is that the key-value pair must be defined first thing in each TOML map.

Command palette:

  • Uses the ui table widget

  • Does not show all commands bound to no_op in config.

  • Removed a lot of character clutter for showing key-events bound to each command.

Before After
image 2023-01-22-174716_screenshot

Future work

It would probably be wise to visit the merged PRs to check which issues can now be closed.

Custom user commands appear in command palette

Typeable command binding such as C-k = ":open ~/.config/helix/config.toml" currently loses all its context
in the command palette. There it's currently shown as:
open | C-k | Open file" in the command palette
Which I believe could be improved to:
Custom | C-k | :open ~/.config/helix/config.toml

Furthermore, it would "be nice" for customized commands to also be included in the command palette.
Scenarios would be:

  • Typable command binding with description: Custom | C-k | Open config
  • Command Sequence with description: Custom | . | Goto last modification and view in center
  • Command Sequence with no description: Custom | . | goto_last_modification → align_view_center

I began implementing this functionality but gave up as a cleaner implementation would leverage refactorings of commands.rs, which definitely drop the chances of this PR getting merged down to 0. So I'll leave that for a later date.

I can open up an issue and move this part there if you would deem it appropriate.

Conflicting PRs (16)

#5435 #5523 #5577 #5621 #5645 #5660 #5748 #5839 #6056 #6059 #6061 #6065 #6067 #4443 #2869 #3393
* That are mergeable with master and pass all CI checks as of 2023-02-22

bbodi and others added 30 commits October 9, 2022 17:33
Co-authored-by: Michael Davis <mcarsondavis@gmail.com>
Was not implemented correctly as the order was
in the most cases assigned from the values of
an HashMap, which does not guarantee order.
This applied to when a keymap was both
deserialized and merged.

Info box body was then being sorted as a
function of the fallacious `order` property,
and by a method that yielded at worst a time
complexity of at least n^2.

Furthermore, the body contents were inerted
at first by the order of the hash map keys,
in which `order` itself was based on.

A more reliable predifined sorting order is to be
implemented.
Exist under the wrong (possibly just outdated)
assumption that command descriptions are written
with their KeyTrie name prefixed (Space, View,
Goto etc.). For examle: The command `file_picker`
is assumed to have the description
"Space Open file picker", which is not the case
, nor for any other command description.
BTree was being used to store a list of keyevents
for a given command. This list was only iterated
over twice to in the end be converted to a Vec.

Better to just use a Vec from start given the use-
case. Temporalily reverts helix-editor#952.
Infobox body was being filled with description
then KeyEvent list to only later be iterated over
for the purpose of flippingin this order.
Vec<KeyEvent> was being created as an
intermediary into Info.rs for it to be
converted into a comma separated string.

This commit removes this intermediate step
but also the single purpose from_keymap
public function in Info.rs, as it is no
longer needed.
* Descriptive naming and correct use of data structure terminology
  example translations:
    keymap.reverse_map -> keymap.command_list
    keytrienode -> keytrie (struct)
      search->traverse
      merge->merge_keytrie
    keytrie -> keytrienode (enum describing node variants)
      Leaf -> MappableCommand
      Sequence -> CommandSequence
      Node -> KeyTrie
      (A leaf is a node, and the sequence was also a node.
	  So old naming made absolutely no sense whatsoever.)
* Splitting parts of the keymap.rs file into
  {keytrienode, keytrie, keymaps, tests}.rs.
  (Should be self-explanatory by looking at the old keymap.rs.)
* Removed KeytrieNode enum functions node(&self) -> Option<&KeyTrie>
  and node_mut(&mut self) -> Option<&mut KeyTrie> from KeyTrieNode
  as they served no purpose that could not be used from elsewhere.
  Was also a bit strange to return a "parent struct" from an enum
  "building block".
* Removed getters that could be achieved by making fields public for
  now. (E.g .root(), .map(), .name())
* Removed keymap.merge() and keytrienode.merge_nodes()
  All merging could be handled by keytrie.merge() and
  keymaps.merge_with_default(), unnecessary to have a second
  unused system that does about the same thing.
  We also don't want functions that can cause panics as
  merge_nodes() could.
* Initial simplification and formatting of command palette.
  Formatting is done during the creation of the command lists,
  removes the need for the creation and and traversal of an
  intermediary Vec<Vec<String>>.
  Keyevent formatting changed from "(<space>w<C-q>)" to ["space>w>C-q"].
  Clarifying how commands are triggered by moving through
  submenus/keytries.
For it not to be confused with the upcoming ":help"
feature.
This, along with previous commit reverts regression in helix-editor#952
Sort duration calculations averaged to about
0.04 ms when opening the larger infoboxes.
* Elaborated on test descriptions
* Removed duplicate unit test
* Unified keytrie traversal in tests
* Moved config merging Keymaps to Config
* Removed unused default EditorView function
It produces, after all, a Keytrie. It would
be a bit like saying: let engine = car!()
I had introduced a bug in which it wasn't possible to override
mappable commands with keytries. Test has been added to catch this miss.
@@ -43,31 +40,38 @@ impl Display for ConfigLoadError {
}

impl Config {
pub fn load(config_path: PathBuf) -> Result<Config, ConfigLoadError> {
match std::fs::read_to_string(config_path) {
// REFACTOR? code similar to config assignment in main.rs,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in #5636

...in the hopes that it simplifies review.
The changes should have probably be codified into
`cargo fmt`.
Replace two `.expect()` with returning Err.
Using the let else pattern for this. Only became
stable in 1.65.0, hence the bump.
@gibbz00
Copy link
Contributor Author

gibbz00 commented Feb 15, 2023

@the-mikedavis @archseer PR feels ready for a review <3

@archseer
Copy link
Member

Commits in this branch now seem duplicated. I see "Keymap infobox: Use Vec in place of BTree" appearing twice for example.

@GloopShlugger
Copy link

I've been testing it and noticed setting a description/label for a menu like spacemode gets overridden by the default keymap, there have been no issues with sticky

Maybe out of scope but would it be possible to have an ordering method based on toml config order?

Great work

@gibbz00
Copy link
Contributor Author

gibbz00 commented Feb 16, 2023

Commits in this branch now seem duplicated. I see "Keymap infobox: Use Vec in place of BTree" appearing twice for example.

I'm not sure how that happened :/ only saw that now. Suspect it happened in conjunction when cherry-picking commits from last PR and rebasing with master. Anyway I can fix that or is it ok if it is let be?

* Allow override of predifinde infobox descriptions.
* Make sure that custom description overrider on infoboxes can be done
without additional remappings of the same keytrie.
@gibbz00
Copy link
Contributor Author

gibbz00 commented Feb 17, 2023

I've been testing it and noticed setting a description/label for a menu like spacemode gets overridden by the default keymap, there have been no issues with sticky

Thanks for letting me now! I added some failing tests for that,, and fixed them.

Ordering method based on toml config order is unfortunately out of my reach and interest frankly. One would still have to live with them not being ordered together with the pre-defined ordering, unless one rewrites the entire default keymap in a config.toml... Hence why I just prefer the automatic sorting. Also makes it very easy to spot which keys aren't mapped.

@GloopShlugger
Copy link

Ordering method based on toml config order is unfortunately out of my reach and interest frankly. One would still have to live with them not being ordered together with the pre-defined ordering, unless one rewrites the entire default keymap in a config.toml... Hence why I just prefer the automatic sorting. Also makes it very easy to spot which keys aren't mapped.

Rewriting the entire keymap is exactly my use-case and only just noticed in that case its aleady toml order (after setting sorted-infobox to false) except for when you override default keys

Would it make sense for future proofing sake to change the sorted-infobox config key from boolean to something like by_key, so future PR's can implement different behaviors? I do see this as a fairly menial point and would understand to just keep it as is

Because of that behavior once something like default-keymap = false lands it should be solved, for all the cases where the entire keymap is replaced

@gibbz00
Copy link
Contributor Author

gibbz00 commented Feb 17, 2023

I can look into it when time allows!!

Curious though, how come you want to rewrite the entire keymap?

@GloopShlugger
Copy link

Curious though, how come you want to rewrite the entire keymap?

i like having similar functions clustered all around the homerow
things like next word previous word next to each other rather than semantically or the letter they begin with (w,b i believe)
so for me space mode for example has all the pickers on my right hand hjkl instead of f-file b-buffer
same with insert append delete open newline all next to each other above my left homerow
plus i also fell for the special keyboard layout meme so i usually adjust shortcuts to be on easy to reach places

@GloopShlugger
Copy link

I can look into it when time allows!!

Thank you! no rush

@gibbz00 gibbz00 mentioned this pull request Feb 22, 2023
@eugenesvk
Copy link

eugenesvk commented Apr 21, 2023

I can look into it when time allows!!

Thanks, I have the same use case as I've rewritten the entire keymap for similar reasons (keycap mnemonics aren't useful as opposed to better locations), so would be nice if manual ordering were available

@HactarCE
Copy link

Any update on this? / Anything I can do to help it move faster?

@gibbz00
Copy link
Contributor Author

gibbz00 commented Jun 18, 2023

Any update on this? / Anything I can do to help it move faster?

There are actually! I've recently began splitting this PR into smaller ones, and I'm currently waiting for those that I've sent in to be merged before moving onto the next. First major one being #7216 :)

@daedroza
Copy link
Contributor

@gibbz00 : would it be possible to rebase this on latest head?

@gibbz00
Copy link
Contributor Author

gibbz00 commented Dec 14, 2023

@gibbz00 : would it be possible to rebase this on latest head?

Possible? Always :) Worth it? Probably not, at least not for me. I simply don't have the time to put efforts into PRs that I know won't get merged. See: #5635 (comment).

@pascalkuthe
Copy link
Member

Closing as stale

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 A-keymap Area: Keymap and keybindings S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants