[CLOSED] Safer focusedEditorChanged event

[core-ai-bot]

Issue by peterflynn
Thursday Oct 18, 2012 at 06:02 GMT
Originally opened as adobe/brackets#1877

---

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 [CLOSED] Update build number for sprint 16 in package.json #1864).

---

peterflynn included the following code: https://github.com/adobe/brackets/pull/1877/commits

[core-ai-bot]

Comment by peterflynn
Thursday Oct 18, 2012 at 06:13 GMT

---

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

[core-ai-bot]

Comment by peterflynn
Thursday Oct 18, 2012 at 06:25 GMT

---

@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!

[core-ai-bot]

Comment by jasonsanjose
Thursday Oct 18, 2012 at 15:06 GMT

---

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

[core-ai-bot]

Comment by gruehle
Thursday Oct 18, 2012 at 16:05 GMT

---

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.

[core-ai-bot]

Comment by gruehle
Thursday Oct 18, 2012 at 16:07 GMT

---

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

[core-ai-bot]

Comment by peterflynn
Thursday Oct 18, 2012 at 17:42 GMT

---

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.

[core-ai-bot]

Comment by jasonsanjose
Thursday Oct 18, 2012 at 18:03 GMT

---

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