support modifying default keybindings prototype #356

miniscruff · 2024-04-14T08:07:48Z

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

ui/ui.go

dlvhdr

Looks like a great direction. I have some suggestions you play around with that might simplify the code

dlvhdr · 2024-04-19T14:29:47Z

config/parser.go

+		helpDesc = previous.Help().Desc
+	}
+
+	if len(kb.Keys) > 0 {


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

Yea I wasn't entirely sure, I can't see myself particularly using it. So that is fair.

Removed in latest commit

ui/keys/keys.go

dlvhdr · 2024-04-19T14:32:15Z

ui/keys/keys.go

+
+func rebindUniversal(universal []config.Keybinding) error {
+	for _, kb := range universal {
+		log.Debug("Rebinding key", "builtin", kb.Builtin, "key", kb.Key)


Will be adding to prs and issue keys as well.

ui/keys/keys.go

dlvhdr · 2024-04-19T14:36:30Z

ui/keys/keys.go

+			Keys.Up = kb.NewBinding(&Keys.Up)
+		case "down":
+			Keys.Down = kb.NewBinding(&Keys.Down)
+		case "firstline": // should this be first-line or firstLine?


I think firstLine would be more readable

Using camel case for keys in next commit.

ui/keys/prKeys.go

* Remove keys and just use key * Handle issue and pr keys * Handle all keys in universal

ws141 · 2024-05-28T07:32:49Z

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

dlvhdr · 2024-06-01T16:31:58Z

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

miniscruff · 2024-06-01T16:41:43Z

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 · 2024-06-08T05:48:19Z

ui/ui.go

+            // I do not think this state machine works quite well enough with custom keybinds
+            // as it kinda breaks down if you use two different keys across the views.
+            // Is it worth refactoring this to first check the "view" before the key?


@dlvhdr just a question here about the keybind checks. I noticed we are checking for both PR and issue keys at once which sort of breaks if you bind to two separate keys. Wanted to clarify before I made any major changes.

Yeah you're right, we would have to first check the current view

Do you want me to make that change now or leave it for you?

Making the change would be very appreciated 🔥

dlvhdr · 2024-06-08T15:54:21Z

ui/ui.go

+            // I do not think this state machine works quite well enough with custom keybinds
+            // as it kinda breaks down if you use two different keys across the views.
+            // Is it worth refactoring this to first check the "view" before the key?


Yeah you're right, we would have to first check the current view

miniscruff · 2024-06-15T18:08:13Z

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

Omnikron13 · 2024-06-16T03:12:56Z

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).

handle the pr and issue keys in different cases * remove universal switch view for per view binds v4

dlvhdr · 2024-06-20T21:57:26Z

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

support modifying default keybindings prototype

8e02b15

miniscruff commented Apr 14, 2024

View reviewed changes

ui/ui.go Show resolved Hide resolved

dlvhdr reviewed Apr 19, 2024

View reviewed changes

Further work on custom keybindings

2641b13

* Remove keys and just use key * Handle issue and pr keys * Handle all keys in universal

miniscruff commented Jun 8, 2024

View reviewed changes

dlvhdr previously approved these changes Jun 8, 2024

View reviewed changes

dlvhdr self-requested a review June 10, 2024 10:25

miniscruff dismissed dlvhdr’s stale review via 321278c June 15, 2024 18:52

miniscruff marked this pull request as ready for review June 15, 2024 19:29

miniscruff and others added 3 commits June 21, 2024 00:19

Merge branch 'main' into 214-ui-remapping

2375451

Add comment about key check

600c226

handle the pr and issue keys in different cases * remove universal switch view for per view binds v4

fix merge conflicts + format

ff37975

dlvhdr self-assigned this Jun 20, 2024

dlvhdr force-pushed the 214-ui-remapping branch from 321278c to ff37975 Compare June 20, 2024 21:39

dlvhdr added 2 commits June 21, 2024 00:49

readme

01755fc

readme

994578d

dlvhdr merged commit da1221b into dlvhdr:main Jun 20, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support modifying default keybindings prototype #356

support modifying default keybindings prototype #356

miniscruff commented Apr 14, 2024 •

edited

Loading

dlvhdr left a comment

dlvhdr Apr 19, 2024

miniscruff Apr 19, 2024

miniscruff May 10, 2024

dlvhdr Apr 19, 2024

miniscruff May 10, 2024

dlvhdr Apr 19, 2024

miniscruff May 10, 2024

ws141 commented May 28, 2024 •

edited

Loading

dlvhdr commented Jun 1, 2024

miniscruff commented Jun 1, 2024

miniscruff Jun 8, 2024

dlvhdr Jun 8, 2024

miniscruff Jun 8, 2024

dlvhdr Jun 9, 2024

dlvhdr Jun 8, 2024

miniscruff commented Jun 15, 2024

Omnikron13 commented Jun 16, 2024 •

edited

Loading

dlvhdr commented Jun 20, 2024

support modifying default keybindings prototype #356

support modifying default keybindings prototype #356

Conversation

miniscruff commented Apr 14, 2024 • edited Loading

Summary

How did you test this change?

Images/Videos

dlvhdr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ws141 commented May 28, 2024 • edited Loading

dlvhdr commented Jun 1, 2024

miniscruff commented Jun 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miniscruff commented Jun 15, 2024

Omnikron13 commented Jun 16, 2024 • edited Loading

dlvhdr commented Jun 20, 2024

miniscruff commented Apr 14, 2024 •

edited

Loading

ws141 commented May 28, 2024 •

edited

Loading

Omnikron13 commented Jun 16, 2024 •

edited

Loading