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
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.js b/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.js
index 12b30b2..6419a7c 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.js
@@ -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);
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
index f260646..9fc9232 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
@@ -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() {
