unify keyboard shortcut and codemirror interaction #5296
Conversation
This code was repeated in both CodeCell and TextCell, both of which are extensions of Cell, so this just unifies the logic in Cell. TextCell had logic here to check if the cell was rendered or not, but I don't believe it is possible to end up triggering such a code path. (Should that be required, I can always just add back these methods to TextCell, performing the .rendered==True check, and calling the Cell prior to this, code mirror at_top would only return true on if the cursor was at the first character of the top line. Now, pressing up arrow on any character on the top line will take you to the cell above. The same applies for the bottom line. Pressing down arrow would only go to the next cell if the cursor was at a location *after* the last character (something that is only possible to achieve in vim mode if the last line is empty, for example). Now, down arrow on any character of the last line will go to the next cell.
This method was identical in both CodeCell and TextCell
Our edit mode keyboard shortcuts don't distinguish between being in a code cell or in a text cell, so it makes sense to handle both in one place. This is a first step in that direction.
this way, you can ask if a particular event will be handled by the shortcuts system. This takes away the need to special-case many different possible keys which should be ignored by codemirror by ignoring them en masse.
| @@ -237,6 +237,12 @@ IPython.keyboard = (function (IPython) { | |||
| return true; | |||
| } | |||
|
|
|||
| ShortcutManager.prototype.use_shortcut = function (event) { | |||
There was a problem hiding this comment.
Not sure I like this name, just that inline on our shortcut managers it would look like edit_shortcuts.use_shortcut(event)
There was a problem hiding this comment.
What about replacing it with .use(event)?
There was a problem hiding this comment.
use sounds imperative, but this is just a query, right? What about .uses(event)?
There was a problem hiding this comment.
since it's events, let's use handles.
|
pinging fellow keyboard handling co-conspirators @ellisonbg and @jdfreder |
|
First impressions:
|
|
I'm inclined to agree - unless there's a bug that can't practically be fixed without it, it seems best to push refactoring to after the release. |
|
+1 for treading lightly on this so close to release. We already have a candidate out, let's leave this for post-2.0, no matter how good. We always find surprises with big changes :) It could go into 2.1, which I'm sure we'll need to put out before too long. @ivanov, thanks for the great work! |
|
I understand the hesitation, so let me work on a summary of the things that would not be possible to do if this doesn't go in, just so we can make an informed decision about whether or not this would be worth it. |
|
Thanks @ivanov that would help On Mon, Mar 10, 2014 at 12:11 PM, Paul Ivanov notifications@github.comwrote:
Brian E. Granger |
|
Ok, so here are the issues this PR fixes which are currently in master: It is not possible to have In This inconsistency leads to the following: you cannot get But the special casing for The check for that is I don't expect such code to make it into IPython proper, which is why I created vimception. More generally, any IPython Edit Mode keyboard shortcuts that a user wishes to have will still be passed along to code mirror. Except for up and down arrow keys. Even though it is possible to remove them from IPython Edit Mode shortcuts, they cannot be replaced with anything else (for example, to go to the next cell), because an event involving either of those two in So, overall, this PR minimizes the number of keyboard shortcuts which are hard-coded, allowing for their removal and re-assignment, and reduces the number of places customization of non-default behavior needs to happen (due to Pinging @minrk who wanted to weigh in on this as well. |
|
I was waiting for Paul to enumerate the issues this fixes, but I would prefer this to get into 2.0. The changes here have been planned for quite a while, but Paul kept having to rebase as events and keyboard stuff kept being refactored in master. To me, this is a basic fix for shortcut customization. If I had known people would be hesitant about this change, I would not have cut the beta without it. |
| @@ -230,6 +254,53 @@ var IPython = (function (IPython) { | |||
| }; | |||
|
|
|||
| /** | |||
| * Either delegates keyboard shortcut handling to either IPython keyboard | |||
|
Fair enough, I didn't originally realize that they really were necessary for things we want now. At first, I thought it was mostly refactoring/cleanup to make future work easier. Given it seems like it's really important work for 2.0, let's just be extra careful in the review process, so we don't miss anything surprising. We should also cut an RC with this included, just to be safe. |
|
I am fine with this, but do feel that this stuff should be used in master for more than a few days. +1 for an RC with this stuff. |
| * | ||
| * @method handle_keyevent | ||
| * @param {CodeMirror} editor - The codemirror instance bound to the cell | ||
| * @param {event} event - |
There was a problem hiding this comment.
Min RK, on 2014-03-17 20:21, wrote:
still missing here
ok, I tried... but it's the event parameter of a function called
handle_keyevent, so I'm not sure how to be more descriptive
there.
|
@pi I just had a chance to look through this code some more, looks good. |
|
Let's move |
| var that = this; | ||
| var shortcuts = IPython.keyboard_manager.edit_shortcuts; | ||
|
|
||
| // if this is an edit_shortcuts shortcut, we've already handled it. |
There was a problem hiding this comment.
I think technically the code mirror event fires before the document events that the global keyboard manager is listening to. Maybe change this from "we've already handle it" to "the global keyboard/shortcut manager will handle it"
|
Two problems in interactive testing:
|
|
|
I can also replicate the problem you describe with moving between multiline cells. |
|
I think ↑ at the top of a cell moving to the previous cell is at least as reasonable as treating a cell boundary as a document boundary unless you are at cursor position zero. I think the two most logical choices are:
I think the current in-between behavior is the most confusing choice we could make. I would lean toward 1, personally. |
|
I agree that 1 is better than 2, but still feel that our current behavior On Fri, Mar 14, 2014 at 4:24 PM, Min RK notifications@github.com wrote:
Brian E. Granger |
Sets the cursor on the last line of the cell when moved up from the top of the cell below, and sets the cursors to the first line when moving down from the bottom of a last line. Here, we retain the character that the cursor was on, so that users wishing to have up-down functionality like one document can still use this shortcut handler and simple adjust the at_top and at_bottom methods
|
@ivanov just came over to bring this to my attention; just by chatting about it, it appears I'm on the side of @ellisonbg on this one. Paul can provide further details if needed, and we also talked about the edge case for vim-mode extensively. |
up arrow at the top line first goes to char 0, and only goes to the cell above if already on char 0. Same with down arrow on the bottom line: transition cursor to the end of the line, and only go down a cell if already at the end of the last line. this makes for an unhappy experience in code-mirror's vim mode for j and k keys, but we'll fix that in the next commit
|
I've fixed the multi-line cursor memory behavior. The up and down arrow keys didn't work properly in the top and bottom cells, that's also been fixed. Also, upon further thought and experimentation with this, it doesn't really make sense to have any kind of logic for vim mode here, since up and down keys still work the right way even when vim mode is enabled, and getting This is now ready for final review and merge. |
|
Strictly an improvement over master, merging. |
unify keyboard shortcut and codemirror interaction
unify keyboard shortcut and codemirror interaction
Ok, this is in a state where it can use some review. I haven't yet verified that this provides everything one needs to integrate with CodeMirror's vim mode, but this makes the situation a lot better.
So to start with, the relevant code was repeated in both CodeCell and TextCell, both of which are extensions of Cell, so this just unifies the logic in Cell.
TextCell had logic here to check if the cell was rendered or not, but I don't believe it is possible to end up triggering such a code path. (Should that be required, I can always just add back these methods to TextCell, performing the .rendered==True check, and calling the Cell)
Prior to this PR, code mirror at_top would only return true on if the cursor was at the first character of the top line. Now, pressing up arrow on any character on the top line will take you to the cell above.
The same applies for the bottom line. Pressing down arrow would only go to the next cell if the cursor was at a location after the last character (something that is only possible to achieve in vim mode if the last line is empty, for example). Now, down arrow on any character of the last line will go to the next cell.
I've also added a use_shortcut method to ShortcutManager, which allows us to a remove a bunch of special-casing of key events like up and down arrow, escape, shift/ctrl/alt-enter.
For example, this PR makes it possible to remove our special-casing of up arrow at the top of the cell, preventing it from going to the cell above. This can be done via this javascript:
Previously, the up arrow functionality at the top of a cell was welded into the code base as an if statement.