Skip to content

DRAFT - Fix Various Home / End key binding issues #462

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

Closed
wants to merge 4 commits into from
Closed

DRAFT - Fix Various Home / End key binding issues #462

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4afe301
use the smkx sequence to change terminal mode
kcculhwch Aug 25, 2022
3e86762
Add spacing, comments, and return to rx mode
kcculhwch Aug 25, 2022
50c43f6
Add key_delete and kdch1 codes to ansi.rb and key_delete code to the …
kcculhwch Aug 25, 2022
537ee1e
didn't need this here
kcculhwch Aug 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions lib/reline/ansi.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ class Reline::ANSI
'cud' => :ed_next_history,
'cuf' => :ed_next_char,
'cub' => :ed_prev_char,
'kdch1' => :key_delete
Copy link
Contributor

@sshock sshock Sep 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @kcculhwch, I have another suggestion that makes sense to include in your PR:

The four lines above this for 'cuu', 'cud', 'cuf', and 'cub' don't belong and should be removed (along with the special handling for them on lines 100-101). With your smkx fix in place, I don't think they'll be needed anymore.

I think what happened is @aycabta discovered that the cursor keys weren't working with some terminal emulators, including xterm, and he discovered that by mapping these it fixed it.

But that wasn't the right fix; the right fix is your PR, because as you and I both discovered independently, until the terminal has been set to 'smkx' mode, the home and end keys may send different escape sequences that don't align with the output of terminfo. Well, the same is true for the cursor keys.

In other words, this list already includes 'kcuu1', 'kcud1', 'kcuf1', and 'kcub1', which are the correct things to map for the arrow keys. They just weren't working (for some terminals) because reline wasn't putting the terminal in 'smkx' mode. Now with your PR they will work.

The escape sequences for 'cuu', 'cud', 'cuf', and 'cub' are not something the terminal sends when arrow keys are pressed! They are what you send to the terminal to make the cursor move.

It just so happens that in some terminals (well xterm at least), the escape sequences for 'cuu', 'cud', 'cuf', and 'cub' with the "%p1%d" removed from the middle happen to result in the escape sequences for the arrow keys in normal mode, but we don't need to rely on this happenstance anymore.

}

if Reline::Terminfo.enabled?
Expand Down Expand Up @@ -60,8 +61,36 @@ def self.set_default_key_bindings(config)
config.add_default_key_binding_by_keymap(:emacs, key, func)
end
end

Copy link
Contributor

@sshock sshock Sep 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These 10 lines of code already exist below, so now it is in two places. Did you mean to remove it from below?

Copy link
Author

@kcculhwch kcculhwch Sep 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh yes, I'll clean that up.

@@input = STDIN
def self.input=(val)
@@input = val
end

@@output = STDOUT
def self.output=(val)
@@output = val
end

def self.set_default_key_bindings_terminfo(config)

# put terminal in tx mode so that
Copy link
Contributor

@sshock sshock Sep 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "Put terminal in keypad transmit mode" would be a little more accurate.

# keys match terminfo's bindings
begin
@@output.write Reline::Terminfo.tigetstr('smkx')
rescue Reline::Terminfo::TerminfoError
# capname is undefined
end

# return terminal to rx mode
Copy link
Contributor

@sshock sshock Sep 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment needs to be fixed too. It's not about "transmit" vs "receive". Leaving keypad transmit mode doesn't mean the terminal goes into "receive" mode.

I think either of these would work fine:

  • "Leave keypad transmit mode"
  • "Return terminal to normal mode".

For more context, consider xterm: with xterm smkx turns into \E[?1h and \E=, which sets the cursor keys and keypad to application mode, whereas rmkx turns into \E[?1l and \E>, which sets the cursor keys and keypad to normal mode.

at_exit do
Copy link
Contributor

@sshock sshock Mar 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should send the "smkx" and "rmkx" in prep and deprep rather than doing it here in set_default_key_bindings_terminfo and having to rely on an at_exit, which can be problematic.

begin
@@output.write Reline::Terminfo.tigetstr('rmkx')
rescue Reline::Terminfo::TerminfoError
# capname is undefined
end
Comment on lines +87 to +91
Copy link
Contributor

@Maumagnaguagno Maumagnaguagno Sep 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that you can avoid the extra begin/end for the rescue to work inside the block since #12906.

end

key_bindings = CAPNAME_KEY_BINDINGS.map do |capname, key_binding|
begin
key_code = Reline::Terminfo.tigetstr(capname)
Expand Down