Skip to content

Conversation

markgandolfo
Copy link
Contributor

@markgandolfo markgandolfo commented Jun 24, 2025

Fixes #753
Fixes #764

This PR updates the logic to only update count_prefix if the pressed digit key is not handled as bind command or action

Vim motions won't be active on ui.popup
Copy link
Owner

@aome510 aome510 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick fix. While I think it works, I now realize we probably need to handle this feature more carefully.

For example, we prob don't want user to define a keybind with numbers as the prefix. This can be done by returning an error if there are any keybind starting with numbers in

pub fn new(path: &std::path::Path) -> Result<Self> {
let mut config = Self::default();
config.parse_config_file(path)?;
Ok(config)

Secondly, let's say if there is a keybind g 0 c, what happens when you press g then 0 or g then 1. I don't think the current implementation handles either case correctly

  • g -> 0, count_prefix=None, key_sequence.keys=[g 0]
  • g -> 1, count_prefix=1, key_sequence.keys=[]

Unit test is optional in this project (I should have done a better job of setting testing framework). If you can add tests, that's super great but otherwise, it's fine for me.

@markgandolfo
Copy link
Contributor Author

markgandolfo commented Jun 26, 2025

For example, we prob don't want user to define a keybind with numbers as the prefix. This can be done by returning an error if there are any keybind starting with numbers in

pub fn new(path: &std::path::Path) -> Result<Self> {
let mut config = Self::default();
config.parse_config_file(path)?;
Ok(config)

We could do something like the below to warn users.

diff --git a/spotify_player/src/config/keymap.rs b/spotify_player/src/config/keymap.rs
index ac40896..e984aa5 100644
--- a/spotify_player/src/config/keymap.rs
+++ b/spotify_player/src/config/keymap.rs
@@ -372,6 +372,29 @@ impl KeymapConfig {
                         self.actions.push(action);
                     }
                 });
+
+                // Validate that no keybinds start with numbers
+                let validate_key_sequence = |key_seq: &KeySequence, name: &str| -> Result<()> {
+                    if let Some(first_key) = key_seq.keys.first() {
+                        if let Key::None(crossterm::event::KeyCode::Char(c)) = first_key {
+                            if c.is_numeric() {
+                                return Err(anyhow::anyhow!(
+                                    "{} '{}' starts with a number. This isn't supported as it clashes with vim-style motions.",
+                                    name, key_seq
+                                ));
+                            }
+                        }
+                    }
+                    Ok(())
+                };
+
+                for keymap in &self.keymaps {
+                    validate_key_sequence(&keymap.key_sequence, "Keybind")?;
+                }
+
+                for action in &self.actions {
+                    validate_key_sequence(&action.key_sequence, "Action keybind")?;
+                }
             }
         }
         Ok(())

Secondly, let's say if there is a keybind g 0 c, what happens when you press g then 0 or g then 1. I don't think the current implementation handles either case correctly

  • g -> 0, count_prefix=None, key_sequence.keys=[g 0]
  • g -> 1, count_prefix=1, key_sequence.keys=[]

Unit test is optional in this project (I should have done a better job of setting testing framework). If you can add tests, that's super great but otherwise, it's fine for me.

I've been thinking about this..
A simple approach may be to

  1. check if the current keysequence belongs to a command
  2. if not, check if the digit is the start or in the middle of a keysequence
  3. treat it like a count otherwise.

(see below)

The other approach would be to do what vim does. I believe it checks to see if the keysequence partially matches a command, and if that's the case it'll wait to see if another keypress occurs.

i.e.
gc -> command1
gcc -> command2

If the user presses gc vim will wait to see if another c is pressed. if not, it'll run command1.
The wait is configurable in vim vim.o.timeoutlen i have mine set to around 300ms.

Option 3 is we revert because this might be a lot to work around, and there could be other unforeseen issues.

