Keybinding system for global keys #135

phaedrus-jaf · 2020-12-04T12:22:45Z

I've been really enjoying using amfora, but it has different keybindings from qutebrowser and my terminal translates ctrl-t to ctrl-a,c to get screen to create a new tab/view. Also, my compositor eats all F1 keypresses. Re-doing the hardcoding has been nice for a couple weeks, but that doesn't let me keep up to date with y'all.

I think this code is pretty straightforward, let me know what you think or if you wanted a totally different design. (I know I didn't ask, I just wrote something that worked for me :)

makew0rld · 2020-12-05T01:43:31Z

Hello, happy to hear you've been enjoying Amfora! Thanks for making this PR, keybindings is a good feature, and it's even better when it's not just on your fork :)

I think your code is good, but I find the format you chose kind of ugly/unclear to write. Using Rune: is verbose, it's weird to have Alt: but Ctrl+, etc. I was thinking you could take a look at cbind's key.go file, for an idea of how decoding would work. I find the way cbind encodes keybindings much cleaner and more intuitive. Feel free to copy that code entirely, it's under the MIT license. Note I'm not looking for the entire functionality of cbind to be copied, just the ideas in that one file.

Some other things:

Use TOML arrays instead separating keybindings with commas. This allows for easily binding commands to the comma key, as well as just simpler parsing
Please keep shiftnumbers in there. It's ugly, but I don't want to break existing configs.
Make sure you're not exporting functions/variables/structs that aren't used elsewhere

phaedrus-jaf · 2020-12-08T07:12:53Z

Good point about the TOML parser... I feel dumb for not seeing that.

I decided to go with Alt- and keep Ctrl-, because it required only minor tweaks to what I had written (and it's simpler than the cbind code). I got rid of the 'Rune:' business. I also decided to handle Space as a special case. If we want to be able to have whitespace in there, a couple strings.ReplaceAll calls would allow that.

And finally, I re-added shift_numbers, because not breaking non-US configs is a good idea. The code I wrote does enforce 10 characters, where the old code did not. I might go back and make it more liberal in a bit here, I want to clear the merge conflict first.

…amfora into key-remapping

phaedrus-jaf · 2020-12-08T07:55:58Z

Ok, I think this should be ready to merge.

phaedrus-jaf · 2020-12-14T20:34:28Z

Oh! I just realized something that should probably go with this: the help screen is static. I'll take a shot at updating it to display the configured keys later tonight. (I'm also happy to update the wiki to add a keybindings page under the configuration page)

makew0rld · 2020-12-14T20:37:31Z

Nice catch, thanks. I'll take a look at everything when I can, after you update the help page.

makew0rld · 2020-12-15T20:08:06Z

Looks like maybe that part is done? Let me know and I'll take a look soon.

phaedrus-jaf · 2020-12-18T02:08:38Z

Yes, I got the help page working, and I think it's ready.

makew0rld

Thank you for working on this! Overall it looks good, I've made some specific comments. Also:

In general, put a space after the hashtag in the config file. So #bind_whatever becomes # bind_whatever.
Add comments to document functions

Also, are you interested in writing a wiki page on this? It would be under the configuration section, like the Proxying page.

config/config.go

default-config.toml

makew0rld · 2020-12-19T00:24:02Z

config/keybindings.go

+	CmdInvalid = 0
+	CmdLink1   = 1
+	CmdLink2   = 2
+	CmdLink3   = 3
+	CmdLink4   = 4
+	CmdLink5   = 5
+	CmdLink6   = 6
+	CmdLink7   = 7
+	CmdLink8   = 8
+	CmdLink9   = 9
+	CmdLink0   = 10
+	CmdTab1    = 11
+	CmdTab2    = 12
+	CmdTab3    = 13
+	CmdTab4    = 14
+	CmdTab5    = 15
+	CmdTab6    = 16
+	CmdTab7    = 17
+	CmdTab8    = 18
+	CmdTab9    = 19
+	CmdTab0    = 20
+	CmdBottom  = iota


Is there any reason why you did it like this? You can just specify = iota at the start.

It was meant as a semantic warning: there's some code that depends on these values being in this relation to each other. Essentially: CmdTab0 - CmdTab1 == 9, and CmdTab[N+1] == 1 + CmdTabN, thus rearranging these would break stuff. Basically, get future programmers to ask, wait, why does this break convention?

I'll add a comment to this effect.

Alright, that makes sense. And yeah a comment is helpful, thanks.

config/keybindings.go

makew0rld · 2020-12-19T00:40:10Z

config/keybindings.go

+	k := e.Key()
+	if k == tcell.KeyRune {
+		cmd, ok = bindings[keyBinding{k, e.Modifiers(), e.Rune()}]
+	} else { // Sometimes tcell sets e.Rune() on non-KeyRune events.


Where have you seen this? Just curious.

Primarily on the Ctrl-Letter-Key events. The event will have e.Key() == KeyCtrlA, e.Modifers() == 2 (for Control) and e.Rune() == 'A'. I originally added a correction for this when generating the bindings table, but it felt awkward, and would break if tcell changed behavior.

display/display.go

display/help.go

Committing suggested changes with diffs Co-authored-by: makeworld <25111343+makeworld-the-better-one@users.noreply.github.com>

phaedrus-jaf · 2020-12-19T05:56:59Z

Just commited the suggestions that could be committed in the GitHub UI. I'll go work through the rest of the conversations in a bit here. (And fix the linter error)

phaedrus-jaf · 2020-12-19T05:58:18Z

And I think I mentioned it, but I'm definitely happy to update the wiki and add a page for keybindings under the configuration page.

phaedrus-jaf · 2020-12-19T08:58:21Z

I think I addressed all the comments and the linter is happy. It probably makes sense to hold off on trying to update the wiki until after the merge.

Merge branch 'master' into key-remapping

makew0rld · 2020-12-24T21:13:30Z

Thanks for adding this! I'm having a little trouble grokking it but I think I'm just a bit tired right now. Overall it looks good.

phaedrus-jaf added 4 commits December 4, 2020 03:59

Keybinding system for global keys

302755b

Cleaner handling of tcell's EventKey.Rune() quirks

cefe158

go fmt and other lint errors

a476eb6

Didn't scroll far enough, should be all the linter errors

49feba4

phaedrus-jaf mentioned this pull request Dec 4, 2020

Customizable keybindings #88

Closed

phaedrus-jaf added 2 commits December 7, 2020 22:10

Derp. I already have a TOML parser...

a4bdbf4

Simplify keybinding format and here-document for top of default.sh

c4a9d3a

phaedrus-jaf added 3 commits December 7, 2020 23:20

Merge branch 'master' of https://github.com/makeworld-the-better-one/…

7e4baf4

…amfora into key-remapping

keybinding support for subscriptions

a15fed6

blah. camelCaseOnly...

f75229c

phaedrus-jaf added 5 commits December 14, 2020 22:40

Display configurable keys correctly in the help panel

85decae

Use clearer formatting for tab and link keys

3c3f805

Explanatory comment and try appease linter

7176ad6

Bleh. Now I understand what it meant. Appeasing the linter

d85adac

I really need to fix my vimrc for golang...

b5e61eb

makew0rld requested changes Dec 19, 2020

View reviewed changes

Apply suggestions from code review

b5c7d92

Committing suggested changes with diffs Co-authored-by: makeworld <25111343+makeworld-the-better-one@users.noreply.github.com>

Better comments, documentation and code clarity edits

da2eaac

Merge master

60e724a

Merge branch 'master' into key-remapping

makew0rld merged commit f10337e into makew0rld:master Dec 24, 2020

makew0rld added a commit that referenced this pull request Dec 24, 2020

📝 Doc for #135

405087d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keybinding system for global keys #135

Keybinding system for global keys #135

phaedrus-jaf commented Dec 4, 2020

makew0rld commented Dec 5, 2020 •

edited

Loading

phaedrus-jaf commented Dec 8, 2020

phaedrus-jaf commented Dec 8, 2020

phaedrus-jaf commented Dec 14, 2020

makew0rld commented Dec 14, 2020

makew0rld commented Dec 15, 2020

phaedrus-jaf commented Dec 18, 2020

makew0rld left a comment

makew0rld Dec 19, 2020

phaedrus-jaf Dec 19, 2020

makew0rld Dec 21, 2020

makew0rld Dec 19, 2020

phaedrus-jaf Dec 19, 2020

phaedrus-jaf commented Dec 19, 2020

phaedrus-jaf commented Dec 19, 2020

phaedrus-jaf commented Dec 19, 2020

makew0rld commented Dec 24, 2020

Keybinding system for global keys #135

Keybinding system for global keys #135

Conversation

phaedrus-jaf commented Dec 4, 2020

makew0rld commented Dec 5, 2020 • edited Loading

phaedrus-jaf commented Dec 8, 2020

phaedrus-jaf commented Dec 8, 2020

phaedrus-jaf commented Dec 14, 2020

makew0rld commented Dec 14, 2020

makew0rld commented Dec 15, 2020

phaedrus-jaf commented Dec 18, 2020

makew0rld left a comment

Choose a reason for hiding this comment

makew0rld Dec 19, 2020

Choose a reason for hiding this comment

phaedrus-jaf Dec 19, 2020

Choose a reason for hiding this comment

makew0rld Dec 21, 2020

Choose a reason for hiding this comment

makew0rld Dec 19, 2020

Choose a reason for hiding this comment

phaedrus-jaf Dec 19, 2020

Choose a reason for hiding this comment

phaedrus-jaf commented Dec 19, 2020

phaedrus-jaf commented Dec 19, 2020

phaedrus-jaf commented Dec 19, 2020

makew0rld commented Dec 24, 2020

makew0rld commented Dec 5, 2020 •

edited

Loading