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

Fixes #1256 #1769

Merged
merged 4 commits into from
Oct 10, 2012
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
10 changes: 9 additions & 1 deletion src/document/TextRange.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ define(function (require, exports, module) {
* - lostSync -- When the backing Document changes in such a way that the range can no longer
* accurately be maintained. Generally, occurs whenever an edit spans a range boundary.
* After this, startLine & endLine will be unusable (set to null).
* Also occurs when the document is deleted, thought startLine & endLine won't be modified
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it a typo? thought?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! It's supposed to say "though". Good catch!

* These events only ever occur in response to Document changes, so if you are already listening
* to the Document, you could ignore the TextRange events and just read its updated value in your
* own Document change handler.
Expand All @@ -59,16 +60,19 @@ define(function (require, exports, module) {

this.document = document;
document.addRef();
// store this-bound version of listener so we can remove them later
// store this-bound versions of listeners so we can remove them later
this._handleDocumentChange = this._handleDocumentChange.bind(this);
this._handleDocumentDeleted = this._handleDocumentDeleted.bind(this);
$(document).on("change", this._handleDocumentChange);
$(document).on("deleted", this._handleDocumentDeleted);
}

/** Detaches from the Document. The TextRange will no longer update or send change events */
TextRange.prototype.dispose = function (editor, change) {
// Disconnect from Document
this.document.releaseRef();
$(this.document).off("change", this._handleDocumentChange);
$(this.document).off("deleted", this._handleDocumentDeleted);
};


Expand Down Expand Up @@ -167,6 +171,10 @@ define(function (require, exports, module) {
this._applyChangesToRange(changeList);
};

TextRange.prototype._handleDocumentDeleted = function (event) {
$(this).triggerHandler("lostSync");
};


/* (pretty toString(), to aid debugging) */
TextRange.prototype.toString = function () {
Expand Down
5 changes: 3 additions & 2 deletions src/editor/Editor.js
Original file line number Diff line number Diff line change
Expand Up @@ -561,8 +561,9 @@ define(function (require, exports, module) {
* Responds to the Document's underlying file being deleted. The Document is now basically dead,
* so we must close.
*/
Editor.prototype._handleDocumentDeleted = function () {
$(this).triggerHandler("lostContent");
Editor.prototype._handleDocumentDeleted = function (event) {
// Pass the delete event along as the cause (needed in MultiRangeInlineEditor)
$(this).triggerHandler("lostContent", [event]);
};


Expand Down
14 changes: 10 additions & 4 deletions src/editor/InlineTextEditor.js
Original file line number Diff line number Diff line change
Expand Up @@ -247,11 +247,9 @@ define(function (require, exports, module) {
$lineNumber.text(inlineInfo.editor.getFirstVisibleLine() + 1);
});

// If Document's file is deleted, or Editor loses sync with Document, just close
// If Document's file is deleted, or Editor loses sync with Document
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need to update this comment since it is still correct. You don't close it right here directly, but you're still closing it in your _onLostContent apply call. Besides, your updated comment becomes an incomplete sentence. (ie. if condition A or condition B, instead of if condition A or condition B, take this action)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hear you on the incomplete sentence issue, I'd prefer it otherwise as well!

With regards to the specific action the idea was to not give any false impressions. All this code does is call this._onLostContent, which (due to subclassing) might be a different version than the one in the same file. This comment might therefore become outdated without any chance of the author noticing the discrepancy (and is, in fact, flat out wrong for MultiRangeInlineEditor.js, where a different _onLostContent gets called by the same code).

So I'd propose to replace "just close" with "call this._onLostContent" or something less literal - but I'm definitely open for counter positions!

Copy link
Contributor

Choose a reason for hiding this comment

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

You're absolutely right. So no objection to your proposal.

$(inlineInfo.editor).on("lostContent", function () {
// Note: this closes the entire inline widget if any one Editor loses sync. This seems
// better than leaving it open but suddenly removing one rule from the result list.
self.close();
self._onLostContent.apply(self, arguments);
});

// set dirty indicator state
Expand Down Expand Up @@ -285,6 +283,14 @@ define(function (require, exports, module) {
});
};

/**
* If Document's file is deleted, or Editor loses sync with Document, just close
*/
InlineTextEditor.prototype._onLostContent = function () {
// Note: this closes the entire inline widget if any one Editor loses sync. This seems
// better than leaving it open but suddenly removing one rule from the result list.
this.close();
};

// consolidate all dirty document updates
$(DocumentManager).on("dirtyFlagChange", _dirtyFlagChangeHandler);
Expand Down
69 changes: 58 additions & 11 deletions src/editor/MultiRangeInlineEditor.js
Original file line number Diff line number Diff line change
Expand Up @@ -146,25 +146,24 @@ define(function (require, exports, module) {

// create range list & add listeners for range textrange changes
var rangeItemText;
this._ranges.forEach(function (range, i) {
this._ranges.forEach(function (range) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice clean up!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks :)

// Create list item UI
var $rangeItem = $(window.document.createElement("li")).appendTo($rangeList);
_updateRangeLabel($rangeItem, range);
$rangeItem.mousedown(function () {
self.setSelectedIndex(i);
self.setSelectedIndex(self._ranges.indexOf(range));
});

self._ranges[i].$listItem = $rangeItem;
range.$listItem = $rangeItem;

// Update list item as TextRange changes
$(self._ranges[i].textRange).on("change", function () {
$(range.textRange).on("change", function () {
_updateRangeLabel($rangeItem, range);
});

// If TextRange lost sync, react just as we do for an inline Editor's lostContent event:
// close the whole inline widget
$(self._ranges[i].textRange).on("lostSync", function () {
self.close();
// If TextRange lost sync, remove it from the list (and close the widget if no other ranges are left)
$(range.textRange).on("lostSync", function () {
self._removeRange(range);
});
});

Expand Down Expand Up @@ -207,10 +206,10 @@ define(function (require, exports, module) {
}

// Remove selected class(es)
var previousItem = (this._selectedRangeIndex >= 0) ? this._ranges[this._selectedRangeIndex].$listItem : null;
var $previousItem = (this._selectedRangeIndex >= 0) ? this._ranges[this._selectedRangeIndex].$listItem : null;

if (previousItem) {
previousItem.toggleClass("selected", false);
if ($previousItem) {
$previousItem.toggleClass("selected", false);
}

this._selectedRangeIndex = newIndex;
Expand Down Expand Up @@ -249,6 +248,44 @@ define(function (require, exports, module) {
this.sizeInlineWidgetToContents(true, false);
this._updateRelatedContainer();

this._updateSelectedMarker();
};

MultiRangeInlineEditor.prototype._removeRange = function (range) {
// If this is the last range, just close the whole widget
if (this._ranges.length <= 1) {
this.close();
return;
}

// Now we know there is at least one other range -> found out which one this is
var index = this._ranges.indexOf(range);

// If the range to be removed is the selected one, first switch to another one
if (index === this._selectedRangeIndex) {
// If possible, select the one below, else select the one above
if (index + 1 < this._ranges.length) {
this.setSelectedIndex(index + 1);
} else {
this.setSelectedIndex(index - 1);
}
}

// Now we can remove this range
range.$listItem.remove();
range.textRange.dispose();
this._ranges.splice(index, 1);

// If the selected range is below, we need to update the index
if (index < this._selectedRangeIndex) {
this._selectedRangeIndex--;
this._updateSelectedMarker();
}
};

MultiRangeInlineEditor.prototype._updateSelectedMarker = function () {
var $rangeItem = this._ranges[this._selectedRangeIndex].$listItem;

// scroll the selection to the rangeItem, use setTimeout to wait for DOM updates
var self = this;
window.setTimeout(function () {
Expand Down Expand Up @@ -429,6 +466,16 @@ define(function (require, exports, module) {
this._ensureCursorVisible();
};

/**
* If Document's file is deleted, or Editor loses sync with Document
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, an incomplete sentence here :)

*/
MultiRangeInlineEditor.prototype._onLostContent = function (event, cause) {
// Ignore when the editor's content got lost due to a deleted file, this is handled via TextRange's lostSync
if (cause && cause.type === 'deleted') { return; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Our coding style is to use double quotes for strings

// Else yield to the parent's implementation
return this.parentClass._onLostContent.apply(this, arguments);
};

/**
* @return {Array.<SearchResultItem>}
*/
Expand Down