-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Fixes #1256 #1769
Fixes #1256 #1769
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 |
---|---|---|
|
@@ -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 | ||
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 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) 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 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! 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. 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 | ||
|
@@ -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); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
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. Nice clean up! 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. 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); | ||
}); | ||
}); | ||
|
||
|
@@ -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; | ||
|
@@ -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 () { | ||
|
@@ -429,6 +466,16 @@ define(function (require, exports, module) { | |
this._ensureCursorVisible(); | ||
}; | ||
|
||
/** | ||
* If Document's file is deleted, or Editor loses sync with Document | ||
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. 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; } | ||
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. 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>} | ||
*/ | ||
|
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.
Is it a typo? thought?
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.
Yes! It's supposed to say "though". Good catch!