-
Notifications
You must be signed in to change notification settings - Fork 7.7k
Safer focusedEditorChanged event #1877
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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"; | ||
|
@@ -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; | ||
/** @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(...)>} | ||
|
@@ -360,34 +377,19 @@ define(function (require, exports, module) { | |
resizeEditor(true); | ||
} | ||
|
||
|
||
/** | ||
* @private | ||
*/ | ||
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(); | ||
} | ||
|
||
$(exports).triggerHandler("focusedEditorChange", [current, previous]); | ||
_lastFocusedEditor = current; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this happen after There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, _lastFocusedEditor]); | ||
} | ||
|
||
/** | ||
|
@@ -396,7 +398,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(); | ||
} | ||
|
||
/** | ||
|
@@ -426,12 +433,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); | ||
} | ||
} | ||
|
||
|
@@ -674,12 +685,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 () { | ||
|
@@ -754,7 +762,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; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ define(function (require, exports, module) { | |
|
||
var AppInit = require("utils/AppInit"), | ||
StatusBarHTML = require("text!widgets/StatusBar.html"), | ||
EditorManager = require("editor/EditorManager"), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
@@ -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 | ||
} | ||
} | ||
|
||
/** | ||
|
@@ -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) { | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good