Keymap presentation refinements and underlying restructuring + consistent config use #5565

gibbz00 · 2023-01-17T16:21:11Z

Commit messages explain most of it. Proceeds: #3053

Before	After

Command palette:
Should I change the ">" character to a unicode arrow like: → ?
Should I properly escape less than and greater than from lt and gt to < and > respectively?

Infoboxes issues which haven't been touched upon: #5203, #3958

EDIT: Accidentally snuck in #5130, so guess this proceeds that one too, can open up a new PR which leaves it out if needed.

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. Suffice to say that the order property and the info box sorting algorithm are now removed.

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.

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.

Makes sure: j, C-j, down k, up, C-k Appears as: j, C-j, down k, C-k, up Even though "up" is shorter than "C-k"

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

Changed in fdehau/tui-rs@a68e38e.

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

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

* Config::merge_in_default_config() takes in self as parameter.

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

* Removed config dir check and creation from helix-term main.rs, it was being done in the proceeding line with helix_loader::initialize_config_file() * Log path setup moved to helix_loader, and setup not done before setting up logging. It would create the default log folder even if user specifies thier own log file. * Fixed helix_loader::config_file() and log_file() assumption of existing parent directory.

Return Ok(0) to main instead of directly doing process::exit(0) on only some of the args. Makes it easier to implement special exit handling before quitting.

* Don't setup config directory twice in main.

* Remove duplicate config loading code and clean up main.rs

* RUNTIME_DIR now behaves similar to the other runtime files * lib.rs cleanup * merge_toml_values: Moved function explanation comment in a function call to its documentation.

* RUNTIME_DIR now behaves similar to the other runtime files * lib.rs cleanup * merge_toml_values: Moved function explanation comment in a function call to its documentation. * slight appalication.rs cleanup * Reduce use of relative filepaths with a helix_loader::repo_paths.rs

health.rs * Removed duplicate clipboard provider writeout. * Removed 'all' flag option but added a 'paths' option. * Removed check if user config exist or show default part as default is always merged with user config. docgen: * removed xtask::helper::lang_config(), using LanguageConfiguarions::default() instead. * moved xtask::query_check to xtask::main and made it use TSFeature.runtime_filename()

archseer · 2023-01-17T16:23:39Z

It looks like the keys are now sorted in the popup? We deliberately keep the order the same as defined in the keymap, because left/right/up/down should be grouped together and not sorted alphabetically for example.

pascalkuthe · 2023-01-17T16:27:07Z

@gibbz00 could you maybe provide a summary of what exactly this does. There are 43 commits in this PR so reading trough them all is kind of a chore

gibbz00 · 2023-01-17T16:29:55Z

It looks like the keys are now sorted in the popup? We deliberately keep the order the same as defined in the keymap, because left/right/up/down should be grouped together and not sorted alphabetically for example.

Sorting only worked when keymap was added by the keymap! macro and not when loaded by user config. Also, arrow keys are often also bound with hjkl keys, which will show first. Order is: one letter -> C keys -> multi-letter. They aren't shown in the images because I have no_op them.

archseer · 2023-01-17T16:30:02Z

helix-core/src/macros.rs

