-
Notifications
You must be signed in to change notification settings - Fork 248
fix bug #753 - Action menu shorcut numbers not working anymore #754
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Vim motions won't be active on ui.popup
There was a problem hiding this 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
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.
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(())
I've been thinking about this..
(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. If the user presses 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 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. |
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