opendev/gerrit
Code Issues Proposed changes

Merge "Reduce confusion between side and commentSide"

Browse Source
This commit is contained in:
Wyatt Allen 2017-03-16 00:17:46 +00:00 committed by Gerrit Code Review
commit 81ef082667
14 changed files with 61 additions and 44 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
polygerrit-ui/app/elements/change/gr-comment-list/gr-comment-list.js
View File

@ -38,7 +38,7 @@
var diffURL = this._computeFileDiffURL(file, changeNum, patchNum);
if (comment.line) {
diffURL += '#';
if (comment.side === 'PARENT') { diffURL += 'b'; }
if (comment.__isOnParent) { diffURL += 'b'; }
diffURL += comment.line;
}
return diffURL;
@ -51,7 +51,7 @@
},
_computePatchDisplayName: function(comment) {
if (comment.side == 'PARENT') {
if (comment.__isOnParent) {
return 'Base, ';
}
if (comment.patch_set != this.patchNum) {

4
polygerrit-ui/app/elements/change/gr-comment-list/gr-comment-list_test.html
View File

@ -71,7 +71,7 @@ limitations under the License.
assert.equal(actual, expected);
comment.line = 321;
comment.side = 'PARENT';
comment.__isOnParent = true;
expected = '/c/<change>/<patch>/<file>#b321';
actual = element._computeDiffLineURL('<file>', '<change>', '<patch>',
@ -87,7 +87,7 @@ limitations under the License.
element.patchNum = 9;
assert.equal(element._computePatchDisplayName(comment), 'PS10, ');
comment.side = 'PARENT';
comment.__isOnParent = true;
assert.equal(element._computePatchDisplayName(comment), 'Base, ');
});
});

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

