Refactoring Notebook.command_mode #5268
Conversation
| // Notify the keyboard manager if this is a change of mode for the | ||
| // notebook as a whole. | ||
| Notebook.prototype.handle_command_mode = function (cell) { | ||
| if (cell === null) { return; } // TODO: do I really need this? |
There was a problem hiding this comment.
I don't think you need this. It only existed for command_mode the callable method, and doesn't need to exist in the cell event handler.
|
@minrk and @takluyver can you test this branch to see if you find any problems with the modal/keyboard stuff? |
| @@ -457,6 +457,11 @@ var IPython = (function (IPython) { | |||
| if (this.is_valid_cell_index(index)) { | |||
| var sindex = this.get_selected_index(); | |||
| if (sindex !== null && index !== sindex) { | |||
| // If we are about to select a different cell, make sure we are | |||
| // first in command mode. | |||
| if (this.mode !== 'command') { | |||
There was a problem hiding this comment.
what is the cost of calling command_mode if already in command_mode? If there is a cost, perhaps this check should be inside the command_mode method so it never happens.
There was a problem hiding this comment.
Notebook.command_mode is a no-op if already in command mode.
There was a problem hiding this comment.
But I think it is important that the code path in select also has the test - mainly as a reminder for devs.
|
Testing reveals something similar to what I've already seen in master: if I refresh the page while in edit mode, it is sometimes left in between modes - keyboard shortcuts work, but uncaptured key presses type, and the text cursor is visible within a grey border. |
|
Ahem. Scratch that. I was testing on a notebook where I had tested switching to edit mode in javascript, and that was interfering with it. When I remove that cell, it works. |
|
Hmm, I just tried various combinations of page refreshing and can't reproduce this. Can you give you details on an exact sequence of steps require to reproduce? Can I assume you are Linux+Chrome? (@jdfreder can test this) |
| @@ -1467,15 +1466,19 @@ var IPython = (function (IPython) { | |||
|
|
|||
| // If we are at the end always insert a new cell and return | |||
| if (cell_index === (this.ncells()-1)) { | |||
| this.command_mode(); | |||
There was a problem hiding this comment.
Just for my peace of mind, this command_mode call doesn't need to happen before the insert_cell_below call, right? In fact, it shouldn't be needed at all, it's just a best practice before calling select, right?
|
I just have one comment inline, other than that the code looks good, makes sense, and seems to work okay. |
|
Yep. On Wed, Mar 5, 2014 at 11:55 AM, Jonathan Frederic <notifications@github.com
Brian E. Granger |
Refactoring Notebook.command_mode
Refactoring Notebook.command_mode
This splits
Notebook.command_modeinto two parts, likeedit_modeis already. This is needed to avoid having to callCell.focus_cellmanually each timeNotebook.command_modeis called.