Adding support for selecting text with shift + movement #989
Conversation
scoennz
force-pushed
the
shift-selection
branch
from
b86fd05
to
75db1e1
Compare
|
I haven't done a close review of this but it sounds similar to how I implemented it in my own project (search https://github.com/asmeurer/mypython/blob/860f212d341956bf39cb0ee75db3248ffbfc1615/mypython/keys.py for "shift_arrow"). |
|
Hi, asmeurer. Indeed, same sort of logic as yours. |
|
I just tested this and it seems to have the same sort of behavior that I have been using. The only difference I noticed is that I made it in mine so that if there is a selection, pressing "up" cancels the selection without going back in history. I'm not really sure why I did that though and I'm not sure if it really makes sense. |
|
I wasn't able to test this directly on my prompt application because it doesn't work with prompt-toolkit 3.0 yet. Is 3.0 going to be another big backwards incompatible release? |
|
Also I never noticed until now that Shift-Arrow doesn't work by default in my Linux terminal (Konsole) or the default macOS Terminal. It works in iTerm2, but I can't remember if that's out of the box or if I configured it. |
|
I can easily port my implementation to the 2.0.9 branch. I'll do a separate branch for that. |
|
I prefer to not backport this change to 2.0, and have new features only on the 3.0 branch. The 3.0 branch is for 90% compatible with 2.0. The only change is that it uses asyncio natively and requires Python 3.6 because of this (async-for requirement). If really important to many people we can probably make it run on Python 3.5. Many applications don't require any changes for the 3.0 branch. IPython requires a few, but not much: https://github.com/ipython/ipython/pull/11726/files edit: I have not had the chance yet to review the changes in this PR, but hope to do soon. |
| @@ -139,6 +144,11 @@ def is_returnable() -> bool: | |||
| handle('escape', 'enter', filter=insert_mode & is_returnable)( | |||
| get_by_name('accept-line')) | |||
|
|
|||
| # It appears that on windows, Enter -> ^M but Ctrl-Enter brings in a ^J; | |||
| # We handle this the same as escape-enter | |||
| handle('escape', 'c-j', filter=insert_mode & is_returnable)( | |||
There was a problem hiding this comment.
This is probably due to a bug in input/win32.py. It's supposed to be 'escape', 'c-m' too.
There was a problem hiding this comment.
Yes, I wasn't too sure how to address this, and didn't want to break things by modifying `input/win32.py' incorrectly. There seems to be some inconsistencies in the code. The issue is that on windows,
enterbrings'\r'- ascii 13 -'c-m'ctrl-enterbrings'\n'- ascii 10 -'c-j'and theCTRL_PRESSEDflag
input/win32.py converts CTRL_PRESSED 'c-j' (which is ctrl-enter) into 'escape', 'c-j', but then that is NOT handled by the 'escape','enter' binding, because 'enter' is 'c-m' (issue 1).
Also earlier in input/win32.py, we find this piece of code:
if self.mappings[ascii_char] == Keys.ControlJ:
u_char = '\n' # Windows sends \n, turn into \r for unix compatibility.
result = KeyPress(self.mappings[ascii_char], u_char)
The comment seems to tell the correct story, but then that is not what is done. u_char should be set to '\r' here (setting it to '\n' doesn't do anything; it is already that) and then we should call again ascii_char = u_char.encode('utf-8') so that in the next line (result = ...) we get KeyPress('c-m', '\r'). At the moment, hitting ctrl-enter still leaves us with a 'c-j' (issue 2). On top of that fix then, we have to fix issue 1 above: it should be CTRL_PRESSED 'c-m' that is converted into 'escape', 'c-m' rather than CTRL_PRESSED 'c-j.'
I can see this would work, but then wasn't sure if this wouldn't break anything else. Given the importance of the enter key, I didn't want to mess it up. Should I fix that as part of this pull request, or should I make it a separate pull request?
There was a problem hiding this comment.
Have actually tried the solution I propose above, but for some reason it does not work. When hitting ctrl-enter, the prompt displays ^[ and then insert a carrier return (adds a line). Like the two keys, 'escape','c-m', are treated separately. Note that when I hit ctrl-Q (to display escape sequences) and then ctrl-enter, it does display ^[^M, so the c-j is correctly converted to c-m, but somehow something else happens after that ...
There was a problem hiding this comment.
Maybe there is a problem with the KEY_ALIASES mechanism. Like the 'escape','c-m' is not actually converted into 'escape','enter' ... Just my 5c here, not too sure when this is called.
There was a problem hiding this comment.
Got a bit deeper in this issue here. The problem seems to be that 'escape','c-m' is turned into a BracketedPaste event because win32.ConsoleInputReader._is_paste returns true for that key combination ('escape' is considered str and 'c-m' is a newline.) Not sure what to do here. Might be simpler to pass ctrl-enter as 'escape','c-j' and binds to that rather than tamper with the paste mechanism? Or even define a 'c-enter' key, and not translate it into an `escape' combination at all?
There was a problem hiding this comment.
Right. Thanks for taking the time to dig into this issue.
I'm not sure yet what's the best approach. The _is_paste method is a workaround, because the Windows console doesn't support bracketed paste, but it's important to detect it.
Probably we can simply ignore the escape key in here, and it should be fine.
The chance that people literally paste "escape + enter" and expect it to be inserted as text literally like that is very small...
| @@ -234,6 +244,133 @@ def _(event: E) -> None: | |||
| """ | |||
| event.current_buffer.exit_selection() | |||
|
|
|||
| def unshift_move(event: E): | |||
There was a problem hiding this comment.
This needs a return annotation -> None: in order to tell mypy to typecheck this function.
There was a problem hiding this comment.
Can you add a docstring too for this function?
| @@ -42,6 +42,10 @@ def __init__(self, original_cursor_position: int = 0, | |||
|
|
|||
| self.original_cursor_position = original_cursor_position | |||
| self.type = type | |||
| self.shift_mode = False | |||
There was a problem hiding this comment.
I think this attribute needs to go into key_bindings.emacs_state.EmacsState, because it's specific to the Emacs key bindings.
There was a problem hiding this comment.
That's what I had initially done, but then you need to make sure that that flag is set back to False when the selection is removed by any existing or future keybinding. So either that means adding stuff in quite a few key bindings, or handling this in buffer.py, wherever selection_state is set to None (in exit_selection and copy_selection and maybe somewhere else). That's why I prefer having the shift_mode flag in selection_state, since it then becomes automatic. Also, having it in selection_state opens the way to extending the shift-selection mode to maybe the vi mode (or any other future mode?) in the future? See also my two points at the top of my initial pull request description. What do you think?
There was a problem hiding this comment.
That makes sense. Thanks! Just keep it.
| command = 'beginning-of-buffer' | ||
| if key == Keys.ShiftControlEnd: | ||
| command = 'end-of-buffer' | ||
| get_by_name(command)(event) |
There was a problem hiding this comment.
Keep in mind that command is potentially undefined. This can raise a NameError.
I think I'd prefer a lookup table that maps the keys to the commands. Something like this:
if key == Keys.ShiftUp:
...
return
...
key_to_command = {
Keys.ShiftHome: 'beginning-of-line',
Keys.ShiftEnd: 'end-of-line',
...
}
try:
# Both the dict lookup and `get_by_name` can raise KeyError.
handler = get_by_name(key_to_command[key])
except KeyError:
pass
else: # (`else` is not really needed here.)
handler(event)
|
This looks very nice actually. I have a few remarks (see comments). |
|
@jonathanslenders will you be making another porting guide? I guess there are other changes. It seems my method of adding custom keys no longer works. I haven't had a chance to look into it. How long will it be until 3.0 is released? |
|
Yes, there is an upgrade guide in the works: https://github.com/prompt-toolkit/python-prompt-toolkit/blob/master/docs/pages/upgrading/3.0.rst I have to add a few more things. I'm not sure what changed to the key bindings. I really hope to release soon, definitely before the end of 2019. I prefer not to hurry, because I really don't like to do breaking changes after the release and want to make sure it's stable. (Right now, I think stabliity is already fine though.) |
|
Thanks. I'll try to port my code soon so that I can report any issues before the release. And sorry for hijacking this pull request discussion. I'll open new issues for any issues I come across. |
|
Hi Jonhatan, I have implemented your suggestions. The only remaining issue is the |
|
Thanks a lot for this @scoennz, for the changes! I really appreciate it. |
| if result.key == Keys.End: | ||
| result.key = Keys.ShiftControlEnd | ||
|
|
||
| # Correctly handle Control-Arrow/Home/End and Control-Insert keys. | ||
| if (ev.ControlKeyState & self.LEFT_CTRL_PRESSED or |
There was a problem hiding this comment.
I wonder whether this doesn't have to be an elif? If both shift and control are pressed don't we end up in both this branch and the if` above? I think we replace Keys.ShiftControlLeftbyControlLeft`` like this. (Maybe I'm missing something.)
There was a problem hiding this comment.
Never mind. The assignment to result.key ensures that none of the other conditions satisfy. This is fine.
| @@ -139,6 +144,11 @@ def is_returnable() -> bool: | |||
| handle('escape', 'enter', filter=insert_mode & is_returnable)( | |||
| get_by_name('accept-line')) | |||
|
|
|||
| # It appears that on windows, Enter -> ^M but Ctrl-Enter brings in a ^J; | |||
| # We handle this the same as escape-enter | |||
| handle('escape', 'c-j', filter=insert_mode & is_returnable)( | |||
There was a problem hiding this comment.
Right. Thanks for taking the time to dig into this issue.
I'm not sure yet what's the best approach. The _is_paste method is a workaround, because the Windows console doesn't support bracketed paste, but it's important to detect it.
Probably we can simply ignore the escape key in here, and it should be fine.
The chance that people literally paste "escape + enter" and expect it to be inserted as text literally like that is very small...
|
Hi @scoennz, If you squash the commits into one, I'll merge it. Thanks a lot for this. I just tried on both "tmux in mintty on WSL" and on the Windows console, and it works perfect out of the box. |
This refers to issue prompt-toolkit#979. Text can be selected and the selection extended with shift + cursor movement (`left`, `right`, `up`, `down`, `home`, `end`, `ctrl-left`, `ctrl-right`, `ctrl-home`, `ctrl-end`). For completeness, `ctrl-home` and `ctrl-end` bindings (beginning and end of buffer) are also added, through new readline style commands (`'beginning-of-buffer'` and `'end-of-buffer'`). The selection is replaced by any text that is typed (including pasting/yanking, and enter, if multi-line). A cursor movement without shift while in selection mode cancels the selection. This works alongside the standard Emacs marking mode (started with `Ctrl-space`). The difference is handled by adding a `shift_mode` boolean in the `SelectionState` (set by the `enter_shift_mode()` method) and a corresponding `shift_selection_mode filter`. All the related bindings are defined in a new `load_emacs_shift_selection_bindings()` function, now called in `key_binding.defaults.load_key_bindings`. Other changes include * adding the required keys in `keys.py` * processing them properly in `input.win32.ConsoleInputReader._event_to_key_presses` * defining default empty handlers in `key_binding.bindings.basic.py`
scoennz
force-pushed
the
shift-selection
branch
from
c2afd79
to
9c83ea6
Compare
|
Hi Jonathan, I have removed the |
| handler = get_by_name(key_to_command[key]) | ||
| except KeyError: | ||
| pass | ||
| else: # (`else` is not really needed here.) |
There was a problem hiding this comment.
Actually, else is needed here, because we don't want handler to execute if there was a KeyError.
(I can remove the comment in a separate commit.)
|
Merged! Thank you @scoennz, for taking the time for this. I really appreciate it. |
This is a proposal for an implementation of text selection using shift + cursor movement as is customary in many GUI. This refers to issue #979.
The implementation is based on defining a new boolean flag
shift_modein theSelectionStateclass.shift_modeis initialized toFalseby the class constructor and set toTrueby a call toSelectionState. enter_shift_mode()whenever a shift+movement binding is called to start selecting text in this fashion. There is an associated newshift_selection_modefilter. The advantage of proceeding this way are thatshift_modeflag is automatically destroyed when selection is exited by any means through any other bindings (because theSelectionStateinstance is destroyed). In that way, we are sure that theshift_modeflag will not persist toTrueby accident.Behavior:
Cursor movements above refers to left, right, up, down, home & end (beginning & end of line), ctrl-left/right (word-by-word), ctrl-home/end (beginning/end of buffer). All missing shift- combinations of these keys have been added to
keys.py(and set by default to do nothing inbindings/basic.py). Ctrl-home/end have also been added, with the corresponding commands implemented as new readline-style commands innamed_commands.py. For completeness, ctrl/shift-insert keys have also been added as these are sometimes used for copy/paste. All these new key combinations are correctly converted to key-presses ininput/win32.py.I have tested this implementation stand-alone as well as in IPython 7.6.1 (through applying the same changes to prompt_toolkit 2.0.9 - apart from removing the type hints). Everything seems to work well, without any changes required to the IPython source code itself.
Happy to get some feedback at this stage, of what could be improved or done differently. I will continue to do more testing with multiline pasting in the meantime.