search

dlvhdr / gh-dash

A beautiful CLI dashboard for GitHub 🚀
https://dlvhdr.github.io/gh-dash
MIT License
6.77k stars 191 forks source link

support modifying default keybindings prototype #356

Closed miniscruff closed 2 weeks ago

miniscruff commented 2 months ago

Fixes #214

Open questions: Not sure if this mega switch case is really the way to go, I thought of creating an enum but either way we have a separate string to keybind map.

Not sure the yaml value for multiple words should be "switchview", "switchView" or some other method.

I had to change the Keys to a pointer so I can actually update them after the initial load. I am not sure if this is ok or if there is some other refactoring/change that would make more sense. For example, I tried putting a KeyMap on the shared ctx that has the config on it. And then just loading it on init, but it pushed more changes that I thought would be appropriate.

I am not sure how to handle the multiple key options for alts. I created a Keys []string that can be used that mimics Key string but with multiple. So it should be backwards compatible but not sure if there was another method you were expecting.

There is an error log on an invalid config, which will need to expand to an invalid config or after that, an invalid keybinding, such as a bad builtin. I am not sure I handled this properly, or if there is a better way, let me know.

Summary

Allow modifying the builtin command keybindings.

How did you test this change?

I have a test config with the new universal option. Currently this is all I am testing.

keybindings:
  universal:
    - key: v
      builtin: switchview

Images/Videos

https://www.twitch.tv/videos/2176382326

ws141 commented 1 month ago

Any chance for a new of this by @dlvhdr ? Would love to be able to rebind the comment navigation.

dlvhdr commented 1 month ago

@miniscruff would you like to resolve the merge conflict and merge?

miniscruff commented 1 month ago

If I remember correctly I was running into issues still around keybinds used in both views not behaving properly. I can put in some time in the next few days to try and finish it.

miniscruff commented 3 weeks ago

Today is the day I finish, and now that I said it, it must come true.

Omnikron13 commented 3 weeks ago

At risk of causing any late-stage headaches here, I have to question if it might not be best to specify the view/context the other way around in the config format; have a keybinding specify which context or contexts it should apply in.

I don't know the full scope you (currently) want the project to cover, but there are theoretically quite a lot of different screens and contexts that could be added in future, which could result in some technical debt with this design. It could easily be desirable to have bindings that apply to multiple views, or to contexts which aren't strictly views but, well, broad contexts (dialogue boxes, text entry fields, lists/tables... even the current 'universal' tbh, though that option wouldn't be without it's own potential technical debt), and having the binding list where it applies is IMO a lot more robust, flexible, and future-proof (added bonus that you could disable a binding by commenting out it's context in the config file).

dlvhdr commented 2 weeks ago

Thanks for contributing this @miniscruff, it's very useful!