opendev/gerrit
Code Issues Proposed changes

Add comments/drafts to gr-new-diff

Browse Source 
Change-Id: I8110b521ab83a31be5db7e3d65268186485db04d
This commit is contained in:
Andrew Bonventre
2016-03-20 14:14:20 -04:00
parent 00f0839714
commit 02f71327cf
12 changed files with 306 additions and 30 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -15,6 +15,7 @@ limitations under the License.
-->
<link rel="import" href="../../../bower_components/polymer/polymer.html">
<link rel="import" href="../../shared/gr-rest-api-interface/gr-rest-api-interface.html">
<link rel="import" href="../gr-diff-comment/gr-diff-comment.html">
<dom-module id="gr-diff-comment-thread">
@@ -41,7 +42,7 @@ limitations under the License.
change-num="[[changeNum]]"
patch-num="[[patchNum]]"
draft="[[comment.__draft]]"
show-actions="[[showActions]]"
show-actions="[[_showActions]]"
project-config="[[projectConfig]]"
on-height-change="_handleCommentHeightChange"
on-reply="_handleCommentReply"
@@ -49,6 +50,7 @@ limitations under the License.
on-done="_handleCommentDone"></gr-diff-comment>
</template>
</div>
<gr-rest-api-interface id="restAPI"></gr-rest-api-interface>
</template>
<script src="gr-diff-comment-thread.js"></script>
</dom-module>

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

@@ -37,9 +37,9 @@
},
patchNum: String,
path: String,
showActions: Boolean,
projectConfig: Object,
_showActions: Boolean,
_boundWindowResizeHandler: {
type: Function,
value: function() { return this._handleWindowResize.bind(this); }
@@ -57,6 +57,10 @@
],
attached: function() {
this._getLoggedIn().then(function(loggedIn) {
this._showActions = loggedIn;
}.bind(this));
window.addEventListener('resize', this._boundWindowResizeHandler);
},
@@ -64,6 +68,10 @@
window.removeEventListener('resize', this._boundWindowResizeHandler);
},
_getLoggedIn: function() {
return this.$.restAPI.getLoggedIn();
},
_handleWindowResize: function(e) {
this._heightChanged();
},

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

