Many commands (including save) apply to wrong file just after using Quick Open

[njx]

I think this set of steps reproduces the problem reliably.

1. Open two JS files in the working set (and view both of them).
2. Switch to the first file.
3. Type something into the file that would cause a JSLint error (e.g. add "x" at the beginning of the file) and save it so the JSLint errors show up.
4. Switch to the second file.
5. Type something into the file that would cause a JSLint error, but don't save it yet.
6. Using Quick Open, switch back to the first file (the one you modified in step 3).
7. Click back on the second file in the working set.
8. Try to save it.

Result: Saving doesn't work--Cmd-S or File > Save don't do anything (dirty dot is still visible and file isn't changed on disk). However, if you try to close the file and choose to save it, it does work; or if you switch to another file and switch back, that fixes it.

Also, a number of times when this happens, I've noticed that JS code hinting is in a weird state, where it thinks I'm in the middle of typing a string when I'm not, so it shows me string hints. I'm not sure how that would be related, but they happen together too often for this to be a coincidence, I think.

Finally, note that if you don't use Quick Open to switch to the first file (as in step 6), the problem doesn't seem to occur.

UPDATE: Possibly also related...I've noticed that in a couple of cases, the string "use strict" seemed to be randomly added to the very beginning of files I might not have actually touched. That reinforces my thinking that JS code hinting is somehow getting messed up, because that string might come from the string hints that come from JS code hinting.

[njx]

BTW, when this happens I don't see any errors in the inspector.

[njx]

Adding to sprint 21--need to figure out who can look into this.

[peterflynn]

@njx If you hit it again, can you put a breakpoint in DocumentCommandHandlers.handleFileSave() and see if it's even getting called? And if so, what does it get as activeEditor? (seems like that getting out of sync could possibly account for both Save and code hints breaking).

[njx]

I just updated the steps--I think the recipe above is reliable.

Weirdly, I can't seem to ever hit a breakpoint in handleFileSave() or doSave() even when save is working, which doesn't make sense. (I just relaunched Brackets, and I can hit other breakpoints.) Not sure what's going on. I don't have time to look further into it right at the moment though.

@peterflynn -- can you check quickly if you can repro this using my steps above? Since I'm going to be out tomorrow I want to make sure at least one other person can repro (even if you don't end up fixing the bug).

[RaymondLim]

I confirmed that JS code hinting extension is not the culprit; I remove the entire JS Code hints extension and I still can reproduce the issue with your updated steps. And also it seems to be affecting Save command only and not all keyboard shortcuts; I still can undo with Ctrl+Z when Save command is failing.

[njx]

Interesting. It does seem, however, that the JS hint extension is somehow affected. @peterflynn suggested that there might be some problem with the active editor being set incorrectly, which would explain some of these mysterious issues.

[jbalsas]

@njx I've been looking into this, and I think you can reproduce it with an easier recipe:

1. Open two JS files in the working set (and view both of them).
2. Switch to the second of them
3. Using Quick Open, switch back to the first file.
4. Click back again on the second file in the working set.
5. Write anything and try to save it.

I've checked and it is indeed an issue with the active editor being a wrong one (the dirtyflag is then read from the clean editor), and it also seems somehow a race condition (If I set a breakpoint in handleFileSave or doSave, it does work for me all the time).

I'm not that familiar with this code, so I still don't have a fix, but maybe this'll help narrow it down.

[jasonsanjose]

Assigned to me

[jasonsanjose]

Confirmed using @jbalsas steps.

[jasonsanjose]

I'm seeing runtime errors in the console

Uncaught TypeError: Cannot read property 'forceSelect' of undefined
/Applications/Brackets%20Sprint%2021.app/Contents/dev/src/thirdparty/smart-auto-complete/jquery.smart_autocomplete.js:348

[njx]

I don't think I saw those--maybe something unrelated? It's also possible my dev tools are somehow hosed though, since I was having the breakpoint trouble mentioned earlier.

[peterflynn]

I've narrowed it down a tad more. When you do the final editor switch -- going to the file with unsaved changes -- Editor's focus listener fires for the other document instead. Clearly this is incorrect, because when you type or hit the arrow keys it goes to the editor with unsaved changes, not that other one.

So it seems possible this is a CM bug. Could the extra refreshes caused by the JSLint stuff jumping around be throwing off its own focus listeners somehow??

[peterflynn]

