Skip to content

Add custom keybindings #325

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.

Sign up for GitHub

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

Jump to bottom
Closed
ghost wants to merge 4 commits into from
Closed

Add custom keybindings #325

ghost wants to merge 4 commits into from

Conversation

@ghost
Copy link

@ghost ghost commented Nov 18, 2021

Hi and thanks for doing gomuks!
This PR is an approach to add customizable keybindings; this has been discussed in several related issues like #69 #70 #112 and #128. This is just a proposal, I'd love to improve this PR if you have any suggestions.

I've never written much golang code before and was mainly adapting what I already saw, so please let me know if you spot anything that's just bad practice.

This PR

  • adds a keybinding config, that is read from $HOME/.config/gomuks/keybindings.yaml
  • uses cbind's Encode to translate key(combination)s to strings
  • adds some callbacks to ui/view-main.go

Also

  • Key combinations or callbacks that cannot be found in the maps will be ignored and handled as usual by gomuks
  • I choose the versions of the added dependencies to be compatible i.e. with go-runewidth, though I'm not particularly happy I had to add tcell/v2, not sure if that's a problem. To me, the convenience of using Encode seemed to be worth it.
  • I'm also not sure if the callbacks should stay in ui/view-main.go or if there would be a more suitable place. I could also imagine a separate keybindings struct that the main view "registers" to.
  • Names of callbacks or mapping strings can of course be changed to anything more suitable.

Would be happy to hear your thoughts. My keybindings.yaml looks like this:

keybindings:
  "Ctrl+P": "previous_room"
  "Ctrl+N": "next_room"
  "Alt+g": "goto_room"
  "Alt+a": "active_room"
  "Alt+b": "word_left"
  "Alt+f": "word_right"
  "Ctrl+B": "char_left"
  "Ctrl+F": "char_right"
  "Alt+j": "scroll_down"
  "Alt+k": "scroll_up"
  "Ctrl+D": "remove_char"
  "Ctrl+W": "remove_previous_word"
  "Alt+d": "remove_next_word"
  "Ctrl+A": "move_to_beginning"
  "Ctrl+E": "move_to_end"
  "Ctrl+U": "kill_to_beginning"
  "Ctrl+K": "kill_to_end"

@3nprob
Copy link

3nprob commented Dec 5, 2021 •
edited
Loading

This is a well-needed feature so this PR is much appreciated and it looks like a promising start.

One thought I have, though, is that this is a new system completely separate from the existing shortcuts in https://docs.mau.fi/gomuks/shortcuts.html like https://github.com/tulir/gomuks/blob/0a493d643c3a15779dcee35583f8e5c59fde3031/ui/view-main.go#L183

So it only adds new shortcuts, it can not disable or change.

Is thee a way to have a unified config that (as far as reasonable) keeps current keybinding behavior as default but allows rebinding anything?

One thing to consider is that some keybindings are contextual. Maybe the simple thing to do is just to have that e.g. a s a prefix in the property name instead of adding complexity for that. Or it could be nice as toml.

@ghost
Copy link
Author

ghost commented Dec 5, 2021

@3nprob thanks for the encouraging feedback!

So it only adds new shortcuts, it can not disable or change.

Is thee a way to have a unified config that (as far as reasonable) keeps current keybinding behavior as default but allows rebinding anything?

I'm not sure if that's considered a clean way, but the way I've implemented this shortcut system, it looks up key events in the custom keybindings map before any key events are handled in the original function. If a custom keybinding is found, the key event is not forwarded, otherwise the default is used. So effectively, the new keybindings overwrite the existing default, but with no specified keybinding, default is always there as fallback.

So unbind existing defaults, I would probably ad a noop function to map it in the custom keybindings file.

One thing to consider is that some keybindings are contextual. Maybe the simple thing to do is just to have that e.g. a s a prefix in the property name instead of adding complexity for that. Or it could be nice as toml.

Right, so far I haven't thought about contextual bindings at all. I could very well imagine prefix or toml or more nested yaml as options to implement that.

@3nprob 3nprob mentioned this pull request Dec 7, 2021
Merged
@3nprob
Copy link

3nprob commented Dec 7, 2021 •
edited
Loading

@Schubisu I made my own attempt according to the above at #328 . overall very similar to what you have here, but replacing current behavior fully. If it turns out to be better starting point we should bring over your added functionality there as well.

Hopefully together we can get through something good ^^

@tulir
Copy link
Member

tulir commented Mar 6, 2022

The unified keybinding system in #328 does sound preferable

@tulir tulir closed this in f652501 Mar 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@3nprob @tulir