Add key bindings for PgUp and PgDn

[sshock]

This adds default key bindings for PgUp and PgDn so that instead of spewing out ugly escape codes they will just be ignored provide history-search-backward and history-search-forward behavior (just like the latest readline 8.2).

Helps with issue ruby/irb#330.

[sshock]

@ima1zumi here is the MR to handle PgUp and PgDn, which we talked about in #434 and decided to pull it out into a separate MR.

Rather than provide the same functionality as up/down arrow keys, I'm still leaning towards just making them be ignored, but I'm also open to having them provide a different, but still useful functionality (that is still related to history).

For example, I found these lines in my /etc/inputrc, which makes me believe there are others who see usefulness in mapping PgUp/PgDn to something else that is still history-related, but not the same thing as up/down arrow keys:

# mappings for "page up" and "page down" to step to the beginning/end of the history
# "\e[5~": beginning-of-history
# "\e[6~": end-of-history

# alternate mappings for "page up" and "page down" to search the history
# "\e[5~": history-search-backward
# "\e[6~": history-search-forward

(It looks like reline doesn't support beginning_of_history and end_of_history yet, but it does support history_search_backward and history_search_forward.)

Interestingly, I tested a bunch of shells and none of them support PgUp/PgDn out of the box. Of course, that doesn't necessarily mean reline shouldn't support them by default.

Here's what different shells do:

• csh: the escape codes get spit out (^[[5~ and ^[[6~)
• tcsh: they both print out a ~
• ksh: changes case on a character (like hitting ~ in vi command mode)
• bash: PgUp is ignored; PgDn prints out a ~
• zsh: changes case on 5 or 6 characters (this is psycho!)
• fish: they both get ignored

I believe the shells that print out ~ (or change case like a ~ in vim cmd mode) aren't doing that on purpose, but rather it's a side effect of the first part of the escape sequence (^[[5) getting ignored while the last character (~) doesn't.

So we have most shells really just don't handle these at all, and only one or two do handle them, but only to make them ignored.

So I don't think it is a bad decision to have reline ignore them. At least it's a step up from allowing the escape codes to get spewed out. (I don't think anyone gets pleasure from that).

So maybe shells are either just really old and never bothered to support PgUp/PgDn, or there was never a general consensus on what they should do, so they've decided to leave it up to people to choose the desired behavior by modifying their inputrc.

So here are some choices we have for PgUp/PgDn in reline:

1. Leave unhandled so ugly escape codes will get printed out.
2. Ignore them by mapping to :ed_ignore.
3. Make them do the exact same thing as up/down arrow keys.
4. Make them do history-search-backward and history-search-forward.
5. Make them do beginning-of-history and end-of-history (this requires first adding those methods).

Given there are no shells that enable behavior by default, I think the best course of action would be for reline also to not provide a default behavior, but rather left it up to individuals to introduce a behavior (which they can do by modifying their inputrc).

However, leaving the code as is and just allowing the escape codes to get printed out is distracting to say the least, so I think it is a step up and good to at least ignore them by default, like fish does, which is why number 2 is my preferred choice and what I've implemented here.

But I am open to discussion and am willing to do 3 or 4 if there is a good argument for doing so. Of those two I would lean towards 4 just so people don't get in the habit of thinking pgup/pgdn should behave the same way as
up/down arrow keys, which they probably won't in any other readline application.

[sshock]

@ima1zumi, have you had a chance to look at this?

[tompng]

I think this pull request is related to #514
Many key input will display ugly escape sequence and it should all be ignored.
This pull request seems a workaround only for PgUp and PgDn.

[sshock]

I think this pull request is related to #514 Many key input will display ugly escape sequence and it should all be ignored. This pull request seems a workaround only for PgUp and PgDn.

Hi @tompng . Yep, this PR handles PgUp and PgDn, and it does so in the correct and best way possible, using capname key bindings that utilizes your terminfo, so they will get ignored regardless of what escape sequence your terminal sends for them.

Another key we can handle this way is the Insert key, by mapping the kich1 capname. This is the next key I really want to be handled instead of spewing out ugly escape codes, but I haven't verified yet if reline supports going into overwrite/replace mode. If so, we'll want to map it to that action. If not, we'll want to map it to :ed_ignore.

We can also handle all the function keys this way, by mapping kf1, kf2, etc to :ed_ignore. I'll add a note to your issue #514 about that.

I think we're both after the same thing here, but maybe your thinking about it a different way and thinking we should try to fix a bunch of keys at once in one PR. I'm not opposed to that, but I already tried fixing four keys in one PR (delete key, pgup, pgdn, and insert), by handling delete properly and by ignoring the other three, but not only did it take 10 months to get that approved, and I had to go out of my way to add unit tests for ansi.rb that never had unit tests before, but I was also asked to remove everything except the handling of the delete key and create separate PRs for other keys.

So that is why I am now taking it slow and only fixing one or two keys at a time; also, this allows for a discussion about exactly how the keys should be handled, and with PgUp and PgDn especially, it's not necessarily true that we want to ignore them; we could also very well choose to map them to specific useful actions instead. I am waiting for @ima1zumi to weigh in on how we want to handle these two keys.

[ima1zumi]

Thanks for the report. I propose another idea.

[ima1zumi]

It would be better to make it behave the same as IRB using Readline.

Suggested change

	'kpp' => :ed_ignore,
	'knp' => :ed_ignore,
	'kpp' => :ed_prev_history,
	'knp' => :ed_next_history,

[sshock]

Oh, I didn't even think about checking to see what IRB does when using Readline instead of Reline.

@ima1zumi can you tell me how to test IRB with Readline? Do I just run irb --nomultiline?

Because I tried that and PgUp and PgDn did not behave that way. PgUp got ignored and PgDn turned into a ~.

What is your OS and version of libreadline? I'm running Debian Linux 11 and have libreadline version 8.1-1.

Also, out of curiosity what shell are you using and how does it behave?

I think you are only seeing this behavior because of your specific setup. I think maybe your readline has been specifically configured to handle them. Can you check your ~/.inputrc and /etc/inputrc? Can you test again after temporarily removing those files or anything else that could be influencing readline?

[st0012]

@sshock I also observed what @ima1zumi described because that's a new behaviour added to readline 8.2 (changelog). Quote:

d. Automatically bind termcap key sequences for page-up and page-down to
history-search-backward and history-search-forward, respectively.

I appreciate that you tested readline with IRB yourself and shared your findings. However, it'd be great if you don't make quick assumptions on other people's setup follow by a series of questions. I know the intention was good but it could be seen as a bit aggressive and could make others uncomfortable.

Also, a quick way to share setup info is to paste the output of the irb_info command:

For example,

irb(main):001:0> irb_info
Ruby version: 3.2.1
IRB version: irb 1.6.3 (2023-03-06)            
InputMethod: RelineInputMethod with Reline 0.3.2
RUBY_PLATFORM: arm64-darwin22                  
LANG env: en_US.UTF-8                          
East Asian Ambiguous Width: 1  

With ~/.inputrb

irb(main):001:0> irb_info
Ruby version: 3.2.1
IRB version: irb 1.6.3 (2023-03-06)            
InputMethod: RelineInputMethod with Reline 0.3.2 and /Users/hung-wulo/.inputrc
RUBY_PLATFORM: arm64-darwin22                  
LANG env: en_US.UTF-8                          
East Asian Ambiguous Width: 1  

With readline

irb(main):001:0> irb_info
Ruby version: 3.2.1
IRB version: irb 1.6.3 (2023-03-06)
InputMethod: ReadlineInputMethod with ext/readline 8.2
RUBY_PLATFORM: arm64-darwin22
LANG env: en_US.UTF-8
East Asian Ambiguous Width: 1

[sshock]

@sshock I also observed what @ima1zumi described because that's a new behaviour added to readline 8.2 (changelog). Quote:

d. Automatically bind termcap key sequences for page-up and page-down to
history-search-backward and history-search-forward, respectively.

Oh, neat!

I appreciate that you tested readline with IRB yourself and shared your findings. However, it'd be great if you don't make quick assumptions on other people's setup follow by a series of questions. I know the intention was good but it could be seen as a bit aggressive and could make others uncomfortable.

Very sorry. My system was up-to-date, so it never occurred to me that I might have an old libreadline, and even if I did think of that I never would have imagined that they would begin to provide default behavior for these keys after so many years of not really handling them.

I will try to be more careful in the future and not jump to conclusions.

Also, a quick way to share setup info is to paste the output of the irb_info command:

This is very useful, thanks!

[sshock]

@sshock I also observed what @ima1zumi described because that's a new behaviour added to readline 8.2 (changelog). Quote:

d. Automatically bind termcap key sequences for page-up and page-down to
history-search-backward and history-search-forward, respectively.

Good news. I was able to test on a newer system with readline 8.2 and confirm that pgup/pgdn using IRB w/ readline does indeed have history-search-backward and history-search-forward behavior as described in the changelog.

I will update the PR shortly. I am in 100% in agreement with making reline match the behavior of readline by default.

[sshock]

@st0012 @ima1zumi ready for re-review 🙏 Let's get this in!

I would like to clear up one point of confusion.

• For history-search-backward behavior we need to use :history_search_backward or :ed_search_prev_history.
• For history-search-forward behavior we need to use :history_search_forward or :ed_search_next_history.

So I've used :ed_search_prev_history and :ed_search_next_history, and it is working well and as expected.

Keep in mind that :ed_prev_history and :ed_next_history are not the same thing as :ed_search_prev_history and :ed_search_next_history, although they do behave almost identically when no characters have been entered yet.

[ima1zumi]

So I've used :ed_search_prev_history and :ed_search_next_history, and it is working well and as expected.

Thank you! I agree with you.

[ima1zumi]

👍