fix bug #753 - Action menu shorcut numbers not working anymore

[markgandolfo]

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

[aome510]

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

spotify-player/spotify_player/src/config/keymap.rs

Lines 333 to 337 in f408b48

	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]

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

spotify-player/spotify_player/src/config/keymap.rs

Lines 333 to 337 in f408b48

	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]

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

spotify-player/spotify_player/src/config/keymap.rs

Lines 333 to 337 in f408b48

	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.