focusedEditorChange does not work as documented

[DennisKehrig]

In EditorManager.js, it says: "focusedEditorChange -- When the focused editor (full or inline) changes and size/visibility are complete"

In the same file, function createInlineEditorForDocument is documented as "Creates a new inline Editor instance for the given Document. [...] The editor is not yet visible or attached to a host editor." Yet, this function triggers the focusedEditorChange event.

Consequently, if a handler then calls getFocusedEditor() it will get the editor that is about to lose focus, not the one that just got focus. Since the focusedEditorChange event does not pass along the editor it replaces, this behavior could be relied upon to determine which editor previously had focus. But depending on which part of the documentation is correct, this behavior could change.

Also, an event handler for focusedEditorChange might call a function that is otherwise independent of the event and therefore needs to call getFocusedEditor(). It would then not get the editor that now has focus.

In my opinion focusedEditorChange should only be triggered when the new editor is visible and actually has the focus - as documented. getFocusedEditor() would then return the same editor as passed on by the event.
In addition, I would pass the previously focused editor to the handler function as well.

function onFocusedEditorChange(event, editor, previousEditor)

[DennisKehrig]

In addition, EditorManager.getInlineEditors(EditorManager.getCurrentFullEditor()) returns an empty array when opening the first inline editor (when called in the focusedEditorChange handler).

[pthiess]

Reviewed - assigning to Jason

[jasonsanjose]

Nominating sprint 15

[jasonsanjose]

@DennisKehrig Can you confirm and close? Thanks.

[DennisKehrig]

I was happy to see that...

• opening an inline editor in a file correctly triggers the event, with the second parameter pointing to the new (inline) editor and the third pointing to the previous (full) editor
• when Brackets regains focus, it correctly uses the inline editor as the second parameter or the full editor if focus changed back to it.
• when clicking on an entry in the project tree or in the list of files of an inline editor that doesn't already have focus, the parameters are set as expected

However, I also saw a couple of issues:

• When I start Brackets and then open a file by clicking on its name in the project tree, I get TWO identical focusedEditorChange events. I expected one.
• Switching to a different file in the same manner again results in two identical events. While the second parameter (the first being the event) correctly points to the new editor, the third parameter is null, as if no file had been open before. I expected the third parameter to refer to the previous editor. If that is not possible for some reason, maybe a "editorFocusChange" event makes more sense, one parameter being the editor in question, the other being whether it gained (true) or lost (false) focus. Or two separate events.
• Switching to a different window, then switching back to Brackets triggers another editorFocusChange event, the third parameter being undefined instead of null this time (I suggest using null consistently). However, I would not expect this event at all (at least not without a counterpart indicating that the focus has been lost when switching to a different window). If it is sent only once, I'd expect the third parameter to be identical to the second.
• When changing focus back and forth between a full editor and an inline editor (or between different inline editors) by clicking on parts of the content, the third parameter is always undefined. I expected it to point to the editor that had focus before.
• When switching from one entry in an inline editor to another via its selector, the third parameter is null

Potentially there are some more edge cases that don't work as I would expect, but I think that's enough for now.

[jasonsanjose]

I've filed #1860 to capture these new bugs. @DennisKehrig let me know if this looks good and go ahead and close this issue. Thanks!

[DennisKehrig]

@jasonsanjose: Thanks for keeping me posted!

The second argument (the new editor with focus) of the event is always correct. If that is all we need, the event's documentation in EditorManager.js should be updated to document the new parameters, and then we're good.

However, a call to EditorManager.getFocusedEditor() still yields incorrect results in some cases.

• When switching to a different file (full editor) the first call to getFocusedEditor() in the first focusedEditorChange event returns null
• When opening an inline editor via Cmd-E, getFocusedEditor() returns the full editor at first
• When switching from an inline editor to a full editor by clicking the code in it, getFocusedEditor() returns the inline editor at first

A call to getFocusedEditor() within the event handler correctly returns the new focused editor when:

• Brackets regains focus
• Closing an inline editor via Esc or Cmd-E
• Switching to a different file in the inline editor (shortcut or click)

Calling getFocusedEditor() again after 100ms always returned the correct editor in my tests. Sometimes calling the function via setTimeout, but without a delay, works too.

Sorry!

[peterflynn]

After pull #1877 I think there are only two cases of this where getFocusedEditor() still lags behind the focusedEditorChange event: opening an inline editor; and moving focus from an inline editor to its host editor.

In both cases, the event args give a correct picture of which editors lost/gained focus, so the workaround is for clients of this API just to use those events.

Here's an analysis of why those cases are still wrong:

• Opening an inline editor -- because the inline editor is programmatically focused before it's actually been added as an inline child, getFocusedEditor() will never find it. (This happens because MultiRangeInlineEditor.load() selects the first editor & focuses it, but is called before CSSInlineEditor resolves its promise and hence before the inline is attached to its host editor).
• Moving focus from inline editor to host editor -- because CodeMirror's onFocus/onBlur events (and the internal methods that parallel them) occur in the reverse of the normal order, the host editor gets focus before its inline gets blur. Thus for a moment, both the host and its inline think they have focus; and getFocusedEditor() happens to prefer the inline. (This preference is why the reverse -- moving focus back into the inline -- works fine. Simple swaps from one full-size editor to another also have a moment where both editors think they have focus, for this same reason, but there's no bug there because getFocusedEditor() only asks _currentEditor if it has focus... not anyone else).

The simplest fix for both cases might be to just have getFocusedEditor() return the value of _lastFocusedEditor directly. (This would move the disconnect: from getFE() disagreeing with focusedEditorChange, to the both of them together disagreeing with Editor.hasFocus() -- which seems like a safer discrepancy).

[DennisKehrig]

@peterflynn: I like your proposed fix. If I needed access to the focused editor consistent with the event, I would store the last reference gotten from the event in some variable and use that instead of getFocusedEditor(). Seems like your suggestion basically amounts to the same thing.

[peterflynn]

Moving remainder of this out to Sprint 16. This may dovetail pretty well with the focus management fixes that are coming up on our backlog soon anyway.

[DennisKehrig]

Looking good, very happy about all the changes! Thanks everybody, especially @jasonsanjose!
Also like the updated detailed documentation.

[peterflynn]

@gruehle, we might just be able to call this one closed once your changes land (now that we've decided not to try to "fix" getFocusedEditor(), and we've added a viable alternative API that does track to the event 100% reliably).

[gruehle]

Marking fixed. The getActiveEditor() function tracks the event reliably. (plus, there are the arguments to the event, which are also correct).

[njx]

FBNC to @DennisKehrig

[njx]

This won't be regressed before the end of the sprint, so I'm going to consider this an exception. @DennisKehrig, please let us know ASAP if you see a problem with it. Thanks.

[DennisKehrig]

Works perfectly! Outstanding work, guys, thanks a lot!

[DennisKehrig]

Closed