Pretty up KeyCombos representation in user-facing errors (#193)

* Self review

* Improve user-facing keycombo representation

.github/workflows/CI.yml

@@ -7,6 +7,8 @@ env:
   CARGO_TERM_COLOR: always
   RUSTFLAGS: "-C debuginfo=0"
 
+# TODO: build/test on stable and beta?
+
 jobs:
   build:
     strategy:
@@ -29,6 +31,8 @@ jobs:
     steps:
       - uses: actions/checkout@v3
 
+      # FIXME: This is in the wrong area. It should be below setting up the
+      # toolchain
       - name: Cache
         uses: Swatinem/rust-cache@v2
 

src/interpreter/mod.rs

@@ -624,6 +624,7 @@ impl TokenSink for HtmlInterpreter {
 
                             self.push_current_textbox();
                         }
+                        // FIXME: `input` is self closing. This never gets called
                         "input" => {
                             self.push_current_textbox();
                             self.state.element_stack.pop();

src/keybindings/mod.rs

@@ -10,14 +10,14 @@ use std::{collections::BTreeMap, fmt, slice::Iter, str::FromStr, vec::IntoIter};
 use action::Action;
 
 use serde::Deserialize;
-use winit::event::{ModifiersState, ScanCode, VirtualKeyCode};
+use winit::event::{ModifiersState, ScanCode, VirtualKeyCode as VirtKey};
 
 #[derive(Debug, Clone, Deserialize, PartialEq, Eq)]
 pub struct Keybindings(Vec<(Action, KeyCombo)>);
 
 #[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord)]
 pub enum Key {
-    Resolved(VirtualKeyCode),
+    Resolved(VirtKey),
     ScanCode(ScanCode),
 }
 
@@ -49,16 +49,16 @@ impl Default for Keybindings {
 }
 
 impl Key {
-    pub fn new(resolved: Option<VirtualKeyCode>, scan_code: ScanCode) -> Self {
+    pub fn new(resolved: Option<VirtKey>, scan_code: ScanCode) -> Self {
         match resolved {
             Some(key_code) => Self::Resolved(key_code),
             None => Self::ScanCode(scan_code),
         }
     }
 }
 
-impl From<VirtualKeyCode> for Key {
-    fn from(key_code: VirtualKeyCode) -> Self {
+impl From<VirtKey> for Key {
+    fn from(key_code: VirtKey) -> Self {
         Self::Resolved(key_code)
     }
 }
@@ -72,10 +72,10 @@ impl fmt::Display for Key {
                     .find_map(|&(key_str, key)| (*resolved == key).then_some(key_str));
                 match maybe_key {
                     Some(key) => f.write_str(key),
-                    None => write!(f, "<unsupported: {:?}>", resolved),
+                    None => write!(f, "<unsupported: {resolved:?}>"),
                 }
             }
-            Key::ScanCode(_) => write!(f, "{:?}", self),
+            Key::ScanCode(scan_code) => write!(f, "<scan code: {scan_code}>"),
         }
     }
 }
@@ -97,7 +97,43 @@ pub struct ModifiedKey(pub Key, pub ModifiersState);
 impl fmt::Display for ModifiedKey {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         if self.1 == ModifiersState::default() {
-            write!(f, "{}", self.0)
+            let is_not_visible = [
+                VirtKey::F1,
+                VirtKey::F2,
+                VirtKey::F3,
+                VirtKey::F4,
+                VirtKey::F5,
+                VirtKey::F6,
+                VirtKey::F7,
+                VirtKey::F8,
+                VirtKey::F9,
+                VirtKey::F10,
+                VirtKey::F11,
+                VirtKey::F12,
+                VirtKey::Up,
+                VirtKey::Right,
+                VirtKey::Down,
+                VirtKey::Left,
+                VirtKey::Escape,
+                VirtKey::Tab,
+                VirtKey::Insert,
+                VirtKey::Delete,
+                VirtKey::Back,
+                VirtKey::Return,
+                VirtKey::Home,
+                VirtKey::End,
+                VirtKey::PageUp,
+                VirtKey::PageDown,
+                VirtKey::Space,
+            ]
+            .map(Key::from)
+            .contains(&self.0);
+
+            if is_not_visible {
+                write!(f, "<{}>", self.0)
+            } else {
+                write!(f, "{}", self.0)
+            }
         } else {
             let mut mod_list = Vec::new();
 
@@ -114,14 +150,14 @@ impl fmt::Display for ModifiedKey {
                 mod_list.push("Shift");
             }
 
-            let mods = mod_list.join(", ");
-            write!(f, "{{ {}, [{mods}] }}", self.0)
+            let mods = mod_list.join("+");
+            write!(f, "<{}+{}>", mods, self.0)
         }
     }
 }
 
-impl From<VirtualKeyCode> for ModifiedKey {
-    fn from(keycode: VirtualKeyCode) -> Self {
+impl From<VirtKey> for ModifiedKey {
+    fn from(keycode: VirtKey) -> Self {
         Self(Key::from(keycode), ModifiersState::empty())
     }
 }
@@ -131,12 +167,11 @@ pub struct KeyCombo(pub Vec<ModifiedKey>);
 
 impl fmt::Display for KeyCombo {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        let keys = self
-            .iter()
-            .map(ToString::to_string)
-            .collect::<Vec<_>>()
-            .join(", ");
-        write!(f, "[{}]", keys)
+        for key in self.iter() {
+            write!(f, "{key}")?;
+        }
+
+        Ok(())
     }
 }
 
@@ -158,8 +193,8 @@ impl KeyCombo {
     }
 }
 
-impl From<VirtualKeyCode> for KeyCombo {
-    fn from(key_code: VirtualKeyCode) -> Self {
+impl From<VirtKey> for KeyCombo {
+    fn from(key_code: VirtKey) -> Self {
         KeyCombo(vec![ModifiedKey::from(key_code)])
     }
 }
@@ -267,14 +302,14 @@ impl KeyCombos {
         // We ignore modifier keys since they aren't considered part of combos
         if let Key::Resolved(key) = &modified_key.0 {
             if [
-                VirtualKeyCode::LAlt,
-                VirtualKeyCode::RAlt,
-                VirtualKeyCode::LControl,
-                VirtualKeyCode::RControl,
-                VirtualKeyCode::LWin,
-                VirtualKeyCode::RWin,
-                VirtualKeyCode::LShift,
-                VirtualKeyCode::RShift,
+                VirtKey::LAlt,
+                VirtKey::RAlt,
+                VirtKey::LControl,
+                VirtKey::RControl,
+                VirtKey::LWin,
+                VirtKey::RWin,
+                VirtKey::LShift,
+                VirtKey::RShift,
             ]
             .contains(key)
             {

src/opts/config.rs

@@ -85,13 +85,18 @@ pub struct Config {
 }
 
 impl Config {
+    pub fn load_from_str(s: &str) -> anyhow::Result<Self> {
+        let config = toml::from_str(s)?;
+        Ok(config)
+    }
+
     pub fn load_from_file(path: &Path) -> anyhow::Result<Self> {
         let config_content = read_to_string(path).context(format!(
             "Failed to read configuration file at '{}'",
             path.display()
         ))?;
 
-        Ok(toml::from_str(&config_content)?)
+        Self::load_from_str(&config_content)
     }
 
     pub fn load_from_system() -> anyhow::Result<Self> {

src/opts/tests/error_msg.rs

@@ -1,14 +1,16 @@
+use crate::{keybindings::KeyCombos, opts::Config};
+
 macro_rules! snapshot_config_parse_error {
-    ( $( ($test_name:ident, $md_text:ident) ),* $(,)? ) => {
+    ( $( ($test_name:ident, $config_text:ident) ),* $(,)? ) => {
         $(
             #[test]
             fn $test_name() {
                 $crate::test_utils::init_test_log();
 
-                let err = ::toml::from_str::<$crate::opts::Config>($md_text).unwrap_err();
+                let err = $crate::opts::Config::load_from_str($config_text).unwrap_err();
 
                 ::insta::with_settings!({
-                    description => $md_text,
+                    description => $config_text,
                 }, {
                     ::insta::assert_display_snapshot!(err);
                 });
@@ -20,7 +22,65 @@ macro_rules! snapshot_config_parse_error {
 const UNKNOWN_THEME: &str = r#"light-theme.code-highlighter = "doesnt-exist""#;
 const INVALID_THEME_TY: &str = "light-theme.code-highlighter = []";
 
+const FIX_THIS_SUCKY_ERROR_MESSAGE: &str = r#"
+[keybindings]
+base = [
+    ["ToBottom", { key = " ", mod = ["Ctrl", "shift"] }],
+    ["ZoomOut", [{ key = " ", mod = ["ctrl", "shift"] }, "Enter"]],
+]
+"#;
+
 snapshot_config_parse_error!(
     (unknown_theme, UNKNOWN_THEME),
     (invalid_theme_ty, INVALID_THEME_TY),
+    // FIXME: vv
+    (fix_this_sucky_error_message, FIX_THIS_SUCKY_ERROR_MESSAGE),
+);
+
+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_default();
+    combined_keybindings.extend(keybindings.extra.unwrap_or_default());
+    let err = KeyCombos::new(combined_keybindings).unwrap_err();
+    Ok(err)
+}
+
+macro_rules! snapshot_keycombo_conflict_err {
+    ( $( ($test_name:ident, $config_text:ident) ),* $(,)? ) => {
+        $(
+            #[test]
+            fn $test_name() {
+                $crate::test_utils::init_test_log();
+
+                let err = keycombo_conflict_from_config($config_text).unwrap();
+
+                ::insta::with_settings!({
+                    description => $config_text,
+                }, {
+                    ::insta::assert_display_snapshot!(err);
+                });
+            }
+        )*
+    }
+}
+
+const BASIC_EQUALITY: &str = r#"
+[keybindings]
+base = [
+    ["ToTop", "a"],
+    ["ZoomReset", "a"],
+]
+"#;
+
+const SPECIAL_PREFIX: &str = r#"
+[keybindings]
+base = [
+    ["ToBottom", { key = "Space", mod = ["Ctrl", "Shift"] }],
+    ["ZoomOut", [{ key = "Space", mod = ["Ctrl", "Shift"] }, "Enter", 39]],
+]
+"#;
+
+snapshot_keycombo_conflict_err!(
+    (basic_equality, BASIC_EQUALITY),
+    (special_prefix, SPECIAL_PREFIX),
 );

src/opts/tests/snapshots/inlyne__opts__tests__error_msg__basic_equality.snap

@@ -0,0 +1,8 @@
+---
+source: src/opts/tests/error_msg.rs
+description: "\n[keybindings]\nbase = [\n    [\"ToTop\", \"a\"],\n    [\"ZoomReset\", \"a\"],\n]\n"
+expression: err
+---
+A keycombo starts with another keycombo making it unreachable
+	Combo: a
+	Prefix: a

src/opts/tests/snapshots/inlyne__opts__tests__error_msg__fix_this_sucky_error_message.snap

@@ -0,0 +1,11 @@
+---
+source: src/opts/tests/error_msg.rs
+description: "\n[keybindings]\nbase = [\n    [\"ToBottom\", { key = \" \", mod = [\"Ctrl\", \"shift\"] }],\n    [\"ZoomOut\", [{ key = \" \", mod = [\"ctrl\", \"shift\"] }, \"Enter\"]],\n]\n"
+expression: err
+---
+TOML parse error at line 4, column 5
+  |
+4 |     ["ToBottom", { key = " ", mod = ["Ctrl", "shift"] }],
+  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+data did not match any variant of untagged enum ModifiedKeyOrModifiedKeys
+

src/opts/tests/snapshots/inlyne__opts__tests__error_msg__special_prefix.snap

@@ -0,0 +1,8 @@
+---
+source: src/opts/tests/error_msg.rs
+description: "\n[keybindings]\nbase = [\n    [\"ToBottom\", { key = \"Space\", mod = [\"Ctrl\", \"Shift\"] }],\n    [\"ZoomOut\", [{ key = \"Space\", mod = [\"Ctrl\", \"Shift\"] }, \"Enter\", 39]],\n]\n"
+expression: err
+---
+A keycombo starts with another keycombo making it unreachable
+	Combo: <Ctrl+Shift+Space><Enter><scan code: 39>
+	Prefix: <Ctrl+Shift+Space>