Dual mode bug fixes. #5146

jdfreder · 2014-02-18T20:46:57Z

ivanov · 2014-02-18T22:43:51Z

Glad you're figuring out the FF and Chrome differences, I've been struggling with those in ipython-vimception.

jdfreder · 2014-02-18T22:49:51Z

Glad you're figuring out the FF and Chrome differences, I've been struggling with those in ipython-vimception.

I'm trying really hard... The current thing I'm stuck on is "focusout" events not being fired for bootstrap text boxes on FF. Argh! I'm surprised no one has reported this in master. It seems as is focusing on a textbox completely disables the keyboard manager until page refresh...

jdfreder · 2014-02-18T23:49:49Z

This is ready for some testing by others & code review. I've left console.log statements everywhere just in case there are more bugs I need to track. When it is decided this PR is ready for merge, I'll remove them all.

juhasch · 2014-02-19T17:31:22Z

This is better, I could not reproduce the jumping cells behavior.
Still, when hitting shift+return in a new notebook:

jdfreder · 2014-02-19T19:10:48Z

This is better, I could not reproduce the jumping cells behavior.
Still, when hitting shift+return in a new notebook:

Thanks for pointing this out @juhasch - I made a couple more changes that should fix the problem, tell me if they fix the problem for you too. I also retested the other issues this PR addresses, they are all still fixed.

juhasch · 2014-02-19T19:47:31Z

Yep, this is fixed for me, too. Unfortunately, the old trick

cell 1 in edit mode
click outside of the browser
click on the In area of cell 2
still gives me the same effect as in my previous comment.

jdfreder · 2014-02-19T20:13:17Z

Unfortunately I can't replicate that problem with either of my browsers (FF 25 & Chrome 31) 😞 . Following the steps you provided, my notebook ends up in a state with only cell one selected, in edit mode. This behavior is consistent for me, two questions:

Did you make sure the cache was cleared?
Is it an intermittent failure or constant?

jdfreder · 2014-02-19T20:17:41Z

Scratch that! I was just able to replicate it, VERY weird though... Had to click on the blank space not the text of the prompt.

jdfreder · 2014-02-19T21:20:34Z

@juhasch I found and removed a race condition which was making that incorrect behavior exist approx. 1/2 times on my machine. Can you verify that it fixes it for you too?

juhasch · 2014-02-19T21:33:33Z

Looks good here. Thanks for digging so deep into this!

jdfreder · 2014-02-19T21:36:13Z

Looks good here. Thanks for digging so deep into this!

Woohoo! That's great news.

ellisonbg · 2014-02-19T22:07:55Z

I will try to have a look at this soon.

On Wed, Feb 19, 2014 at 1:36 PM, Jonathan Frederic <notifications@github.com

wrote:

Looks good here. Thanks for digging so deep into this!

Woohoo! That's great news.

Reply to this email directly or view it on GitHubhttps://github.com//pull/5146#issuecomment-35551916
.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

ellisonbg · 2014-02-21T20:53:05Z

IPython/html/static/notebook/js/cell.js

