feat: extra keybindings override base (#224)

* refactor: use default for base keybinding

* refactor: move `Keybindings` to its own file

* feat: implement `iter` on `Keybindings`

* feat: from `KeybindingsSection` to `Keybindings`

* refactor: replace `Keybindings` by `KeybindingsSection` in opts

* refactor(Keybindings): remove unused methods

* feat: extra keybindings override base

* docs: document Keybindings

* refactor: add log in case a keybinding is dropped

src/keybindings/keybindings.rs

@@ -0,0 +1,128 @@
+use serde::Deserialize;
+
+use crate::opts::KeybindingsSection;
+
+use super::{action::Action, KeyCombo};
+
+/// A list of [`keybindings`](KeyCombo) each associated with an [`Action`].
+#[derive(Debug, Clone, Deserialize, PartialEq, Eq)]
+pub struct Keybindings(Vec<(Action, KeyCombo)>);
+
+impl Keybindings {
+    /// Returns an iterator over the [`Action`]s and [`KeyCombo`]s
+    pub fn iter(&self) -> std::slice::Iter<'_, (Action, KeyCombo)> {
+        self.0.iter()
+    }
+}
+
+impl Extend<(Action, KeyCombo)> for Keybindings {
+    fn extend<I: IntoIterator<Item = (Action, KeyCombo)>>(&mut self, iter: I) {
+        self.0.extend(iter)
+    }
+}
+
+impl IntoIterator for Keybindings {
+    type Item = (Action, KeyCombo);
+    type IntoIter = <Vec<(Action, KeyCombo)> as IntoIterator>::IntoIter;
+
+    fn into_iter(self) -> Self::IntoIter {
+        self.0.into_iter()
+    }
+}
+
+impl Default for Keybindings {
+    fn default() -> Self {
+        Self(super::defaults::defaults())
+    }
+}
+
+impl From<KeybindingsSection> for Keybindings {
+    /// Converts from [`KeybindingsSection`] to [`Keybindings`].
+    ///
+    /// If an `extra` keybinding collides with a `base` one, then the `base` one is dropped in
+    /// favor of the `extra` keybinding
+    fn from(value: KeybindingsSection) -> Self {
+        let mut base = value.base;
+
+        if let Some(extra) = value.extra {
+            for (_, extra_combo) in extra.iter() {
+                base.0 = base
+                    .clone()
+                    .into_iter()
+                    .filter(|(_, combo)| {
+                        if combo.starts_with(extra_combo) {
+                            tracing::debug!("Base keybinding {combo} ignored in favor of extra keybinding {extra_combo}");
+                            false
+                        } else {
+                            true
+                        }
+                    })
+                    .collect();
+            }
+
+            base.extend(extra)
+        }
+
+        base
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use winit::event::ModifiersState;
+
+    use crate::keybindings::{action::VertDirection, Key, ModifiedKey};
+
+    use super::*;
+
+    #[test]
+    fn from_keybinding_section_base() {
+        assert_eq!(
+            Keybindings::from(KeybindingsSection {
+                base: Keybindings::default(),
+                extra: None
+            }),
+            Keybindings::default()
+        );
+    }
+
+    #[test]
+    fn from_keybinding_section_extra() {
+        let combo = KeyCombo(vec![ModifiedKey(
+            Key::Resolved(winit::event::VirtualKeyCode::A),
+            ModifiersState::empty(),
+        )]);
+
+        let mut expected = Keybindings::default();
+        expected.0.push((Action::Quit, combo.clone()));
+
+        assert_eq!(
+            Keybindings::from(KeybindingsSection {
+                base: Keybindings::default(),
+                extra: Some(Keybindings(vec![(Action::Quit, combo)]))
+            }),
+            expected
+        );
+    }
+
+    #[test]
+    fn from_keybinding_section_extra_override_base() {
+        let j_combo = KeyCombo(vec![ModifiedKey(
+            Key::Resolved(winit::event::VirtualKeyCode::J),
+            ModifiersState::empty(),
+        )]);
+
+        let base = Keybindings(vec![(Action::Scroll(VertDirection::Down), j_combo.clone())]);
+        let extra = Keybindings(vec![(Action::Page(VertDirection::Down), j_combo.clone())]);
+
+        let expected = Keybindings(vec![(Action::Page(VertDirection::Down), j_combo.clone())]);
+
+        assert_eq!(
+            Keybindings::from(KeybindingsSection {
+                base,
+                extra: Some(extra)
+            }),
+            expected
+        );
+    }
+}

src/keybindings/mod.rs

@@ -1,5 +1,7 @@
 pub mod action;
 mod defaults;
+#[allow(clippy::module_inception)]
+mod keybindings;
 mod mappings;
 mod serialization;
 #[cfg(test)]
@@ -11,52 +13,19 @@ use std::slice::Iter;
 use std::str::FromStr;
 use std::vec;
 
-use action::Action;
-
-use serde::Deserialize;
 use winit::event::{ModifiersState, ScanCode, VirtualKeyCode as VirtKey};
 
-#[derive(Debug, Clone, Deserialize, PartialEq, Eq)]
-pub struct Keybindings(Vec<(Action, KeyCombo)>);
+use action::Action;
+pub use keybindings::Keybindings;
+
+use crate::opts::KeybindingsSection;
 
 #[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord)]
 pub enum Key {
     Resolved(VirtKey),
     ScanCode(ScanCode),
 }
 
-impl Keybindings {
-    pub fn new(bindings: Vec<(Action, KeyCombo)>) -> Self {
-        Self(bindings)
-    }
-
-    #[cfg(test)]
-    pub fn empty() -> Self {
-        Self(Vec::new())
-    }
-}
-
-impl Extend<(Action, KeyCombo)> for Keybindings {
-    fn extend<I: IntoIterator<Item = (Action, KeyCombo)>>(&mut self, iter: I) {
-        self.0.extend(iter)
-    }
-}
-
-impl IntoIterator for Keybindings {
-    type Item = (Action, KeyCombo);
-    type IntoIter = <Vec<(Action, KeyCombo)> as IntoIterator>::IntoIter;
-
-    fn into_iter(self) -> Self::IntoIter {
-        self.0.into_iter()
-    }
-}
-
-impl Default for Keybindings {
-    fn default() -> Self {
-        Self(defaults::defaults())
-    }
-}
-
 impl Key {
     pub fn new(resolved: Option<VirtKey>, scan_code: ScanCode) -> Self {
         match resolved {
@@ -231,16 +200,14 @@ pub struct KeyCombos {
 }
 
 impl KeyCombos {
-    pub fn new(keybinds: Keybindings) -> anyhow::Result<Self> {
-        let keybinds = keybinds.0;
+    pub fn new(keybinds: KeybindingsSection) -> anyhow::Result<Self> {
+        let keybinds: Keybindings = keybinds.into();
         let position = ROOT_INDEX;
 
         // A keycombo that starts with another keycombo will never be reachable since the prefixing
         // combo will always be activated first
-        for i in 0..keybinds.len() {
-            for j in (i + 1)..keybinds.len() {
-                let combo1 = &keybinds[i].1;
-                let combo2 = &keybinds[j].1;
+        for (i, (_, combo1)) in keybinds.iter().enumerate() {
+            for (_, combo2) in keybinds.iter().skip(i + 1) {
                 if combo1.starts_with(combo2) {
                     anyhow::bail!(
                         "A keycombo starts with another keycombo making it unreachable\n\tCombo: \

src/keybindings/tests.rs

@@ -1,6 +1,5 @@
 use super::action::{Action, VertDirection};
 use super::{KeyCombos, ModifiedKey};
-use crate::keybindings::Keybindings;
 use crate::opts::Config;
 use crate::test_utils::init_test_log;
 
@@ -22,9 +21,7 @@ base = [
 
     // TODO: move this to a helper somewhere
     let Config { keybindings, .. } = Config::load_from_str(config).unwrap();
-    let mut bindings = keybindings.base.unwrap_or_else(Keybindings::empty);
-    bindings.extend(keybindings.extra.unwrap_or_else(Keybindings::empty));
-    let mut key_combos = KeyCombos::new(bindings).unwrap();
+    let mut key_combos = KeyCombos::new(keybindings.into()).unwrap();
 
     let g: ModifiedKey = VirtKey::G.into();
     let l_shift = VirtKey::LShift.into();

src/opts/config.rs

@@ -66,9 +66,10 @@ impl Default for LinesToScroll {
     }
 }
 
-#[derive(Deserialize, Debug, Default, PartialEq)]
+#[derive(Deserialize, Clone, Debug, Default, PartialEq)]
 pub struct KeybindingsSection {
-    pub base: Option<Keybindings>,
+    #[serde(default)]
+    pub base: Keybindings,
     pub extra: Option<Keybindings>,
 }
 

src/opts/mod.rs

@@ -5,10 +5,9 @@ mod tests;
 
 use std::path::{Path, PathBuf};
 
-pub use self::cli::{Args, ThemeType};
-pub use self::config::{Config, FontOptions};
 use crate::color;
-use crate::keybindings::Keybindings;
+pub use cli::{Args, ThemeType};
+pub use config::{Config, FontOptions, KeybindingsSection};
 
 use anyhow::Result;
 use serde::Deserialize;
@@ -48,7 +47,7 @@ pub struct Opts {
     pub page_width: Option<f32>,
     pub lines_to_scroll: f32,
     pub font_opts: FontOptions,
-    pub keybindings: Keybindings,
+    pub keybindings: KeybindingsSection,
 }
 
 impl Opts {
@@ -85,7 +84,7 @@ impl Opts {
             light_theme,
             dark_theme,
             font_options,
-            keybindings: config_keybindings,
+            keybindings,
         } = config;
 
         let Args {
@@ -116,10 +115,6 @@ impl Opts {
         let font_opts = font_options.unwrap_or_default();
         let page_width = args_page_width.or(config_page_width);
         let lines_to_scroll = lines_to_scroll.into();
-        let mut keybindings = config_keybindings.base.unwrap_or_default();
-        if let Some(extra) = config_keybindings.extra {
-            keybindings.extend(extra);
-        }
 
         Ok(Self {
             file_path,

src/opts/tests/error_msg.rs

@@ -1,4 +1,4 @@
-use crate::keybindings::{KeyCombos, Keybindings};
+use crate::keybindings::KeyCombos;
 use crate::opts::Config;
 
 macro_rules! snapshot_config_parse_error {
@@ -40,9 +40,7 @@ snapshot_config_parse_error!(
 
 fn keycombo_conflict_from_config(s: &str) -> anyhow::Result<anyhow::Error> {
     let Config { keybindings, .. } = Config::load_from_str(s)?;
-    let mut combined_keybindings = keybindings.base.unwrap_or_else(Keybindings::empty);
-    combined_keybindings.extend(keybindings.extra.unwrap_or_else(Keybindings::empty));
-    let err = KeyCombos::new(combined_keybindings).unwrap_err();
+    let err = KeyCombos::new(keybindings.into()).unwrap_err();
     Ok(err)
 }
 

src/opts/tests/parse.rs

@@ -2,7 +2,6 @@ use std::ffi::OsString;
 use std::path::PathBuf;
 
 use crate::color::{SyntaxTheme, Theme, ThemeDefaults};
-use crate::keybindings::Keybindings;
 use crate::opts::config::{self, FontOptions, LinesToScroll};
 use crate::opts::{cli, Args, Opts, ResolvedTheme, ThemeType};
 use crate::test_utils::init_test_log;
@@ -25,7 +24,7 @@ impl Opts {
             page_width: None,
             font_opts: FontOptions::default(),
             lines_to_scroll: LinesToScroll::default().0,
-            keybindings: Keybindings::default(),
+            keybindings: Default::default(),
         }
     }
 }