Better reverse search KeyboardInterrupt handling #2895

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants

kanzure commented Feb 7, 2013

The previous KeyboardInterrupt handling is dangerous because previous
lines or commands can be mistakenly executed.

When a user interrupts a readline reverse search (C-r) with C-c, a
KeyboardInterrupt is thrown, but the readline buffer still has a line in
the buffer from the search attempt. The previous behavior of IPython was
to show a blank line instead of the current readline buffer, causing a
previous command to be executed if the user presses enter on the
seemingly blank line.

Additionally, keyboard input was not shown on the blank line because
readline was still in reverse search mode. For example, pressing C-k on
the blank line would reset readline, but most users would expect the
input buffer that IPython displays to be correct because of the
prominently displayed KeyboardInterrupt.

This modifies how a KeyboardInterrupt is handled in IPython. Under
normal circumstances in the operation of readline, a KeyboardInterrupt
should never be thrown. IPython now displays a helpful reminder to use
default readline keybindings (which might be overridden by ~/.inputrc,
but whatever), and then displays the current line buffer on the next
line.

Another possible solution would be to send C-g (abort reverse search) or
C-k to readline through raw_input somehow. However, this solution would
require checking ~/.inputrc to figure out what the current "abort
reverse search" keybinding is.

fixes #1286
fixes #2486

@kanzure kanzure better reverse search KeyboardInterrupt handling
The previous KeyboardInterrupt handling is dangerous because previous
lines or commands can be mistakenly executed.

When a user interrupts a readline reverse search (C-r) with C-c, a
KeyboardInterrupt is thrown, but the readline buffer still has a line in
the buffer from the search attempt. The previous behavior of IPython was
to show a blank line instead of the current readline buffer, causing a
previous command to be executed if the user presses enter on the
seemingly blank line.

Additionally, keyboard input was not shown on the blank line because
readline was still in reverse search mode. For example, pressing C-k on
the blank line would reset readline, but most users would expect the
input buffer that IPython displays to be correct because of the
prominently displayed KeyboardInterrupt.

This modifies how a KeyboardInterrupt is handled in IPython. Under
normal circumstances in the operation of readline, a KeyboardInterrupt
should never be thrown. IPython now displays a helpful reminder to use
default readline keybindings (which might be overridden by ~/.inputrc,
but whatever), and then displays the current line buffer on the next
line.

Another possible solution would be to send C-g (abort reverse search) or
C-k to readline through raw_input somehow. However, this solution would
require checking ~/.inputrc to figure out what the current "abort
reverse search" keybinding is.

fixes #1286
fixes #2486
15b8d71

@takluyver takluyver commented on the diff Feb 7, 2013

IPython/frontend/terminal/interactiveshell.py
source_raw = self.input_splitter.source_raw_reset()[1]
hlen_b4_cell = \
self._replace_rlhist_multiline(source_raw, hlen_b4_cell)
more = False
+ # If a user reverse searches with C-r, then readline will
+ # keep the last line in the input buffer, even if the input
+ # is interrupted with C-c. Instead, the user should use
+ # C-g to exit reverse search mode. There is no good reason
+ # for a user to raise a KeyboardInterrupt under normal
+ # circumstances, and instead the readline keybindings (like
+ # C-g, C-u, C-k) should be used. See issue #1286 or #2486.
+ # Note: ommitting this fix is dangerous because a user
+ # could otherwise cause a dangerous line to be re-executed.
+ self.set_next_input(self.readline.get_line_buffer())
@takluyver

takluyver Feb 7, 2013

Owner

We should make sure this doesn't break with pyreadline on Windows. Pinging @jstenar .

@minrk minrk commented on the diff Feb 8, 2013

IPython/frontend/terminal/interactiveshell.py
try:
- self.write('\nKeyboardInterrupt\n')
+ # these keybindings vary based on ~/.inputrc for GNU
+ # readline
+ self.write('\nKeyboardInterrupt\nUse C-g to exit reverse '
+ 'search mode. Use C-u/C-k for line deletes.')
@minrk

minrk Feb 8, 2013

Owner

Let's not write this longer message. It's much too verbose, and doesn't apply the vast majority of the time the message is shown.

@kanzure

kanzure Feb 8, 2013

I would argue that it always applies. C-c is never the correct key to press, according to the source code (prior to my comments I added to the area).

@minrk

minrk Feb 8, 2013

Owner

But it simply isn't. Telling a user who may have never typed ^r in their life that ^g is how to get out of it is the opposite of helpful. Further, the ^u / ^k are actually dependent on readline config (inputrc), and may not even be true.

Owner

minrk commented Feb 8, 2013

This is better behavior when ^r search is active, but it's actually worse if ^r is not, which is most of the time (normally, the line is just cleared). The right behavior here is to kill the line, but Python stubbornly refuses to expose that part of the readline API.

kanzure commented Feb 8, 2013

While I agree with your complaints about the verbosity and how readline might be configured differently, I still insist that C-c is never correct because readline doesn't support that. Maybe a simpler solution would be to do nothing on C-c?

Owner

takluyver commented Feb 8, 2013

-1. I never use C-r, but I hit C-c to clear the line very often. It works
fine for me - as it does in bash, and the python >>> shell - and I'm not
really interested in learning lots of readline shortcuts.

Thomas

Owner

minrk commented Feb 8, 2013

@takluyver this is definitely not the solution as-is. If this is to be merged, it needs to figure out how to avoid triggering this extra handling outside of ^r.

Owner

takluyver commented Feb 8, 2013

OK, that makes sense. Does Ctrl-r go into any specific part of our code, or is that a pure readline feature?

Owner

minrk commented Feb 9, 2013

Pure readline. I don't know how to tell the difference.

Owner

ellisonbg commented Feb 20, 2013

I agree with @takluyver that I never use C-r, but use C-c to clear the line. Most of our users don't know anything about readline.

Owner

Carreau commented Mar 27, 2013

Status of this PR ? OR should we close it and open a referring issue ?

Owner

minrk commented Mar 27, 2013

Can't be merged as-is until the behavior when ^r is inactive is corrected. The bug is already open as #1286 and #2486, so I don't think there's any new issue to be opened.

Owner

minrk commented Mar 29, 2013

Closing per stale PR policy, with a note in #1286.

minrk closed this Mar 29, 2013

minrk referenced this pull request Mar 31, 2013

Closed

prefer pyrepl to readline #3112

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment