Safer focusedEditorChanged event

[peterflynn]

Make editor-swapping more independent from the focusedEditorChanged event, and make the event to fire more consistently.

This fixes the scrolling issue Randy reported in #1864. I think it also fixes all the cases listed in #1860, and it further improves upon #1257 to the point where it's almost fixed.

Code changes:

• Revert main-editor-swapping codepath back to how it used to work, mostly disentangled from focus events.
  (Doing a git difftool a82cfded^ -- src/editor/EditorManager.js on this branch helps show the similarity to the original code).
• Don't fire focusedEditorChanged on the many times focus returns to the same Editor that had it last (due to window reactivation, closing a dialog, closing a search bar, etc.). All the current use cases for this event don't care about those cases. Add more detailed docs on event cases.
• Simplify how Editor signals focus: trigger only from "onFocus" (a superset of the other case), and remove _internalFocus flag that was guarding against the overlap.
• Don't call resizeEditor() on every editor swap just because statusbar might have been shown/hidden: only call when statusbar actually did change (going to/from no editor). (Required to fix scrolling issue in Fix bug #1627 ("Go to Definition" prepopulated text doesn't work via menu) #1864).

[peterflynn]

I've added detailed notes in #1257 about the two known cases that this patch didn't fix.

[peterflynn]

@jasonsanjose, @gruehle: I wouldn't mind a double review on this one if you're both up for giving it a once-over :-)

I've run unit tests and gone through a gauntlet of different cases, verifying that the status bar appears correct, editors get disposed properly, focusedEditorChange is fired a sane number of times & with the right arguments, etc. But with something core like this, the more testing the better too!

[jasonsanjose]

The circular dependency should be ok but I thought I'd mention it in case you had any concerns.

[jasonsanjose]

I suppose we could address this later by refactoring the statusbar updating code out of EditorManager.

[peterflynn]

Good catch. I think you're right that it's safe for now: EditorManager doesn't reference StatusBar unit APP_READY, and StatusBar doesn't reference EditorManager until someone calls show()/hide().

But ideally yeah, I'd like to fix #1869 so that EditorManager no longer has any references to StatusBar.

[jasonsanjose]

Shouldn't this happen after triggerHandler? Looks like we're firing the event passing the same editor as the current and previous arguments.

[peterflynn]

Yikes, very good point! Looks like there might even conceivably be edge cases where this could cause bugs in status bar updating. Will fix.

[jasonsanjose]

Initial review complete. Thanks for helping out with this one.

[gruehle]

In this comment for #1257, you mention changing getFocusedEditor() to simply return _lastFocusedEditor. Did you try that? It seems like that may be a good solution to the timing discrepancies.

[gruehle]

I did some scenario testing with this branch and everything works great.

[peterflynn]

Re making getFocusedEditor() return _lastFocusedEditor: I'd like to look at its callers first to verify that no one is expecting null whenever something other than an Editor is focused. We could try to hack it to return null in those cases to preserve the old behavior, but it'd be more useful if we changed its behavior to only return null when there's no editor open. That would actually help us solve some of our focus management problems (e.g. #301, #547). I'd rather wait till next sprint so we can take our time on that.

[jasonsanjose]

Looks good. Did some scenario testing with the working set, project tree and inline editors.