opendev/gerrit
Code Issues Proposed changes

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

Browse Source
This commit is contained in:
Renan Oliveira
2019-11-14 13:08:33 +00:00
committed by Gerrit Code Review
parent 2226864547 4176ad6817
commit 57825d0c34
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;