kdheepak / taskwarrior-tui

`taskwarrior-tui`: A terminal user interface for taskwarrior
https://kdheepak.com/taskwarrior-tui
MIT License
1.4k stars 68 forks source link

Fix duplicate key issue #585

Open RedEtherbloom opened 1 month ago

RedEtherbloom commented 1 month ago

Cause of Issue #579 was found in line https://github.com/kdheepak/taskwarrior-tui/blob/a768ee4e8bfd227fe5662f83904827b3ecb16939/src/keyconfig.rs#L166.

We needed something to distract ourselves today, so we decided to cleanup the keyconfig.rs while we were at it.

An issue for a bug I found has been created at #586

kdheepak commented 1 month ago

Nice job with the PR! Thanks for the refactor as well! And for writing tests!

RedEtherbloom commented 1 month ago

Nice job with the PR! Thanks for the refactor as well! And for writing tests!

Thank you! It's a pleasure working with you.

RedEtherbloom commented 1 month ago

Thinking through a doc example we now figured out how to sort it in this scenario. (Under the assumption that the KeyCodes are all Chars. We did not entirely understand the code if non-char shortcuts are supported yet.)

Would you be okay with us replacing the type of KeyConfigs field with a new a Action struct of the following form: struct Action { pub key_code: KeyCode, pub action_config_name: &'static str } (and also implementing a from implementation from Action to Key).

This way the name of the config field/action would be defined in one place, preventing errors from misspellings of e.g. "previous-tab" between e.g. update and check and providing more debug information for functions like check(key conflict), at close to zero cost.

The downside is that taskwarrior-tui would have to introduce a lifetime parameter to Action if you'd want to add dynamic shortcuts, as the action names wouldn't be static anymore. The lifetime should be trivial, but could still be annoying to work with.

kdheepak commented 1 month ago

Let's keep that out of this PR and discuss it more in a separate issue or PR?

I was toying with the idea of doing this:

#[derive(Clone, Debug, Default, Deref, DerefMut)]
pub struct KeyBindings(pub HashMap<Mode, HashMap<Vec<KeyEvent>, Action>>);

and experimented with it a bunch as part of other projects. I wrote a template for ratatui based on that experimentation:

https://github.com/ratatui-org/templates/blob/aaed9173b1126a0ffc272cceacf8047ae61da3ed/component/template/src/config.rs#L16-L34

But haven't found the time to pull in those ideas here.

If we can make it a HashMap<Vec<KeyEvent>, Action>, then we'd be able to have chorded keyboard shortcuts (e.g. gt is go to top, gb is go to bottom, gm is go to middle` etc)

We'd be able to have shortcut keys configured per Mode and we'd be able to define keybindings in a json file that can be deserialized to a config struct

{
  "keybindings": {
    "Home": {
      "<q>": "Quit", // Quit the application
      "<Ctrl-c><Ctrl-c>": "Quit", // Yet another way to quit
      "<g><g>": "GoToTop" 
    },
  }
}

That is my goal ideally but open to suggestions. One related issue is https://github.com/kdheepak/taskwarrior-tui/issues/520