Support creating comments on merge parents
Bug: Issue 4760
Change-Id: I66ced578819b6f48d7d1535a54f1debf0e35374e
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html
index 1cfab26..a7f1db0 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html
@@ -95,6 +95,7 @@
baseImage: Object,
revisionImage: Object,
projectName: String,
+ parentIndex: Number,
_builder: Object,
_groups: Array,
_layers: Array,
@@ -251,18 +252,25 @@
},
_getDiffBuilder(diff, comments, prefs) {
+ let builder = null;
if (this.isImageDiff) {
- return new GrDiffBuilderImage(diff, comments, prefs,
+ builder = new GrDiffBuilderImage(diff, comments, prefs,
this.projectName, this.diffElement, this.baseImage,
this.revisionImage);
} else if (this.viewMode === DiffViewMode.SIDE_BY_SIDE) {
- return new GrDiffBuilderSideBySide(diff, comments, prefs,
+ builder = new GrDiffBuilderSideBySide(diff, comments, prefs,
this.projectName, this.diffElement, this._layers);
} else if (this.viewMode === DiffViewMode.UNIFIED) {
- return new GrDiffBuilderUnified(diff, comments, prefs,
+ builder = new GrDiffBuilderUnified(diff, comments, prefs,
this.projectName, this.diffElement, this._layers);
}
- throw Error('Unsupported diff view mode: ' + this.viewMode);
+ if (!builder) {
+ throw Error('Unsupported diff view mode: ' + this.viewMode);
+ }
+ if (this.parentIndex) {
+ builder.setParentIndex(this.parentIndex);
+ }
+ return builder;
},
_clearDiffContent() {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js
index 9434487..6ffbe8a 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js
@@ -47,6 +47,7 @@
this._outputEl = outputEl;
this.groups = [];
this._blameInfo = null;
+ this._parentIndex = undefined;
this.layers = layers || [];
@@ -353,6 +354,7 @@
threadGroupEl.isOnParent = isOnParent;
threadGroupEl.projectName = this._projectName;
threadGroupEl.range = range;
+ threadGroupEl.parentIndex = this._parentIndex;
return threadGroupEl;
};
@@ -368,7 +370,9 @@
let isOnParent = comments[0].side === 'PARENT' || false;
if (line.type === GrDiffLine.Type.REMOVE ||
opt_side === GrDiffBuilder.Side.LEFT) {
- if (this._comments.meta.patchRange.basePatchNum === 'PARENT') {
+ if (this._comments.meta.patchRange.basePatchNum === 'PARENT' ||
+ Gerrit.PatchSetBehavior.isMergeParent(
+ this._comments.meta.patchRange.basePatchNum)) {
isOnParent = true;
} else {
patchNum = this._comments.meta.patchRange.basePatchNum;
@@ -586,6 +590,10 @@
}
};
+ GrDiffBuilder.prototype.setParentIndex = function(index) {
+ this._parentIndex = index;
+ };
+
/**
* Find the blame cell for a given line number.
* @param {number} lineNum
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.html b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.html
index fdb7b6a..bf94bb6 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.html
@@ -34,6 +34,7 @@
comments="[[thread.comments]]"
comment-side="[[thread.commentSide]]"
is-on-parent="[[isOnParent]]"
+ parent-index="[[parentIndex]]"
change-num="[[changeNum]]"
location-range="[[thread.locationRange]]"
patch-num="[[thread.patchNum]]"
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.js b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.js
index b6af0d8..1cd7c89 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.js
@@ -30,6 +30,10 @@
type: Boolean,
value: false,
},
+ parentIndex: {
+ type: Number,
+ value: null,
+ },
_threads: {
type: Array,
value() { return []; },
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js
index e9ab3d6..11d0403 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js
@@ -48,7 +48,10 @@
type: Boolean,
value: false,
},
-
+ parentIndex: {
+ type: Number,
+ value: null,
+ },
_showActions: Boolean,
_lastComment: Object,
_orderedComments: Array,
@@ -301,6 +304,9 @@
end_character: opt_range.endChar,
};
}
+ if (this.parentIndex) {
+ d.parent = this.parentIndex;
+ }
return d;
},
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
index f32234a..d0c0b84 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
@@ -270,7 +270,8 @@
line-wrapping="[[lineWrapping]]"
is-image-diff="[[isImageDiff]]"
base-image="[[_baseImage]]"
- revision-image="[[_revisionImage]]">
+ revision-image="[[_revisionImage]]"
+ parent-index="[[_parentIndex]]">
<table
id="diffTable"
class$="[[_diffTableClass]]"
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 c3add28..33e3642 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
@@ -137,6 +137,11 @@
notify: true,
computed: '_computeIsBlameLoaded(_blame)',
},
+
+ _parentIndex: {
+ type: Number,
+ computed: '_computeParentIndex(patchRange.*)',
+ },
},
behaviors: [
@@ -415,12 +420,27 @@
return threadEl;
},
- /** @return {number} */
+ /**
+ * 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.
+ *
+ * @return {number}
+ */
_getPatchNumByLineAndContent(lineEl, contentEl) {
let patchNum = this.patchRange.patchNum;
+
if ((lineEl.classList.contains(DiffSide.LEFT) ||
contentEl.classList.contains('remove')) &&
- this.patchRange.basePatchNum !== 'PARENT') {
+ this.patchRange.basePatchNum !== 'PARENT' &&
+ !this.isMergeParent(this.patchRange.basePatchNum)) {
patchNum = this.patchRange.basePatchNum;
}
return patchNum;
@@ -428,13 +448,13 @@
/** @return {boolean} */
_getIsParentCommentByLineAndContent(lineEl, contentEl) {
- let isOnParent = false;
if ((lineEl.classList.contains(DiffSide.LEFT) ||
contentEl.classList.contains('remove')) &&
- this.patchRange.basePatchNum === 'PARENT') {
- isOnParent = true;
+ (this.patchRange.basePatchNum === 'PARENT' ||
+ this.isMergeParent(this.patchRange.basePatchNum))) {
+ return true;
}
- return isOnParent;
+ return false;
},
/** @return {string} */
@@ -717,5 +737,15 @@
_computeWarningClass(showWarning) {
return showWarning ? 'warn' : '';
},
+
+ /**
+ * @return {number|null}
+ */
+ _computeParentIndex(patchRangeRecord) {
+ if (!this.isMergeParent(patchRangeRecord.base.basePatchNum)) {
+ return null;
+ }
+ return this.getParentIndex(patchRangeRecord.base.basePatchNum);
+ },
});
})();
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
index e422354..ea872a0 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
@@ -64,6 +64,92 @@
assert.equal(element._diffLength(mock.diffResponse), 52);
});
+
+ suite('_get{PatchNum|IsParentComment}ByLineAndContent', () => {
+ let lineEl;
+ let contentEl;
+
+ setup(() => {
+ element = fixture('basic');
+ lineEl = document.createElement('td');
+ 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);
+ });
+ });
+
+ 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.isTrue(
+ element._getIsParentCommentByLineAndContent(lineEl, contentEl));
+ });
+
+ 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.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));
+ });
+ });
+ });
+
suite('not logged in', () => {
setup(() => {
stub('gr-rest-api-interface', {
