opendev/gerrit
Code Issues Proposed changes

Move thread group creation out of diff builder

Browse Source 
Because, formerly, comment threads were instantiated by low-level diff
builder instances, they needed to be parameterized with information that
was only relevant to the thread groups (namely the project name and the
parent index). This resulted in tight coupling between gr-diff-builder
and both gr-diff-comment-thread-group and gr-rest-api-interface.

To decouple the builder from these two implementations, thread creation
is relocated to the gr-diff level and a create function is passed down
into the builder.

Change-Id: I8c39518088af97a49a3c4f24bd32ae073ce24b76
This commit is contained in:
Wyatt Allen
2018-07-11 15:08:58 -07:00
parent ce8fa68e2f
commit 614bec662b
9 changed files with 81 additions and 74 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -20,8 +20,8 @@
// Prevent redefinition.
if (window.GrDiffBuilderBinary) { return; }
function GrDiffBuilderBinary(diff, comments, prefs, projectName, outputEl) {
GrDiffBuilder.call(this, diff, comments, prefs, projectName, outputEl);
function GrDiffBuilderBinary(diff, comments, prefs, outputEl) {
GrDiffBuilder.call(this, diff, comments, null, prefs, outputEl);
}
GrDiffBuilderBinary.prototype = Object.create(GrDiffBuilder.prototype);

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

@@ -22,10 +22,10 @@
const IMAGE_MIME_PATTERN = /^image\/(bmp|gif|jpeg|jpg|png|tiff|webp)$/;
function GrDiffBuilderImage(
diff, comments, prefs, projectName, outputEl, baseImage, revisionImage) {
GrDiffBuilderSideBySide.call(
this, diff, comments, prefs, projectName, outputEl, []);
function GrDiffBuilderImage(diff, comments, createThreadGroupFn, prefs,
outputEl, baseImage, revisionImage) {
GrDiffBuilderSideBySide.call(this, diff, comments, createThreadGroupFn,
prefs, outputEl, []);
this._baseImage = baseImage;
this._revisionImage = revisionImage;
}

8
polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-side-by-side.js
View File

@@ -20,10 +20,10 @@
// Prevent redefinition.
if (window.GrDiffBuilderSideBySide) { return; }
function GrDiffBuilderSideBySide(
diff, comments, prefs, projectName, outputEl, layers) {
GrDiffBuilder.call(
this, diff, comments, prefs, projectName, outputEl, layers);
function GrDiffBuilderSideBySide(diff, comments, createThreadGroupFn, prefs,
outputEl, layers) {
GrDiffBuilder.call(this, diff, comments, createThreadGroupFn, prefs,
outputEl, layers);
}
GrDiffBuilderSideBySide.prototype = Object.create(GrDiffBuilder.prototype);
GrDiffBuilderSideBySide.prototype.constructor = GrDiffBuilderSideBySide;

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

@@ -20,10 +20,10 @@
// Prevent redefinition.
if (window.GrDiffBuilderUnified) { return; }
function GrDiffBuilderUnified(
diff, comments, prefs, projectName, outputEl, layers) {
GrDiffBuilder.call(
this, diff, comments, prefs, projectName, outputEl, layers);
function GrDiffBuilderUnified(diff, comments, createThreadGroupFn, prefs,
outputEl, layers) {
GrDiffBuilder.call(this, diff, comments, createThreadGroupFn, prefs,
outputEl, layers);
}
GrDiffBuilderUnified.prototype = Object.create(GrDiffBuilder.prototype);
GrDiffBuilderUnified.prototype.constructor = GrDiffBuilderUnified;

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

@@ -116,6 +116,12 @@ limitations under the License.
* @type {Defs.LineOfInterest|null}
*/
lineOfInterest: Object,
/**
* @type {function(number, booleam, !string)}
*/
createCommentFn: Function,
_builder: Object,
_groups: Array,
_layers: Array,
@@ -250,12 +256,6 @@ limitations under the License.
GrDiffBuilder.Side.RIGHT : GrDiffBuilder.Side.LEFT;
},
createCommentThreadGroup(changeNum, patchNum, path,
isOnParent, commentSide) {
return this._builder.createCommentThreadGroup(changeNum, patchNum,
path, isOnParent, commentSide);
},
emitGroup(group, sectionEl) {
this._builder.emitGroup(group, sectionEl);
},
@@ -303,27 +303,24 @@ limitations under the License.
}
let builder = null;
const createFn = this.createCommentFn;
if (this.isImageDiff) {
builder = new GrDiffBuilderImage(diff, comments, prefs,
this.projectName, this.diffElement, this.baseImage,
this.revisionImage);
builder = new GrDiffBuilderImage(diff, comments, createFn, prefs,
this.diffElement, this.baseImage, this.revisionImage);
} else if (diff.binary) {
// If the diff is binary, but not an image.
return new GrDiffBuilderBinary(diff, comments, prefs,
this.projectName, this.diffElement);
this.diffElement);
} else if (this.viewMode === DiffViewMode.SIDE_BY_SIDE) {
builder = new GrDiffBuilderSideBySide(diff, comments, prefs,
this.projectName, this.diffElement, this._layers);
builder = new GrDiffBuilderSideBySide(diff, comments, createFn,
prefs, this.diffElement, this._layers);
} else if (this.viewMode === DiffViewMode.UNIFIED) {
builder = new GrDiffBuilderUnified(diff, comments, prefs,
this.projectName, this.diffElement, this._layers);
builder = new GrDiffBuilderUnified(diff, comments, createFn, prefs,
this.diffElement, this._layers);
}
if (!builder) {
throw Error('Unsupported diff view mode: ' + this.viewMode);
}
if (this.parentIndex) {
builder.setParentIndex(this.parentIndex);
}
return builder;
},

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

