opendev/gerrit
Code Issues Proposed changes

Merge "Show blame in diff"

Browse Source
This commit is contained in:
Wyatt Allen
2017-09-30 09:54:32 +00:00
committed by Gerrit Code Review
parent b4f2b3ade6 ed628d7f37
commit a1c4658a97
14 changed files with 454 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -46,8 +46,12 @@
const width = fontSize * 4;
const colgroup = document.createElement('colgroup');
// Add the blame column.
let col = this._createElement('col', 'blame');
colgroup.appendChild(col);
// Add left-side line number.
let col = document.createElement('col');
col = document.createElement('col');
col.setAttribute('width', width);
colgroup.appendChild(col);
@@ -73,6 +77,8 @@
row.setAttribute('right-type', rightLine.type);
row.tabIndex = -1;
row.appendChild(this._createBlameCell(leftLine));
this._appendPair(section, row, leftLine, leftLine.beforeNumber,
GrDiffBuilder.Side.LEFT);
this._appendPair(section, row, rightLine, rightLine.afterNumber,

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

@@ -45,8 +45,12 @@
const width = fontSize * 4;
const colgroup = document.createElement('colgroup');
// Add the blame column.
let col = this._createElement('col', 'blame');
colgroup.appendChild(col);
// Add left-side line number.
let col = document.createElement('col');
col = document.createElement('col');
col.setAttribute('width', width);
colgroup.appendChild(col);
@@ -63,6 +67,8 @@
GrDiffBuilderUnified.prototype._createRow = function(section, line) {
const row = this._createElement('tr', line.type);
row.appendChild(this._createBlameCell(line));
let lineEl = this._createLineEl(line, line.beforeNumber,
GrDiffLine.Type.REMOVE);
lineEl.classList.add('left');

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

@@ -403,6 +403,11 @@ limitations under the License.
}, false);
}, false);
},
setBlame(blame) {
if (!this._builder || !blame) { return; }
this._builder.setBlame(blame);
},
});
})();
</script>

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

@@ -37,6 +37,7 @@
this._projectName = projectName;
this._outputEl = outputEl;
this.groups = [];
this._blameInfo = null;
this.layers = layers || [];
@@ -625,5 +626,105 @@
});
};
/**
* Set the blame information for the diff. For any already-rednered line,
* re-render its blame cell content.
* @param {Object} blame
*/
GrDiffBuilder.prototype.setBlame = function(blame) {
this._blameInfo = blame;
// TODO(wyatta): make this loop asynchronous.
for (const commit of blame) {
for (const range of commit.ranges) {
for (let i = range.start; i <= range.end; i++) {
// TODO(wyatta): this query is expensive, but, when traversing a
// range, the lines are consecutive, and given the previous blame
// cell, the next one can be reached cheaply.
const el = this._getBlameByLineNum(i);
if (!el) { continue; }
// Remove the element's children (if any).
while (el.hasChildNodes()) {
el.removeChild(el.lastChild);
}
const blame = this._getBlameForBaseLine(i, commit);
el.appendChild(blame);
}
}
}
};
/**
* Find the blame cell for a given line number.
* @param {number} lineNum
* @return {HTMLTableDataCellElement}
*/
GrDiffBuilder.prototype._getBlameByLineNum = function(lineNum) {
const root = Polymer.dom(this._outputEl);
return root.querySelector(`td.blame[data-line-number="${lineNum}"]`);
};
/**
* Given a base line number, return the commit containing that line in the
* current set of blame information. If no blame information has been
* provided, null is returned.
* @param {number} lineNum
* @return {Object} The commit information.
*/
GrDiffBuilder.prototype._getBlameCommitForBaseLine = function(lineNum) {
if (!this._blameInfo) { return null; }
for (const blameCommit of this._blameInfo) {
for (const range of blameCommit.ranges) {
if (range.start <= lineNum && range.end >= lineNum) {
return blameCommit;
}
}
}
return null;
};
/**
* Given the number of a base line, get the content for the blame cell of that
* line. If there is no blame information for that line, returns null.
* @param {number} lineNum
* @param {Object=} opt_commit Optionally provide the commit object, so that
* it does not need to be searched.
* @return {HTMLSpanElement}
*/
GrDiffBuilder.prototype._getBlameForBaseLine = function(lineNum, opt_commit) {
const commit = opt_commit || this._getBlameCommitForBaseLine(lineNum);
if (!commit) { return null; }
const isStartOfRange = commit.ranges.some(r => r.start === lineNum);
const date = (new Date(commit.time * 1000)).toLocaleDateString();
const blameNode = this._createElement('span',
isStartOfRange ? 'startOfRange' : '');
const shaNode = this._createElement('span', 'sha');
shaNode.innerText = commit.id.substr(0, 7);
blameNode.appendChild(shaNode);
blameNode.append(` on ${date} by ${commit.author}`);
return blameNode;
};
/**
* Create a blame cell for the given base line. Blame information will be
* included in the cell if available.
* @param {GrDiffLine} line
* @return {HTMLTableDataCellElement}
*/
GrDiffBuilder.prototype._createBlameCell = function(line) {
const blameTd = this._createElement('td', 'blame');
blameTd.setAttribute('data-line-number', line.beforeNumber);
if (line.beforeNumber) {
const content = this._getBlameForBaseLine(line.beforeNumber);
if (content) {
blameTd.appendChild(content);
}
}
return blameTd;
};
window.GrDiffBuilder = GrDiffBuilder;
})(window, GrDiffGroup, GrDiffLine);

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

