From 86a403de4a6da4e91eebeddc6776ddefdc38770d Mon Sep 17 00:00:00 2001 From: gibbz00 Date: Wed, 31 May 2023 23:32:55 +0200 Subject: [PATCH 1/5] keymap: Derive `Default` for KeyTrieNode --- helix-term/src/keymap.rs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/helix-term/src/keymap.rs b/helix-term/src/keymap.rs index 3033c6a4883c..ea34209b1b5f 100644 --- a/helix-term/src/keymap.rs +++ b/helix-term/src/keymap.rs @@ -18,7 +18,7 @@ use std::{ pub use default::default; use macros::key; -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Default)] pub struct KeyTrieNode { /// A label for keys coming under this node, like "Goto mode" name: String, @@ -117,12 +117,6 @@ impl KeyTrieNode { } } -impl Default for KeyTrieNode { - fn default() -> Self { - Self::new("", HashMap::new(), Vec::new()) - } -} - impl PartialEq for KeyTrieNode { fn eq(&self, other: &Self) -> bool { self.map == other.map From 60d20c4aca64ded17fb745309c5eb1e623ffc93e Mon Sep 17 00:00:00 2001 From: gibbz00 Date: Wed, 31 May 2023 23:40:02 +0200 Subject: [PATCH 2/5] keymap: Rename KeyTrie::Leaf -> KeyTrie::MapppableCommand The variant Sequence is technically also a leaf. --- helix-term/src/keymap.rs | 26 +++++++++++++------------- helix-term/src/keymap/macros.rs | 2 +- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/helix-term/src/keymap.rs b/helix-term/src/keymap.rs index ea34209b1b5f..761d068ffe0e 100644 --- a/helix-term/src/keymap.rs +++ b/helix-term/src/keymap.rs @@ -80,7 +80,7 @@ impl KeyTrieNode { let mut body: Vec<(&str, BTreeSet)> = Vec::with_capacity(self.len()); for (&key, trie) in self.iter() { let desc = match trie { - KeyTrie::Leaf(cmd) => { + KeyTrie::MappableCommand(cmd) => { if cmd.name() == "no_op" { continue; } @@ -139,7 +139,7 @@ impl DerefMut for KeyTrieNode { #[derive(Debug, Clone, PartialEq)] pub enum KeyTrie { - Leaf(MappableCommand), + MappableCommand(MappableCommand), Sequence(Vec), Node(KeyTrieNode), } @@ -168,7 +168,7 @@ impl<'de> serde::de::Visitor<'de> for KeyTrieVisitor { { command .parse::() - .map(KeyTrie::Leaf) + .map(KeyTrie::MappableCommand) .map_err(E::custom) } @@ -205,14 +205,14 @@ impl KeyTrie { pub fn node(&self) -> Option<&KeyTrieNode> { match *self { KeyTrie::Node(ref node) => Some(node), - KeyTrie::Leaf(_) | KeyTrie::Sequence(_) => None, + KeyTrie::MappableCommand(_) | KeyTrie::Sequence(_) => None, } } pub fn node_mut(&mut self) -> Option<&mut KeyTrieNode> { match *self { KeyTrie::Node(ref mut node) => Some(node), - KeyTrie::Leaf(_) | KeyTrie::Sequence(_) => None, + KeyTrie::MappableCommand(_) | KeyTrie::Sequence(_) => None, } } @@ -229,7 +229,7 @@ impl KeyTrie { trie = match trie { KeyTrie::Node(map) => map.get(key), // leaf encountered while keys left to process - KeyTrie::Leaf(_) | KeyTrie::Sequence(_) => None, + KeyTrie::MappableCommand(_) | KeyTrie::Sequence(_) => None, }? } Some(trie) @@ -269,7 +269,7 @@ impl Keymap { // recursively visit all nodes in keymap fn map_node(cmd_map: &mut ReverseKeymap, node: &KeyTrie, keys: &mut Vec) { match node { - KeyTrie::Leaf(cmd) => match cmd { + KeyTrie::MappableCommand(cmd) => match cmd { MappableCommand::Typable { name, .. } => { cmd_map.entry(name.into()).or_default().push(keys.clone()) } @@ -371,7 +371,7 @@ impl Keymaps { }; let trie = match trie_node.search(&[*first]) { - Some(KeyTrie::Leaf(ref cmd)) => { + Some(KeyTrie::MappableCommand(ref cmd)) => { return KeymapResult::Matched(cmd.clone()); } Some(KeyTrie::Sequence(ref cmds)) => { @@ -390,7 +390,7 @@ impl Keymaps { } KeymapResult::Pending(map.clone()) } - Some(KeyTrie::Leaf(cmd)) => { + Some(KeyTrie::MappableCommand(cmd)) => { self.state.clear(); KeymapResult::Matched(cmd.clone()) } @@ -479,19 +479,19 @@ mod tests { // Assumes that `g` is a node in default keymap assert_eq!( keymap.root().search(&[key!('g'), key!('$')]).unwrap(), - &KeyTrie::Leaf(MappableCommand::goto_line_end), + &KeyTrie::MappableCommand(MappableCommand::goto_line_end), "Leaf should be present in merged subnode" ); // Assumes that `gg` is in default keymap assert_eq!( keymap.root().search(&[key!('g'), key!('g')]).unwrap(), - &KeyTrie::Leaf(MappableCommand::delete_char_forward), + &KeyTrie::MappableCommand(MappableCommand::delete_char_forward), "Leaf should replace old leaf in merged subnode" ); // Assumes that `ge` is in default keymap assert_eq!( keymap.root().search(&[key!('g'), key!('e')]).unwrap(), - &KeyTrie::Leaf(MappableCommand::goto_last_line), + &KeyTrie::MappableCommand(MappableCommand::goto_last_line), "Old leaves in subnode should be present in merged node" ); @@ -523,7 +523,7 @@ mod tests { .root() .search(&[key!(' '), key!('s'), key!('v')]) .unwrap(), - &KeyTrie::Leaf(MappableCommand::vsplit), + &KeyTrie::MappableCommand(MappableCommand::vsplit), "Leaf should be present in merged subnode" ); // Make sure an order was set during merge diff --git a/helix-term/src/keymap/macros.rs b/helix-term/src/keymap/macros.rs index c4a1bfbb3064..b2822069cb75 100644 --- a/helix-term/src/keymap/macros.rs +++ b/helix-term/src/keymap/macros.rs @@ -81,7 +81,7 @@ macro_rules! alt { #[macro_export] macro_rules! keymap { (@trie $cmd:ident) => { - $crate::keymap::KeyTrie::Leaf($crate::commands::MappableCommand::$cmd) + $crate::keymap::KeyTrie::MappableCommand($crate::commands::MappableCommand::$cmd) }; (@trie From 531fe2e34474e3b1f0fbb84ee46dfea6a4e5ed87 Mon Sep 17 00:00:00 2001 From: gibbz00 Date: Wed, 31 May 2023 23:59:29 +0200 Subject: [PATCH 3/5] Make `Keymap` a tuple struct. --- helix-term/src/keymap.rs | 66 ++++++++++++++++++++++------------------ 1 file changed, 36 insertions(+), 30 deletions(-) diff --git a/helix-term/src/keymap.rs b/helix-term/src/keymap.rs index 761d068ffe0e..dc578e66de8f 100644 --- a/helix-term/src/keymap.rs +++ b/helix-term/src/keymap.rs @@ -252,17 +252,14 @@ pub enum KeymapResult { #[derive(Debug, Clone, PartialEq, Deserialize)] #[serde(transparent)] -pub struct Keymap { - /// Always a Node - root: KeyTrie, -} +pub struct Keymap(KeyTrie); /// A map of command names to keybinds that will execute the command. pub type ReverseKeymap = HashMap>>; impl Keymap { pub fn new(root: KeyTrie) -> Self { - Keymap { root } + Keymap(root) } pub fn reverse_map(&self) -> ReverseKeymap { @@ -290,30 +287,28 @@ impl Keymap { } let mut res = HashMap::new(); - map_node(&mut res, &self.root, &mut Vec::new()); + map_node(&mut res, &self.0, &mut Vec::new()); res } - - pub fn root(&self) -> &KeyTrie { - &self.root - } - - pub fn merge(&mut self, other: Self) { - self.root.merge_nodes(other.root); - } } impl Deref for Keymap { - type Target = KeyTrieNode; + type Target = KeyTrie; fn deref(&self) -> &Self::Target { - self.root.node().unwrap() + &self.0 + } +} + +impl DerefMut for Keymap { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.0 } } impl Default for Keymap { fn default() -> Self { - Self::new(KeyTrie::Node(KeyTrieNode::default())) + Self(KeyTrie::Node(KeyTrieNode::default())) } } @@ -367,7 +362,7 @@ impl Keymaps { let first = self.state.get(0).unwrap_or(&key); let trie_node = match self.sticky { Some(ref trie) => Cow::Owned(KeyTrie::Node(trie.clone())), - None => Cow::Borrowed(&keymap.root), + None => Cow::Borrowed(&keymap.0), }; let trie = match trie_node.search(&[*first]) { @@ -412,7 +407,7 @@ impl Default for Keymaps { /// Merge default config keys with user overwritten keys for custom user config. pub fn merge_keys(dst: &mut HashMap, mut delta: HashMap) { for (mode, keys) in dst { - keys.merge(delta.remove(mode).unwrap_or_default()) + keys.merge_nodes(delta.remove(mode).unwrap_or_default().0) } } @@ -478,25 +473,39 @@ mod tests { let keymap = merged_keyamp.get_mut(&Mode::Normal).unwrap(); // Assumes that `g` is a node in default keymap assert_eq!( - keymap.root().search(&[key!('g'), key!('$')]).unwrap(), + keymap.search(&[key!('g'), key!('$')]).unwrap(), &KeyTrie::MappableCommand(MappableCommand::goto_line_end), "Leaf should be present in merged subnode" ); // Assumes that `gg` is in default keymap assert_eq!( - keymap.root().search(&[key!('g'), key!('g')]).unwrap(), + keymap.search(&[key!('g'), key!('g')]).unwrap(), &KeyTrie::MappableCommand(MappableCommand::delete_char_forward), "Leaf should replace old leaf in merged subnode" ); // Assumes that `ge` is in default keymap assert_eq!( - keymap.root().search(&[key!('g'), key!('e')]).unwrap(), + keymap.search(&[key!('g'), key!('e')]).unwrap(), &KeyTrie::MappableCommand(MappableCommand::goto_last_line), "Old leaves in subnode should be present in merged node" ); - assert!(merged_keyamp.get(&Mode::Normal).unwrap().len() > 1); - assert!(merged_keyamp.get(&Mode::Insert).unwrap().len() > 0); + assert!( + merged_keyamp + .get(&Mode::Normal) + .and_then(|key_trie| key_trie.node()) + .unwrap() + .len() + > 1 + ); + assert!( + merged_keyamp + .get(&Mode::Insert) + .and_then(|key_trie| key_trie.node()) + .unwrap() + .len() + > 0 + ); } #[test] @@ -519,22 +528,19 @@ mod tests { let keymap = merged_keyamp.get_mut(&Mode::Normal).unwrap(); // Make sure mapping works assert_eq!( - keymap - .root() - .search(&[key!(' '), key!('s'), key!('v')]) - .unwrap(), + keymap.search(&[key!(' '), key!('s'), key!('v')]).unwrap(), &KeyTrie::MappableCommand(MappableCommand::vsplit), "Leaf should be present in merged subnode" ); // Make sure an order was set during merge - let node = keymap.root().search(&[crate::key!(' ')]).unwrap(); + let node = keymap.search(&[crate::key!(' ')]).unwrap(); assert!(!node.node().unwrap().order().is_empty()) } #[test] fn aliased_modes_are_same_in_default_keymap() { let keymaps = Keymaps::default().map(); - let root = keymaps.get(&Mode::Normal).unwrap().root(); + let root = keymaps.get(&Mode::Normal).unwrap(); assert_eq!( root.search(&[key!(' '), key!('w')]).unwrap(), root.search(&["C-w".parse::().unwrap()]).unwrap(), From 9060b9fef7bc1528bd68258c9cd625098d65b308 Mon Sep 17 00:00:00 2001 From: gibbz00 Date: Mon, 29 May 2023 21:36:23 +0200 Subject: [PATCH 4/5] `helix_term::keymap`: Remove one-liner solely used for a test. --- helix-term/src/keymap.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/helix-term/src/keymap.rs b/helix-term/src/keymap.rs index dc578e66de8f..79d6b46a7d7e 100644 --- a/helix-term/src/keymap.rs +++ b/helix-term/src/keymap.rs @@ -111,10 +111,6 @@ impl KeyTrieNode { } Info::from_keymap(self.name(), body) } - /// Get a reference to the key trie node's order. - pub fn order(&self) -> &[KeyEvent] { - self.order.as_slice() - } } impl PartialEq for KeyTrieNode { @@ -534,7 +530,7 @@ mod tests { ); // Make sure an order was set during merge let node = keymap.search(&[crate::key!(' ')]).unwrap(); - assert!(!node.node().unwrap().order().is_empty()) + assert!(!node.node().unwrap().order.as_slice().is_empty()) } #[test] From 70c65654b02ad93f07125895c2f4bc5f9376246a Mon Sep 17 00:00:00 2001 From: gibbz00 Date: Mon, 29 May 2023 21:42:24 +0200 Subject: [PATCH 5/5] Remove superfluous command description pruning for keymap infobox: Exist under the wrong (possibly just outdated) assumption that command descriptions are written with their `KeyTrie` name prefixed --- helix-term/src/keymap.rs | 7 ------- 1 file changed, 7 deletions(-) diff --git a/helix-term/src/keymap.rs b/helix-term/src/keymap.rs index 79d6b46a7d7e..f5626ee8621c 100644 --- a/helix-term/src/keymap.rs +++ b/helix-term/src/keymap.rs @@ -102,13 +102,6 @@ impl KeyTrieNode { .position(|&k| k == *keys.iter().next().unwrap()) .unwrap() }); - let prefix = format!("{} ", self.name()); - if body.iter().all(|(desc, _)| desc.starts_with(&prefix)) { - body = body - .into_iter() - .map(|(desc, keys)| (desc.strip_prefix(&prefix).unwrap(), keys)) - .collect(); - } Info::from_keymap(self.name(), body) } }