@@ -41,6 +41,9 @@ limitations under the License.
suite('gr-diff-comment-thread tests', function() {
var element;
setup(function() {
stub('gr-rest-api-interface', {
getLoggedIn: function() { return Promise.resolve(false); },
});
element = fixture('basic');
});
@@ -124,6 +127,9 @@ limitations under the License.
var server;
setup(function() {
stub('gr-rest-api-interface', {
getLoggedIn: function() { return Promise.resolve(false); },
});
element = fixture('withComment');
element.comments = [{
author: {

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

@@ -33,7 +33,6 @@
prefs: {
type: Object,
notify: true,
value: function() { return {}; },
},
disabled: {
type: Boolean,

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

@@ -411,7 +411,6 @@
threadEl.patchNum = thread.patchNum || this.patchNum;
threadEl.path = this.path;
threadEl.comments = thread.comments;
threadEl.showActions = this.canComment;
threadEl.projectConfig = this.projectConfig;
this.$.numbers.insertBefore(lineEl, beforeLineEl);

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

@@ -14,8 +14,8 @@
(function(window, GrDiffBuilder) {
'use strict';
function GrDiffBuilderSideBySide(diff, prefs, outputEl) {
GrDiffBuilder.call(this, diff, prefs, outputEl);
function GrDiffBuilderSideBySide(diff, comments, prefs, outputEl) {
GrDiffBuilder.call(this, diff, comments, prefs, outputEl);
}
GrDiffBuilderSideBySide.prototype = Object.create(GrDiffBuilder.prototype);
GrDiffBuilderSideBySide.prototype.constructor = GrDiffBuilderSideBySide;
@@ -35,23 +35,28 @@
GrDiffBuilderSideBySide.prototype._createRow = function(section, leftLine,
rightLine) {
var row = this._createElement('tr');
this._createPair(section, row, leftLine, leftLine.beforeNumber, 'left');
this._createPair(section, row, rightLine, rightLine.afterNumber, 'right');
this._appendPair(section, row, leftLine, leftLine.beforeNumber,
GrDiffBuilder.Side.LEFT);
this._appendPair(section, row, rightLine, rightLine.afterNumber,
GrDiffBuilder.Side.RIGHT);
return row;
};
GrDiffBuilderSideBySide.prototype._createPair = function(section, row, line,
GrDiffBuilderSideBySide.prototype._appendPair = function(section, row, line,
lineNumber, side) {
row.appendChild(this._createLineEl(line, lineNumber, line.type));
var action = this._createContextControl(section, line);
if (action) {
row.appendChild(action);
} else {
var el = this._createTextEl(line);
el.classList.add(side);
row.appendChild(el);
var textEl = this._createTextEl(line);
textEl.classList.add(side);
var threadEl = this._createCommentThread(line, side);
if (threadEl) {
textEl.appendChild(threadEl);
}
row.appendChild(textEl);
}
return row;
};
window.GrDiffBuilderSideBySide = GrDiffBuilderSideBySide;

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

@@ -14,8 +14,8 @@
(function(window, GrDiffBuilder) {
'use strict';
function GrDiffBuilderUnified(diff, prefs, outputEl) {
GrDiffBuilder.call(this, diff, prefs, outputEl);
function GrDiffBuilderUnified(diff, comments, prefs, outputEl) {
GrDiffBuilder.call(this, diff, comments, prefs, outputEl);
}
GrDiffBuilderUnified.prototype = Object.create(GrDiffBuilder.prototype);
GrDiffBuilderUnified.prototype.constructor = GrDiffBuilderUnified;
@@ -41,7 +41,12 @@
if (action) {
row.appendChild(action);
} else {
row.appendChild(this._createTextEl(line));
var textEl = this._createTextEl(line);
var threadEl = this._createCommentThread(line);
if (threadEl) {
textEl.appendChild(threadEl);
}
row.appendChild(textEl);
}
return row;
};

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

@@ -14,7 +14,8 @@
(function(window, GrDiffGroup, GrDiffLine) {
'use strict';
function GrDiffBuilder(diff, prefs, outputEl) {
function GrDiffBuilder(diff, comments, prefs, outputEl) {
this._comments = comments;
this._prefs = prefs;
this._outputEl = outputEl;
this._groups = [];
@@ -38,6 +39,11 @@
REMOVED: 'a',
};
GrDiffBuilder.Side = {
LEFT: 'left',
RIGHT: 'right',
};
GrDiffBuilder.prototype.emitDiff = function() {
for (var i = 0; i < this._groups.length; i++) {
this.emitGroup(this._groups[i]);
@@ -167,6 +173,40 @@
return td;
};
GrDiffBuilder.prototype._getCommentsForLine = function(comments, line,
opt_side) {
var leftComments = comments[GrDiffBuilder.Side.LEFT].filter(
function(c) { return c.line === line.beforeNumber; });
var rightComments = comments[GrDiffBuilder.Side.RIGHT].filter(
function(c) { return c.line === line.afterNumber; });
var result;
switch (opt_side) {
case GrDiffBuilder.Side.LEFT:
result = leftComments;
break;
case GrDiffBuilder.Side.RIGHT:
result = rightComments;
break;
default:
result = leftComments.concat(rightComments);
break;
}
return result;
};
GrDiffBuilder.prototype._createCommentThread = function(line, opt_side) {
var comments = this._getCommentsForLine(this._comments, line, opt_side);
if (!comments || comments.length === 0) {
return null;
}
var threadEl = document.createElement('gr-diff-comment-thread');
threadEl.comments = comments;
return threadEl;
};
GrDiffBuilder.prototype._createLineEl = function(line, number, type) {
var td = this._createElement('td', 'lineNum');
if (line.type === GrDiffLine.Type.BLANK) {

46
polygerrit-ui/app/elements/diff/gr-new-diff/gr-diff-builder_test.html
View File

@@ -207,7 +207,8 @@ limitations under the License.
line_length: 10,
tab_size: 4,
};
var builder = new GrDiffBuilder({content: []}, prefs);
var builder =
new GrDiffBuilder({content: []}, {left: [], right: []}, prefs);
var text = 'abcdef';
assert.equal(builder._addNewlines(text, text), text);
@@ -240,7 +241,8 @@ limitations under the License.
show_tabs: true,
tab_size: 4,
};
var builder = new GrDiffBuilder({content: []}, prefs);
var builder =
new GrDiffBuilder({content: []}, {left: [], right: []}, prefs);
var html = 'abc\tdef';
var wrapper = builder._getTabWrapper(prefs.tab_size, prefs.show_tabs);
@@ -252,5 +254,45 @@ limitations under the License.
'"><img src="/" onerror="alert(1);"><span class="',
true));
});
test('comments', function() {
var line = new GrDiffLine(GrDiffLine.Type.BOTH);
line.beforeNumber = 3;
line.afterNumber = 5;
var comments = {left: [], right:[]};
assert.deepEqual(
GrDiffBuilder.prototype._getCommentsForLine(comments, line), []);
assert.deepEqual(
GrDiffBuilder.prototype._getCommentsForLine(comments, line,
GrDiffBuilder.Side.LEFT),
[]);
assert.deepEqual(
GrDiffBuilder.prototype._getCommentsForLine(comments, line,
GrDiffBuilder.Side.RIGHT),
[]);
comments = {
left: [
{id: 'l3', line: 3},
{id: 'l5', line: 5},
],
right: [
{id: 'r3', line: 3},
{id: 'r5', line: 5},
],
};
assert.deepEqual(
GrDiffBuilder.prototype._getCommentsForLine(comments, line),
[{id: 'l3', line: 3}, {id: 'r5', line: 5}]);
assert.deepEqual(
GrDiffBuilder.prototype._getCommentsForLine(comments, line,
GrDiffBuilder.Side.LEFT),
[{id: 'l3', line: 3}]);
assert.deepEqual(
GrDiffBuilder.prototype._getCommentsForLine(comments, line,
GrDiffBuilder.Side.RIGHT),
[{id: 'r5', line: 5}]);
});
});
</script>

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

@@ -20,6 +20,7 @@ limitations under the License.
<link rel="import" href="../../shared/gr-request/gr-request.html">
<link rel="import" href="../../shared/gr-rest-api-interface/gr-rest-api-interface.html">
<link rel="import" href="../gr-diff-comment-thread/gr-diff-comment-thread.html">
<link rel="import" href="../gr-diff-preferences/gr-diff-preferences.html">
<link rel="import" href="../gr-patch-range-select/gr-patch-range-select.html">
@@ -70,6 +71,13 @@ limitations under the License.
.lineNum:before {
content: attr(data-value);
}
.canComment .lineNum[data-value] {
cursor: pointer;
text-decoration: underline;
}
.canComment .lineNum[data-value]:hover {
background-color: #ccc;
}
.content {
overflow: hidden;
width: var(--content-width, 80ch);
@@ -138,7 +146,7 @@ limitations under the License.
on-cancel="_handlePrefsCancel"></gr-diff-preferences>
</gr-overlay>
<div class="diffContainer"
<div class$="[[_computeContainerClass(_loggedIn)]]"
on-tap="_handleTap"
on-mousedown="_handleMouseDown"
on-copy="_handleCopy">

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

@@ -38,6 +38,10 @@
},
projectConfig: Object,
_loggedIn: {
type: Boolean,
value: false,
},
_loading: {
type: Boolean,
value: true,
@@ -52,20 +56,43 @@
type: String,
observer: '_selectionSideChanged',
},
_comments: Object,
},
observers: [
'_render(_diff, prefs.*)',
'_render(_diff, _comments, prefs.*)',
],
attached: function() {
this._getLoggedIn().then(function(loggedIn) {
this._loggedIn = loggedIn;
}.bind(this));
},
reload: function() {
this.$.diffTable.innerHTML = null;
this._loading = true;
return this._getDiff().then(function(diff) {
var promises = [];
promises.push(this._getDiff().then(function(diff) {
this._diff = diff;
this._loading = false;
}.bind(this));
}.bind(this)));
promises.push(this._getDiffCommentsAndDrafts().then(function(comments) {
this._comments = comments;
}.bind(this)));
return Promise.all(promises);
},
_computeContainerClass: function(loggedIn) {
var classes = ['diffContainer'];
if (loggedIn) {
classes.push('canComment');
}
return classes.join(' ');
},
_handleTap: function(e) {
@@ -135,12 +162,11 @@
sectionEl.parentNode.removeChild(sectionEl);
},
_render: function(diff, prefsChangeRecord) {
_render: function(diff, comments, prefsChangeRecord) {
var prefs = prefsChangeRecord.base;
this.customStyle['--content-width'] = prefs.line_length + 'ch';
this.updateStyles();
this._builder = this._getDiffBuilder(diff, prefs);
this._builder = this._getDiffBuilder(diff, comments, prefs);
this._builder.emitDiff(diff.content);
},
@@ -152,11 +178,63 @@
this.path);
},
_getDiffBuilder: function(diff, prefs) {
_getDiffComments: function() {
return this.$.restAPI.getDiffComments(
this.changeNum,
this.patchRange.basePatchNum,
this.patchRange.patchNum,
this.path);
},
_getDiffDrafts: function() {
return this._getLoggedIn().then(function(loggedIn) {
if (!loggedIn) {
return Promise.resolve({baseComments: [], comments: []});
}
return this.$.restAPI.getDiffDrafts(
this.changeNum,
this.patchRange.basePatchNum,
this.patchRange.patchNum,
this.path);
}.bind(this));
},
_getDiffCommentsAndDrafts: function() {
var promises = [];
promises.push(this._getDiffComments());
promises.push(this._getDiffDrafts());
return Promise.all(promises).then(function(results) {
return Promise.resolve({
comments: results[0],
drafts: results[1],
});
}).then(this._normalizeDiffCommentsAndDrafts);
},
_normalizeDiffCommentsAndDrafts: function(results) {
function markAsDraft(d) {
d.__draft = true;
return d;
}
var baseDrafts = results.drafts.baseComments.map(markAsDraft);
var drafts = results.drafts.comments.map(markAsDraft);
return Promise.resolve({
left: results.comments.baseComments.concat(baseDrafts),
right: results.comments.comments.concat(drafts),
});
},
_getLoggedIn: function() {
return this.$.restAPI.getLoggedIn();
},
_getDiffBuilder: function(diff, comments, prefs) {
if (this._viewMode === DiffViewMode.SIDE_BY_SIDE) {
return new GrDiffBuilderSideBySide(diff, prefs, this.$.diffTable);
return new GrDiffBuilderSideBySide(diff, comments, prefs,
this.$.diffTable);
} else if (this._viewMode === DiffViewMode.UNIFIED) {
return new GrDiffBuilderUnified(diff, prefs, this.$.diffTable);
return new GrDiffBuilderUnified(diff, comments, prefs,
this.$.diffTable);
}
throw Error('Unsupported diff view mode: ' + this._viewMode);
},

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

@@ -35,14 +35,98 @@ limitations under the License.
var element;
setup(function() {
stub('gr-rest-api-interface', {
getLoggedIn: function() { return Promise.resolve(false); },
})
element = fixture('basic');
});
teardown(function() {
});
test('basic', function() {
test('get drafts logged out', function(done) {
element.patchRange = {basePatchNum: 0, patchNum: 0};
var getDraftsStub = sinon.stub(element.$.restAPI, 'getDiffDrafts');
var loggedInStub = sinon.stub(element, '_getLoggedIn',
function() { return Promise.resolve(false); });
element._getDiffDrafts().then(function(result) {
assert.deepEqual(result, {baseComments: [], comments: []});
sinon.assert.notCalled(getDraftsStub);
loggedInStub.restore();
getDraftsStub.restore();
done();
});
});
test('get drafts logged in', function(done) {
element.patchRange = {basePatchNum: 0, patchNum: 0};
var draftsResponse = {
baseComments: [{id: 'foo'}],
comments: [{id: 'bar'}],
};
var getDraftsStub = sinon.stub(element.$.restAPI, 'getDiffDrafts',
function() { return Promise.resolve(draftsResponse); });
var loggedInStub = sinon.stub(element, '_getLoggedIn',
function() { return Promise.resolve(true); });
element._getDiffDrafts().then(function(result) {
assert.deepEqual(result, draftsResponse);
loggedInStub.restore();
getDraftsStub.restore();
done();
});
});
test('get comments and drafts', function(done) {
var loggedInStub = sinon.stub(element, '_getLoggedIn',
function() { return Promise.resolve(true); });
var comments = {
baseComments: [
{id: 'bc1'},
{id: 'bc2'},
],
comments: [
{id: 'c1'},
{id: 'c2'},
],
};
var diffCommentsStub = sinon.stub(element, '_getDiffComments',
function() { return Promise.resolve(comments); });
var drafts = {
baseComments: [
{id: 'bd1'},
{id: 'bd2'},
],
comments: [
{id: 'd1'},
{id: 'd2'},
],
};
var diffDraftsStub = sinon.stub(element, '_getDiffDrafts',
function() { return Promise.resolve(drafts); });
element._getDiffCommentsAndDrafts().then(function(result) {
assert.deepEqual(result, {
left: [
{id: 'bc1'},
{id: 'bc2'},
{id: 'bd1', __draft: true},
{id: 'bd2', __draft: true},
],
right: [
{id: 'c1'},
{id: 'c2'},
{id: 'd1', __draft: true},
{id: 'd2', __draft: true},
],
});
diffCommentsStub.restore();
diffDraftsStub.restore();
loggedInStub.restore();
done();
});
});
});
</script>