Create comment immediately when double-clicking after a line

This assumes that any empty selection of just one line that ends at the
start of a line is an after-end-of-line selection. We capture it on
mouseup to avoid comments appearing while dragging.

Known issues:

Occasionally, when the comment is created on an empty line, discarding
the comment can trigger another mouseup that creates a new comment.
Occasionally the selection of a line hangs around and double-clicking
after the end of the line doesn't do anything until the selection is cleared.

Change-Id: Ifeea4f25e2ee4d4643766e4bdff31a74fedca68f
This commit is contained in:
Lars Clausen
2019-01-04 16:26:49 +01:00
parent 714f7d3c3f
commit 51a9cb00e4
2 changed files with 51 additions and 10 deletions

View File

@@ -65,14 +65,17 @@
*
* @param {Selection} selection A DOM Selection living in the shadow DOM of
* the diff element.
* @param {boolean} isMouseUp If true, this is called due to a mouseup
* event, in which case we might want to immediately create a comment.
*/
handleSelectionChange(selection) {
handleSelectionChange(selection, isMouseUp) {
// Can't use up or down events to handle selection started and/or ended in
// in comment threads or outside of diff.
// Debounce removeActionBox to give it a chance to react to click/tap.
this._removeActionBoxDebounced();
this.debounce(
'selectionChange', () => this._handleSelection(selection), 200);
'selectionChange', () => this._handleSelection(selection, isMouseUp),
200);
},
_getThreadEl(e) {
@@ -112,8 +115,12 @@
_indexOfCommentRange(side, range) {
function rangesEqual(a, b) {
if (!a && !b) { return true; }
if (!a || !b) { return false; }
if (!a && !b) {
return true;
}
if (!a || !b) {
return false;
}
return a.start_line === b.start_line &&
a.start_character === b.start_character &&
a.end_line === b.end_line &&
@@ -290,7 +297,7 @@
actionBox.placeBelow(range);
},
_handleSelection(selection) {
_handleSelection(selection, isMouseUp) {
const normalizedRange = this._getNormalizedRange(selection);
if (!normalizedRange) {
return;
@@ -313,6 +320,24 @@
// TODO (viktard): Drop empty first and last lines from selection.
// If this was a mouse-up event, we create a comment immediately if
// the selection is from the end of a line to the start of the next line.
// Rather than trying to find the line contents, we just check if the
// selection is empty to see that it's at the end of a line.
// In this case, we select the entire start line.
if (isMouseUp && start.line === end.line - 1 && end.column === 0) {
const content = domRange.cloneContents().querySelector('.contentText');
if (this._getLength(content) === 0) {
this.fire('create-range-comment', {side: start.side, range: {
start_line: start.line,
start_character: 0,
end_line: start.line,
end_character: start.column,
}});
return;
}
}
const actionBox = document.createElement('gr-selection-action-box');
const root = Polymer.dom(this.root);
root.insertBefore(actionBox, root.firstElementChild);

View File

@@ -282,22 +282,38 @@
_enableSelectionObserver(loggedIn, isAttached) {
if (loggedIn && isAttached) {
this.listen(document, 'selectionchange', '_handleSelectionChange');
this.listen(document, 'mouseup', '_handleMouseUp');
} else {
this.unlisten(document, 'selectionchange', '_handleSelectionChange');
this.unlisten(document, 'mouseup', '_handleMouseUp');
}
},
_handleSelectionChange() {
// Because of shadow DOM selections, we handle the selectionchange here,
// and pass the shadow DOM selection into gr-diff-highlight, where the
// corresponding range is determined and normalized.
const selection = this._getShadowOrDocumentSelection();
this.$.highlights.handleSelectionChange(selection, false);
},
_handleMouseUp(e) {
// To handle double-click outside of text creating comments, we check on
// mouse-up if there's a selection that just covers a line change. We
// can't do that on selection change since the user may still be dragging.
const selection = this._getShadowOrDocumentSelection();
this.$.highlights.handleSelectionChange(selection, true);
},
/** Gets the current selection, preferring the shadow DOM selection. */
_getShadowOrDocumentSelection() {
// When using native shadow DOM, the selection returned by
// document.getSelection() cannot reference the actual DOM elements making
// up the diff, because they are in the shadow DOM of the gr-diff element.
// For this reason, we handle the selectionchange here, and pass the
// shadow DOM selection into gr-diff-highlight, where the corresponding
// range is determined and normalized.
const selection = this.root.getSelection ?
// This takes the shadow DOM selection if one exists.
return this.root.getSelection ?
this.root.getSelection() :
document.getSelection();
this.$.highlights.handleSelectionChange(selection);
},
_observeNodes() {