Determine CommentSide in 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.
Change-Id: Iacfc4b6eb798f2a94c7f0d846f52fb704508b174
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 7610e5e..799baec 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
@@ -709,7 +709,8 @@
}
_handleCreateComment(e: CustomEvent) {
- const {lineNum, side, patchNum, range, path, commentSide} = e.detail;
+ const {lineNum, side, patchNum, range, path} = e.detail;
+ const commentSide = this._sideToCommentSide(side);
const threadEl = this._getOrCreateThread(
patchNum,
lineNum,
@@ -723,6 +724,15 @@
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 20063f4..25a4eff 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
@@ -21,7 +21,7 @@
import {GerritNav} from '../../core/gr-navigation/gr-navigation.js';
import {dom} from '@polymer/polymer/lib/legacy/polymer.dom.js';
import {createCommentThreads} from '../../../utils/comment-util.js';
-import {Side, CommentSide} from '../../../constants/constants.js';
+import {Side} from '../../../constants/constants.js';
import {createChange} from '../../../test/test-data-generators.js';
import {CoverageType} from '../../../types/types.js';
import {addListenerForTest, stubRestApi} from '../../../test/test-utils.js';
@@ -958,14 +958,16 @@
suite('create-comment', () => {
test('creates comments if they do not exist yet', () => {
const diffSide = Side.LEFT;
- const commentSide = CommentSide.PARENT;
+ element.patchRange = {
+ basePatchNum: 'PARENT',
+ patchNum: 2,
+ };
element.dispatchEvent(new CustomEvent('create-comment', {
detail: {
- patchNum: '2',
+ patchNum: 2,
lineNum: 3,
side: diffSide,
- commentSide,
path: '/p',
},
}));
@@ -975,6 +977,7 @@
assert.equal(threads.length, 1);
assert.equal(threads[0].diffSide, diffSide);
+ assert.isTrue(threads[0].isOnParent);
assert.equal(threads[0].range, undefined);
assert.equal(threads[0].patchNum, 2);
@@ -988,10 +991,9 @@
element.dispatchEvent(new CustomEvent('create-comment', {
detail: {
- patchNum: '3',
+ patchNum: 3,
lineNum: 1,
side: diffSide,
- commentSide,
path: '/p',
range,
},
@@ -1002,21 +1004,101 @@
assert.equal(threads.length, 2);
assert.equal(threads[1].diffSide, diffSide);
+ assert.isTrue(threads[0].isOnParent);
assert.equal(threads[1].range, range);
assert.equal(threads[1].patchNum, 3);
});
+ test('should not be on parent if on the right', () => {
+ element.patchRange = {
+ basePatchNum: 2,
+ patchNum: 3,
+ };
+
+ element.dispatchEvent(new CustomEvent('create-comment', {
+ detail: {
+ patchNum: 2,
+ side: Side.RIGHT,
+ },
+ }));
+
+ const thread = dom(element.$.diff)
+ .queryDistributedElements('gr-comment-thread')[0];
+
+ assert.isFalse(thread.isOnParent);
+ });
+
+ test('should be on parent if right and base is PARENT', () => {
+ element.patchRange = {
+ basePatchNum: 'PARENT',
+ patchNum: 3,
+ };
+
+ element.dispatchEvent(new CustomEvent('create-comment', {
+ detail: {
+ patchNum: 3,
+ side: Side.LEFT,
+ },
+ }));
+
+ const thread = dom(element.$.diff)
+ .queryDistributedElements('gr-comment-thread')[0];
+
+ assert.isTrue(thread.isOnParent);
+ });
+
+ test('should be on parent if right and base negative', () => {
+ element.patchRange = {
+ basePatchNum: -2, // merge parents have negative numbers
+ patchNum: 3,
+ };
+
+ element.dispatchEvent(new CustomEvent('create-comment', {
+ detail: {
+ patchNum: 3,
+ side: Side.LEFT,
+ },
+ }));
+
+ const thread = dom(element.$.diff)
+ .queryDistributedElements('gr-comment-thread')[0];
+
+ assert.isTrue(thread.isOnParent);
+ });
+
+ test('should not be on parent otherwise', () => {
+ element.patchRange = {
+ basePatchNum: 2, // merge parents have negative numbers
+ patchNum: 3,
+ };
+
+ element.dispatchEvent(new CustomEvent('create-comment', {
+ detail: {
+ patchNum: 3,
+ side: Side.LEFT,
+ },
+ }));
+
+ const thread = dom(element.$.diff)
+ .queryDistributedElements('gr-comment-thread')[0];
+
+ assert.isFalse(thread.isOnParent);
+ });
+
test('thread should use old file path if first created ' +
'on patch set (left) before renaming', () => {
const diffSide = Side.LEFT;
+ element.patchRange = {
+ basePatchNum: 2,
+ patchNum: 3,
+ };
element.file = {basePath: 'file_renamed.txt', path: element.path};
element.dispatchEvent(new CustomEvent('create-comment', {
detail: {
- patchNum: '2',
+ patchNum: 2,
lineNum: 3,
side: diffSide,
- commentSide: CommentSide.REVISION,
path: '/p',
},
}));
@@ -1032,14 +1114,17 @@
test('thread should use new file path if first created' +
'on patch set (right) after renaming', () => {
const diffSide = Side.RIGHT;
+ element.patchRange = {
+ basePatchNum: 2,
+ patchNum: 3,
+ };
element.file = {basePath: 'file_renamed.txt', path: element.path};
element.dispatchEvent(new CustomEvent('create-comment', {
detail: {
- patchNum: '2',
+ patchNum: 2,
lineNum: 3,
side: diffSide,
- commentSide: CommentSide.REVISION,
path: '/p',
},
}));
@@ -1055,14 +1140,17 @@
test('thread should use new file path if first created' +
'on patch set (left) but is base', () => {
const diffSide = Side.LEFT;
+ element.patchRange = {
+ basePatchNum: 'PARENT',
+ patchNum: 3,
+ };
element.file = {basePath: 'file_renamed.txt', path: element.path};
element.dispatchEvent(new CustomEvent('create-comment', {
detail: {
- patchNum: '2',
+ patchNum: 2,
lineNum: 3,
side: diffSide,
- commentSide: CommentSide.PARENT,
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 359db1f..79d529d 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts
@@ -63,7 +63,7 @@
PolymerDomWrapper,
} from '../../../types/types';
import {CommentRangeLayer} from '../gr-ranged-comment-layer/gr-ranged-comment-layer';
-import {CommentSide, DiffViewMode, Side} from '../../../constants/constants';
+import {DiffViewMode, Side} from '../../../constants/constants';
import {KeyLocations} from '../gr-diff-processor/gr-diff-processor';
import {FlattenedNodesObserver} from '@polymer/polymer/lib/utils/flattened-nodes-observer';
import {PolymerDeepPropertyChange} from '@polymer/polymer/interfaces';
@@ -647,11 +647,7 @@
lineEl,
contentEl
);
- const isOnParent = this._getIsParentCommentByLineAndContent(
- lineEl,
- contentEl
- );
- const commentSide = isOnParent ? CommentSide.PARENT : CommentSide.REVISION;
+
this.dispatchEvent(
new CustomEvent('create-comment', {
bubbles: true,
@@ -659,7 +655,6 @@
detail: {
lineNum,
side,
- commentSide,
patchNum: patchForNewThreads,
range,
path: this.path,
@@ -715,25 +710,11 @@
return patchNum;
}
- _getIsParentCommentByLineAndContent(lineEl: Element, contentEl: Element) {
- if (!this.patchRange) throw Error('patch range not set');
- return (
- (lineEl.classList.contains(Side.LEFT) ||
- contentEl.classList.contains('remove')) &&
- (this.patchRange.basePatchNum === 'PARENT' ||
- isMergeParent(this.patchRange.basePatchNum))
- );
- }
-
_getCommentSideByLineAndContent(lineEl: Element, contentEl: Element): Side {
- let side = Side.RIGHT;
- if (
- lineEl.classList.contains(Side.LEFT) ||
+ return lineEl.classList.contains(Side.LEFT) ||
contentEl.classList.contains('remove')
- ) {
- side = Side.LEFT;
- }
- return side;
+ ? Side.LEFT
+ : Side.RIGHT;
}
_prefsObserver(newPrefs: DiffPreferencesInfo, oldPrefs: DiffPreferencesInfo) {
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 2cc9c10..baee7aa 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,7 +87,7 @@
assert.isNotOk(getComputedStyleValue('--line-limit', element));
});
- suite('_get{PatchNum|IsParentComment}ByLineAndContent', () => {
+ suite('_getPatchNumByLineAndContent', () => {
let lineEl;
let contentEl;
@@ -97,78 +97,39 @@
contentEl = document.createElement('span');
});
- suite('_getPatchNumByLineAndContent', () => {
- 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);
- });
+ test('right side', () => {
+ element.patchRange = {patchNum: 4, basePatchNum: 'PARENT'};
+ lineEl.classList.add('right');
+ assert.equal(element._getPatchNumByLineAndContent(lineEl, contentEl),
+ 4);
});
- suite('_getIsParentCommentByLineAndContent', () => {
- test('right side', () => {
- element.patchRange = {patchNum: 4, basePatchNum: 'PARENT'};
- lineEl.classList.add('right');
- assert.isFalse(
- element._getIsParentCommentByLineAndContent(lineEl, contentEl));
- });
+ 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 linenum', () => {
- element.patchRange = {patchNum: 4, basePatchNum: 'PARENT'};
- lineEl.classList.add('left');
- assert.isTrue(
- element._getIsParentCommentByLineAndContent(lineEl, contentEl));
- });
+ 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 parent by content', () => {
- element.patchRange = {patchNum: 4, basePatchNum: 'PARENT'};
- contentEl.classList.add('remove');
- assert.isTrue(
- element._getIsParentCommentByLineAndContent(lineEl, contentEl));
- });
+ 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 merge parent', () => {
- element.patchRange = {patchNum: 4, basePatchNum: -2};
- contentEl.classList.add('remove');
- assert.isTrue(
- element._getIsParentCommentByLineAndContent(lineEl, contentEl));
- });
-
- test('left side non parent', () => {
- element.patchRange = {patchNum: 4, basePatchNum: 3};
- contentEl.classList.add('remove');
- assert.isFalse(
- element._getIsParentCommentByLineAndContent(lineEl, contentEl));
- });
+ test('left side non parent', () => {
+ element.patchRange = {patchNum: 4, basePatchNum: 3};
+ contentEl.classList.add('remove');
+ assert.equal(element._getPatchNumByLineAndContent(lineEl, contentEl),
+ 3);
});
});