@@ -1003,5 +1003,58 @@ limitations under the License.
assert.equal(result, expected);
});
});
suite('blame', () => {
let mockBlame;
setup(() => {
mockBlame = [
{id: 'commit 1', ranges: [{start: 1, end: 2}, {start: 10, end: 16}]},
{id: 'commit 2', ranges: [{start: 4, end: 10}, {start: 17, end: 32}]},
];
});
test('setBlame attempts to render each blamed line', () => {
const getBlameStub = sandbox.stub(builder, '_getBlameByLineNum')
.returns(null);
builder.setBlame(mockBlame);
assert.equal(getBlameStub.callCount, 32);
});
test('_getBlameCommitForBaseLine', () => {
builder.setBlame(mockBlame);
assert.isOk(builder._getBlameCommitForBaseLine(1));
assert.equal(builder._getBlameCommitForBaseLine(1).id, 'commit 1');
assert.isOk(builder._getBlameCommitForBaseLine(11));
assert.equal(builder._getBlameCommitForBaseLine(11).id, 'commit 1');
assert.isOk(builder._getBlameCommitForBaseLine(32));
assert.equal(builder._getBlameCommitForBaseLine(32).id, 'commit 2');
assert.isNull(builder._getBlameCommitForBaseLine(33));
});
test('_getBlameCommitForBaseLine w/o blame returns null', () => {
assert.isNull(builder._getBlameCommitForBaseLine(1));
assert.isNull(builder._getBlameCommitForBaseLine(11));
assert.isNull(builder._getBlameCommitForBaseLine(31));
});
test('_createBlameCell', () => {
const mocbBlameCell = document.createElement('span');
const getBlameStub = sinon.stub(builder, '_getBlameForBaseLine')
.returns(mocbBlameCell);
const line = new GrDiffLine(GrDiffLine.Type.BOTH);
line.beforeNumber = 3;
line.afterNumber = 5;
const result = builder._createBlameCell(line);
assert.isTrue(getBlameStub.calledWithExactly(3));
assert.equal(result.getAttribute('data-line-number'), '3');
assert.equal(result.firstChild, mocbBlameCell);
});
});
});
</script>

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

