Move comment patchset logic to gr-diff-host
This is a step towards making gr-diff oblivious of the specifics of the
selected PatchRange. This makes it easier to reuse gr-diff independently
of the underlying versioning system.
A comment's patchset (and commentSide, already in gr-diff-host) together
determine which patch set a comment is stored on. This is usually the
patchset the user left the comment on, except in the case of PARENT and
merge parent versions which are not patchsets and cannot store comments.
Change-Id: I5e7e30f93ee5a74fe8f2c2e6b225282246c51bd9
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_test.js b/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_test.js
index ba81829..35bbe4a 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_test.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor_test.js
@@ -521,10 +521,9 @@
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;
+ const {lineNum, range, side} = e.detail;
assert.equal(lineNum, 2);
assert.equal(range, undefined);
- assert.equal(patchNum, 1);
assert.equal(side, 'left');
done();
});
@@ -534,17 +533,16 @@
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;
+ const {lineNum, range, side} = 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 => {
+ test('creates comment for range if selected', done => {
const someRange = {
start_line: 2,
start_character: 3,
@@ -556,17 +554,16 @@
range: someRange,
};
diffElement.addEventListener('create-comment', e => {
- const {lineNum, range, side, patchNum} = e.detail;
+ const {lineNum, range, side} = 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', () => {
+ test('ignores call if nothing is selected', () => {
const createRangeCommentStub = sinon.stub(diffElement,
'createRangeComment');
const addDraftAtLineStub = sinon.stub(diffElement, 'addDraftAtLine');
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
index 7b58eb6..2fc8197 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
@@ -32,6 +32,7 @@
import {appContext} from '../../../services/app-context';
import {
getParentIndex,
+ isAParent,
isMergeParent,
isNumber,
} from '../../../utils/patch-set-util';
@@ -729,8 +730,26 @@
}
_handleCreateComment(e: CustomEvent) {
- const {lineNum, side, patchNum, range, path} = e.detail;
- const commentSide = this._sideToCommentSide(side);
+ if (!this.patchRange) throw Error('patch range not set');
+
+ const {lineNum, side, range, path} = e.detail;
+
+ // Usually, the comment is stored on the patchset shown on the side the
+ // user added the comment on, and the commentSide will be REVISION.
+ // However, if the comment is added on the left side of the diff and the
+ // version shown there is not a patchset that is part the change, but
+ // instead a base (a PARENT or a merge parent commit), the comment is
+ // stored on the patchset shown on the right, and commentSide=PARENT
+ // indicates that the comment should still be shown on the left side.
+ const patchNum =
+ side === Side.LEFT && !isAParent(this.patchRange.basePatchNum)
+ ? this.patchRange.basePatchNum
+ : this.patchRange.patchNum;
+ const commentSide =
+ side === Side.LEFT && isAParent(this.patchRange.basePatchNum)
+ ? CommentSide.PARENT
+ : CommentSide.REVISION;
+
const threadEl = this._getOrCreateThread(
patchNum,
lineNum,
@@ -744,15 +763,6 @@
this.reporting.recordDraftInteraction();
}
- _sideToCommentSide(side: Side): CommentSide {
- if (!this.patchRange) throw Error('patch range not set');
- return side === Side.LEFT &&
- (this.patchRange.basePatchNum === 'PARENT' ||
- isMergeParent(this.patchRange.basePatchNum))
- ? CommentSide.PARENT
- : CommentSide.REVISION;
- }
-
/**
* Gets or creates a comment thread at a given location.
* May provide a range, to get/create a range comment.
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js
index ad46bcd..0c1a92d 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js
@@ -966,7 +966,6 @@
element.dispatchEvent(new CustomEvent('create-comment', {
detail: {
- patchNum: 2,
lineNum: 3,
side: diffSide,
path: '/p',
@@ -989,10 +988,13 @@
end_line: 1,
end_character: 3,
};
+ element.patchRange = {
+ basePatchNum: 'PARENT',
+ patchNum: 3,
+ };
element.dispatchEvent(new CustomEvent('create-comment', {
detail: {
- patchNum: 3,
lineNum: 1,
side: diffSide,
path: '/p',
@@ -1018,7 +1020,6 @@
element.dispatchEvent(new CustomEvent('create-comment', {
detail: {
- patchNum: 2,
side: Side.RIGHT,
},
}));
@@ -1037,7 +1038,6 @@
element.dispatchEvent(new CustomEvent('create-comment', {
detail: {
- patchNum: 3,
side: Side.LEFT,
},
}));
@@ -1056,7 +1056,6 @@
element.dispatchEvent(new CustomEvent('create-comment', {
detail: {
- patchNum: 3,
side: Side.LEFT,
},
}));
@@ -1075,7 +1074,6 @@
element.dispatchEvent(new CustomEvent('create-comment', {
detail: {
- patchNum: 3,
side: Side.LEFT,
},
}));
@@ -1097,8 +1095,6 @@
element.dispatchEvent(new CustomEvent('create-comment', {
detail: {
- patchNum: 2,
- lineNum: 3,
side: diffSide,
path: '/p',
},
@@ -1123,8 +1119,6 @@
element.dispatchEvent(new CustomEvent('create-comment', {
detail: {
- patchNum: 2,
- lineNum: 3,
side: diffSide,
path: '/p',
},
@@ -1149,11 +1143,8 @@
element.dispatchEvent(new CustomEvent('create-comment', {
detail: {
- patchNum: 2,
- lineNum: 3,
side: diffSide,
path: '/p',
- range: undefined,
},
}));
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts
index 44437b4..84bb69f 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts
@@ -40,7 +40,6 @@
rangesEqual,
} from './gr-diff-utils';
import {getHiddenScroll} from '../../../scripts/hiddenscroll';
-import {isMergeParent} from '../../../utils/patch-set-util';
import {customElement, observe, property} from '@polymer/decorators';
import {
BlameInfo,
@@ -639,22 +638,16 @@
) {
const contentEl = this.$.diffBuilder.getContentTdByLineEl(lineEl);
if (!contentEl) throw Error('content el not found for line el');
- side = side || this._getCommentSideByLineAndContent(lineEl, contentEl);
- const patchForNewThreads = this._getPatchNumByLineAndContent(
- lineEl,
- contentEl
- );
-
+ side = side ?? this._getCommentSideByLineAndContent(lineEl, contentEl);
this.dispatchEvent(
new CustomEvent('create-comment', {
bubbles: true,
composed: true,
detail: {
- lineNum,
- side,
- patchNum: patchForNewThreads,
- range,
path: this.path,
+ side,
+ lineNum,
+ range,
},
})
);
@@ -680,33 +673,6 @@
return threadGroupEl;
}
- /**
- * The value to be used for the patch number of new comments created at the
- * given line and content elements.
- *
- * In two cases of creating a comment on the left side, the patch number to
- * be used should actually be right side of the patch range:
- * - When the patch range is against the parent comment of a normal change.
- * Such comments declare themmselves to be on the left using side=PARENT.
- * - If the patch range is against the indexed parent of a merge change.
- * Such comments declare themselves to be on the given parent by
- * specifying the parent index via parent=i.
- */
- _getPatchNumByLineAndContent(lineEl: Element, contentEl: Element) {
- if (!this.patchRange) throw Error('patch range not set');
- let patchNum = this.patchRange.patchNum;
-
- if (
- (lineEl.classList.contains(Side.LEFT) ||
- contentEl.classList.contains('remove')) &&
- this.patchRange.basePatchNum !== 'PARENT' &&
- !isMergeParent(this.patchRange.basePatchNum)
- ) {
- patchNum = this.patchRange.basePatchNum;
- }
- return patchNum;
- }
-
_getCommentSideByLineAndContent(lineEl: Element, contentEl: Element): Side {
return lineEl.classList.contains(Side.LEFT) ||
contentEl.classList.contains('remove')
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.js b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.js
index baee7aa..5882f10 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.js
@@ -87,52 +87,6 @@
assert.isNotOk(getComputedStyleValue('--line-limit', element));
});
- suite('_getPatchNumByLineAndContent', () => {
- let lineEl;
- let contentEl;
-
- setup(() => {
- element = basicFixture.instantiate();
- lineEl = document.createElement('td');
- contentEl = document.createElement('span');
- });
-
- test('right side', () => {
- element.patchRange = {patchNum: 4, basePatchNum: 'PARENT'};
- lineEl.classList.add('right');
- assert.equal(element._getPatchNumByLineAndContent(lineEl, contentEl),
- 4);
- });
-
- test('left side parent by linenum', () => {
- element.patchRange = {patchNum: 4, basePatchNum: 'PARENT'};
- lineEl.classList.add('left');
- assert.equal(element._getPatchNumByLineAndContent(lineEl, contentEl),
- 4);
- });
-
- test('left side parent by content', () => {
- element.patchRange = {patchNum: 4, basePatchNum: 'PARENT'};
- contentEl.classList.add('remove');
- assert.equal(element._getPatchNumByLineAndContent(lineEl, contentEl),
- 4);
- });
-
- test('left side merge parent', () => {
- element.patchRange = {patchNum: 4, basePatchNum: -2};
- contentEl.classList.add('remove');
- assert.equal(element._getPatchNumByLineAndContent(lineEl, contentEl),
- 4);
- });
-
- test('left side non parent', () => {
- element.patchRange = {patchNum: 4, basePatchNum: 3};
- contentEl.classList.add('remove');
- assert.equal(element._getPatchNumByLineAndContent(lineEl, contentEl),
- 3);
- });
- });
-
suite('not logged in', () => {
setup(() => {
const getLoggedInPromise = Promise.resolve(false);
diff --git a/polygerrit-ui/app/utils/patch-set-util.ts b/polygerrit-ui/app/utils/patch-set-util.ts
index 55d98be..4738527 100644
--- a/polygerrit-ui/app/utils/patch-set-util.ts
+++ b/polygerrit-ui/app/utils/patch-set-util.ts
@@ -64,6 +64,14 @@
return `${n}`[0] === '-';
}
+/**
+ * Whether the given patch is a parent, either a regular parent or a merge
+ * parent.
+ */
+export function isAParent(n: PatchSetNum) {
+ return n === ParentPatchSetNum || isMergeParent(n);
+}
+
export function isPatchSetNum(patchset: string) {
if (!isNaN(Number(patchset))) return true;
return patchset === EditPatchSetNum || patchset === ParentPatchSetNum;
