Skip to content
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

Fix duplicate key issue #585

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

RedEtherbloom
Copy link
Contributor

@RedEtherbloom RedEtherbloom commented Jun 11, 2024

Cause of Issue #579 was found in line

self.duplicate = edit.unwrap_or(self.duplicate);
.

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
Copy link
Owner

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

src/keyconfig.rs Outdated Show resolved Hide resolved
@RedEtherbloom
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Owner

kdheepak commented Jun 13, 2024

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 #520

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.

2 participants