This repository has been archived by the owner on Sep 6, 2021. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 7.7k
Clean up addWidget flow #535
Merged
Merged
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
3b319c9
Get rid of EditorManager._addInlineWidget, an dmake it so Editor.addI…
njx 683ac5b
Merge remote-tracking branch 'origin/master' into nj/inline-cleanup
njx 25fd303
Made Editor.setInlineWidgetHeight() also take a widget instead of an ID
njx 1f74f8a
Code review cleanup: changed onClosed() to close(), other minor fixes.
njx 3d23f64
Reverted close() back to onClosed() since close() already means somet…
njx 409416e
Merge from master
njx File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -346,8 +346,8 @@ define(function (require, exports, module) { | |
|
||
// Destroying us destroys any inline widgets we're hosting. Make sure their closeCallbacks | ||
// run, at least, since they may also need to release Document refs | ||
this._inlineWidgets.forEach(function (inlineInfo) { | ||
inlineInfo.closeCallback(); | ||
this._inlineWidgets.forEach(function (inlineWidget) { | ||
inlineWidget.onClosed(); | ||
}); | ||
}; | ||
|
||
|
@@ -689,40 +689,31 @@ define(function (require, exports, module) { | |
return this._codeMirror.getScrollerElement(); | ||
}; | ||
|
||
|
||
/** | ||
* Adds an inline widget below the given line. If any inline widget was already open for that | ||
* line, it is closed without warning. | ||
* @param {!{line:number, ch:number}} pos Position in text to anchor the inline. | ||
* @param {!DOMElement} domContent DOM node of widget UI to insert. | ||
* @param {number} initialHeight Initial height to accomodate. | ||
* @param {function()} parentShowCallback Function called when the host editor is shown | ||
* (via Editor.setVisible()). | ||
* @param {function()} closeCallback Function called when inline is closed, either automatically | ||
* by CodeMirror, or by this host Editor closing, or manually via removeInlineWidget(). | ||
* @param {Object} data Extra data to track along with the widget. Accessible later via | ||
* {@link #getInlineWidgets()}. | ||
* @return {number} id for this inline widget instance; unique to this Editor | ||
*/ | ||
Editor.prototype.addInlineWidget = function (pos, domContent, initialHeight, parentShowCallback, closeCallback, data) { | ||
// Now add the new widget | ||
* @param {!InlineWidget} inlineWidget The widget to add. | ||
*/ | ||
Editor.prototype.addInlineWidget = function (pos, inlineWidget) { | ||
var self = this; | ||
var inlineId = this._codeMirror.addInlineWidget(pos, domContent, initialHeight, function (id) { | ||
inlineWidget.id = this._codeMirror.addInlineWidget(pos, inlineWidget.htmlContent, inlineWidget.height, function (id) { | ||
self._removeInlineWidgetInternal(id); | ||
closeCallback(); | ||
inlineWidget.onClosed(); | ||
}); | ||
this._inlineWidgets.push({ id: inlineId, data: data, parentShowCallback: parentShowCallback, closeCallback: closeCallback }); | ||
this._inlineWidgets.push(inlineWidget); | ||
inlineWidget.onAdded(); | ||
|
||
return inlineId; | ||
return inlineWidget.id; | ||
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. Return value no longer used. |
||
}; | ||
|
||
/** | ||
* Removes the given inline widget. | ||
* @param {number} inlineId id returned by addInlineWidget(). | ||
* @param {number} inlineWidget The widget to remove. | ||
*/ | ||
Editor.prototype.removeInlineWidget = function (inlineId) { | ||
Editor.prototype.removeInlineWidget = function (inlineWidget) { | ||
// _removeInlineWidgetInternal will get called from the destroy callback in CodeMirror. | ||
this._codeMirror.removeInlineWidget(inlineId); | ||
this._codeMirror.removeInlineWidget(inlineWidget.id); | ||
}; | ||
|
||
/** | ||
|
@@ -749,13 +740,13 @@ define(function (require, exports, module) { | |
}; | ||
|
||
/** | ||
* Sets the height of the inline widget for this editor. The inline editor is identified by id. | ||
* @param {!number} id | ||
* @param {!height} height | ||
* @param {boolean} ensureVisible | ||
* Sets the height of an inline widget in this editor. | ||
* @param {!InlineWidget} inlineWidget The widget whose height should be set. | ||
* @param {!height} height The height of the widget. | ||
* @param {boolean} ensureVisible Whether to scroll the entire widget into view. | ||
*/ | ||
Editor.prototype.setInlineWidgetHeight = function (id, height, ensureVisible) { | ||
this._codeMirror.setInlineWidgetHeight(id, height, ensureVisible); | ||
Editor.prototype.setInlineWidgetHeight = function (inlineWidget, height, ensureVisible) { | ||
this._codeMirror.setInlineWidgetHeight(inlineWidget.id, height, ensureVisible); | ||
}; | ||
|
||
|
||
|
@@ -786,10 +777,8 @@ define(function (require, exports, module) { | |
$(this._codeMirror.getWrapperElement()).css("display", (show ? "" : "none")); | ||
this._codeMirror.refresh(); | ||
if (show) { | ||
this._inlineWidgets.forEach(function (widget) { | ||
if (widget.parentShowCallback) { | ||
widget.parentShowCallback(); | ||
} | ||
this._inlineWidgets.forEach(function (inlineWidget) { | ||
inlineWidget.onParentShown(); | ||
}); | ||
} | ||
}; | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -108,30 +108,6 @@ define(function (require, exports, module) { | |
return new Editor(doc, makeMasterEditor, mode, container, extraKeys, range); | ||
} | ||
|
||
/** | ||
* @private | ||
* Adds a new widget to the host Editor. | ||
* @param {!Editor} editor the candidate host editor | ||
* @param !{line:number, ch:number} pos | ||
* @param {!InlineWidget} inlineWidget | ||
*/ | ||
function _addInlineWidget(editor, pos, inlineWidget) { | ||
$(inlineWidget.htmlContent).append('<div class="shadow top"/>') | ||
.append('<div class="shadow bottom"/>'); | ||
|
||
var closeCallback = function () { | ||
inlineWidget.onClosed(); | ||
}; | ||
var parentShowCallback = function () { | ||
inlineWidget.onParentShown(); | ||
}; | ||
|
||
var inlineId = editor.addInlineWidget(pos, inlineWidget.htmlContent, inlineWidget.height, | ||
parentShowCallback, closeCallback, inlineWidget); | ||
|
||
inlineWidget.onAdded(inlineId); | ||
} | ||
|
||
/** | ||
* @private | ||
* Bound to Ctrl+E on outermost editors. | ||
|
@@ -154,7 +130,7 @@ define(function (require, exports, module) { | |
// If one of them will provide a widget, show it inline once ready | ||
if (inlinePromise) { | ||
inlinePromise.done(function (inlineWidget) { | ||
_addInlineWidget(editor, pos, inlineWidget); | ||
editor.addInlineWidget(pos, inlineWidget); | ||
result.resolve(); | ||
}).fail(function () { | ||
result.reject(); | ||
|
@@ -174,11 +150,11 @@ define(function (require, exports, module) { | |
* @param {!boolean} moveFocus If true, focuses hostEditor and ensures the cursor position lies | ||
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. param above this one should be {!InlineWidget} inlineWidget |
||
* near the inline's location. | ||
*/ | ||
function closeInlineWidget(hostEditor, inlineId, moveFocus) { | ||
function closeInlineWidget(hostEditor, inlineWidget, moveFocus) { | ||
if (moveFocus) { | ||
// Place cursor back on the line just above the inline (the line from which it was opened) | ||
// If cursor's already on that line, leave it be to preserve column position | ||
var widgetLine = hostEditor._codeMirror.getInlineWidgetInfo(inlineId).line; | ||
var widgetLine = hostEditor._codeMirror.getInlineWidgetInfo(inlineWidget.id).line; | ||
var cursorLine = hostEditor.getCursorPos().line; | ||
if (cursorLine !== widgetLine) { | ||
hostEditor.setCursorPos({ line: widgetLine, pos: 0 }); | ||
|
@@ -187,7 +163,7 @@ define(function (require, exports, module) { | |
hostEditor.focus(); | ||
} | ||
|
||
hostEditor.removeInlineWidget(inlineId); | ||
hostEditor.removeInlineWidget(inlineWidget); | ||
|
||
} | ||
|
||
|
@@ -219,8 +195,8 @@ define(function (require, exports, module) { | |
function getInlineEditors(hostEditor) { | ||
var inlineEditors = []; | ||
hostEditor.getInlineWidgets().forEach(function (widget) { | ||
if (widget.data instanceof InlineTextEditor) { | ||
inlineEditors.concat(widget.data.editors); | ||
if (widget instanceof InlineTextEditor) { | ||
inlineEditors.concat(widget.editors); | ||
} | ||
}); | ||
return inlineEditors; | ||
|
@@ -449,8 +425,8 @@ define(function (require, exports, module) { | |
|
||
// See if any inlines have focus | ||
_currentEditor.getInlineWidgets().forEach(function (widget) { | ||
if (widget.data instanceof InlineTextEditor) { | ||
widget.data.editors.forEach(function (editor) { | ||
if (widget instanceof InlineTextEditor) { | ||
widget.editors.forEach(function (editor) { | ||
if (editor.hasFocus()) { | ||
focusedInline = editor; | ||
} | ||
|
@@ -480,7 +456,6 @@ define(function (require, exports, module) { | |
|
||
// For unit tests | ||
exports._openInlineWidget = _openInlineWidget; | ||
exports._addInlineWidget = _addInlineWidget; | ||
|
||
// Define public API | ||
exports.setEditorHolder = setEditorHolder; | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 just rename onClosed() to close()? Not sure when attribute-style naming (e.g. onmousedown) makes sense though.