support modifying default keybindings prototype #356
Conversation
| @@ -33,7 +33,7 @@ import ( | |||
| ) | |||
|
|
|||
| type Model struct { | |||
| keys keys.KeyMap | |||
| keys *keys.KeyMap | |||
There was a problem hiding this comment.
I am actually not sure this is used, it should be a duplicate of the pre-created KeyMap from the keys.go file.
config/parser.go
Outdated
| helpDesc = previous.Help().Desc | ||
| } | ||
|
|
||
| if len(kb.Keys) > 0 { |
There was a problem hiding this comment.
Do you expect people to want alts? Is this a personal need of yours? I don't expect many users will actually define multiple keys for the same action.
If there's not a big use case, maybe it's better to omit to reduce the config bloat
There was a problem hiding this comment.
Yea I wasn't entirely sure, I can't see myself particularly using it. So that is fair.
There was a problem hiding this comment.
Removed in latest commit
| @@ -158,3 +159,52 @@ var Keys = KeyMap{ | |||
| key.WithHelp("q", "quit"), | |||
| ), | |||
| } | |||
|
|
|||
| // Rebind will update our saved keybindings from configuration values. | |||
| func Rebind(universal, issueKeys, prKeys []config.Keybinding) error { | |||
There was a problem hiding this comment.
do you mean to handle issueKeys in a later PR?
There was a problem hiding this comment.
It matches the pr keys stuff so I just didn't handle it yet with the questions I had, but it will be part of this PR.
ui/keys/keys.go
Outdated
|
|
||
| func rebindUniversal(universal []config.Keybinding) error { | ||
| for _, kb := range universal { | ||
| log.Debug("Rebinding key", "builtin", kb.Builtin, "key", kb.Key) |
There was a problem hiding this comment.
Will be adding to prs and issue keys as well.
| for _, kb := range universal { | ||
| log.Debug("Rebinding key", "builtin", kb.Builtin, "key", kb.Key) | ||
| switch kb.Builtin { | ||
| case "up": |
There was a problem hiding this comment.
I think you can use the keys.Keys global var here instead of the switch case.
If you don't like updating a global var, you can expose a method in the keys package. I think this will be automatically picked up in the ui.go file
There was a problem hiding this comment.
The switch is used to map the yaml string to the keybinding in the global var. I am not sure how to go bind the two otherwise. Other then maybe reflection.
ui/keys/keys.go
Outdated
| Keys.Up = kb.NewBinding(&Keys.Up) | ||
| case "down": | ||
| Keys.Down = kb.NewBinding(&Keys.Down) | ||
| case "firstline": // should this be first-line or firstLine? |
There was a problem hiding this comment.
I think firstLine would be more readable
There was a problem hiding this comment.
Using camel case for keys in next commit.
| @@ -74,3 +75,45 @@ func PRFullHelp() []key.Binding { | |||
| PRKeys.WatchChecks, | |||
| } | |||
| } | |||
|
|
|||
| func rebindPRKeys(keys []config.Keybinding) error { | |||
There was a problem hiding this comment.
I see there's the SetKeys method on the KeyBinding so maybe you can update the global Keys struct that way?
* Remove keys and just use key * Handle issue and pr keys * Handle all keys in universal
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 []stringthat can be used that mimicsKey stringbut 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.
Images/Videos
coming soon, I plan on streaming on twitch after the questions above are answered, so I will share a clip or two, let me know if you have any particular use cases you will want to see.