opendev/gerrit
Code Issues Proposed changes

Refactoring: Unifying add comment shortcut for both range and line number comments

Browse Source 
Change-Id: I8301a7b4e185b648d41dc1e3dea8d781dee9da77
This commit is contained in:
Renan Oliveira
2019-11-12 16:51:46 +01:00
parent 8b2b42086f
commit 4176ad6817
11 changed files with 209 additions and 154 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor.js
View File

@@ -246,6 +246,20 @@
this._listeningForScroll = true;
},
createCommentInPlace() {
const diffWithRangeSelected = this.diffs.find(diff => {
return diff.isRangeSelected();
});
if (diffWithRangeSelected) {
diffWithRangeSelected.createRangeComment();
} else {
const line = this.getTargetLineElement();
if (line) {
this.getTargetDiffElement().addDraftAtLine(line);
}
}
},
/**
* Get an object describing the location of the cursor. Such as
* {leftSide: false, number: 123} for line 123 of the revision, or

64
polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_test.html
View File

@@ -259,6 +259,70 @@ limitations under the License.
);
});
suite('createCommentInPlace', () => {
setup(() => {
diffElement.loggedIn = true;
});
test('adds new draft for selected line on the left', done => {
cursorElement.moveToLineNumber(2, 'left');
diffElement.addEventListener('create-comment', e => {
const {lineNum, range, side, patchNum} = e.detail;
assert.equal(lineNum, 2);
assert.equal(range, undefined);
assert.equal(patchNum, 1);
assert.equal(side, 'left');
done();
});
cursorElement.createCommentInPlace();
});
test('adds draft for selected line on the right', done => {
cursorElement.moveToLineNumber(4, 'right');
diffElement.addEventListener('create-comment', e => {
const {lineNum, range, side, patchNum} = e.detail;
assert.equal(lineNum, 4);
assert.equal(range, undefined);
assert.equal(patchNum, 2);
assert.equal(side, 'right');
done();
});
cursorElement.createCommentInPlace();
});
test('createCommentInPlace creates comment for range if selected', done => {
const someRange = {
start_line: 2,
start_character: 3,
end_line: 6,
end_character: 1,
};
diffElement.$.highlights.selectedRange = {
side: 'right',
range: someRange,
};
diffElement.addEventListener('create-comment', e => {
const {lineNum, range, side, patchNum} = e.detail;
assert.equal(lineNum, 6);
assert.equal(range, someRange);
assert.equal(patchNum, 2);
assert.equal(side, 'right');
done();
});
cursorElement.createCommentInPlace();
});
test('createCommentInPlace ignores call if nothing is selected', () => {
const createRangeCommentStub = sandbox.stub(diffElement,
'createRangeComment');
const addDraftAtLineStub = sandbox.stub(diffElement, 'addDraftAtLine');
cursorElement.diffRow = undefined;
cursorElement.createCommentInPlace();
assert.isFalse(createRangeCommentStub.called);
assert.isFalse(addDraftAtLineStub.called);
});
});
test('getAddress', () => {
// It should initialize to the first chunk: line 5 of the revision.
assert.deepEqual(cursorElement.getAddress(),

50
polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight.js
View File

@@ -33,6 +33,19 @@
* @type {?HTMLElement}
* */
_cachedDiffBuilder: Object,
/**
* Which range is currently selected by the user.
* Stored in order to add a range-based comment
* later.
* undefined if no range is selected.
*
* @type {{side: string, range: Gerrit.Range}|undefined}
*/
selectedRange: {
type: Object,
notify: true,
},
},
behaviors: [
@@ -42,7 +55,7 @@
listeners: {
'comment-thread-mouseleave': '_handleCommentThreadMouseleave',
'comment-thread-mouseenter': '_handleCommentThreadMouseenter',
'create-range-comment': '_createRangeComment',
'create-comment-requested': '_handleRangeCommentRequest',
},
get diffBuilder() {
@@ -53,10 +66,6 @@
return this._cachedDiffBuilder;
},
isRangeSelected() {
return !!this.$$('gr-selection-action-box');
},
/**
* Determines side/line/range for a DOM selection and shows a tooltip.
*
@@ -350,12 +359,12 @@
// is empty to see that it's at the end of a line.
const content = domRange.cloneContents().querySelector('.contentText');
if (isMouseUp && this._getLength(content) === 0) {
this.fire('create-range-comment', {side: start.side, range: {
this._fireCreateRangeComment(start.side, {
start_line: start.line,
start_character: 0,
end_line: start.line,
end_character: start.column,
}});
});
}
return;
}
@@ -366,13 +375,15 @@
const root = Polymer.dom(this.root);
root.insertBefore(actionBox, root.firstElementChild);
}
actionBox.range = {
start_line: start.line,
start_character: start.column,
end_line: end.line,
end_character: end.column,
this.selectedRange = {
range: {
start_line: start.line,
start_character: start.column,
end_line: end.line,
end_character: end.column,
},
side: start.side,
};
actionBox.side = start.side;
if (start.line === end.line) {
this._positionActionBox(actionBox, start.line, domRange);
} else if (start.node instanceof Text) {
@@ -389,11 +400,22 @@
}
},
_createRangeComment(e) {
_fireCreateRangeComment(side, range) {
this.fire('create-range-comment', {side, range});
this._removeActionBox();
},
_handleRangeCommentRequest(e) {
e.stopPropagation();
if (!this.selectedRange) {
throw Error('Selected Range is needed for new range comment!');
}
const {side, range} = this.selectedRange;
this._fireCreateRangeComment(side, range);
},
_removeActionBox() {
this.selectedRange = undefined;
const actionBox = this.$$('gr-selection-action-box');
if (actionBox) {
Polymer.dom(this.root).removeChild(actionBox);

121
polygerrit-ui/app/elements/diff/gr-diff-highlight/gr-diff-highlight_test.html
View File

@@ -228,13 +228,25 @@ limitations under the License.
assert.isFalse(element.set.called);
});
test('on create-range-comment action box is removed', () => {
test(`create-range-comment for range when create-comment-requested
is fired`, () => {
sandbox.stub(element, '_removeActionBox');
element.fire('create-range-comment', {
comment: {
range: {},
element.selectedRange = {
side: 'left',
range: {
start_line: 7,
start_character: 11,
end_line: 24,
end_character: 42,
},
};
const requestEvent = new CustomEvent('create-comment-requested');
let createRangeEvent;
element.addEventListener('create-range-comment', e => {
createRangeEvent = e;
});
element.dispatchEvent(requestEvent);
assert.deepEqual(element.selectedRange, createRangeEvent.detail);
assert.isTrue(element._removeActionBox.called);
});
});
@@ -271,14 +283,6 @@ limitations under the License.
element._handleSelection(selection);
};
const getActionRange = () =>
Polymer.dom(element.root).querySelector(
'gr-selection-action-box').range;
const getActionSide = () =>
Polymer.dom(element.root).querySelector(
'gr-selection-action-box').side;
const getLineElByChild = node => {
const stubs = contentStubs.find(stub => stub.contentTd.contains(node));
return stubs && stubs.lineEl;
@@ -329,14 +333,14 @@ limitations under the License.
sandbox.spy(element, '_positionActionBox');
emulateSelection(content.firstChild, 5, content.firstChild, 12);
const actionBox = element.$$('gr-selection-action-box');
assert.isTrue(element.isRangeSelected());
assert.deepEqual(getActionRange(), {
const {range, side} = element.selectedRange;
assert.deepEqual(range, {
start_line: 138,
start_character: 5,
end_line: 138,
end_character: 12,
});
assert.equal(getActionSide(), 'left');
assert.equal(side, 'left');
assert.notOk(actionBox.positionBelow);
});
@@ -346,16 +350,15 @@ limitations under the License.
sandbox.spy(element, '_positionActionBox');
emulateSelection(
startContent.firstChild, 10, endContent.lastChild, 7);
assert.isTrue(element.isRangeSelected());
const actionBox = element.$$('gr-selection-action-box');
assert.deepEqual(getActionRange(), {
const {range, side} = element.selectedRange;
assert.deepEqual(range, {
start_line: 119,
start_character: 10,
end_line: 120,
end_character: 36,
});
assert.equal(getActionSide(), 'right');
assert.equal(side, 'right');
assert.notOk(actionBox.positionBelow);
});
@@ -381,8 +384,8 @@ limitations under the License.
removeAllRanges: sandbox.stub(),
};
element._handleSelection(selection);
assert.isTrue(element.isRangeSelected());
assert.deepEqual(getActionRange(), {
const {range} = element.selectedRange;
assert.deepEqual(range, {
start_line: 119,
start_character: 10,
end_line: 120,
@@ -394,43 +397,43 @@ limitations under the License.
const startContent = stubContent(119, 'right');
const endContent = stubContent(120, 'right');
emulateSelection(startContent.firstChild, 10, endContent.firstChild, 2);
assert.isTrue(element.isRangeSelected());
assert.deepEqual(getActionRange(), {
const {range, side} = element.selectedRange;
assert.deepEqual(range, {
start_line: 119,
start_character: 10,
end_line: 120,
end_character: 2,
});
assert.equal(getActionSide(), 'right');
assert.equal(side, 'right');
});
test('collapsed', () => {
const content = stubContent(138, 'left');
emulateSelection(content.firstChild, 5, content.firstChild, 5);
assert.isOk(window.getSelection().getRangeAt(0).startContainer);
assert.isFalse(element.isRangeSelected());
assert.isFalse(!!element.selectedRange);
});
test('starts inside hl', () => {
const content = stubContent(140, 'left');
const hl = content.querySelector('.foo');
emulateSelection(hl.firstChild, 2, hl.nextSibling, 7);
assert.isTrue(element.isRangeSelected());
assert.deepEqual(getActionRange(), {
const {range, side} = element.selectedRange;
assert.deepEqual(range, {
start_line: 140,
start_character: 8,
end_line: 140,
end_character: 23,
});
assert.equal(getActionSide(), 'left');
assert.equal(side, 'left');
});
test('ends inside hl', () => {
const content = stubContent(140, 'left');
const hl = content.querySelector('.bar');
emulateSelection(hl.previousSibling, 2, hl.firstChild, 3);
assert.isTrue(element.isRangeSelected());
assert.deepEqual(getActionRange(), {
const {range} = element.selectedRange;
assert.deepEqual(range, {
start_line: 140,
start_character: 18,
end_line: 140,
@@ -442,14 +445,14 @@ limitations under the License.
const content = stubContent(140, 'left');
const hl = content.querySelectorAll('hl')[4];
emulateSelection(content.firstChild, 2, hl.firstChild, 2);
assert.isTrue(element.isRangeSelected());
assert.deepEqual(getActionRange(), {
const {range, side} = element.selectedRange;
assert.deepEqual(range, {
start_line: 140,
start_character: 2,
end_line: 140,
end_character: 61,
});
assert.equal(getActionSide(), 'left');
assert.equal(side, 'left');
});
test('starts outside of diff', () => {
@@ -458,21 +461,21 @@ limitations under the License.
emulateSelection(contentTd.previousElementSibling, 0,
contentText.firstChild, 2);
assert.isFalse(element.isRangeSelected());
assert.isFalse(!!element.selectedRange);
});
test('ends outside of diff', () => {
const content = stubContent(140, 'left');
emulateSelection(content.nextElementSibling.firstChild, 2,
content.firstChild, 2);
assert.isFalse(element.isRangeSelected());
assert.isFalse(!!element.selectedRange);
});
test('starts and ends on different sides', () => {
const startContent = stubContent(140, 'left');
const endContent = stubContent(130, 'right');
emulateSelection(startContent.firstChild, 2, endContent.firstChild, 2);
assert.isFalse(element.isRangeSelected());
assert.isFalse(!!element.selectedRange);
});
test('starts in comment thread element', () => {
@@ -481,14 +484,14 @@ limitations under the License.
'.comment-thread');
const endContent = stubContent(141, 'left');
emulateSelection(comment.firstChild, 2, endContent.firstChild, 4);
assert.isTrue(element.isRangeSelected());
assert.deepEqual(getActionRange(), {
const {range, side} = element.selectedRange;
assert.deepEqual(range, {
start_line: 140,
start_character: 83,
end_line: 141,
end_character: 4,
});
assert.equal(getActionSide(), 'left');
assert.equal(side, 'left');
});
test('ends in comment thread element', () => {
@@ -496,14 +499,14 @@ limitations under the License.
const comment = content.parentElement.querySelector(
'.comment-thread');
emulateSelection(content.firstChild, 4, comment.firstChild, 1);
assert.isTrue(element.isRangeSelected());
assert.deepEqual(getActionRange(), {
const {range, side} = element.selectedRange;
assert.deepEqual(range, {
start_line: 140,
start_character: 4,
end_line: 140,
end_character: 83,
});
assert.equal(getActionSide(), 'left');
assert.equal(side, 'left');
});
test('starts in context element', () => {
@@ -512,7 +515,7 @@ limitations under the License.
const content = stubContent(146, 'right');
emulateSelection(contextControl, 0, content.firstChild, 7);
// TODO (viktard): Select nearest line.
assert.isFalse(element.isRangeSelected());
assert.isFalse(!!element.selectedRange);
});
test('ends in context element', () => {
@@ -521,35 +524,35 @@ limitations under the License.
const content = stubContent(141, 'left');
emulateSelection(content.firstChild, 2, contextControl, 1);
// TODO (viktard): Select nearest line.
assert.isFalse(element.isRangeSelected());
assert.isFalse(!!element.selectedRange);
});
test('selection containing context element', () => {
const startContent = stubContent(130, 'right');
const endContent = stubContent(146, 'right');
emulateSelection(startContent.firstChild, 3, endContent.firstChild, 14);
assert.isTrue(element.isRangeSelected());
assert.deepEqual(getActionRange(), {
const {range, side} = element.selectedRange;
assert.deepEqual(range, {
start_line: 130,
start_character: 3,
end_line: 146,
end_character: 14,
});
assert.equal(getActionSide(), 'right');
assert.equal(side, 'right');
});
test('ends at a tab', () => {
const content = stubContent(140, 'left');
emulateSelection(
content.firstChild, 1, content.querySelector('span'), 0);
assert.isTrue(element.isRangeSelected());
assert.deepEqual(getActionRange(), {
const {range, side} = element.selectedRange;
assert.deepEqual(range, {
start_line: 140,
start_character: 1,
end_line: 140,
end_character: 51,
});
assert.equal(getActionSide(), 'left');
assert.equal(side, 'left');
});
test('starts at a tab', () => {
@@ -557,14 +560,14 @@ limitations under the License.
emulateSelection(
content.querySelectorAll('hl')[3], 0,
content.querySelectorAll('span')[1].nextSibling, 1);
assert.isTrue(element.isRangeSelected());
assert.deepEqual(getActionRange(), {
const {range, side} = element.selectedRange;
assert.deepEqual(range, {
start_line: 140,
start_character: 51,
end_line: 140,
end_character: 71,
});
assert.equal(getActionSide(), 'left');
assert.equal(side, 'left');
});
test('properly accounts for syntax highlighting', () => {
@@ -593,14 +596,14 @@ limitations under the License.
const startContent = stubContent(119, 'right');
const endContent = stubContent(120, 'right');
emulateSelection(startContent.firstChild, 0, endContent.firstChild, 0);
assert.isTrue(element.isRangeSelected());
assert.deepEqual(getActionRange(), {
const {range, side} = element.selectedRange;
assert.deepEqual(range, {
start_line: 119,
start_character: 0,
end_line: 119,
end_character: element._getLength(startContent),
});
assert.equal(getActionSide(), 'right');
assert.equal(side, 'right');
});
test('_fixTripleClickSelection empty line', () => {
@@ -608,14 +611,14 @@ limitations under the License.
const endContent = stubContent(165, 'left');
emulateSelection(startContent.firstChild, 0,
endContent.parentElement.previousElementSibling, 0);
assert.isTrue(element.isRangeSelected());
assert.deepEqual(getActionRange(), {
const {range, side} = element.selectedRange;
assert.deepEqual(range, {
start_line: 146,
start_character: 0,
end_line: 146,
end_character: 84,
});
assert.equal(getActionSide(), 'right');
assert.equal(side, 'right');
});
});
});

4
polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js
View File

@@ -395,6 +395,10 @@
return this.$.diff.isRangeSelected();
},
createRangeComment() {
return this.$.diff.createRangeComment();
},
toggleLeftDiff() {
this.$.diff.toggleLeftDiff();
},

11
polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
View File

@@ -394,15 +394,10 @@
},
_handleNewComment(e) {
if (this.shouldSuppressKeyboardShortcut(e)) { return; }
if (this.$.diffHost.isRangeSelected()) { return; }
if (this.modifierPressed(e)) { return; }
if (this.shouldSuppressKeyboardShortcut(e) ||
this.modifierPressed(e)) { return; }
e.preventDefault();
const line = this.$.cursor.getTargetLineElement();
if (line) {
this.$.diffHost.addDraftAtLine(line);
}
this.$.cursor.createCommentInPlace();
},
_handlePrevFile(e) {

21
polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
View File

@@ -434,7 +434,7 @@
/** @return {boolean} */
isRangeSelected() {
return this.$.highlights.isRangeSelected();
return !!this.$.highlights.selectedRange;
},
toggleLeftDiff() {
@@ -517,17 +517,28 @@
this._createComment(el, lineNum);
},
_handleCreateRangeComment(e) {
const range = e.detail.range;
const side = e.detail.side;
createRangeComment() {
if (!this.isRangeSelected()) {
throw Error('Selection is needed for new range comment');
}
const {side, range} = this.$.highlights.selectedRange;
this._createCommentForSelection(side, range);
},
_createCommentForSelection(side, range) {
const lineNum = range.end_line;
const lineEl = this.$.diffBuilder.getLineElByNumber(lineNum, side);
if (this._isValidElForComment(lineEl)) {
this._createComment(lineEl, lineNum, side, range);
}
},
_handleCreateRangeComment(e) {
const range = e.detail.range;
const side = e.detail.side;
this._createCommentForSelection(side, range);
},
/** @return {boolean} */
_isValidElForComment(el) {
if (!this.loggedIn) {

36
polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.js
View File

@@ -21,9 +21,9 @@
is: 'gr-selection-action-box',
/**
* Fired when the comment creation action was taken (hotkey, click).
* Fired when the comment creation action was taken (click).
*
* @event create-range-comment
* @event create-comment-requested
*/
properties: {
@@ -31,35 +31,17 @@
type: Object,
value() { return document.body; },
},
range: {
type: Object,
value: {
start_line: NaN,
start_character: NaN,
end_line: NaN,
end_character: NaN,
},
},
positionBelow: Boolean,
side: {
type: String,
value: '',
},
},
behaviors: [
Gerrit.FireBehavior,
Gerrit.KeyboardShortcutBehavior,
],
listeners: {
mousedown: '_handleMouseDown', // See https://crbug.com/gerrit/4767
},
keyBindings: {
c: '_handleCKey',
},
placeAbove(el) {
Polymer.dom.flush();
const rect = this._getTargetBoundingRect(el);
@@ -102,23 +84,11 @@
return rect;
},
_handleCKey(e) {
if (this.shouldSuppressKeyboardShortcut(e) ||
this.modifierPressed(e)) { return; }
e.preventDefault();
this._fireCreateComment();
},
_handleMouseDown(e) {
if (e.button !== 0) { return; } // 0 = main button
e.preventDefault();
e.stopPropagation();
this._fireCreateComment();
},
_fireCreateComment() {
this.fire('create-range-comment', {side: this.side, range: this.range});
this.fire('create-comment-requested');
},
});
})();