@@ -20,7 +20,8 @@ limitations under the License.
<template>
<style include="shared-styles">
.contentWrapper ::content .content,
.contentWrapper ::content .contextControl {
.contentWrapper ::content .contextControl,
.contentWrapper ::content .blame {
-webkit-user-select: none;
-moz-user-select: none;
-ms-user-select: none;
@@ -33,7 +34,8 @@ limitations under the License.
:host-context(.selected-right:not(.selected-comment)) .contentWrapper ::content .unified .right.lineNum ~ .content .contentText,
:host-context(.selected-left.selected-comment) .contentWrapper ::content .side-by-side .left + .content .message,
:host-context(.selected-right.selected-comment) .contentWrapper ::content .side-by-side .right + .content .message :not(.collapsedContent),
:host-context(.selected-comment) .contentWrapper ::content .unified .message :not(.collapsedContent){
:host-context(.selected-comment) .contentWrapper ::content .unified .message :not(.collapsedContent),
:host-context(.selected-blame) .contentWrapper ::content .blame {
-webkit-user-select: text;
-moz-user-select: text;
-ms-user-select: text;

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

@@ -22,6 +22,7 @@
COMMENT: 'selected-comment',
LEFT: 'selected-left',
RIGHT: 'selected-right',
BLAME: 'selected-blame',
};
const getNewCache = () => { return {left: null, right: null}; };
@@ -66,20 +67,36 @@
_handleDown(e) {
const lineEl = this.diffBuilder.getLineElByChild(e.target);
if (!lineEl) {
return;
}
const commentSelected =
this._elementDescendedFromClass(e.target, 'gr-diff-comment');
const side = this.diffBuilder.getSideByLineEl(lineEl);
const targetClasses = [];
targetClasses.push(side === 'left' ?
SelectionClass.LEFT :
SelectionClass.RIGHT);
const blameSelected = this._elementDescendedFromClass(e.target, 'blame');
if (!lineEl && !blameSelected) { return; }
if (commentSelected) {
targetClasses.push(SelectionClass.COMMENT);
const targetClasses = [];
if (blameSelected) {
targetClasses.push(SelectionClass.BLAME);
} else {
const commentSelected =
this._elementDescendedFromClass(e.target, 'gr-diff-comment');
const side = this.diffBuilder.getSideByLineEl(lineEl);
targetClasses.push(side === 'left' ?
SelectionClass.LEFT :
SelectionClass.RIGHT);
if (commentSelected) {
targetClasses.push(SelectionClass.COMMENT);
}
}
this._setClasses(targetClasses);
},
/**
* Set the provided list of classes on the element, to the exclusion of all
* other SelectionClass values.
* @param {!Array<!string>} targetClasses
*/
_setClasses(targetClasses) {
// Remove any selection classes that do not belong.
for (const key in SelectionClass) {
if (SelectionClass.hasOwnProperty(key)) {

42
polygerrit-ui/app/elements/diff/gr-diff-selection/gr-diff-selection_test.html
View File

@@ -30,6 +30,7 @@ limitations under the License.
<gr-diff-selection>
<table id="diffTable" class="side-by-side">
<tr class="diff-row">
<td class="blame" data-line-number="1"></td>
<td class="lineNum left" data-value="1">1</td>
<td class="content">
<div class="contentText" data-side="left">ba ba</div>
@@ -47,6 +48,7 @@ limitations under the License.
</td>
</tr>
<tr class="diff-row">
<td class="blame" data-line-number="2"></td>
<td class="lineNum left" data-value="2">2</td>
<td class="content">
<div class="contentText" data-side="left">zin</div>
@@ -64,6 +66,7 @@ limitations under the License.
</td>
</tr>
<tr class="diff-row">
<td class="blame" data-line-number="3"></td>
<td class="lineNum left" data-value="3">3</td>
<td class="content">
<div class="contentText" data-side="left">ga ga</div>
@@ -78,6 +81,7 @@ limitations under the License.
<td class="lineNum right" data-value="3">3</td>
</tr>
<tr class="diff-row">
<td class="blame" data-line-number="4"></td>
<td class="lineNum left" data-value="4">4</td>
<td class="content">
<div class="contentText" data-side="left">ga ga</div>
@@ -169,6 +173,18 @@ limitations under the License.
element.classList.contains('selected-left'), 'removes selected-left');
});
test('applies selected-blame on blame click', () => {
element.classList.add('selected-left');
element.diffBuilder.getLineElByChild.returns(null);
sandbox.stub(element, '_elementDescendedFromClass',
(el, className) => className === 'blame');
MockInteractions.down(element);
assert.isTrue(
element.classList.contains('selected-blame'), 'adds selected-right');
assert.isFalse(
element.classList.contains('selected-left'), 'removes selected-left');
});
test('ignores copy for non-content Element', () => {
sandbox.stub(element, '_getSelectedText');
emulateCopyOn(element.querySelector('.not-diff-row'));
@@ -203,6 +219,32 @@ limitations under the License.
['Text', 'the text'], event.clipboardData.setData.lastCall.args);
});
test('_setClasses adds given SelectionClass values, removes others', () => {
element.classList.add('selected-right');
element._setClasses(['selected-comment', 'selected-left']);
assert.isTrue(element.classList.contains('selected-comment'));
assert.isTrue(element.classList.contains('selected-left'));
assert.isFalse(element.classList.contains('selected-right'));
assert.isFalse(element.classList.contains('selected-blame'));
element._setClasses(['selected-blame']);
assert.isFalse(element.classList.contains('selected-comment'));
assert.isFalse(element.classList.contains('selected-left'));
assert.isFalse(element.classList.contains('selected-right'));
assert.isTrue(element.classList.contains('selected-blame'));
});
test('_setClasses removes before it ads', () => {
element.classList.add('selected-right');
const addStub = sandbox.stub(element.classList, 'add');
const removeStub = sandbox.stub(element.classList, 'remove', () => {
assert.isFalse(addStub.called);
});
element._setClasses(['selected-comment', 'selected-left']);
assert.isTrue(addStub.called);
assert.isTrue(removeStub.called);
});
test('copies content correctly', () => {
// Fetch the line number.
element._cachedDiffBuilder.getLineElByChild = function(child) {

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

@@ -159,6 +159,12 @@ limitations under the License.
.editLoaded .hideOnEdit {
display: none;
}
.blameLoader {
display: none;
}
.blameLoader.show {
display: inline;
}
@media screen and (max-width: 50em) {
header {
padding: .5em var(--default-horizontal-margin);
@@ -312,6 +318,13 @@ limitations under the License.
on-tap="_handlePrefsTap">Preferences</gr-button>
</span>
</span>
<span class$="blameLoader [[_computeBlameLoaderClass(_isImageDiff, _isBlameSupported)]]">
<span class="separator">/</span>
<gr-button
link
disabled="[[_isBlameLoading]]"
on-tap="_toggleBlame">[[_computeBlameToggleLabel(_isBlameLoaded, _isBlameLoading)]]</gr-button>
</span>
</div>
</div>
<div class="fileNav mobile">
@@ -340,6 +353,7 @@ limitations under the License.
project-config="[[_projectConfig]]"
project-name="[[_change.project]]"
view-mode="[[_diffMode]]"
is-blame-loaded="{{_isBlameLoaded}}"
on-line-selected="_onLineSelected">
</gr-diff>
<gr-diff-preferences

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

@@ -18,6 +18,8 @@
const MERGE_LIST_PATH = '/MERGE_LIST';
const ERR_REVIEW_STATUS = 'Couldn’t change file review status.';
const MSG_LOADING_BLAME = 'Loading blame...';
const MSG_LOADED_BLAME = 'Blame loaded';
const PARENT = 'PARENT';
@@ -125,6 +127,16 @@
type: Boolean,
computed: '_computeEditLoaded(_patchRange.*)',
},
_isBlameSupported: {
type: Boolean,
value: false,
},
_isBlameLoaded: Boolean,
_isBlameLoading: {
type: Boolean,
value: false,
},
},
behaviors: [
@@ -160,6 +172,10 @@
this._loggedIn = loggedIn;
});
this.$.restAPI.getConfig().then(config => {
this._isBlameSupported = config.change.allow_blame;
});
this.$.cursor.push('diffs', this.$.diff);
},
@@ -805,5 +821,36 @@
_computeContainerClass(editLoaded) {
return editLoaded ? 'editLoaded' : '';
},
_computeBlameToggleLabel(loaded, loading) {
if (loaded) { return 'Hide blame'; }
return 'Show blame';
},
/**
* Load and display blame information if it has not already been loaded.
* Otherwise hide it.
*/
_toggleBlame() {
if (this._isBlameLoaded) {
this.$.diff.clearBlame();
return;
}
this._isBlameLoading = true;
this.fire('show-alert', {message: MSG_LOADING_BLAME});
this.$.diff.loadBlame()
.then(() => {
this._isBlameLoading = false;
this.fire('show-alert', {message: MSG_LOADED_BLAME});
})
.catch(() => {
this._isBlameLoading = false;
});
},
_computeBlameLoaderClass(isImageDiff, supported) {
return !isImageDiff && supported ? 'show' : '';
},
});
})();

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