@@ -42,15 +42,15 @@
*/
const REGEX_TAB_OR_SURROGATE_PAIR = /\t|[\uD800-\uDBFF][\uDC00-\uDFFF]/;
function GrDiffBuilder(diff, comments, prefs, projectName, outputEl, layers) {
function GrDiffBuilder(diff, comments, createThreadGroupFn, prefs, outputEl,
layers) {
this._diff = diff;
this._comments = comments;
this._createThreadGroupFn = createThreadGroupFn;
this._prefs = prefs;
this._projectName = projectName;
this._outputEl = outputEl;
this.groups = [];
this._blameInfo = null;
this._parentIndex = undefined;
this.layers = layers || [];
@@ -62,7 +62,6 @@
throw Error('Invalid line length from preferences.');
}
for (const layer of this.layers) {
if (layer.addListener) {
layer.addListener(this._handleLayerUpdate.bind(this));
@@ -353,28 +352,6 @@
return result;
};
/**
* @param {number} changeNum
* @param {number|string} patchNum
* @param {string} path
* @param {boolean} isOnParent
* @param {string} commentSide
* @return {!Object}
*/
GrDiffBuilder.prototype.createCommentThreadGroup = function(changeNum,
patchNum, path, isOnParent, commentSide) {
const threadGroupEl =
document.createElement('gr-diff-comment-thread-group');
threadGroupEl.changeNum = changeNum;
threadGroupEl.commentSide = commentSide;
threadGroupEl.patchForNewThreads = patchNum;
threadGroupEl.path = path;
threadGroupEl.isOnParent = isOnParent;
threadGroupEl.projectName = this._projectName;
threadGroupEl.parentIndex = this._parentIndex;
return threadGroupEl;
};
/**
* @param {number} line
* @param {string=} opt_side
@@ -400,9 +377,8 @@
patchNum = this._comments.meta.patchRange.basePatchNum;
}
}
const threadGroupEl = this.createCommentThreadGroup(
this._comments.meta.changeNum, patchNum, this._comments.meta.path,
isOnParent, opt_side);
const threadGroupEl = this._createThreadGroupFn(patchNum, isOnParent,
opt_side);
threadGroupEl.comments = comments;
if (opt_side) {
threadGroupEl.setAttribute('data-side', opt_side);
@@ -616,10 +592,6 @@
}
};
GrDiffBuilder.prototype.setParentIndex = function(index) {
this._parentIndex = index;
};
/**
* Find the blame cell for a given line number.
* @param {number} lineNum

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

@@ -59,6 +59,7 @@ limitations under the License.
suite('gr-diff-builder tests', () => {
let element;
let builder;
let createThreadGroupFn;
let sandbox;
const LINE_FEED_HTML = '<span class="style-scope gr-diff br"></span>';
@@ -74,9 +75,11 @@ limitations under the License.
show_tabs: true,
tab_size: 4,
};
const projectName = 'my-project';
createThreadGroupFn = sinon.spy(() => ({
setAttribute: sinon.spy(),
}));
builder = new GrDiffBuilder(
{content: []}, {left: [], right: []}, prefs, projectName);
{content: []}, {left: [], right: []}, createThreadGroupFn, prefs);
});
teardown(() => { sandbox.restore(); });
@@ -311,11 +314,8 @@ limitations under the License.
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.isOnParent, isOnParent);
assert.deepEqual(threadGroupEl.projectName, 'my-project');
assert.equal(createThreadGroupFn.lastCall.args[0], patchNum);
assert.equal(createThreadGroupFn.lastCall.args[1], isOnParent);
assert.deepEqual(threadGroupEl.comments, comments);
}
@@ -323,27 +323,33 @@ limitations under the License.
line.beforeNumber = 5;
line.afterNumber = 5;
let threadGroupEl = builder._commentThreadGroupForLine(line);
assert.isTrue(createThreadGroupFn.calledOnce);
checkThreadGroupProps(threadGroupEl, '3', false, [l5, r5]);
threadGroupEl =
builder._commentThreadGroupForLine(line, GrDiffBuilder.Side.RIGHT);
assert.isTrue(createThreadGroupFn.calledTwice);
checkThreadGroupProps(threadGroupEl, '3', false, [r5]);
threadGroupEl =
builder._commentThreadGroupForLine(line, GrDiffBuilder.Side.LEFT);
assert.isTrue(createThreadGroupFn.calledThrice);
checkThreadGroupProps(threadGroupEl, '3', true, [l5]);
builder._comments.meta.patchRange.basePatchNum = '1';
threadGroupEl = builder._commentThreadGroupForLine(line);
assert.equal(createThreadGroupFn.callCount, 4);
checkThreadGroupProps(threadGroupEl, '3', false, [l5, r5]);
threadEl =
builder._commentThreadGroupForLine(line, GrDiffBuilder.Side.LEFT);
assert.equal(createThreadGroupFn.callCount, 5);
checkThreadGroupProps(threadEl, '1', false, [l5]);
threadGroupEl =
builder._commentThreadGroupForLine(line, GrDiffBuilder.Side.RIGHT);
assert.equal(createThreadGroupFn.callCount, 6);
checkThreadGroupProps(threadGroupEl, '3', false, [r5]);
builder._comments.meta.patchRange.basePatchNum = 'PARENT';
@@ -352,12 +358,14 @@ limitations under the License.
line.beforeNumber = 5;
line.afterNumber = 5;
threadGroupEl = builder._commentThreadGroupForLine(line);
assert.equal(createThreadGroupFn.callCount, 7);
checkThreadGroupProps(threadGroupEl, '3', true, [l5, r5]);
line = new GrDiffLine(GrDiffLine.Type.ADD);
line.beforeNumber = 3;
line.afterNumber = 5;
threadGroupEl = builder._commentThreadGroupForLine(line);
assert.equal(createThreadGroupFn.callCount, 8);
checkThreadGroupProps(threadGroupEl, '3', false, [l3, r5]);
});
@@ -920,7 +928,7 @@ limitations under the License.
outputEl = element.queryEffectiveChildren('#diffTable');
sandbox.stub(element, '_getDiffBuilder', () => {
const builder = new GrDiffBuilder(
{content}, {left: [], right: []}, prefs, 'my-project', outputEl);
{content}, {left: [], right: []}, null, prefs, outputEl);
sandbox.stub(builder, 'addColumns');
builder.buildSectionElement = function(group) {
const section = document.createElement('stub');

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

@@ -286,6 +286,7 @@ limitations under the License.
base-image="[[_baseImage]]"
revision-image="[[_revisionImage]]"
parent-index="[[_parentIndex]]"
create-comment-fn="[[_createThreadGroupFn]]"
line-of-interest="[[lineOfInterest]]">
<table
id="diffTable"

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

@@ -182,6 +182,16 @@
type: String,
computed: '_computeNewlineWarning(_diff)',
},
/**
* @type {function(number, boolean, !string)}
*/
_createThreadGroupFn: {
type: Function,
value() {
return this._createCommentThreadGroup.bind(this);
},
},
},
behaviors: [
@@ -481,8 +491,8 @@
// Check if thread group exists.
let threadGroupEl = this._getThreadGroupForLine(contentEl);
if (!threadGroupEl) {
threadGroupEl = this.$.diffBuilder.createCommentThreadGroup(
this.changeNum, patchNum, this.path, isOnParent, commentSide);
threadGroupEl = this._createCommentThreadGroup(patchNum, isOnParent,
commentSide);
contentEl.appendChild(threadGroupEl);
}
@@ -496,6 +506,25 @@
return threadEl;
},
/**
* @param {number} patchNum
* @param {boolean} isOnParent
* @param {!string} commentSide
* @return {!Object}
*/
_createCommentThreadGroup(patchNum, isOnParent, commentSide) {
const threadGroupEl =
document.createElement('gr-diff-comment-thread-group');
threadGroupEl.changeNum = this.changeNum;
threadGroupEl.commentSide = commentSide;
threadGroupEl.patchForNewThreads = patchNum;
threadGroupEl.path = this.path;
threadGroupEl.isOnParent = isOnParent;
threadGroupEl.projectName = this.projectName;
threadGroupEl.parentIndex = this._parentIndex;
return threadGroupEl;
},
/**
* The value to be used for the patch number of new comments created at the
* given line and content elements.