opendev/gerrit
Code Issues Proposed changes

Support creating comments on merge parents

Browse Source 
Bug: Issue 4760
Change-Id: I66ced578819b6f48d7d1535a54f1debf0e35374e
This commit is contained in:
Wyatt Allen
2017-11-17 10:57:46 -08:00
parent a744e2303f
commit c56ece73e2
8 changed files with 157 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files

16
polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html
View File

@@ -95,6 +95,7 @@ limitations under the License.
baseImage: Object,
revisionImage: Object,
projectName: String,
parentIndex: Number,
_builder: Object,
_groups: Array,
_layers: Array,
@@ -251,18 +252,25 @@ limitations under the License.
},
_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() {

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

@@ -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

1
polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.html
View File

@@ -34,6 +34,7 @@ limitations under the License.
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]]"

4
polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.js
View File

@@ -30,6 +30,10 @@
type: Boolean,
value: false,
},
parentIndex: {
type: Number,
value: null,
},
_threads: {
type: Array,
value() { return []; },

8
polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js
View File

@@ -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;
},

3
polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
View File

@@ -270,7 +270,8 @@ limitations under the License.
line-wrapping="[[lineWrapping]]"
is-image-diff="[[isImageDiff]]"
base-image="[[_baseImage]]"
revision-image="[[_revisionImage]]">
revision-image="[[_revisionImage]]"
parent-index="[[_parentIndex]]">
<table
id="diffTable"
class$="[[_diffTableClass]]"

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

@@ -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);
},
});
})();

86
polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
View File

@@ -64,6 +64,92 @@ limitations under the License.
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', {