Keymap presentation refinements and underlying restructuring #5635

gibbz00 · 2023-01-22T16:48:25Z

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`

Personal

Before	After	After with `sorted-infobox = true`

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"]

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

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

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.

gibbz00 · 2023-02-15T12:20:02Z

helix-term/src/config.rs

@@ -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,


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 · 2023-02-15T22:51:33Z

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

archseer · 2023-02-16T11:36:45Z

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

GloopShlugger · 2023-02-16T14:27:35Z

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 · 2023-02-16T23:12:26Z

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 · 2023-02-17T00:24:19Z

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 · 2023-02-17T10:07:14Z

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 · 2023-02-17T10:47:20Z

I can look into it when time allows!!

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

GloopShlugger · 2023-02-17T11:39:29Z

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 · 2023-02-17T11:41:11Z

I can look into it when time allows!!

Thank you! no rush

eugenesvk · 2023-04-21T15:30:22Z

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 · 2023-06-13T05:50:07Z

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

gibbz00 · 2023-06-18T08:20:17Z

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 · 2023-12-14T16:53:30Z

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

gibbz00 · 2023-12-14T19:22:45Z

@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 · 2024-04-20T12:46:25Z

Closing as stale

bbodi and others added 30 commits October 9, 2022 17:33

Make stickiness configurable

671f297

Add support for labels in custom menu keymaps

09ea56b

Add support for labels on typable commands

608687d

refactor keymap map visitor to reduce # of cases

091b3f5

Simplify labelled command pattern match

56d624e

Co-authored-by: Michael Davis <mcarsondavis@gmail.com>

Add some basic docs

3642213

fix typos in menu label docs

7804572

return errors for ambiguous and unsupported labels in menus

0e6c4af

remove one-liner solely used for a test

6d50d7f

Inline helper fuction of singular reference

37f49f6

Keymap infobox: Use Vec in place of BTree:

8d35e64

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.

Keymap infobox: Place in correct order from start:

f187f2d

Infobox body was being filled with description then KeyEvent list to only later be iterated over for the purpose of flippingin this order.

Rename MappableCommand field doc to description:

d5520e8

For it not to be confused with the upcoming ":help" feature.

Initial sorting of keymap info window

aea10f7

Infobox: Consistently place lowercase equivalents first

7f86d01

This, along with previous commit reverts regression in helix-editor#952

Remove infobox optimization suggestion comment:

7935f77

Sort duration calculations averaged to about 0.04 ms when opening the larger infoboxes.

keymap testing touchups

0482d09

Exclude config no_op bindings in command palette.

6b01af5

Cleaner infobox join operation

7a8397c

use statement cleanups

546010b

Move Config related tests from keymap tests Config tests

036ce4a

Config test cleanup

ac98d82

* Elaborated on test descriptions * Removed duplicate unit test * Unified keytrie traversal in tests

Load keymap config consistently

109228b

* Moved config merging Keymaps to Config * Removed unused default EditorView function

Make use of new self context in keymap config load

527198a

Minor keymap module visibilyti cleanup

917ca0d

Renamed the keymap! macro to keytrie!:

8b8fadb

It produces, after all, a Keytrie. It would be a bit like saying: let engine = car!()

gibbz00 added 3 commits February 15, 2023 12:35

fix keytrie on default mappable command override bug

6a81215

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.

keytrie.rs: merge_keytrie() refactor

91d8d92

replace custom join with vec::join

159150b

gibbz00 commented Feb 15, 2023

View reviewed changes

gibbz00 force-pushed the keymap-presentation branch from b04e4bb to 095188d Compare February 15, 2023 13:18

revert some cleanup of use statements

683931a

...in the hopes that it simplifies review. The changes should have probably be codified into `cargo fmt`.

gibbz00 force-pushed the keymap-presentation branch from 095188d to 683931a Compare February 15, 2023 13:19

improve keymap deserialize error handlig

e98afd2

Replace two `.expect()` with returning Err. Using the let else pattern for this. Only became stable in 1.65.0, hence the bump.

bbodi mentioned this pull request Feb 16, 2023

Configurable labels for custom menus and typable commands #3958

Open

gibbz00 added 2 commits February 17, 2023 00:17

fix clippy lints

d326827

custom description refinements

a0d918a

* 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 mentioned this pull request Feb 22, 2023

Clarify Context #6056

Closed

This was referenced May 29, 2023

Minor touchups in keymap.rs #7167

Closed

Configurable keymap ordering. #7216

Closed

pascalkuthe closed this Apr 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keymap presentation refinements and underlying restructuring #5635

Keymap presentation refinements and underlying restructuring #5635

gibbz00 commented Jan 22, 2023 •

edited

gibbz00 Feb 15, 2023

gibbz00 commented Feb 15, 2023

archseer commented Feb 16, 2023

GloopShlugger commented Feb 16, 2023

gibbz00 commented Feb 16, 2023

gibbz00 commented Feb 17, 2023 •

edited

GloopShlugger commented Feb 17, 2023

gibbz00 commented Feb 17, 2023

GloopShlugger commented Feb 17, 2023

GloopShlugger commented Feb 17, 2023

eugenesvk commented Apr 21, 2023 •

edited

HactarCE commented Jun 13, 2023

gibbz00 commented Jun 18, 2023

daedroza commented Dec 14, 2023

gibbz00 commented Dec 14, 2023

pascalkuthe commented Apr 20, 2024

Keymap presentation refinements and underlying restructuring #5635

Keymap presentation refinements and underlying restructuring #5635

Conversation

gibbz00 commented Jan 22, 2023 • edited

Keymap infobox:

Sorting

Custom labels

Command palette:

Future work

Custom user commands appear in command palette

Conflicting PRs (16)

gibbz00 Feb 15, 2023

Choose a reason for hiding this comment

gibbz00 commented Feb 15, 2023

archseer commented Feb 16, 2023

GloopShlugger commented Feb 16, 2023

gibbz00 commented Feb 16, 2023

gibbz00 commented Feb 17, 2023 • edited

GloopShlugger commented Feb 17, 2023

gibbz00 commented Feb 17, 2023

GloopShlugger commented Feb 17, 2023

GloopShlugger commented Feb 17, 2023

eugenesvk commented Apr 21, 2023 • edited

HactarCE commented Jun 13, 2023

gibbz00 commented Jun 18, 2023

daedroza commented Dec 14, 2023

gibbz00 commented Dec 14, 2023

pascalkuthe commented Apr 20, 2024

gibbz00 commented Jan 22, 2023 •

edited

gibbz00 commented Feb 17, 2023 •

edited

eugenesvk commented Apr 21, 2023 •

edited