@@ -7,7 +7,7 @@ macro_rules! hashmap {
    ($($key:expr => $value:expr),*) => {
        {
            let _cap = hashmap!(@count $($key),*);
-            let mut _map = ::std::collections::HashMap::with_capacity(_cap);
+            let mut _map = std::collections::HashMap::with_capacity(_cap);


This is deliberate for macro hygiene purposes.

Mm sure, I can revert this. Although I doubt there will be a namespace are namespace collisions with: std::collections::HashMap::with_capacity

archseer · 2023-01-17T16:30:22Z

helix-term/src/keymap/macros.rs

-        ::helix_view::input::KeyEvent {
-            code: ::helix_view::keyboard::KeyCode::$key,
-            modifiers: ::helix_view::keyboard::KeyModifiers::NONE,
+        helix_view::input::KeyEvent {


ditto in this file

archseer · 2023-01-17T16:33:50Z

Right, but either way the popup should be arranged in definition order. Certain popups like goto are manually ordered by grouping together similar sets of functionality.

For instance w v and t are common operations so they were listed on top. left/right/up/down are now spread across the whole infobox

gibbz00 · 2023-01-17T16:42:15Z

Right, but either way the popup should be arranged in definition order. Certain popups like goto are manually ordered by grouping together similar sets of functionality.

For instance w v and t are common operations so they were listed on top. left/right/up/down are now spread across the whole infobox

No config:

archseer · 2023-01-17T16:42:51Z

helix-term/src/keymap.rs

@@ -68,7 +68,7 @@ impl KeyTrieNode {
    }

    pub fn infobox(&self) -> Info {
-        let mut body: Vec<(&str, BTreeSet<KeyEvent>)> = Vec::with_capacity(self.len());
+        let mut body: Vec<(&str, Vec<KeyEvent>)> = Vec::with_capacity(self.len());


This seems to revert #952

Posted images disagree.

archseer · 2023-01-17T16:47:09Z

Hmm it could be non-deterministic? I have no config and it's sorting by arrow direction first (not hjkl)

gibbz00 · 2023-01-17T16:49:40Z

Hmm it could be non-deterministic? I have no config and it's sorting by arrow direction first (not hjkl)

When using this PR?

gibbz00 · 2023-01-17T16:51:03Z

Certain popups like goto are manually ordered by grouping together similar sets of functionality.

I would still argue that it is easier to find the command when they're sorted alphabetically.

gibbz00 · 2023-01-17T16:55:58Z

@gibbz00 could you maybe provide a summary of what exactly this does. There are 43 commits in this PR so reading trough them all is kind of a chore

@pascalkuthe clean up of keymap, main.rs, docgen.rs, health.rs. It's a large PR so the commits are the summaries. I'm sorry :(

archseer · 2023-01-18T02:33:30Z

Alphabetical sort: we had that initially then replaced it, so this is strictly a downgrade. The goto popup is a great example:

g is on the top since it's a repetition of the key
h/l are grouped together
t/c/b are grouped together
a/m are grouped together
n/p
All the jumps that are LSP specific are also grouped

Alphabetical sort is useful if you're trying to find what a specific key does. But the current order is intended to make it easier to find based on functionality. Kakoune also sorts this similarly.

I like some of the cleanup in this PR though so I might start cherry picking changes into master to shrink the size of this PR a bit and make it easier to review. But I'd avoid renaming types just for the sake of clarity where possible: this is a large project with a lot of PRs in queue and any type of renaming can cause merge conflicts on a large portion of them.

archseer · 2023-01-18T05:21:49Z

I've merged #5130 and #3053 so they can be rebased out from this PR now

gibbz00 · 2023-01-19T12:29:12Z

Alphabetical sort is useful if you're trying to find what a specific key does. But the current order is intended to make it easier to find based on functionality.

Ok, I'm starting to see your point. I can put the ordering from the default ordering back, can, but can I add alphabetical sort as a config option? I have a lot of config keybindings and the info boxes become because of that very messy.

I can also send in split PRs, maybe that can help with the review process.

But I'd avoid renaming types just for the sake of clarity where possible: this is a large project with a lot of PRs in queue and any type of renaming can cause merge conflicts on a large portion of them.

Of course, I'll try to limit this unless I strongly feel that the naming is outright deceptive.

gibbz00 and others added 30 commits January 17, 2023 15:09

(theme) feat: mode based primary cursor colors

3e51bc9

docs/themes: mode based primary cursor colors

71c7565

remove one-liner solely used for a test

a2b488f

Inlined helper fuction with a singular referecne.

34301f7

Removed unused import

b41790a

Keymap infobox: Use Vec in place of BTree:

76054f7

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.

Keymap infobox: Place in correct order from start:

74833ff

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:

fda6c3b

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

Initial sorting of keymap info window

9d7e9a2

Infobox: Consistently place lowercase equivalents first

8ce551b

Infobox keyevent ordering "C-" aknowledgement:

efdb1a1

Makes sure: j, C-j, down k, up, C-k Appears as: j, C-j, down k, C-k, up Even though "up" is shorter than "C-k"

Remove infobox optimization suggestion comment:

2551b74

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

keymap testing touchups

61d6904

Exclude config no_op bindings in command palette.

c6e3bb9

Reuse table in picker

aa9095b

Use upstream implementation of table column calculation

e5079c4

Changed in fdehau/tui-rs@a68e38e.

Minimize allocation when converting table rows to string

23abcd7

Replace menu::Item::{row, label} with format()

4bf2caf

Cleaner infobox join operation

3d8155c

Helix term use statement cleanups

4d4e87b

Move Config related tests from keymap tests Config tests

be31ee2

Config test cleanup

7231d0f

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

Unify config loading pt 1:

d25d0ac

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

Unify config loading pt 2:

8918553

* Config::merge_in_default_config() takes in self as parameter.

Minor module visibility cleanup

b4ca7d8

Renamed the keymap! macro to keytrie!:

8cb9a91

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

gibbz00 added 9 commits January 17, 2023 15:14

help.rs: use named parameters

87afbe5

Consistent exit handling

73676b5

Return Ok(0) to main instead of directly doing process::exit(0) on only some of the args. Makes it easier to implement special exit handling before quitting.

Unify config loading

7ddeca2

* Don't setup config directory twice in main.

Unify config loading

4731861

* Remove duplicate config loading code and clean up main.rs

Unify config loading

c0ad268

* RUNTIME_DIR now behaves similar to the other runtime files * lib.rs cleanup * merge_toml_values: Moved function explanation comment in a function call to its documentation.

fix rebase

a904e2c

pascalkuthe added S-waiting-on-pr Status: This is waiting on another PR to be merged first A-helix-term Area: Helix term improvements labels Jan 17, 2023

archseer reviewed Jan 17, 2023

View reviewed changes

gibbz00 closed this Jan 19, 2023

gibbz00 reopened this Jan 19, 2023

gibbz00 closed this Jan 19, 2023

gibbz00 mentioned this pull request Jan 22, 2023

Keymap presentation refinements and underlying restructuring #5635

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keymap presentation refinements and underlying restructuring + consistent config use #5565

Keymap presentation refinements and underlying restructuring + consistent config use #5565

gibbz00 commented Jan 17, 2023 •

edited

archseer commented Jan 17, 2023

pascalkuthe commented Jan 17, 2023

gibbz00 commented Jan 17, 2023 •

edited

archseer Jan 17, 2023

gibbz00 Jan 17, 2023 •

edited

archseer Jan 17, 2023

archseer commented Jan 17, 2023

gibbz00 commented Jan 17, 2023

archseer Jan 17, 2023

gibbz00 Jan 17, 2023

archseer commented Jan 17, 2023

gibbz00 commented Jan 17, 2023

gibbz00 commented Jan 17, 2023 •

edited

gibbz00 commented Jan 17, 2023

archseer commented Jan 18, 2023

archseer commented Jan 18, 2023

gibbz00 commented Jan 19, 2023

Keymap presentation refinements and underlying restructuring + consistent config use #5565

Keymap presentation refinements and underlying restructuring + consistent config use #5565

Conversation

gibbz00 commented Jan 17, 2023 • edited

archseer commented Jan 17, 2023

pascalkuthe commented Jan 17, 2023

gibbz00 commented Jan 17, 2023 • edited

archseer Jan 17, 2023

Choose a reason for hiding this comment

gibbz00 Jan 17, 2023 • edited

Choose a reason for hiding this comment

archseer Jan 17, 2023

Choose a reason for hiding this comment

archseer commented Jan 17, 2023

gibbz00 commented Jan 17, 2023

archseer Jan 17, 2023

Choose a reason for hiding this comment

gibbz00 Jan 17, 2023

Choose a reason for hiding this comment

archseer commented Jan 17, 2023

gibbz00 commented Jan 17, 2023

gibbz00 commented Jan 17, 2023 • edited

gibbz00 commented Jan 17, 2023

archseer commented Jan 18, 2023

archseer commented Jan 18, 2023

gibbz00 commented Jan 19, 2023

gibbz00 commented Jan 17, 2023 •

edited

gibbz00 commented Jan 17, 2023 •

edited

gibbz00 Jan 17, 2023 •

edited

gibbz00 commented Jan 17, 2023 •

edited