@ -216,10 +216,10 @@ limitations under the License.
GrDiffBuilder.Side.RIGHT : GrDiffBuilder.Side.LEFT;
},
createCommentThreadGroup: function(changeNum, patchNum, path, side,
commentSide, projectConfig) {
createCommentThreadGroup: function(changeNum, patchNum, path,
isOnParent, commentSide, projectConfig) {
return this._builder.createCommentThreadGroup(changeNum, patchNum,
path, side, commentSide, projectConfig);
path, isOnParent, commentSide, projectConfig);
},
emitGroup: function(group, sectionEl) {

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

@ -336,13 +336,13 @@
};
GrDiffBuilder.prototype.createCommentThreadGroup = function(changeNum,
patchNum, path, side, projectConfig, range) {
patchNum, path, isOnParent, projectConfig, range) {
var threadGroupEl =
document.createElement('gr-diff-comment-thread-group');
threadGroupEl.changeNum = changeNum;
threadGroupEl.patchForNewThreads = patchNum;
threadGroupEl.path = path;
threadGroupEl.side = side;
threadGroupEl.isOnParent = isOnParent;
threadGroupEl.projectConfig = projectConfig;
threadGroupEl.range = range;
return threadGroupEl;
@ -356,11 +356,11 @@
}
var patchNum = this._comments.meta.patchRange.patchNum;
var side = comments[0].side || 'REVISION';
var isOnParent = comments[0].__isOnParent || false ;
if (line.type === GrDiffLine.Type.REMOVE ||
opt_side === GrDiffBuilder.Side.LEFT) {
if (this._comments.meta.patchRange.basePatchNum === 'PARENT') {
side = 'PARENT';
isOnParent = true;
} else {
patchNum = this._comments.meta.patchRange.basePatchNum;
}
@ -369,7 +369,7 @@
this._comments.meta.changeNum,
patchNum,
this._comments.meta.path,
side,
isOnParent,
this._comments.meta.projectConfig);
threadGroupEl.comments = comments;
if (opt_side) {

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

@ -255,11 +255,12 @@ limitations under the License.
right: [r5],
};
function checkThreadGroupProps(threadGroupEl, patchNum, side, comments) {
function checkThreadGroupProps(threadGroupEl, patchNum, isOnParent,
comments) {
assert.equal(threadGroupEl.changeNum, '42');
assert.equal(threadGroupEl.patchForNewThreads, patchNum);
assert.equal(threadGroupEl.path, '/path/to/foo');
assert.equal(threadGroupEl.side, side);
assert.equal(threadGroupEl.isOnParent, isOnParent);
assert.deepEqual(threadGroupEl.projectConfig, {foo: 'bar'});
assert.deepEqual(threadGroupEl.comments, comments);
}
@ -268,28 +269,28 @@ limitations under the License.
line.beforeNumber = 5;
line.afterNumber = 5;
var threadGroupEl = builder._commentThreadGroupForLine(line);
checkThreadGroupProps(threadGroupEl, '3', 'REVISION', [l5, r5]);
checkThreadGroupProps(threadGroupEl, '3', false, [l5, r5]);
threadGroupEl =
builder._commentThreadGroupForLine(line, GrDiffBuilder.Side.RIGHT);
checkThreadGroupProps(threadGroupEl, '3', 'REVISION', [r5]);
checkThreadGroupProps(threadGroupEl, '3', false, [r5]);
threadGroupEl =
builder._commentThreadGroupForLine(line, GrDiffBuilder.Side.LEFT);
checkThreadGroupProps(threadGroupEl, '3', 'PARENT', [l5]);
checkThreadGroupProps(threadGroupEl, '3', true, [l5]);
builder._comments.meta.patchRange.basePatchNum = '1';
threadGroupEl = builder._commentThreadGroupForLine(line);
checkThreadGroupProps(threadGroupEl, '3', 'REVISION', [l5, r5]);
checkThreadGroupProps(threadGroupEl, '3', false, [l5, r5]);
threadEl =
builder._commentThreadGroupForLine(line, GrDiffBuilder.Side.LEFT);
checkThreadGroupProps(threadEl, '1', 'REVISION', [l5]);
checkThreadGroupProps(threadEl, '1', false, [l5]);
threadGroupEl =
builder._commentThreadGroupForLine(line, GrDiffBuilder.Side.RIGHT);
checkThreadGroupProps(threadGroupEl, '3', 'REVISION', [r5]);
checkThreadGroupProps(threadGroupEl, '3', false, [r5]);
builder._comments.meta.patchRange.basePatchNum = 'PARENT';
@ -297,13 +298,13 @@ limitations under the License.
line.beforeNumber = 5;
line.afterNumber = 5;
threadGroupEl = builder._commentThreadGroupForLine(line);
checkThreadGroupProps(threadGroupEl, '3', 'PARENT', [l5, r5]);
checkThreadGroupProps(threadGroupEl, '3', true, [l5, r5]);
line = new GrDiffLine(GrDiffLine.Type.ADD);
line.beforeNumber = 3;
line.afterNumber = 5;
threadGroupEl = builder._commentThreadGroupForLine(line);
checkThreadGroupProps(threadGroupEl, '3', 'REVISION', [l3, r5]);
checkThreadGroupProps(threadGroupEl, '3', false, [l3, r5]);
});
suite('_isTotal', function() {

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

@ -33,11 +33,11 @@ limitations under the License.
<gr-diff-comment-thread
comments="[[thread.comments]]"
comment-side="[[thread.commentSide]]"
is-on-parent="[[isOnParent]]"
change-num="[[changeNum]]"
location-range="[[thread.locationRange]]"
patch-num="[[thread.patchNum]]"
path="[[path]]"
side="[[side]]"
project-config="[[projectConfig]]"></gr-diff-comment-thread>
</template>
</template>

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

@ -26,9 +26,9 @@
patchForNewThreads: String,
projectConfig: Object,
range: Object,
side: {
type: String,
value: 'REVISION',
isOnParent: {
type: Boolean,
value: false,
},
_threads: {
type: Array,

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

@ -59,7 +59,7 @@ limitations under the License.
draft="[[comment.__draft]]"
show-actions="[[_showActions]]"
comment-side="[[comment.__commentSide]]"
side="[[side]]"
is-on-parent="[[isOnParent]]"
project-config="[[projectConfig]]"
on-create-fix-comment="_handleCommentFix"
on-comment-discard="_handleCommentDiscard"></gr-diff-comment>

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

@ -41,9 +41,9 @@
patchNum: String,
path: String,
projectConfig: Object,
side: {
type: String,
value: 'REVISION',
isOnParent: {
type: Boolean,
value: false,
},
_showActions: Boolean,
@ -272,7 +272,7 @@
__date: new Date(),
path: this.path,
patchNum: this.patchNum,
side: this.side,
__isOnParent: this.__isOnParent,
__commentSide: this.commentSide,
};
if (opt_lineNum) {

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

@ -139,6 +139,10 @@
save: function() {
this.comment.message = this._messageText;
// Translate {Boolean} __isOnParent to {String} side for the REST API
// format.
this.comment.side = this.comment.__isOnParent ? 'PARENT' : null;
this.disabled = true;
this._eraseDraftComment();
@ -149,6 +153,7 @@
return this.$.restAPI.getResponseObject(response).then(function(obj) {
var comment = obj;
comment.__isOnParent = comment.side === 'PARENT';
comment.__draft = true;
// Maintain the ephemeral draft ID for identification by other
// elements.
@ -402,16 +407,18 @@
},
_saveDraft: function(draft) {
draft.side = draft.__isOnParent ? 'PARENT' : null;
return this.$.restAPI.saveDiffDraft(this.changeNum, this.patchNum, draft);
},
_deleteDraft: function(draft) {
draft.side = draft.__isOnParent ? 'PARENT' : null;
return this.$.restAPI.deleteDiffDraft(this.changeNum, this.patchNum,
draft);
},
_getPatchNum: function() {
return this.side === 'PARENT' ? 'PARENT' : this.patchNum;
return this.isOnParent ? 'PARENT' : this.patchNum;
},
_loadLocalDraft: function(changeNum, patchNum, comment) {

5
polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html
View File

@ -153,10 +153,10 @@ limitations under the License.
});
test('_getPatchNum', function() {
element.side = 'PARENT';
element.isOnParent = true;
element.patchNum = 1;
assert.equal(element._getPatchNum(), 'PARENT');
element.side = 'REVISION';
element.isOnParent = false;
assert.equal(element._getPatchNum(), 1);
});
@ -508,6 +508,7 @@ limitations under the License.
assert.deepEqual(fireStub.lastCall.args[1], {
comment: {
__commentSide: 'right',
__isOnParent: false,
__draft: true,
__draftID: 'temp_draft_id',
__editing: false,

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

@ -229,9 +229,10 @@
var contentText = this.$.diffBuilder.getContentByLineEl(lineEl);
var contentEl = contentText.parentElement;
var patchNum = this._getPatchNumByLineAndContent(lineEl, contentEl);
var side = this._getSideByLineAndContent(lineEl, contentEl);
var isOnParent =
this._getIsParentCommentByLineAndContent(lineEl, contentEl);
var threadEl = this._getOrCreateThreadAtLineRange(contentEl, patchNum,
diffSide, side, range);
diffSide, isOnParent, range);
threadEl.addOrEditDraft(line, range);
},
@ -241,9 +242,10 @@
var contentEl = contentText.parentElement;
var patchNum = this._getPatchNumByLineAndContent(lineEl, contentEl);
var commentSide = this._getCommentSideByLineAndContent(lineEl, contentEl);
var side = this._getSideByLineAndContent(lineEl, contentEl);
var isOnParent =
this._getIsParentCommentByLineAndContent(lineEl, contentEl);
var threadEl = this._getOrCreateThreadAtLineRange(contentEl, patchNum,
commentSide, side);
commentSide, isOnParent);
threadEl.addOrEditDraft(opt_lineNum);
},
@ -257,7 +259,7 @@
},
_getOrCreateThreadAtLineRange:
function(contentEl, patchNum, commentSide, side, range) {
function(contentEl, patchNum, commentSide, isOnParent, range) {
var rangeToCheck = range ?
'range-' +
range.startLine + '-' +
@ -270,7 +272,7 @@
var threadGroupEl = this._getThreadGroupForLine(contentEl);
if (!threadGroupEl) {
threadGroupEl = this.$.diffBuilder.createCommentThreadGroup(
this.changeNum, patchNum, this.path, side,
this.changeNum, patchNum, this.path, isOnParent,
this.projectConfig);
contentEl.appendChild(threadGroupEl);
}
@ -296,14 +298,14 @@
return patchNum;
},
_getSideByLineAndContent: function(lineEl, contentEl) {
var side = 'REVISION';
_getIsParentCommentByLineAndContent: function(lineEl, contentEl) {
var isOnParent = false;
if ((lineEl.classList.contains(DiffSide.LEFT) ||
contentEl.classList.contains('remove')) &&
this.patchRange.basePatchNum === 'PARENT') {
side = 'PARENT';
isOnParent = true;
}
return side;
return isOnParent;
},
_getCommentSideByLineAndContent: function(lineEl, contentEl) {

5
polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
View File

@ -781,6 +781,7 @@
function onlyParent(c) { return c.side == PARENT_PATCH_NUM; }
function withoutParent(c) { return c.side != PARENT_PATCH_NUM; }
function setPath(c) { c.path = opt_path; }
function setIsOnParent(c) { c.__isOnParent = true; }
var promises = [];
var comments;
@ -800,6 +801,10 @@
if (opt_basePatchNum == PARENT_PATCH_NUM) {
baseComments = comments.filter(onlyParent);
baseComments.forEach(setPath);
// Translate {String} side to {Boolean} __isOnParent for readability
// in the code.
baseComments.forEach(setIsOnParent);
}
comments = comments.filter(withoutParent);

1
polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.html
View File

@ -150,6 +150,7 @@ limitations under the License.
assert.equal(obj.baseComments.length, 1);
assert.deepEqual(obj.baseComments[0], {
side: 'PARENT',
__isOnParent: true,
message: 'how did this work in the first place?',
path: 'sieve.go',
updated: '2017-02-03 22:33:28.000000000',