27
polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box_test.html
View File

@@ -59,11 +59,6 @@ limitations under the License.
assert.isFalse(element.fire.called);
});
test('reacts to hotkey', () => {
MockInteractions.pressAndReleaseKeyOn(document.body, 67, null, 'c');
assert.isTrue(element.fire.called);
});
suite('mousedown reacts only to main button', () => {
let e;
@@ -73,40 +68,22 @@ limitations under the License.
preventDefault: sandbox.stub(),
stopPropagation: sandbox.stub(),
};
sandbox.stub(element, '_fireCreateComment');
});
test('event handled if main button', () => {
element._handleMouseDown(e);
assert.isTrue(e.preventDefault.called);
assert(element.fire.calledWithExactly('create-comment-requested'));
});
test('event ignored if not main button', () => {
e.button = 1;
element._handleMouseDown(e);
assert.isFalse(e.preventDefault.called);
assert.isFalse(element.fire.called);
});
});
test('event fired contains playload', () => {
const side = 'left';
const range = {
start_line: 1,
start_character: 11,
end_line: 2,
end_character: 42,
};
element.side = 'left';
element.range = range;
MockInteractions.pressAndReleaseKeyOn(document.body, 67, null, 'c');
assert(element.fire.calledWithExactly(
'create-range-comment',
{
side,
range,
}));
});
suite('placeAbove', () => {
let target;