@@ -147,23 +147,28 @@ var IPython = (function (IPython) {
        }
        if (this.code_mirror) {
            this.code_mirror.on('focus', function(cm, change) {
-                $([IPython.events]).trigger('edit_mode.Cell', {cell: that});
+                console.log('cell focused');


You have a ton of console.log calls that should be removed before we merge this.

@ellisonbg definitely, there is a disclaimer in one of the comments above "When it is decided this PR is ready for merge, I'll remove them all." 😃

ellisonbg · 2014-02-21T21:51:38Z

This jsfiddle helped me to understand the different events:

http://jsfiddle.net/GK7sN/2/

ellisonbg · 2014-02-21T23:31:43Z

IPython/html/static/notebook/js/keyboardmanager.js

        });
+
        // There are times (raw_input) where we remove the element from the DOM before
        // focusout is called. In this case we bind to the remove event of jQueryUI,
        // which gets triggered upon removal, iff it is focused at the time.
        e.on('remove', function () {
            if (document.activeElement === e[0]) {


We need to replace this test by a call to is_focused to cover the case where the element removed is not focused, but a child is.

ellisonbg · 2014-02-21T23:44:22Z

OK, this PR has reintroduced an old bug:

Create a large code cell with a couple of pages worth of source.
Enter edit mode and put the CM cursor in the first line or so.
Scroll that line of code off the screen.
Click outside the cell to enter command mode.
Click inside the cell on the last line or so. The screen will jump-scroll to the previous cursor position.
But the cursor will be at the new place you clicked.

This is a sight variant of the original bug, which would not even show the cursor at the new position.

ellisonbg · 2014-02-21T23:52:51Z

I see the following:

Enter edit mode in a cell.
Click outside the browser widow (somewhere on the desktop).
Back into the notebook.
Now you can't use the mouse to enter command mode by clicking outside a cell.
The overall state remains somehow broken

jdfreder · 2014-02-22T00:38:44Z

I see the following:

I can't manage to reproduce that on either Chrome or FF with cleared caches, but that doesn't mean I don't believe that there may still be a problem. There is still the timeout for the tooltip and tab completer, I'd like to try to rid this code of that too if possible since it seems to be a source of inconsistent failures. I think I have an idea of how I could do it reliably...

jdfreder · 2014-02-25T00:16:33Z

I've fixed the problems pointed out. I had to make a lot of changes to get everything to work. Fixing one thing meant removing a fix for another. In the end I removed all of the setTimeout calls involved with this code and used alternative fixes. @ellisonbg and @juhasch if you guys could test the changes that would be great!

ivanov · 2014-02-25T00:42:32Z

IPython/html/static/notebook/js/notebook.js

+        if (!cell) {return;}
+
+        // Only respect the blur event if the tooltip and autocompleter are
+        // not visible.


can we make this more generic? there are ways of using dialogs in codemirror that will also trigger blur events

I could possibly add a way to register exceptions like this on the notebook and cell level. I'll look into that now.

Added lots of log calls and a couple small 'fixes' (attempts)

Half overhaul of notebook focus events...

Removed logic for reverse ordered events Removed almost all of the log statements Removed list for should unfocus callbacks Removed all the logic in focus_editor Only call focus_editor if the keyboard was used to enter edit mode

Implemented whiteboard logic

ellisonbg · 2014-02-28T23:31:24Z

@jdfreder and I consider this ready for merge. This should be a massive improvement in stability and we have refactored much of the internals to prevent these problems in the future. This should be merged ASAP to get lots of testing before release.

minrk · 2014-02-28T23:48:54Z

IPython/html/static/notebook/js/notebook.js

+     * @param [index] {int} Cell index to select.  If no index is provided, 
+     *  the current selected cell is used.
+     **/
+    Notebook.prototype.trigger_edit_mode = function (index) {


Presumably edit_mode and command_mode should be symmetrical. I don't see why trigger_ was added, but if it should be, it should be added to both.

Dual mode bug fixes.

This was referenced Feb 18, 2014

Clicking on a TextBoxWidget in FF completely breaks dual mode. #5149

Closed

Notebook focus effects #4999

Closed

ellisonbg reviewed Feb 21, 2014
View reviewed changes

ivanov reviewed Feb 25, 2014
View reviewed changes

jdfreder and others added 23 commits February 27, 2014 15:45

DEBUG

c34bd56

Added lots of log calls and a couple small 'fixes' (attempts)

Fixed lots of bugs

9ad744d

Half overhaul of notebook focus events...

Use is_focused for element kbman removal event

d8d7f5e

Added cell unfocus event canceller API

588bdf1

Fixed rebase problems

96d3ec2

Post in person review

63e3096

Removed logic for reverse ordered events Removed almost all of the log statements Removed list for should unfocus callbacks Removed all the logic in focus_editor Only call focus_editor if the keyboard was used to enter edit mode

s/unfocus/blur

a5dd988

Removed change that is no longer needed

613dbf8

Standardized comments and removed last logs

5f9cdbf

Moved edit_mode canceling logic back into cell.

826236c

Always refresh the CM editor upon TextCell unrender.

bbf444a

Remove fixes that are implemented correct in new CM

d54db33

Reorg index and focus_editor params on edit_mode func

4318928

Final touches?

37f8846

Updated components to point to head

2e6bff8

Re-added widget textbox blur fix FF

7018374

implemented whiteboard logic

86a628e

Merge pull request #18 from jdfreder/modal-rewrite

2e7589d

Implemented whiteboard logic

Couple of whiteboard logic implementation misses

c2015f9

Remove notebook load log succes

24510be

Move should cancel blur into codecell

4ed449c

Added comments to kbm and shrunk focus_cell lines

676cf36

Re-added removed refresh

3683400

minrk reviewed Feb 28, 2014
View reviewed changes

s/trigger_edit_mode/edit_mode

8b012de

ellisonbg added a commit that referenced this pull request Feb 28, 2014

Merge pull request #5146 from jdfreder/modal-fix

edfc0fa

Dual mode bug fixes.

ellisonbg merged commit edfc0fa into ipython:master Feb 28, 2014

jdfreder deleted the modal-fix branch March 10, 2014 18:43

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Merge pull request ipython#5146 from jdfreder/modal-fix

6aeb258

Dual mode bug fixes.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dual mode bug fixes. #5146

Dual mode bug fixes. #5146

jdfreder commented Feb 18, 2014

ivanov commented Feb 18, 2014

jdfreder commented Feb 18, 2014

jdfreder commented Feb 18, 2014

juhasch commented Feb 19, 2014

jdfreder commented Feb 19, 2014

juhasch commented Feb 19, 2014

jdfreder commented Feb 19, 2014

jdfreder commented Feb 19, 2014

jdfreder commented Feb 19, 2014

juhasch commented Feb 19, 2014

jdfreder commented Feb 19, 2014

ellisonbg commented Feb 19, 2014

ellisonbg Feb 21, 2014

jdfreder Feb 21, 2014

ellisonbg commented Feb 21, 2014

ellisonbg Feb 21, 2014

ellisonbg commented Feb 21, 2014

ellisonbg commented Feb 21, 2014

jdfreder commented Feb 22, 2014

jdfreder commented Feb 25, 2014

ivanov Feb 25, 2014

jdfreder Feb 25, 2014

ellisonbg commented Feb 28, 2014

minrk Feb 28, 2014

Dual mode bug fixes. #5146

Dual mode bug fixes. #5146

Conversation

jdfreder commented Feb 18, 2014

ivanov commented Feb 18, 2014

jdfreder commented Feb 18, 2014

jdfreder commented Feb 18, 2014

juhasch commented Feb 19, 2014

jdfreder commented Feb 19, 2014

juhasch commented Feb 19, 2014

jdfreder commented Feb 19, 2014

jdfreder commented Feb 19, 2014

jdfreder commented Feb 19, 2014

juhasch commented Feb 19, 2014

jdfreder commented Feb 19, 2014

ellisonbg commented Feb 19, 2014

ellisonbg Feb 21, 2014

Choose a reason for hiding this comment

jdfreder Feb 21, 2014

Choose a reason for hiding this comment

ellisonbg commented Feb 21, 2014

ellisonbg Feb 21, 2014

Choose a reason for hiding this comment

ellisonbg commented Feb 21, 2014

ellisonbg commented Feb 21, 2014

jdfreder commented Feb 22, 2014

jdfreder commented Feb 25, 2014

ivanov Feb 25, 2014

Choose a reason for hiding this comment

jdfreder Feb 25, 2014

Choose a reason for hiding this comment

ellisonbg commented Feb 28, 2014

minrk Feb 28, 2014

Choose a reason for hiding this comment