@@ -212,6 +212,32 @@ limitations under the License.
#sizeWarning.warn {
display: block;
}
.target-row td.blame {
background: #eee;
}
td.blame {
display: none;
font-family: var(--font-family);
font-size: var(--font-size, 12px);
padding: 0 .5em;
white-space: pre;
}
:host(.showBlame) td.blame {
display: table-cell;
}
td.blame > span {
opacity: 0.6;
}
td.blame > span.startOfRange {
opacity: 1;
}
td.blame .sha {
font-family: var(--monospace-font-family);
}
.full-width td.blame {
overflow: hidden;
width: 200px;
}
</style>
<style include="gr-theme-default"></style>
<div id="diffHeader" hidden$="[[_computeDiffHeaderHidden(_diffHeaderItems)]]">

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

@@ -16,6 +16,7 @@
const ERR_COMMENT_ON_EDIT = 'You cannot comment on an edit.';
const ERR_INVALID_LINE = 'Invalid line number: ';
const MSG_EMPTY_BLAME = 'No blame information for this diff.';
const DiffViewMode = {
SIDE_BY_SIDE: 'SIDE_BY_SIDE',
@@ -125,6 +126,17 @@
},
_showWarning: Boolean,
/** @type {?Object} */
_blame: {
type: Object,
value: null,
},
isBlameLoaded: {
type: Boolean,
notify: true,
computed: '_computeIsBlameLoaded(_blame)',
},
},
behaviors: [
@@ -154,6 +166,7 @@
/** @return {!Promise} */
reload() {
this.$.diffBuilder.cancel();
this.clearBlame();
this._safetyBypass = null;
this._showWarning = false;
this._clearDiffContent();
@@ -191,6 +204,39 @@
this.toggleClass('no-left');
},
/**
* Load and display blame information for the base of the diff.
* @return {Promise} A promise that resolves when blame finishes rendering.
*/
loadBlame() {
return this.$.restAPI.getBlame(this.changeNum, this.patchRange.patchNum,
this.path, true)
.then(blame => {
if (!blame.length) {
this.fire('show-alert', {message: MSG_EMPTY_BLAME});
return Promise.reject(MSG_EMPTY_BLAME);
}
this._blame = blame;
this.$.diffBuilder.setBlame(blame);
this.classList.add('showBlame');
});
},
_computeIsBlameLoaded(blame) {
return !!blame;
},
/**
* Unload blame information for the diff.
*/
clearBlame() {
this._blame = null;
this.$.diffBuilder.setBlame(null);
this.classList.remove('showBlame');
},
/** @return {boolean}} */
_canRender() {
return !!this.changeNum && !!this.patchRange && !!this.path &&
@@ -500,6 +546,8 @@
_prefsChanged(prefs) {
if (!prefs) { return; }
this.clearBlame();
const stylesToUpdate = {};
if (prefs.line_wrapping) {
@@ -589,7 +637,7 @@
const isB = this._diff.meta_b &&
this._diff.meta_b.content_type.startsWith('image/');
return this._diff.binary && (isA || isB);
return !!(this._diff.binary && (isA || isB));
},
/** @return {!Promise} */

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

@@ -959,6 +959,60 @@ limitations under the License.
});
});
});
suite('blame', () => {
setup(() => {
element = fixture('basic');
});
test('clearBlame', () => {
element._blame = [];
const setBlameSpy = sandbox.spy(element.$.diffBuilder, 'setBlame');
element.classList.add('showBlame');
element.clearBlame();
assert.isNull(element._blame);
assert.isTrue(setBlameSpy.calledWithExactly(null));
assert.isFalse(element.classList.contains('showBlame'));
});
test('loadBlame', () => {
const mockBlame = [{id: 'commit id', ranges: [{start: 1, end: 2}]}];
const showAlertStub = sinon.stub();
element.addEventListener('show-alert', showAlertStub);
const getBlameStub = sandbox.stub(element.$.restAPI, 'getBlame')
.returns(Promise.resolve(mockBlame));
element.changeNum = 42;
element.patchRange = {patchNum: 5, basePatchNum: 4};
element.path = 'foo/bar.baz';
return element.loadBlame().then(() => {
assert.isTrue(getBlameStub.calledWithExactly(
42, 5, 'foo/bar.baz', true));
assert.isFalse(showAlertStub.called);
assert.equal(element._blame, mockBlame);
assert.isTrue(element.classList.contains('showBlame'));
});
});
test('loadBlame empty', () => {
const mockBlame = [];
const showAlertStub = sinon.stub();
element.addEventListener('show-alert', showAlertStub);
sandbox.stub(element.$.restAPI, 'getBlame')
.returns(Promise.resolve(mockBlame));
element.changeNum = 42;
element.patchRange = {patchNum: 5, basePatchNum: 4};
element.path = 'foo/bar.baz';
return element.loadBlame()
.then(() => {
assert.isTrue(false, 'Promise should not resolve');
})
.catch(() => {
assert.isTrue(showAlertStub.calledOnce);
assert.isNull(element._blame);
assert.isFalse(element.classList.contains('showBlame'));
});
});
});
});
a11ySuite('basic');

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

@@ -1865,5 +1865,21 @@
opt_params, opt_options);
});
},
/**
* Get blame information for the given diff.
* @param {string|number} changeNum
* @param {string|number} patchNum
* @param {string} path
* @param {boolean=} opt_base If true, requests blame for the base of the
* diff, rather than the revision.
* @return {!Promise<!Object>}
*/
getBlame(changeNum, patchNum, path, opt_base) {
const encodedPath = encodeURIComponent(path);
return this._getChangeURLAndFetch(changeNum,
`/files/${encodedPath}/blame`, patchNum, undefined, undefined,
opt_base ? {base: 't'} : undefined);
},
});
})();