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

Fix Find/Replace unit tests and clean up event handling for animations #5213

Merged
merged 6 commits into from
Sep 17, 2013
Merged

Fix Find/Replace unit tests and clean up event handling for animations #5213

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9d8a24e
Check target of webkitAnimationEnd so we know the right thing is tran…
njx Sep 13, 2013
c0b6510
Add a parameter to FILE_CLOSE to force close even on a dirty file, fo…
njx Sep 13, 2013
86dc22c
Ensure that find bar is closed when switching documents (or closing t…
njx Sep 13, 2013
450642b
Make Find/Replace tests run in a separate test window, since they're …
njx Sep 13, 2013
74c09c8
Merge branch 'master' into nj/issue-5094
njx Sep 16, 2013
60faaec
Code review cleanups
njx Sep 17, 2013
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
9 changes: 6 additions & 3 deletions src/document/DocumentCommandHandlers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -798,13 +798,16 @@ define(function (require, exports, module) {
* promptOnly - If true, only displays the relevant confirmation UI and does NOT actually
* close the document. This is useful when chaining file-close together with other user
* prompts that may be cancelable.
* _forceClose - If true, closes the document without prompting even if there are unsaved
* changes. Only for use in unit tests.
* @return {$.Promise} a promise that is resolved when the file is closed, or if no file is open.
* FUTURE: should we reject the promise if no file is open?
*/
function handleFileClose(commandData) {
// If not specified, file defaults to null; promptOnly defaults to falsy
var file = commandData && commandData.file,
promptOnly = commandData && commandData.promptOnly;
var file = commandData && commandData.file,
promptOnly = commandData && commandData.promptOnly,
_forceClose = commandData && commandData._forceClose;
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems a little cleaner:

    var file, promptOnly, _forceClose;

    if (commandData) {
        file        = commandData.file,
        promptOnly  = commandData.promptOnly,
        _forceClose = commandData._forceClose; 
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


// utility function for handleFileClose: closes document & removes from working set
function doClose(fileEntry) {
Expand Down Expand Up @@ -834,7 +837,7 @@ define(function (require, exports, module) {

var doc = DocumentManager.getOpenDocumentForPath(file.fullPath);

if (doc && doc.isDirty) {
if (doc && doc.isDirty && !_forceClose) {
// Document is dirty: prompt to save changes before closing
var filename = FileUtils.getBaseName(doc.file.fullPath);

Expand Down
46 changes: 32 additions & 14 deletions src/editor/Editor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1101,13 +1101,18 @@ define(function (require, exports, module) {
self._inlineWidgets.push(inlineWidget);

// Set up the widget to start closed, then animate open when its initial height is set.
function finishAnimating(e) {
if (e.target === inlineWidget.$htmlContent.get(0)) {
inlineWidget.$htmlContent
.removeClass("animating")
.off("webkitTransitionEnd", finishAnimating);
deferred.resolve();
}
}
inlineWidget.$htmlContent
.height(0)
.addClass("animating")
.one("webkitTransitionEnd", function () {
inlineWidget.$htmlContent.removeClass("animating");
deferred.resolve();
});
.on("webkitTransitionEnd", finishAnimating);

// Callback to widget once parented to the editor. The widget should call back to
// setInlineWidgetHeight() in order to set its initial height and animate open.
Expand All @@ -1134,10 +1139,22 @@ define(function (require, exports, module) {
* @return {$.Promise} A promise that is resolved when the inline widget is fully closed and removed from the DOM.
*/
Editor.prototype.removeInlineWidget = function (inlineWidget) {
var deferred = new $.Deferred(),
self = this;

function finishAnimating(e) {
if (e.target === inlineWidget.$htmlContent.get(0)) {
inlineWidget.$htmlContent
.removeClass("animating")
.off("webkitTransitionEnd", finishAnimating);
self._codeMirror.removeLineWidget(inlineWidget.info);
self._removeInlineWidgetInternal(inlineWidget);
deferred.resolve();
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

For the number of times this pattern of code is repeated (and number of times I imagine that it will be repeated in the future), it seems like it deserves a utility function. Parameters would be:

  • current element (e.target)
  • target element (inlineWidget.$htmlContent)
  • class to remove (animating)
  • deferred to resolve (deferred)
  • catchall callback:
    self._codeMirror.removeLineWidget(inlineWidget.info);
    self._removeInlineWidgetInternal(inlineWidget);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I didn't end up passing all these things in--instead, I just had the utility function return its own promise, and then did the custom processing for each case in the done() handler.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!


if (!inlineWidget.closePromise) {
var lineNum = this._getInlineWidgetLineNumber(inlineWidget),
deferred = new $.Deferred(),
self = this;
var lineNum = this._getInlineWidgetLineNumber(inlineWidget);

// Remove the inline widget from our internal list immediately, so
// everyone external to us knows it's essentially already gone. We
Expand All @@ -1146,12 +1163,7 @@ define(function (require, exports, module) {
self._removeInlineWidgetFromList(inlineWidget);

inlineWidget.$htmlContent.addClass("animating")
.one("webkitTransitionEnd", function () {
inlineWidget.$htmlContent.removeClass("animating");
self._codeMirror.removeLineWidget(inlineWidget.info);
self._removeInlineWidgetInternal(inlineWidget);
deferred.resolve();
})
.on("webkitTransitionEnd", finishAnimating)
.height(0);

inlineWidget.closePromise = deferred.promise();
Expand Down Expand Up @@ -1264,9 +1276,15 @@ define(function (require, exports, module) {
}

function setOuterHeight() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that I didn't use AnimationUtils.animateUsingClass() here since in this case the class is being handled separately. It didn't seem worth generalizing that function to handle this case as well.

function finishAnimating(e) {
if (e.target === node) {
updateHeight();
$(node).off("webkitTransitionEnd", finishAnimating);
}
}
$(node).height(height);
if ($(node).hasClass("animating")) {
$(node).one("webkitTransitionEnd", updateHeight);
$(node).on("webkitTransitionEnd", finishAnimating);
} else {
updateHeight();
}
Expand Down
15 changes: 14 additions & 1 deletion src/search/FindReplace.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,6 +378,19 @@ define(function (require, exports, module) {
}
$(currentDocument).off("change.replaceAll");
}

/**
* @private
* When the user switches documents (or closes the last document), ensure that the find bar
* closes, and also close the Replace All panel.
*/
function _handleDocumentChange() {
if (modalBar) {
modalBar.close();
modalBar = null;
}
_closeReplaceAllPanel();
}

/**
* @private
Expand Down Expand Up @@ -613,7 +626,7 @@ define(function (require, exports, module) {
});
});

$(DocumentManager).on("currentDocumentChange", _closeReplaceAllPanel);
$(DocumentManager).on("currentDocumentChange", _handleDocumentChange);

CommandManager.register(Strings.CMD_FIND, Commands.EDIT_FIND, _launchFind);
CommandManager.register(Strings.CMD_FIND_NEXT, Commands.EDIT_FIND_NEXT, _findNext);
Expand Down
13 changes: 9 additions & 4 deletions src/widgets/ModalBar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,10 +140,15 @@ define(function (require, exports, module) {
}

var self = this;
this._$root.addClass("modal-bar-hide").one("webkitTransitionEnd", function () {
self._$root.remove();
result.resolve();
});
function removeRoot(e) {
if (e.target === self._$root.get(0)) {
self._$root
.remove()
.off("webkitTransitionEnd", removeRoot);
result.resolve();
}
}
this._$root.addClass("modal-bar-hide").on("webkitTransitionEnd", removeRoot);

// Preserve scroll position of the current full editor across the editor refresh, adjusting for the
// height of the modal bar so the code doesn't appear to shift if possible.
Expand Down
Loading