Refactor gr-diff-processor to use an asynchronous loop
Modifies the `process` method of gr-diff-processor to traverse diff
content using an asynchronous recursive loop rather than a blocking
for-loop. The async version maintains the promise interface already
established.
Refactored to constrain side-effects to the `process` method. Whereas,
formerly, helper methods in gr-diff-processor both read and wrote the
component's internal state, they are rewritten to be more pure, making
them simpler to understand, test, and invoke asynchronously.
Documentation is added throughout as well as tests for helper functions.
Note that this change only makes the processing step asynchronous.
Upgrading the diff-rendering stage to be non-blocking will come in a
later change.
Change-Id: Ifd50eeb75797b173937890caacc92cad5675fc20
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js
index 896b60e..b1256db 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js
@@ -140,7 +140,7 @@
return this._commentLocations[side][lineNum] === true;
};
- // TODO (wyatta): Move this completely into the processor.
+ // TODO(wyatta): Move this completely into the processor.
GrDiffBuilder.prototype._insertContextGroups = function(groups, lines,
hiddenRange) {
var linesBeforeCtx = lines.slice(0, hiddenRange[0]);
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.js b/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.js
index 0d787c1..0a54665 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor.js
@@ -58,94 +58,248 @@
type: Object,
value: function() { return {left: {}, right: {}}; },
},
-
- _content: Object,
},
+ /**
+ * Asynchronously process the diff object into groups. As it processes, it
+ * will splice groups into the `groups` property of the component.
+ * @return {Promise} A promise that resolves when the diff is completely
+ * processed.
+ */
process: function(content) {
return new Promise(function(resolve) {
- var groups = [];
- this._processContent(content, groups, this.context);
- this.groups = groups;
- resolve(groups);
+ this.groups = [];
+ this.push('groups', this._makeFileComments());
+
+ var state = {
+ lineNums: {left: 0, right: 0},
+ sectionIndex: 0,
+ };
+
+ content = this._splitCommonGroupsWithComments(content);
+
+ var nextStep = function() {
+ // If we are done, resolve the promise.
+ if (state.sectionIndex >= content.length) {
+ resolve(this.groups);
+ return;
+ }
+
+ // Process the next section and incorporate the result.
+ var result = this._processNext(state, content);
+ result.groups.forEach(function(group) {
+ this.push('groups', group);
+ }, this);
+ state.lineNums.left += result.lineDelta.left;
+ state.lineNums.right += result.lineDelta.right;
+
+ // Increment the index and recurse.
+ state.sectionIndex++;
+ this.async(nextStep, 1);
+ };
+
+ nextStep.call(this);
}.bind(this));
},
- _processContent: function(content, groups, context) {
- this._appendFileComments(groups);
+ /**
+ * Process the next section of the diff.
+ */
+ _processNext: function(state, content) {
+ var section = content[state.sectionIndex];
- context = content.length > 1 ? context : WHOLE_FILE;
-
- var lineNums = {
- left: 0,
- right: 0,
+ var rows = {
+ both: section[DiffGroupType.BOTH] || null,
+ added: section[DiffGroupType.ADDED] || null,
+ removed: section[DiffGroupType.REMOVED] || null,
};
- content = this._splitCommonGroupsWithComments(content, lineNums);
- for (var i = 0; i < content.length; i++) {
- var group = content[i];
- var lines = [];
- if (group[DiffGroupType.BOTH] !== undefined) {
- var rows = group[DiffGroupType.BOTH];
- this._appendCommonLines(rows, lines, lineNums);
+ var highlights = {
+ added: section[DiffHighlights.ADDED] || null,
+ removed: section[DiffHighlights.REMOVED] || null,
+ };
- var hiddenRange = [context, rows.length - context];
- if (i === 0) {
- hiddenRange[0] = 0;
- } else if (i === content.length - 1) {
- hiddenRange[1] = rows.length;
- }
-
- if (context !== WHOLE_FILE && hiddenRange[1] - hiddenRange[0] > 0) {
- this._insertContextGroups(groups, lines, hiddenRange);
- } else {
- groups.push(new GrDiffGroup(GrDiffGroup.Type.BOTH, lines));
- }
- continue;
+ if (rows.both) { // If it's a shared section.
+ var sectionEnd = null;
+ if (state.sectionIndex === 0) {
+ sectionEnd = 'first';
+ }
+ else if (state.sectionIndex === content.length - 1) {
+ sectionEnd = 'last';
}
- if (group[DiffGroupType.REMOVED] !== undefined) {
- var highlights = undefined;
- if (group[DiffHighlights.REMOVED] !== undefined) {
- highlights = this._normalizeIntralineHighlights(
- group[DiffGroupType.REMOVED],
- group[DiffHighlights.REMOVED]);
- }
- this._appendRemovedLines(group[DiffGroupType.REMOVED], lines,
- lineNums, highlights);
- }
+ var sharedGroups = this._sharedGroupsFromRows(
+ rows.both,
+ content.length > 1 ? this.context : WHOLE_FILE,
+ state.lineNums.left,
+ state.lineNums.right,
+ sectionEnd);
- if (group[DiffGroupType.ADDED] !== undefined) {
- var highlights = undefined;
- if (group[DiffHighlights.ADDED] !== undefined) {
- highlights = this._normalizeIntralineHighlights(
- group[DiffGroupType.ADDED],
- group[DiffHighlights.ADDED]);
- }
- this._appendAddedLines(group[DiffGroupType.ADDED], lines,
- lineNums, highlights);
- }
- groups.push(new GrDiffGroup(GrDiffGroup.Type.DELTA, lines));
+ return {
+ lineDelta: {
+ left: rows.both.length,
+ right: rows.both.length,
+ },
+ groups: sharedGroups,
+ };
+ } else { // Otherwise it's a delta section.
+
+ var deltaGroup = this._deltaGroupFromRows(
+ rows.added,
+ rows.removed,
+ state.lineNums.left,
+ state.lineNums.right,
+ highlights);
+
+ return {
+ lineDelta: {
+ left: rows.removed ? rows.removed.length : 0,
+ right: rows.added ? rows.added.length : 0,
+ },
+ groups: [deltaGroup],
+ };
}
},
- _appendFileComments: function(groups) {
+ /**
+ * Take rows of a shared diff section and produce an array of corresponding
+ * (potentially collapsed) groups.
+ * @param {Array<String>} rows
+ * @param {Number} context
+ * @param {Number} startLineNumLeft
+ * @param {Number} startLineNumRight
+ * @param {String} opt_sectionEnd String representing whether this is the
+ * first section or the last section or neither. Use the values 'first',
+ * 'last' and null respectively.
+ * @return {Array<GrDiffGroup>}
+ */
+ _sharedGroupsFromRows: function(rows, context, startLineNumLeft,
+ startLineNumRight, opt_sectionEnd) {
+ var result = [];
+ var lines = [];
+ var line;
+
+ // Map each row to a GrDiffLine.
+ for (var i = 0; i < rows.length; i++) {
+ line = new GrDiffLine(GrDiffLine.Type.BOTH);
+ line.text = rows[i];
+ line.beforeNumber = ++startLineNumLeft;
+ line.afterNumber = ++startLineNumRight;
+ lines.push(line);
+ }
+
+ // Find the hidden range based on the user's context preference. If this
+ // is the first or the last section of the diff, make sure the collapsed
+ // part of the section extends to the edge of the file.
+ var hiddenRange = [context, rows.length - context];
+ if (opt_sectionEnd === 'first') {
+ hiddenRange[0] = 0;
+ } else if (opt_sectionEnd === 'last') {
+ hiddenRange[1] = rows.length;
+ }
+
+ // If there is a range to hide.
+ if (context !== WHOLE_FILE && hiddenRange[1] - hiddenRange[0] > 0) {
+ var linesBeforeCtx = lines.slice(0, hiddenRange[0]);
+ var hiddenLines = lines.slice(hiddenRange[0], hiddenRange[1]);
+ var linesAfterCtx = lines.slice(hiddenRange[1]);
+
+ if (linesBeforeCtx.length > 0) {
+ result.push(new GrDiffGroup(GrDiffGroup.Type.BOTH, linesBeforeCtx));
+ }
+
+ var ctxLine = new GrDiffLine(GrDiffLine.Type.CONTEXT_CONTROL);
+ ctxLine.contextGroup =
+ new GrDiffGroup(GrDiffGroup.Type.BOTH, hiddenLines);
+ result.push(new GrDiffGroup(GrDiffGroup.Type.CONTEXT_CONTROL,
+ [ctxLine]));
+
+ if (linesAfterCtx.length > 0) {
+ result.push(new GrDiffGroup(GrDiffGroup.Type.BOTH, linesAfterCtx));
+ }
+ } else {
+ result.push(new GrDiffGroup(GrDiffGroup.Type.BOTH, lines));
+ }
+
+ return result;
+ },
+
+ /**
+ * Take the rows of a delta diff section and produce the corresponding
+ * group.
+ * @param {Array<String>} rowsAdded
+ * @param {Array<String>} rowsRemoved
+ * @param {Number} startLineNumLeft
+ * @param {Number} startLineNumRight
+ * @return {GrDiffGroup}
+ */
+ _deltaGroupFromRows: function(rowsAdded, rowsRemoved, startLineNumLeft,
+ startLineNumRight, highlights) {
+ var lines = [];
+ if (rowsRemoved) {
+ lines = lines.concat(this._deltaLinesFromRows(GrDiffLine.Type.REMOVE,
+ rowsRemoved, startLineNumLeft, highlights.removed));
+ }
+ if (rowsAdded) {
+ lines = lines.concat(this._deltaLinesFromRows(GrDiffLine.Type.ADD,
+ rowsAdded, startLineNumRight, highlights.added));
+ }
+ return new GrDiffGroup(GrDiffGroup.Type.DELTA, lines);
+ },
+
+ /**
+ * @return {Array<GrDiffLine>}
+ */
+ _deltaLinesFromRows: function(lineType, rows, startLineNum,
+ opt_highlights) {
+ // Normalize highlights if they have been passed.
+ if (opt_highlights) {
+ opt_highlights = this._normalizeIntralineHighlights(rows,
+ opt_highlights);
+ }
+
+ var lines = [];
+ var line;
+ for (var i = 0; i < rows.length; i++) {
+ line = new GrDiffLine(lineType);
+ line.text = rows[i];
+ if (lineType === GrDiffLine.Type.ADD) {
+ line.afterNumber = ++startLineNum;
+ } else {
+ line.beforeNumber = ++startLineNum;
+ }
+ if (opt_highlights) {
+ line.highlights = opt_highlights.filter(
+ function(hl) { return hl.contentIndex === i; });
+ }
+ lines.push(line);
+ }
+ return lines;
+ },
+
+ _makeFileComments: function() {
var line = new GrDiffLine(GrDiffLine.Type.BOTH);
line.beforeNumber = GrDiffLine.FILE;
line.afterNumber = GrDiffLine.FILE;
- groups.push(new GrDiffGroup(GrDiffGroup.Type.BOTH, [line]));
+ return new GrDiffGroup(GrDiffGroup.Type.BOTH, [line]);
},
/**
* In order to show comments out of the bounds of the selected context,
* treat them as separate chunks within the model so that the content (and
* context surrounding it) renders correctly.
+ * @param {Object} content The diff content object.
+ * @return {Object} A new diff content object with regions split up.
*/
- _splitCommonGroupsWithComments: function(content, lineNums) {
+ _splitCommonGroupsWithComments: function(content) {
var result = [];
- var leftLineNum = lineNums.left;
- var rightLineNum = lineNums.right;
+ var leftLineNum = 0;
+ var rightLineNum = 0;
+
+ // For each section in the diff.
for (var i = 0; i < content.length; i++) {
+
+ // If it isn't a common group, append it as-is and update line numbers.
if (!content[i].ab) {
result.push(content[i]);
if (content[i].a) {
@@ -156,61 +310,43 @@
}
continue;
}
+
var chunk = content[i].ab;
var currentChunk = {ab: []};
+
+ // For each line in the common group.
for (var j = 0; j < chunk.length; j++) {
leftLineNum++;
rightLineNum++;
+ // If this line should not be collapsed.
if (this.keyLocations[DiffSide.LEFT][leftLineNum] ||
this.keyLocations[DiffSide.RIGHT][rightLineNum]) {
+
+ // If any lines have been accumulated into the chunk leading up to
+ // this non-collapse line, then add them as a chunk and start a new
+ // one.
if (currentChunk.ab && currentChunk.ab.length > 0) {
result.push(currentChunk);
currentChunk = {ab: []};
}
+
+ // Add the non-collapse line as its own chunk.
result.push({ab: [chunk[j]]});
} else {
+ // Append the current line to the current chunk.
currentChunk.ab.push(chunk[j]);
}
}
- // != instead of !== because we want to cover both undefined and null.
- if (currentChunk.ab != null && currentChunk.ab.length > 0) {
+
+ if (currentChunk.ab && currentChunk.ab.length > 0) {
result.push(currentChunk);
}
}
+
return result;
},
- _appendCommonLines: function(rows, lines, lineNums) {
- for (var i = 0; i < rows.length; i++) {
- var line = new GrDiffLine(GrDiffLine.Type.BOTH);
- line.text = rows[i];
- line.beforeNumber = ++lineNums.left;
- line.afterNumber = ++lineNums.right;
- lines.push(line);
- }
- },
-
- _insertContextGroups: function(groups, lines, hiddenRange) {
- var linesBeforeCtx = lines.slice(0, hiddenRange[0]);
- var hiddenLines = lines.slice(hiddenRange[0], hiddenRange[1]);
- var linesAfterCtx = lines.slice(hiddenRange[1]);
-
- if (linesBeforeCtx.length > 0) {
- groups.push(new GrDiffGroup(GrDiffGroup.Type.BOTH, linesBeforeCtx));
- }
-
- var ctxLine = new GrDiffLine(GrDiffLine.Type.CONTEXT_CONTROL);
- ctxLine.contextGroup =
- new GrDiffGroup(GrDiffGroup.Type.BOTH, hiddenLines);
- groups.push(new GrDiffGroup(GrDiffGroup.Type.CONTEXT_CONTROL,
- [ctxLine]));
-
- if (linesAfterCtx.length > 0) {
- groups.push(new GrDiffGroup(GrDiffGroup.Type.BOTH, linesAfterCtx));
- }
- },
-
/**
* The `highlights` array consists of a list of <skip length, mark length>
* pairs, where the skip length is the number of characters between the
@@ -271,33 +407,5 @@
}
return normalized;
},
-
- _appendRemovedLines: function(rows, lines, lineNums, opt_highlights) {
- for (var i = 0; i < rows.length; i++) {
- var line = new GrDiffLine(GrDiffLine.Type.REMOVE);
- line.text = rows[i];
- line.beforeNumber = ++lineNums.left;
- if (opt_highlights) {
- line.highlights = opt_highlights.filter(function(hl) {
- return hl.contentIndex === i;
- });
- }
- lines.push(line);
- }
- },
-
- _appendAddedLines: function(rows, lines, lineNums, opt_highlights) {
- for (var i = 0; i < rows.length; i++) {
- var line = new GrDiffLine(GrDiffLine.Type.ADD);
- line.text = rows[i];
- line.afterNumber = ++lineNums.right;
- if (opt_highlights) {
- line.highlights = opt_highlights.filter(function(hl) {
- return hl.contentIndex === i;
- });
- }
- lines.push(line);
- }
- },
});
})();
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor_test.html b/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor_test.html
index 6a24d6a..120c980 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-processor/gr-diff-processor_test.html
@@ -32,6 +32,13 @@
<script>
suite('gr-diff-processor tests', function() {
+ var WHOLE_FILE = -1;
+ var loremIpsum = 'Lorem ipsum dolor sit amet, ei nonumes vituperata ius. ' +
+ 'Duo animal omnesque fabellas et. Id has phaedrum dignissim ' +
+ 'deterruisset, pro ei petentium comprehensam, ut vis solum dicta. ' +
+ 'Eos cu aliquam labores qualisque, usu postea inermis te, et solum ' +
+ 'fugit assum per.';
+
var element;
suite('not logged in', function() {
@@ -401,6 +408,108 @@
}
]);
});
+
+ suite('gr-diff-processor helpers', function() {
+ var rows;
+
+ setup(function() {
+ rows = loremIpsum.split(' ');
+ });
+
+ test('_sharedGroupsFromRows WHOLE_FILE', function() {
+ var context = WHOLE_FILE;
+ var lineNumbers = {left: 10, right: 100};
+ var result = element._sharedGroupsFromRows(
+ rows, context, lineNumbers.left, lineNumbers.right, null);
+
+ // Results in one, uncollapsed group with all rows.
+ assert.equal(result.length, 1);
+ assert.equal(result[0].type, GrDiffGroup.Type.BOTH);
+ assert.equal(result[0].lines.length, rows.length);
+
+ // Line numbers are set correctly.
+ assert.equal(result[0].lines[0].beforeNumber, lineNumbers.left + 1);
+ assert.equal(result[0].lines[0].afterNumber, lineNumbers.right + 1);
+
+ assert.equal(result[0].lines[rows.length - 1].beforeNumber,
+ lineNumbers.left + rows.length);
+ assert.equal(result[0].lines[rows.length - 1].afterNumber,
+ lineNumbers.right + rows.length);
+ });
+
+ test('_sharedGroupsFromRows context', function() {
+ var context = 10;
+ var result = element._sharedGroupsFromRows(
+ rows, context, 10, 100, null);
+ var expectedCollapseSize = rows.length - 2 * context;
+
+ assert.equal(result.length, 3, 'Results in three groups');
+
+ // The first and last are uncollapsed context, whereas the middle has
+ // a single context-control line.
+ assert.equal(result[0].lines.length, context);
+ assert.equal(result[1].lines.length, 1);
+ assert.equal(result[2].lines.length, context);
+
+ // The collapsed group has the hidden lines as its context group.
+ assert.equal(result[1].lines[0].contextGroup.lines.length,
+ expectedCollapseSize);
+ });
+
+ test('_sharedGroupsFromRows first', function() {
+ var context = 10;
+ var result = element._sharedGroupsFromRows(
+ rows, context, 10, 100, 'first');
+ var expectedCollapseSize = rows.length - context;
+
+ assert.equal(result.length, 2, 'Results in two groups');
+
+ // Only the first group is collapsed.
+ assert.equal(result[0].lines.length, 1);
+ assert.equal(result[1].lines.length, context);
+
+ // The collapsed group has the hidden lines as its context group.
+ assert.equal(result[0].lines[0].contextGroup.lines.length,
+ expectedCollapseSize);
+ });
+
+ test('_sharedGroupsFromRows few-rows', function() {
+ // Only ten rows.
+ rows = rows.slice(0, 10);
+ var context = 10;
+ var result = element._sharedGroupsFromRows(
+ rows, context, 10, 100, 'first');
+
+ // Results in one uncollapsed group with all rows.
+ assert.equal(result.length, 1, 'Results in one group');
+ assert.equal(result[0].lines.length, rows.length);
+ });
+
+ test('_deltaLinesFromRows', function() {
+ var startLineNum = 10;
+ var result = element._deltaLinesFromRows(GrDiffLine.Type.ADD, rows,
+ startLineNum);
+
+ assert.equal(result.length, rows.length);
+ assert.equal(result[0].type, GrDiffLine.Type.ADD);
+ assert.equal(result[0].afterNumber, startLineNum + 1);
+ assert.notOk(result[0].beforeNumber);
+ assert.equal(result[result.length - 1].afterNumber,
+ startLineNum + rows.length);
+ assert.notOk(result[result.length - 1].beforeNumber);
+
+ result = element._deltaLinesFromRows(GrDiffLine.Type.REMOVE, rows,
+ startLineNum);
+
+ assert.equal(result.length, rows.length);
+ assert.equal(result[0].type, GrDiffLine.Type.REMOVE);
+ assert.equal(result[0].beforeNumber, startLineNum + 1);
+ assert.notOk(result[0].afterNumber);
+ assert.equal(result[result.length - 1].beforeNumber,
+ startLineNum + rows.length);
+ assert.notOk(result[result.length - 1].afterNumber);
+ });
+ });
});
});
</script>
