opendev/gerrit
Code Issues Proposed changes

Refactor context control to be a group, not a line

Browse Source 
This breaks a dependency cycle between GrDiffLine and GrDiffGroup. The
line only needed to need to know about groups, if it was of type
CONTEXT_CONTROL. But there was not really a need for the context control
to be modeled as a line. Moving this logic from line to group meant to
re-write the diff builders a little bit such that context control is now
handled a bit more explicitly, which is probably an improvement anyway.

This change is driven by the desire to convert GrDiffLine and
GrDiffGroup to TypeScript and the author did not see a great solution
for how to otherwise break the dependency cycle.

Change-Id: Id07b0b9b2c4f5caf7bcc99ac20164dd34c14653d
This commit is contained in:
Ben Rohlfs
2020-08-02 19:24:17 +02:00
parent 5e2f552a7d
commit bcc6bb9eee
8 changed files with 111 additions and 108 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -80,7 +80,7 @@ suite('gr-diff-builder tests', () => {
});
suite('context control', () => {
function createContextLine(options) {
function createContextGroups(options) {
const offset = options.offset || 0;
const numLines = options.count || 10;
const lines = [];
@@ -92,15 +92,12 @@ suite('gr-diff-builder tests', () => {
lines.push(line);
}
return {
contextGroups: [new GrDiffGroup(GrDiffGroup.Type.BOTH, lines)],
};
return [new GrDiffGroup(GrDiffGroup.Type.BOTH, lines)];
}
test('no +10 buttons for 10 or less lines', () => {
const contextLine = createContextLine({count: 10});
const td = builder._createContextControl({}, contextLine);
const contextGroups = createContextGroups({count: 10});
const td = builder._createContextControl({}, contextGroups);
const buttons = td.querySelectorAll('gr-button.showContext');
assert.equal(buttons.length, 1);
@@ -108,10 +105,9 @@ suite('gr-diff-builder tests', () => {
});
test('context control at the top', () => {
const contextLine = createContextLine({offset: 0, count: 20});
const contextGroups = createContextGroups({offset: 0, count: 20});
builder._numLinesLeft = 50;
const td = builder._createContextControl({}, contextLine);
const td = builder._createContextControl({}, contextGroups);
const buttons = td.querySelectorAll('gr-button.showContext');
assert.equal(buttons.length, 2);
@@ -120,10 +116,9 @@ suite('gr-diff-builder tests', () => {
});
test('context control in the middle', () => {
const contextLine = createContextLine({offset: 10, count: 20});
const contextGroups = createContextGroups({offset: 10, count: 20});
builder._numLinesLeft = 50;
const td = builder._createContextControl({}, contextLine);
const td = builder._createContextControl({}, contextGroups);
const buttons = td.querySelectorAll('gr-button.showContext');
assert.equal(buttons.length, 3);
@@ -133,10 +128,9 @@ suite('gr-diff-builder tests', () => {
});
test('context control at the top', () => {
const contextLine = createContextLine({offset: 30, count: 20});
const contextGroups = createContextGroups({offset: 30, count: 20});
builder._numLinesLeft = 50;
const td = builder._createContextControl({}, contextLine);
const td = builder._createContextControl({}, contextGroups);
const buttons = td.querySelectorAll('gr-button.showContext');
assert.equal(buttons.length, 2);
@@ -1203,7 +1197,7 @@ suite('gr-diff-builder tests', () => {
line.beforeNumber = 3;
line.afterNumber = 5;
const result = builder._createBlameCell(line);
const result = builder._createBlameCell(line.beforeNumber);
assert.isTrue(getBlameStub.calledWithExactly(3));
assert.equal(result.getAttribute('data-line-number'), '3');

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

@@ -16,6 +16,7 @@
*/
import {GrDiffBuilder} from './gr-diff-builder.js';
import {GrDiffGroup} from '../gr-diff/gr-diff-group';
/** @constructor */
export function GrDiffBuilderSideBySide(diff, prefs, outputEl, layers) {
@@ -36,6 +37,12 @@ GrDiffBuilderSideBySide.prototype.buildSectionElement = function(group) {
if (group.ignoredWhitespaceOnly) {
sectionEl.classList.add('ignoredWhitespaceOnly');
}
if (group.type === GrDiffGroup.Type.CONTEXT_CONTROL) {
sectionEl.appendChild(
this._createContextRow(sectionEl, group.contextGroups));
return sectionEl;
}
const pairs = group.getSideBySidePairs();
for (let i = 0; i < pairs.length; i++) {
sectionEl.appendChild(this._createRow(sectionEl, pairs[i].left,
@@ -79,7 +86,7 @@ GrDiffBuilderSideBySide.prototype._createRow = function(section, leftLine,
row.setAttribute('right-type', rightLine.type);
row.tabIndex = -1;
row.appendChild(this._createBlameCell(leftLine));
row.appendChild(this._createBlameCell(leftLine.beforeNumber));
this._appendPair(section, row, leftLine, leftLine.beforeNumber,
GrDiffBuilder.Side.LEFT);
@@ -92,13 +99,23 @@ GrDiffBuilderSideBySide.prototype._appendPair = function(section, row, line,
lineNumber, side) {
const lineNumberEl = this._createLineEl(line, lineNumber, line.type, side);
row.appendChild(lineNumberEl);
const action = this._createContextControl(section, line);
if (action) {
row.appendChild(action);
} else {
const textEl = this._createTextEl(lineNumberEl, line, side);
row.appendChild(textEl);
}
row.appendChild(this._createTextEl(lineNumberEl, line, side));
};
GrDiffBuilderSideBySide.prototype._createContextRow = function(section,
contextGroups) {
const row = this._createElement('tr');
row.classList.add('diff-row', 'side-by-side');
row.setAttribute('left-type', GrDiffGroup.Type.CONTEXT_CONTROL);
row.setAttribute('right-type', GrDiffGroup.Type.CONTEXT_CONTROL);
row.tabIndex = -1;
row.appendChild(this._createBlameCell(0));
row.appendChild(this._createElement('td', 'contextLineNum'));
row.appendChild(this._createContextControl(section, contextGroups));
row.appendChild(this._createElement('td', 'contextLineNum'));
row.appendChild(this._createContextControl(section, contextGroups));
return row;
};
GrDiffBuilderSideBySide.prototype._getNextContentOnSide = function(

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

@@ -16,6 +16,7 @@
*/
import {GrDiffLine} from '../gr-diff/gr-diff-line.js';
import {GrDiffBuilder} from './gr-diff-builder.js';
import {GrDiffGroup} from '../gr-diff/gr-diff-group';
export function GrDiffBuilderUnified(diff, prefs, outputEl, layers) {
GrDiffBuilder.call(this, diff, prefs, outputEl, layers);
@@ -35,6 +36,11 @@ GrDiffBuilderUnified.prototype.buildSectionElement = function(group) {
if (group.ignoredWhitespaceOnly) {
sectionEl.classList.add('ignoredWhitespaceOnly');
}
if (group.type === GrDiffGroup.Type.CONTEXT_CONTROL) {
sectionEl.appendChild(
this._createContextRow(sectionEl, group.contextGroups));
return sectionEl;
}
for (let i = 0; i < group.lines.length; ++i) {
const line = group.lines[i];
@@ -76,22 +82,26 @@ GrDiffBuilderUnified.prototype._createRow = function(section, line) {
const row = this._createElement('tr', line.type);
row.classList.add('diff-row', 'unified');
row.tabIndex = -1;
row.appendChild(this._createBlameCell(line));
row.appendChild(this._createBlameCell(line.beforeNumber));
let lineNumberEl = this._createLineEl(line, line.beforeNumber,
GrDiffLine.Type.REMOVE, 'left');
row.appendChild(lineNumberEl);
lineNumberEl = this._createLineEl(line, line.afterNumber,
GrDiffLine.Type.ADD, 'right');
row.appendChild(lineNumberEl);
row.appendChild(this._createTextEl(lineNumberEl, line));
return row;
};
const action = this._createContextControl(section, line);
if (action) {
row.appendChild(action);
} else {
const textEl = this._createTextEl(lineNumberEl, line);
row.appendChild(textEl);
}
GrDiffBuilderUnified.prototype._createContextRow = function(section,
contextGroups) {
const row = this._createElement('tr', GrDiffGroup.Type.CONTEXT_CONTROL);
row.classList.add('diff-row', 'unified');
row.tabIndex = -1;
row.appendChild(this._createBlameCell(0));
row.appendChild(this._createElement('td', 'contextLineNum'));
row.appendChild(this._createElement('td', 'contextLineNum'));
row.appendChild(this._createContextControl(section, contextGroups));
return row;
};

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

@@ -240,12 +240,13 @@ GrDiffBuilder.prototype.getSectionsByLineRange = function(
group => group.element);
};
GrDiffBuilder.prototype._createContextControl = function(section, line) {
if (!line.contextGroups) return null;
GrDiffBuilder.prototype._createContextControl = function(
section, contextGroups) {
if (!contextGroups) return null;
const leftStart = line.contextGroups[0].lineRange.left.start;
const leftStart = contextGroups[0].lineRange.left.start;
const leftEnd =
line.contextGroups[line.contextGroups.length - 1].lineRange.left.end;
contextGroups[contextGroups.length - 1].lineRange.left.end;
const numLines = leftEnd - leftStart + 1;
@@ -256,22 +257,24 @@ GrDiffBuilder.prototype._createContextControl = function(section, line) {
if (showPartialLinks && leftStart > 1) {
td.appendChild(this._createContextButton(
GrDiffBuilder.ContextButtonType.ABOVE, section, line, numLines));
GrDiffBuilder.ContextButtonType.ABOVE, section, contextGroups,
numLines));
}
td.appendChild(this._createContextButton(
GrDiffBuilder.ContextButtonType.ALL, section, line, numLines));
GrDiffBuilder.ContextButtonType.ALL, section, contextGroups, numLines));
if (showPartialLinks && leftEnd < this._numLinesLeft) {
td.appendChild(this._createContextButton(
GrDiffBuilder.ContextButtonType.BELOW, section, line, numLines));
GrDiffBuilder.ContextButtonType.BELOW, section, contextGroups,
numLines));
}
return td;
};
GrDiffBuilder.prototype._createContextButton = function(type, section, line,
numLines) {
GrDiffBuilder.prototype._createContextButton = function(
type, section, contextGroups, numLines) {
const context = PARTIAL_CONTEXT_AMOUNT;
const button = this._createElement('gr-button', 'showContext');
@@ -287,15 +290,15 @@ GrDiffBuilder.prototype._createContextButton = function(type, section, line,
text = 'Show ' + numLines + ' common line';
if (numLines > 1) { text += 's'; }
groups.push(...line.contextGroups);
groups.push(...contextGroups);
} else if (type === GrDiffBuilder.ContextButtonType.ABOVE) {
text = '+' + context + ' above';
groups = GrDiffGroup.hideInContextControl(line.contextGroups,
context, numLines);
groups = GrDiffGroup.hideInContextControl(
contextGroups, context, numLines);
} else if (type === GrDiffBuilder.ContextButtonType.BELOW) {
text = '+' + context + ' below';
groups = GrDiffGroup.hideInContextControl(line.contextGroups,
0, numLines - context);
groups = GrDiffGroup.hideInContextControl(
contextGroups, 0, numLines - context);
}
const textSpan = this._createElement('span', 'showContext');
dom(textSpan).textContent = text;
@@ -319,11 +322,6 @@ GrDiffBuilder.prototype._createLineEl = function(
if (line.type === GrDiffLine.Type.BLANK) {
return td;
}
if (line.type === GrDiffLine.Type.CONTEXT_CONTROL) {
td.classList.add('contextLineNum');
return td;
}
if (line.type === GrDiffLine.Type.BOTH || line.type === type) {
// Both td and button need a number of classes/attributes for various
// selectors to work.
@@ -638,14 +636,14 @@ ${commit.commit_msg}`;
* Create a blame cell for the given base line. Blame information will be
* included in the cell if available.
*
* @param {GrDiffLine} line
* @param {number} lineNumber
* @return {HTMLTableDataCellElement}
*/
GrDiffBuilder.prototype._createBlameCell = function(line) {
GrDiffBuilder.prototype._createBlameCell = function(lineNumber) {
const blameTd = this._createElement('td', 'blame');
blameTd.setAttribute('data-line-number', line.beforeNumber);
if (line.beforeNumber) {
const content = this._getBlameForBaseLine(line.beforeNumber);
blameTd.setAttribute('data-line-number', lineNumber);
if (lineNumber) {
const content = this._getBlameForBaseLine(lineNumber);
if (content) {
blameTd.appendChild(content);
}

48
polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor_test.js
View File

@@ -156,9 +156,9 @@ suite('gr-diff-processor tests', () => {
// group[0] is the file group
assert.equal(groups[1].type, GrDiffGroup.Type.CONTEXT_CONTROL);
assert.instanceOf(groups[1].lines[0].contextGroups[0], GrDiffGroup);
assert.equal(groups[1].lines[0].contextGroups[0].lines.length, 90);
for (const l of groups[1].lines[0].contextGroups[0].lines) {
assert.instanceOf(groups[1].contextGroups[0], GrDiffGroup);
assert.equal(groups[1].contextGroups[0].lines.length, 90);
for (const l of groups[1].contextGroups[0].lines) {
assert.equal(l.text, 'all work and no play make jack a dull boy');
}
@@ -213,9 +213,9 @@ suite('gr-diff-processor tests', () => {
}
assert.equal(groups[3].type, GrDiffGroup.Type.CONTEXT_CONTROL);
assert.instanceOf(groups[3].lines[0].contextGroups[0], GrDiffGroup);
assert.equal(groups[3].lines[0].contextGroups[0].lines.length, 90);
for (const l of groups[3].lines[0].contextGroups[0].lines) {
assert.instanceOf(groups[3].contextGroups[0], GrDiffGroup);
assert.equal(groups[3].contextGroups[0].lines.length, 90);
for (const l of groups[3].contextGroups[0].lines) {
assert.equal(
l.text, 'all work and no play make jill a dull girl');
}
@@ -323,26 +323,26 @@ suite('gr-diff-processor tests', () => {
}
assert.equal(groups[6].type, GrDiffGroup.Type.CONTEXT_CONTROL);
assert.equal(groups[6].lines[0].contextGroups.length, 2);
assert.equal(groups[6].contextGroups.length, 2);
assert.equal(groups[6].lines[0].contextGroups[0].lines.length, 4);
assert.equal(groups[6].lines[0].contextGroups[0].removes.length, 2);
assert.equal(groups[6].lines[0].contextGroups[0].adds.length, 2);
for (const l of groups[6].lines[0].contextGroups[0].removes) {
assert.equal(groups[6].contextGroups[0].lines.length, 4);
assert.equal(groups[6].contextGroups[0].removes.length, 2);
assert.equal(groups[6].contextGroups[0].adds.length, 2);
for (const l of groups[6].contextGroups[0].removes) {
assert.equal(
l.text, 'all work and no play make jill a dull girl');
}
for (const l of groups[6].lines[0].contextGroups[0].adds) {
for (const l of groups[6].contextGroups[0].adds) {
assert.equal(
l.text, ' all work and no play make jill a dull girl');
}
// The final chunk is completely hidden
assert.equal(
groups[6].lines[0].contextGroups[1].type,
groups[6].contextGroups[1].type,
GrDiffGroup.Type.BOTH);
assert.equal(groups[6].lines[0].contextGroups[1].lines.length, 3);
for (const l of groups[6].lines[0].contextGroups[1].lines) {
assert.equal(groups[6].contextGroups[1].lines.length, 3);
for (const l of groups[6].contextGroups[1].lines) {
assert.equal(
l.text, 'all work and no play make jill a dull girl');
}
@@ -372,9 +372,9 @@ suite('gr-diff-processor tests', () => {
}
assert.equal(groups[3].type, GrDiffGroup.Type.CONTEXT_CONTROL);
assert.instanceOf(groups[3].lines[0].contextGroups[0], GrDiffGroup);
assert.equal(groups[3].lines[0].contextGroups[0].lines.length, 80);
for (const l of groups[3].lines[0].contextGroups[0].lines) {
assert.instanceOf(groups[3].contextGroups[0], GrDiffGroup);
assert.equal(groups[3].contextGroups[0].lines.length, 80);
for (const l of groups[3].contextGroups[0].lines) {
assert.equal(
l.text, 'all work and no play make jill a dull girl');
}
@@ -680,11 +680,10 @@ suite('gr-diff-processor tests', () => {
// The first and last are uncollapsed context, whereas the middle has
// a single context-control line.
assert.equal(result.groups[0].lines.length, element.context);
assert.equal(result.groups[1].lines.length, 1);
assert.equal(result.groups[2].lines.length, element.context);
// The collapsed group has the hidden lines as its context group.
assert.equal(result.groups[1].lines[0].contextGroups[0].lines.length,
assert.equal(result.groups[1].contextGroups[0].lines.length,
expectedCollapseSize);
});
@@ -705,11 +704,10 @@ suite('gr-diff-processor tests', () => {
assert.equal(result.groups.length, 2, 'Results in two groups');
// Only the first group is collapsed.
assert.equal(result.groups[0].lines.length, 1);
assert.equal(result.groups[1].lines.length, element.context);
// The collapsed group has the hidden lines as its context group.
assert.equal(result.groups[0].lines[0].contextGroups[0].lines.length,
assert.equal(result.groups[0].contextGroups[0].lines.length,
expectedCollapseSize);
});
@@ -778,9 +776,8 @@ suite('gr-diff-processor tests', () => {
// 2) The context before the key location.
// The key location is not processed in this call to _processNext
assert.equal(result.groups.length, 2);
assert.equal(result.groups[0].lines.length, 1);
// The collapsed group has the hidden lines as its context group.
assert.equal(result.groups[0].lines[0].contextGroups[0].lines.length,
assert.equal(result.groups[0].contextGroups[0].lines.length,
rows.length - element.context);
assert.equal(result.groups[1].lines.length, element.context);
});
@@ -807,9 +804,8 @@ suite('gr-diff-processor tests', () => {
// key location.
assert.equal(result.groups.length, 2);
assert.equal(result.groups[0].lines.length, element.context);
assert.equal(result.groups[1].lines.length, 1);
// The collapsed group has the hidden lines as its context group.
assert.equal(result.groups[1].lines[0].contextGroups[0].lines.length,
assert.equal(result.groups[1].contextGroups[0].lines.length,
rows.length - element.context);
});
});

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

@@ -53,6 +53,8 @@ export function GrDiffGroup(type, opt_lines) {
this.adds = [];
/** @type {!Array<!GrDiffLine>} */
this.removes = [];
/** @type {?Array<Object>} ?Array<!GrDiffGroup> */
this.contextGroups = null;
/** Both start and end line are inclusive. */
this.lineRange = {
@@ -126,10 +128,9 @@ GrDiffGroup.hideInContextControl = function(groups, hiddenStart, hiddenEnd) {
const result = [...before];
if (hidden.length) {
const ctxLine = new GrDiffLine(GrDiffLine.Type.CONTEXT_CONTROL);
ctxLine.contextGroups = hidden;
const ctxGroup = new GrDiffGroup(
GrDiffGroup.Type.CONTEXT_CONTROL, [ctxLine]);
GrDiffGroup.Type.CONTEXT_CONTROL, []);
ctxGroup.contextGroups = hidden;
result.push(ctxGroup);
}
result.push(...after);

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

@@ -138,13 +138,8 @@ suite('gr-diff-group tests', () => {
assert.equal(collapsedGroups[0], groups[0]);
assert.equal(collapsedGroups[1].type, GrDiffGroup.Type.CONTEXT_CONTROL);
assert.equal(collapsedGroups[1].lines.length, 1);
assert.equal(
collapsedGroups[1].lines[0].type, GrDiffLine.Type.CONTEXT_CONTROL);
assert.equal(
collapsedGroups[1].lines[0].contextGroups.length, 1);
assert.equal(
collapsedGroups[1].lines[0].contextGroups[0], groups[1]);
assert.equal(collapsedGroups[1].contextGroups.length, 1);
assert.equal(collapsedGroups[1].contextGroups[0], groups[1]);
assert.equal(collapsedGroups[2], groups[2]);
});
@@ -159,27 +154,23 @@ suite('gr-diff-group tests', () => {
assert.deepEqual(collapsedGroups[1].removes, [groups[1].removes[0]]);
assert.equal(collapsedGroups[2].type, GrDiffGroup.Type.CONTEXT_CONTROL);
assert.equal(collapsedGroups[2].lines.length, 1);
assert.equal(
collapsedGroups[2].lines[0].type, GrDiffLine.Type.CONTEXT_CONTROL);
assert.equal(
collapsedGroups[2].lines[0].contextGroups.length, 2);
assert.equal(collapsedGroups[2].contextGroups.length, 2);
assert.equal(
collapsedGroups[2].lines[0].contextGroups[0].type,
collapsedGroups[2].contextGroups[0].type,
GrDiffGroup.Type.DELTA);
assert.deepEqual(
collapsedGroups[2].lines[0].contextGroups[0].adds,
collapsedGroups[2].contextGroups[0].adds,
groups[1].adds.slice(1));
assert.deepEqual(
collapsedGroups[2].lines[0].contextGroups[0].removes,
collapsedGroups[2].contextGroups[0].removes,
groups[1].removes.slice(1));
assert.equal(
collapsedGroups[2].lines[0].contextGroups[1].type,
collapsedGroups[2].contextGroups[1].type,
GrDiffGroup.Type.BOTH);
assert.deepEqual(
collapsedGroups[2].lines[0].contextGroups[1].lines,
collapsedGroups[2].contextGroups[1].lines,
[groups[2].lines[0]]);
assert.equal(collapsedGroups[3].type, GrDiffGroup.Type.BOTH);

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

@@ -36,9 +36,6 @@ export function GrDiffLine(type, opt_beforeLine, opt_afterLine) {
/** @type {!Array<GrDiffLine.Highlights>} */
this.highlights = [];
/** @type {?Array<Object>} ?Array<!GrDiffGroup> */
this.contextGroups = null;
this.text = '';
}
@@ -47,7 +44,6 @@ GrDiffLine.Type = {
ADD: 'add',
BOTH: 'both',
BLANK: 'blank',
CONTEXT_CONTROL: 'contextControl',
REMOVE: 'remove',
};