Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Safer focusedEditorChanged event #1877

Merged
merged 2 commits into from
Oct 18, 2012
Merged

Safer focusedEditorChanged event #1877

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4a34b88
Safer focusedEditorChanged event:
peterflynn Oct 18, 2012
9a2c59c
Code review fix: focusedEditorChange was passing wrong last arg to its
peterflynn Oct 18, 2012
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 2 additions & 12 deletions src/editor/Editor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -626,14 +626,12 @@ define(function (require, exports, module) {
// Convert CodeMirror onFocus events to EditorManager focusedEditorChanged
this._codeMirror.setOption("onFocus", function () {
self._focused = true;

if (!self._internalFocus) {
EditorManager._doFocusedEditorChanged(self);
}
EditorManager._notifyFocusedEditorChanged(self);
});

this._codeMirror.setOption("onBlur", function () {
self._focused = false;
// EditorManager only cares about other Editors gaining focus, so we don't notify it of anything here
});
};

Expand Down Expand Up @@ -981,15 +979,7 @@ define(function (require, exports, module) {

/** Gives focus to the editor control */
Editor.prototype.focus = function () {
// Capture the currently focused editor before making CodeMirror changes
var previous = EditorManager.getFocusedEditor();

// Prevent duplicate focusedEditorChanged events with this _internalFocus flag
this._internalFocus = true;
this._codeMirror.focus();
this._internalFocus = false;

EditorManager._doFocusedEditorChanged(this, previous);
};

/** Returns true if the editor has focus */
Expand Down
80 changes: 45 additions & 35 deletions src/editor/EditorManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,16 @@
* must have some knowledge about Document's internal state (we access its _editor property).
*
* This module dispatches the following events:
* - focusedEditorChange -- Fires after the focused editor (full or inline)
* changes and size/visibility are complete.
* - focusedEditorChange -- Fires after the focused editor (full or inline) changes and size/visibility
* are complete. Doesn't fire when editor temporarily loses focus to a non-editor
* control (e.g. search toolbar or modal dialog, or window deactivation). Does
* fire when focus moves between inline editor and its full-size container.
* Roughly, this event tracks getFocusedEditor() changes, while DocumentManager's
* currentDocumentChange tracks getCurrentFullEditor() changes.
* The 2nd arg to the listener is which Editor gained focus; the 3rd arg is
* which Editor lost focus as a result. Either one may be null.
* TODO (#1257): getFocusedEditor() sometimes lags behind this event. Listeners
* should use the arguments to reliably see which Editor just gained focus.
*/
define(function (require, exports, module) {
"use strict";
Expand All @@ -60,11 +68,20 @@ define(function (require, exports, module) {
/** @type {jQueryObject} DOM node that contains all editors (visible and hidden alike) */
var _editorHolder = null;

/** @type {Editor} */
/**
* Currently visible full-size Editor, or null if no editors open
* @type {?Editor}
*/
var _currentEditor = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we rename to _currentFullEditor to clarify?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's always had the restriction that it only points to a full-size editor, but it wouldn't hurt to clarify that. Mind if we tackle that later, though, in the interests of keeping the diff small at end of sprint?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good

/** @type {Document} */
/** @type {?Document} */
var _currentEditorsDocument = null;

/**
* Currently focused Editor (full-size, inline, or otherwise)
* @type {?Editor}
*/
var _lastFocusedEditor = null;

/**
* Registered inline-editor widget providers. See {@link #registerInlineEditProvider()}.
* @type {Array.<function(...)>}
Expand Down Expand Up @@ -360,33 +377,20 @@ define(function (require, exports, module) {
resizeEditor(true);
}


/**
* @private
* @param {?Editor} current
*/
function _doFocusedEditorChanged(current, previous) {
// Skip if the new editor is already the focused editor.
// This may happen if the window loses then regains focus.
if (previous === current) {
function _notifyFocusedEditorChanged(current) {
// Skip if the Editor that gained focus was already the most recently focused editor.
// This may happen e.g. if the window loses then regains focus.
if (_lastFocusedEditor === current) {
return;
}

// if switching to no-editor, hide the last full editor
if (_currentEditor && !current) {
_currentEditor.setVisible(false);
}

// _currentEditor must be a full editor or null (show no editor).
// _currentEditor must not be an inline editor
if (!current || (current && !current._visibleRange)) {
_currentEditor = current;
}

// Window may have been resized since last time editor was visible, so kick it now
if (_currentEditor) {
_currentEditor.setVisible(true);
resizeEditor();
}

var previous = _lastFocusedEditor;
_lastFocusedEditor = current;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


$(exports).triggerHandler("focusedEditorChange", [current, previous]);
}

Expand All @@ -396,7 +400,12 @@ define(function (require, exports, module) {
function _doShow(document) {
// Show new editor
_currentEditorsDocument = document;
_doFocusedEditorChanged(document._masterEditor, _currentEditor);
_currentEditor = document._masterEditor;

_currentEditor.setVisible(true);

// Window may have been resized since last time editor was visible, so kick it now
resizeEditor();
}

/**
Expand Down Expand Up @@ -426,12 +435,16 @@ define(function (require, exports, module) {
/** Hide the currently visible editor and show a placeholder UI in its place */
function _showNoEditor() {
if (_currentEditor) {
var origCurrentEditor = _currentEditor;
_currentEditor.setVisible(false);
_destroyEditorIfUnneeded(_currentEditorsDocument);
_doFocusedEditorChanged(null, origCurrentEditor);

_currentEditorsDocument = null;
_currentEditor = null;

$("#not-editor").css("display", "");

// No other Editor is gaining focus, so in this one special case we must trigger event manually
_notifyFocusedEditorChanged(null);
}
}

Expand Down Expand Up @@ -674,12 +687,9 @@ define(function (require, exports, module) {
}

if (!current) {
StatusBar.hide();
resizeEditor();
StatusBar.hide(); // calls resizeEditor() if needed
} else {
// Check if the statusbar is not visible to show it
StatusBar.show();
resizeEditor();
StatusBar.show(); // calls resizeEditor() if needed

$(current).on("cursorActivity.statusbar", _updateCursorInfo);
$(current).on("change.statusbar", function () {
Expand Down Expand Up @@ -754,7 +764,7 @@ define(function (require, exports, module) {
// For unit tests and internal use only
exports._init = _init;
exports._openInlineWidget = _openInlineWidget;
exports._doFocusedEditorChanged = _doFocusedEditorChanged;
exports._notifyFocusedEditorChanged = _notifyFocusedEditorChanged;
exports._createFullEditorForDocument = _createFullEditorForDocument;
exports._destroyEditorIfUnneeded = _destroyEditorIfUnneeded;

Expand Down
13 changes: 10 additions & 3 deletions src/widgets/StatusBar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ define(function (require, exports, module) {

var AppInit = require("utils/AppInit"),
StatusBarHTML = require("text!widgets/StatusBar.html"),
EditorManager = require("editor/EditorManager"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Strings = require("strings");

var _init = false;
Expand Down Expand Up @@ -139,8 +140,11 @@ define(function (require, exports, module) {
if (!_init) {
return;
}

$statusBar.hide();

if ($statusBar.is(":visible")) {
$statusBar.hide();
EditorManager.resizeEditor(); // changes available ht for editor area
}
}

/**
Expand All @@ -151,7 +155,10 @@ define(function (require, exports, module) {
return;
}

$statusBar.show();
if (!$statusBar.is(":visible")) {
$statusBar.show();
EditorManager.resizeEditor(); // changes available ht for editor area
}
}

function init($parent) {
Expand Down