--- a/spotify_player/src/event/mod.rs
+++ b/spotify_player/src/event/mod.rs
@@ -105,20 +105,31 @@ fn handle_key_event(
     if ui.popup.is_none() {
         if let Key::None(KeyCode::Char(c)) = key {
             if c.is_ascii_digit() {
-                let digit = c.to_digit(10).unwrap() as usize;
-                // If we have an existing count prefix, append the digit
-                // Otherwise, start a new count (but ignore leading zeros)
-                ui.count_prefix = match ui.count_prefix {
-                    Some(count) => Some(count * 10 + digit),
-                    None => {
-                        if digit > 0 {
-                            Some(digit)
-                        } else {
-                            None
+                // Check if adding this digit to the current key sequence would match any keybind prefix
+                let mut test_sequence = ui.input_key_sequence.clone();
+                test_sequence.keys.push(key);
+
+                let keymap_config = &config::get_config().keymap_config;
+                if keymap_config.has_matched_prefix(&test_sequence) {
+                    // This digit is part of a keybind sequence, not a count prefix
+                    // Let it be processed normally below
+                } else if ui.input_key_sequence.keys.is_empty() {
+                    // Only treat as count prefix if we're not in the middle of a key sequence
+                    let digit = c.to_digit(10).unwrap() as usize;
+                    // If we have an existing count prefix, append the digit
+                    // Otherwise, start a new count (but ignore leading zeros)
+                    ui.count_prefix = match ui.count_prefix {
+                        Some(count) => Some(count * 10 + digit),
+                        None => {
+                            if digit > 0 {
+                                Some(digit)
+                            } else {
+                                None
+                            }
                         }
-                    }
-                };
-                return Ok(());
+                    };
+                    return Ok(());
+                }

@aome510
Copy link
Owner

aome510 commented Jul 26, 2025

For example, we prob don't want user to define a keybind with numbers as the prefix. This can be done by returning an error if there are any keybind starting with numbers in

pub fn new(path: &std::path::Path) -> Result<Self> {
let mut config = Self::default();
config.parse_config_file(path)?;
Ok(config)

We could do something like the below to warn users.

diff --git a/spotify_player/src/config/keymap.rs b/spotify_player/src/config/keymap.rs
index ac40896..e984aa5 100644
--- a/spotify_player/src/config/keymap.rs
+++ b/spotify_player/src/config/keymap.rs
@@ -372,6 +372,29 @@ impl KeymapConfig {
                         self.actions.push(action);
                     }
                 });
+
+                // Validate that no keybinds start with numbers
+                let validate_key_sequence = |key_seq: &KeySequence, name: &str| -> Result<()> {
+                    if let Some(first_key) = key_seq.keys.first() {
+                        if let Key::None(crossterm::event::KeyCode::Char(c)) = first_key {
+                            if c.is_numeric() {
+                                return Err(anyhow::anyhow!(
+                                    "{} '{}' starts with a number. This isn't supported as it clashes with vim-style motions.",
+                                    name, key_seq
+                                ));
+                            }
+                        }
+                    }
+                    Ok(())
+                };
+
+                for keymap in &self.keymaps {
+                    validate_key_sequence(&keymap.key_sequence, "Keybind")?;
+                }
+
+                for action in &self.actions {
+                    validate_key_sequence(&action.key_sequence, "Action keybind")?;
+                }
             }
         }
         Ok(())

Secondly, let's say if there is a keybind g 0 c, what happens when you press g then 0 or g then 1. I don't think the current implementation handles either case correctly

  • g -> 0, count_prefix=None, key_sequence.keys=[g 0]
  • g -> 1, count_prefix=1, key_sequence.keys=[]

Unit test is optional in this project (I should have done a better job of setting testing framework). If you can add tests, that's super great but otherwise, it's fine for me.

I've been thinking about this.. A simple approach may be to

  1. check if the current keysequence belongs to a command
  2. if not, check if the digit is the start or in the middle of a keysequence
  3. treat it like a count otherwise.

(see below)

The other approach would be to do what vim does. I believe it checks to see if the keysequence partially matches a command, and if that's the case it'll wait to see if another keypress occurs.

i.e. gc -> command1 gcc -> command2

If the user presses gc vim will wait to see if another c is pressed. if not, it'll run command1. The wait is configurable in vim vim.o.timeoutlen i have mine set to around 300ms.

Option 3 is we revert because this might be a lot to work around, and there could be other unforeseen issues.

--- a/spotify_player/src/event/mod.rs
+++ b/spotify_player/src/event/mod.rs
@@ -105,20 +105,31 @@ fn handle_key_event(
     if ui.popup.is_none() {
         if let Key::None(KeyCode::Char(c)) = key {
             if c.is_ascii_digit() {
-                let digit = c.to_digit(10).unwrap() as usize;
-                // If we have an existing count prefix, append the digit
-                // Otherwise, start a new count (but ignore leading zeros)
-                ui.count_prefix = match ui.count_prefix {
-                    Some(count) => Some(count * 10 + digit),
-                    None => {
-                        if digit > 0 {
-                            Some(digit)
-                        } else {
-                            None
+                // Check if adding this digit to the current key sequence would match any keybind prefix
+                let mut test_sequence = ui.input_key_sequence.clone();
+                test_sequence.keys.push(key);
+
+                let keymap_config = &config::get_config().keymap_config;
+                if keymap_config.has_matched_prefix(&test_sequence) {
+                    // This digit is part of a keybind sequence, not a count prefix
+                    // Let it be processed normally below
+                } else if ui.input_key_sequence.keys.is_empty() {
+                    // Only treat as count prefix if we're not in the middle of a key sequence
+                    let digit = c.to_digit(10).unwrap() as usize;
+                    // If we have an existing count prefix, append the digit
+                    // Otherwise, start a new count (but ignore leading zeros)
+                    ui.count_prefix = match ui.count_prefix {
+                        Some(count) => Some(count * 10 + digit),
+                        None => {
+                            if digit > 0 {
+                                Some(digit)
+                            } else {
+                                None
+                            }
                         }
-                    }
-                };
-                return Ok(());
+                    };
+                    return Ok(());
+                }

Sorry for the late reply, I haven't had time to come back to this project the last month. After thinking about this problem for a while, I think a simple approach would be only update the count prefix if the command is not handled (implemented in 17a85cf), similar to how we update ui.input_key_sequence.

This approach makes sense cause updating count prefix should have lower priority than handling a command and I found this also handles all the cases I mentioned previously.

@aome510 aome510 merged commit 3499e6d into aome510:master Jul 26, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to use numbers in search Action menu shortcut numbers not working anymore
2 participants