Skip to content
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.

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

Add keybindings for CTRL, ALT, SHIFT + UP, DOWN, RIGHT, LEFT, HOME, END, BACKSPACE, DELETE & more #3996

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

masmu
Copy link

@masmu masmu commented Sep 13, 2024

This PR implements some missing key bindings.

In particular all possible combinations of

  • CTRL
  • ALT
  • SHIFT
  • ALT+SHIFT
  • CTRL+ALT
  • CTRL+SHIFT
  • CTRL+ALT+SHIFT

in conjunction with Up, Down, Left, Right, Home, End, Backspace, Delete, PageUp, PageDown.

I added tests to the LightRenderer beforehand to make sure I wasn't breaking anything, and expanded them when I added new key bindings.

So you can actually do stuff like:

fzf
  --bind 'ctrl-up:up+up+up+up+up'
  --bind 'ctrl-down:down+down+down+down+down'
  --bind 'ctrl-right:forward-word'
  --bind 'ctrl-left:backward-word'
  --bind 'alt-delete:delete-char'
  --bind 'alt-backspace:backward-kill-word'

This PR fixes #1747.

@junegunn
Copy link
Owner

Thanks, I'll review the code when I get some time.

A few things.

  • We also have to update tcell renderer for Windows
  • man page needs to be updated accordingly
  • Terminal emulators I use (kitty, iterm2, etc) don't distinguish between the CTRL-* chords you added from the ones without CTRL. Which terminal emulator do you use on what platform? If these chords are not widely supported by the terminal emulators, we should warn about that on the manual.
  • Allowing all possible names (ctrl-alt-shift, alt-shift-ctrl, shift-ctrl-alt, etc) will make the code harder to maintain. I'd just allow one sequence. i.e. ctrl-alt-shift.

@masmu
Copy link
Author

masmu commented Sep 17, 2024

No worries, take your time.

We also have to update tcell renderer for Windows

Yep, I checked tcell.go and most of it should be pretty straight forward to implement. My biggest issue is that I do not have any Windows machine to test on. So I can do an implementation, make sure it compiles but that's probably it. Would you mind to assist here and test it? Or is there a way to do this on Linux?

man page needs to be updated accordingly

True. Do you write the man page by hand? I was hoping it might be generated by some docstring, although I didn't find one when I searched for it.

Terminal emulators I use (kitty, iterm2, etc) don't distinguish between the CTRL-* chords you added from the ones without CTRL. Which terminal emulator do you use on what platform? If these chords are not widely supported by the terminal emulators, we should warn about that on the manual.

SGTM. I use gnome-terminal and ddterm both running most of the time zsh + tmux, sometimes just zsh or bash. Platform is Linux.

Allowing all possible names (ctrl-alt-shift, alt-shift-ctrl, shift-ctrl-alt, etc) will make the code harder to maintain. I'd just allow one sequence. i.e. ctrl-alt-shift.

I tried to stick as close to your implementation as possible and saw this case "alt-shift-right", "shift-alt-right": case where you actually support both combinations. But I don't mind removing the redundant ones. Actually I like that choice 😄

@masmu masmu force-pushed the feature/additional-keybindings branch from d7d08bc to aad5e49 Compare September 18, 2024 09:54
@masmu
Copy link
Author

masmu commented Sep 18, 2024

I made the mentioned changes and the remaining exciting question is if the following block (tcell.go:~390) is correct:

case tcell.KeyBackspace2:
	if ctrlAlt {
		return Event{CtrlAltBackspace, 0, nil}
	}
	if ctrl {
		return Event{CtrlBackspace, 0, nil}
	}
	if alt {
		return Event{AltBackspace, 0, nil}
	}
	return Event{Backspace, 0, nil}

My educated guess is that it is actually not and has to be covered in the following block:

case tcell.KeyCtrlH:
	switch ev.Rune() {
	case 0:
		if ctrl {
			return Event{Backspace, 0, nil}
		}
	case rune(tcell.KeyCtrlH):
		switch {
		case ctrl:
			return keyfn('h')
		case alt:
			return Event{AltBackspace, 0, nil}
		case none, shift:
			return Event{Backspace, 0, nil}
		}
	}

@junegunn
Copy link
Owner

Or is there a way to do this on Linux?

You can run go run -tags tcell main.go.

@masmu
Copy link
Author

masmu commented Sep 20, 2024

Thanks for the hint!

But I am trying to get my hands on a windows machine as well because it all depends on what the OS is really gonna send. I will report back after that.

@masmu masmu force-pushed the feature/additional-keybindings branch from aad5e49 to d90cff2 Compare September 30, 2024 11:28
@nirodhvana
Copy link

is this ready for merging?

@masmu
Copy link
Author

masmu commented Oct 2, 2024

Nope, not yet.

The Linux implementation is done. Windows implementation potentially as well, but requires a patch in tcell to make the following shortcuts work:

  • Ctrl-Alt-*
  • Ctrl-Alt-Shift-*

That's why this PR currently points to my own fork of tcell and that keeps this from being reviewed and merged. I created a PR upstream and hope for a quick review.

You can build this PR yourself. That should work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add key binding options for {ctrl,alt}-{backspace,del,left,right}
3 participants