The preceding QuickOpen seems to get CM for the second file (w/ unsaved changes) in a state where it thinks it has focus even though it doesn't. This causes it to not send a focus event when it gains focus later.

The trouble with Quick Open looks to be how the search bar closes: it's triggered by losing focus, and it loses focus due to switching to the other file. So while we're already in the midst of switching to over the first file, the search bar loses focus, closes and asks to restore focus to the "active" editor, which is still the second file (because the focus change gets to the search bar before it gets to EditorManager). So the first file gets focus, which causes the second file to be given focus, and then the first file gets focus again (seemingly because there is some redundant logic in CodeMirror.focus()). I haven't yet pieced together exactly how that causes CodeMirror's state.focused to get out of sync, but that's the high-level of it.

[peterflynn]

Fixing this seems tricky. The timing of Quick Open teardown is extremely fragile, as we know from previous bugs. The timing of activeEditorChange is also fairly delicate, as we know from the really nasty bugs back in October. But I wonder if we could make an exception for the case where currentEditor is changing, essentially changing activeEditor ahead of the focus even in that case... Or perhaps the Quick Open select handler could be smarter about closing its own search bar before initiating the document change...

OTOH, it seems like this bug must also exist in Sprint 20 so that may make it a tad less urgent. OTOOH, it may well be CMv3-dependent, meaning it's still a fairly new regression...

[peterflynn]

This may also be what's causing #2951 -- if so, then the issue is pretty old since that bug has been occurring at least since December (presumably since ModalBar was introduced).

[njx]

I don't feel like I've ever seen this before, and I'm seeing it all the time now, so it does feel like it might have been injected or exacerbated by the cmv3 switch. Might be worth git bisecting to see if that's the case (remembering to do a submodule update when you hit a commit that changes the CM SHA).

[RaymondLim]

Actually, I can reproduce it in Sprint 19. So it may not be related to cmv3.

[peterflynn]

Yup, I can confirm -- repros in Sprint 19 & 20 with the same steps. I'm guessing Sprint 18 too since that's when ModalBar was originally introduced.

[peterflynn]

Lowering to Medium since it's been around for several sprints with no reports until now.

[jbalsas]

Note that this also affects any other operation that would use the activeEditor. For example, a simple search runs on the first editor when the second one is visible. Nothing is highlighted in the visible document, but pressing repeatedly cmd+g advances the hit on the first one and updates the status bar cursor information.

@peterflynn Maybe you could consider renaming the title as the bug is not bound to just saving files and files with jslint errors?

[peterflynn]

Sure -- I've clarified the title

[RaymondLim]

I just confirmed that this issue is introduced in sprint 19; can't reproduce it in sprint 18 and earlier versions.

[peterflynn]

Interesting, thanks Raymond. That makes me suspicious that the subtle timing changed in #2548 may be causing this. Although how, I don't know -- the problem seems to be that hitting Enter opens the new file before it triggers closing the modal bar, and although Enter is processed slightly later after #2548 that ordering was still true before it too.

[pthiess]

Reviewed, agreed that we want this fixed asap.

[peterflynn]

So there are a couple potential fixes I've come up with so far... none risk-free, though:

• Call _close() earlier in _handleItemSelect(), before we start switching documents. _close() only runs once per QO session, so that's enough to avoid the undesirable blur handling later. However, it tears the search bar down a little earlier, which changes the way Editor scrolls back to top when returning to it after Quick Open #2951 behaves -- it seems to now affect the file you're switching to instead of from, which is potentially more noticeable.
• Patch CodeMirror.focus() to not directly call onFocus(); if the CM element actually gains focus, onFocus() will get called anyway via the focus event. (The bug occurs in a case where CM's attempt at programmatically gaining focus is ignored by the browser, it seems because it occurs while on a call stack that's already in the middle of blurring that same element. Since that's possible, CM can't safely assume that calling focus() means it has focus, and that assumption is what gets CM's state.focused to become wrong).
• Glenn suggested patching EditorManager._doShow() to change the value of activeEditor a little early, or perhaps just null it out until the focus change is complete. I haven't tested that yet, but I'm pretty sure it'd also fix this. But it is a little risky if any other code triggered by blur/focus calls getActiveEditor() and needs a different answer.

[njx]

Urgh. I vaguely recall that the original CM search "dialog" (which ModalBar replaced) did something where it did a setTimeout() in order to avoid a similar issue with blur and focus. I got rid of that in my version because it seemed unnecessary, but maybe it was trying to fix something like this. I don't understand the problem well enough to know though.