opendev/gerrit
Code Issues Proposed changes

Lift gr-syntax-layer into gr-diff-host

Browse Source 
This change decouples gr-diff-builder and gr-syntax-layer. gr-syntax-
layer will now live in gr-diff-host.
As a result, from the perspective of gr-diff/gr-diff-builder, plugin
layers and the syntax layer will now be part of the same list.

A future potential refactor could also lift gr-coverage-layer instances
in a similar fashion.

Change-Id: Ic10b45d70a59c2e18d3b408880f66251ab93e06e
This commit is contained in:
Mayank Kumar
2019-10-03 18:04:08 -07:00
parent 83155c13dc
commit 82b0c21832
9 changed files with 154 additions and 138 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -20,7 +20,6 @@ limitations under the License.
<link rel="import" href="../gr-coverage-layer/gr-coverage-layer.html">
<link rel="import" href="../gr-diff-processor/gr-diff-processor.html">
<link rel="import" href="../gr-ranged-comment-layer/gr-ranged-comment-layer.html">
<link rel="import" href="../gr-syntax-layer/gr-syntax-layer.html">
<dom-module id="gr-diff-builder">
<template>
@@ -30,9 +29,6 @@ limitations under the License.
<gr-ranged-comment-layer
id="rangeLayer"
comment-ranges="[[commentRanges]]"></gr-ranged-comment-layer>
<gr-syntax-layer
id="syntaxLayer"
diff="[[diff]]"></gr-syntax-layer>
<gr-coverage-layer
id="coverageLayerLeft"
coverage-ranges="[[_leftCoverageRanges]]"
@@ -64,13 +60,6 @@ limitations under the License.
UNIFIED: 'UNIFIED_DIFF',
};
// If any line of the diff is more than the character limit, then disable
// syntax highlighting for the entire file.
const SYNTAX_MAX_LINE_LENGTH = 500;
// Disable syntax highlighting if the overall diff is too large.
const SYNTAX_MAX_DIFF_LENGTH = 20000;
const TRAILING_WHITESPACE_PATTERN = /\s+$/;
Polymer({
@@ -84,18 +73,11 @@ limitations under the License.
*/
/**
* Fired when the diff finishes rendering text content and starts
* syntax highlighting.
* Fired when the diff finishes rendering text content.
*
* @event render-content
*/
/**
* Fired when the diff finishes syntax highlighting.
*
* @event render-syntax
*/
properties: {
diff: Object,
diffPath: String,
@@ -138,7 +120,7 @@ limitations under the License.
* @type {?Object}
*/
_cancelableRenderPromise: Object,
pluginLayers: {
layers: {
type: Array,
value: [],
},
@@ -171,11 +153,10 @@ limitations under the License.
// attached before plugins are installed.
this._setupAnnotationLayers();
this.$.syntaxLayer.enabled = prefs.syntax_highlighting;
this._showTabs = !!prefs.show_tabs;
this._showTrailingWhitespace = !!prefs.show_whitespace_errors;
// Stop the processor and syntax layer (if they're running).
// Stop the processor if it's running.
this.cancel();
this._builder = this._getDiffBuilder(this.diff, prefs);
@@ -198,16 +179,6 @@ limitations under the License.
}
this.dispatchEvent(new CustomEvent('render-content',
{bubbles: true, composed: true}));
if (this._diffTooLargeForSyntax()) {
this.$.syntaxLayer.enabled = false;
}
return this.$.syntaxLayer.process();
})
.then(() => {
this.dispatchEvent(new CustomEvent(
'render-syntax', {bubbles: true, composed: true}));
}));
return this._cancelableRenderPromise
.finally(() => { this._cancelableRenderPromise = null; })
@@ -220,7 +191,6 @@ limitations under the License.
_setupAnnotationLayers() {
const layers = [
this._createTrailingWhitespaceLayer(),
this.$.syntaxLayer,
this._createIntralineLayer(),
this._createTabIndicatorLayer(),
this.$.rangeLayer,
@@ -228,8 +198,8 @@ limitations under the License.
this.$.coverageLayerRight,
];
if (this.pluginLayers) {
layers.push(...this.pluginLayers);
if (this.layers) {
layers.push(...this.layers);
}
this._layers = layers;
},
@@ -307,7 +277,6 @@ limitations under the License.
cancel() {
this.$.processor.cancel();
this.$.syntaxLayer.cancel();
if (this._cancelableRenderPromise) {
this._cancelableRenderPromise.cancel();
this._cancelableRenderPromise = null;
@@ -446,44 +415,10 @@ limitations under the License.
};
},
/**
* @return {boolean} whether any of the lines in _groups are longer
* than SYNTAX_MAX_LINE_LENGTH.
*/
_anyLineTooLong() {
return this._groups.reduce((acc, group) => {
return acc || group.lines.reduce((acc, line) => {
return acc || line.text.length >= SYNTAX_MAX_LINE_LENGTH;
}, false);
}, false);
},
_diffTooLargeForSyntax() {
return this._anyLineTooLong() ||
this.getDiffLength() > SYNTAX_MAX_DIFF_LENGTH;
},
setBlame(blame) {
if (!this._builder || !blame) { return; }
this._builder.setBlame(blame);
},
/**
* Get the approximate length of the diff as the sum of the maximum
* length of the chunks.
* @return {number}
*/
getDiffLength() {
return this.diff.content.reduce((sum, sec) => {
if (sec.hasOwnProperty('ab')) {
return sum + sec.ab.length;
} else {
return sum + Math.max(
sec.hasOwnProperty('a') ? sec.a.length : 0,
sec.hasOwnProperty('b') ? sec.b.length : 0);
}
}, 0);
},
});
})();
</script>

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

@@ -590,38 +590,38 @@ limitations under the License.
});
});
suite('layers from plugins', () => {
suite('layers', () => {
let element;
let initialLayersCount;
let withPluginLayerCount;
let withLayerCount;
setup(() => {
const pluginLayers = [];
const layers = [];
element = fixture('basic');
element.pluginLayers = pluginLayers;
element.layers = layers;
element._showTrailingWhitespace = true;
element._setupAnnotationLayers();
initialLayersCount = element._layers.length;
});
test('no plugin layers', () => {
test('no layers', () => {
element._setupAnnotationLayers();
assert.equal(element._layers.length, initialLayersCount);
});
suite('with plugin layers', () => {
const pluginLayers = [{}, {}];
suite('with layers', () => {
const layers = [{}, {}];
setup(() => {
element = fixture('basic');
element.pluginLayers = pluginLayers;
element.layers = layers;
element._showTrailingWhitespace = true;
element._setupAnnotationLayers();
withPluginLayerCount = element._layers.length;
withLayerCount = element._layers.length;
});
test('with plugin layers', () => {
test('with layers', () => {
element._setupAnnotationLayers();
assert.equal(element._layers.length, withPluginLayerCount);
assert.equal(initialLayersCount + pluginLayers.length,
withPluginLayerCount);
assert.equal(element._layers.length, withLayerCount);
assert.equal(initialLayersCount + layers.length,
withLayerCount);
});
});
});
@@ -733,7 +733,6 @@ limitations under the License.
element.viewMode = 'SIDE_BY_SIDE';
processStub = sandbox.stub(element.$.processor, 'process')
.returns(Promise.resolve());
sandbox.stub(element, '_anyLineTooLong').returns(true);
keyLocations = {left: {}, right: {}};
prefs = {
line_length: 10,
@@ -862,37 +861,14 @@ limitations under the License.
.map(c => { return c.args[0].type; });
assert.include(firedEventTypes, 'render-start');
assert.include(firedEventTypes, 'render-content');
assert.include(firedEventTypes, 'render-syntax');
done();
});
});
test('rendering normal-sized diff does not disable syntax', () => {
assert.isTrue(element.$.syntaxLayer.enabled);
});
test('rendering large diff disables syntax', done => {
// Before it renders, set the first diff line to 500 '*' characters.
element.diff.content[0].a = [new Array(501).join('*')];
const prefs = {
line_length: 10,
show_tabs: true,
tab_size: 4,
context: -1,
syntax_highlighting: true,
};
element.render(keyLocations, prefs).then(() => {
assert.isFalse(element.$.syntaxLayer.enabled);
done();
});
});
test('cancel', () => {
const processorCancelStub = sandbox.stub(element.$.processor, 'cancel');
const syntaxCancelStub = sandbox.stub(element.$.syntaxLayer, 'cancel');
element.cancel();
assert.isTrue(processorCancelStub.called);
assert.isTrue(syntaxCancelStub.called);
});
});
@@ -921,10 +897,6 @@ limitations under the License.
});
});
test('getDiffLength', () => {
assert.equal(element.getDiffLength(diff), 52);
});
test('getContentByLine', () => {
let actual;

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

@@ -23,6 +23,7 @@ limitations under the License.
<link rel="import" href="../../shared/gr-comment-thread/gr-comment-thread.html">
<link rel="import" href="../../shared/gr-js-api-interface/gr-js-api-interface.html">
<link rel="import" href="../gr-diff/gr-diff.html">
<link rel="import" href="../gr-syntax-layer/gr-syntax-layer.html">
<dom-module id="gr-diff-host">
<template>
@@ -49,8 +50,13 @@ limitations under the License.
revision-image=[[_revisionImage]]
coverage-ranges="[[_coverageRanges]]"
blame="[[_blame]]"
plugin-layers="[[pluginLayers]]"
diff="[[_diff]]"></gr-diff>
layers="[[_layers]]"
diff="[[_diff]]">
</gr-diff>
<gr-syntax-layer
id="syntaxLayer"
enabled="[[_syntaxHighlightingEnabled]]"
diff="[[_diff]]"></gr-syntax-layer>
<gr-js-api-interface id="jsAPI"></gr-js-api-interface>
<gr-rest-api-interface id="restAPI"></gr-rest-api-interface>
<gr-reporting id="reporting" category="diff"></gr-reporting>

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

@@ -35,6 +35,13 @@
SYNTAX: 'Diff Syntax Render',
};
// Disable syntax highlighting if the overall diff is too large.
const SYNTAX_MAX_DIFF_LENGTH = 20000;
// If any line of the diff is more than the character limit, then disable
// syntax highlighting for the entire file.
const SYNTAX_MAX_LINE_LENGTH = 500;
const WHITESPACE_IGNORE_NONE = 'IGNORE_NONE';
/**
@@ -205,7 +212,13 @@
computed: '_computeParentIndex(patchRange.*)',
},
pluginLayers: {
_syntaxHighlightingEnabled: {
type: Boolean,
computed:
'_isSyntaxHighlightingEnabled(prefs.syntax_highlighting, _diff)',
},
_layers: {
type: Array,
value: [],
},
@@ -230,7 +243,6 @@
'render-start': '_handleRenderStart',
'render-content': '_handleRenderContent',
'render-syntax': '_handleRenderSyntax',
'normalize-range': '_handleNormalizeRange',
},
@@ -258,13 +270,13 @@
this._errorMessage = null;
const whitespaceLevel = this._getIgnoreWhitespace();
const pluginLayers = [];
const layers = [this.$.syntaxLayer];
// Get layers from plugins (if any).
for (const pluginLayer of this.$.jsAPI.getDiffLayers(
this.diffPath, this.changeNum, this.patchNum)) {
pluginLayers.push(pluginLayer);
layers.push(pluginLayer);
}
this.push('pluginLayers', ...pluginLayers);
this._layers = layers;
this._coverageRanges = [];
const {changeNum, path, patchRange: {basePatchNum, patchNum}} = this;
@@ -851,6 +863,25 @@
item => item.__draftID === comment.__draftID);
},
_isSyntaxHighlightingEnabled(preference, diff) {
if (!preference) return false;
return !this._anyLineTooLong(diff) &&
this.$.diff.getDiffLength(diff) <= SYNTAX_MAX_DIFF_LENGTH;
},
/**
* @return {boolean} whether any of the lines in diff are longer
* than SYNTAX_MAX_LINE_LENGTH.
*/
_anyLineTooLong(diff) {
return diff.content.some(section => {
const lines = section.ab ?
section.ab :
(section.a || []).concat(section.b || []);
return lines.some(line => line.length >= SYNTAX_MAX_LINE_LENGTH);
});
},
_handleRenderStart() {
this.$.reporting.time(TimingLabel.TOTAL);
this.$.reporting.time(TimingLabel.CONTENT);
@@ -859,11 +890,10 @@
_handleRenderContent() {
this.$.reporting.timeEnd(TimingLabel.CONTENT);
this.$.reporting.time(TimingLabel.SYNTAX);
},
_handleRenderSyntax() {
this.$.reporting.timeEnd(TimingLabel.SYNTAX);
this.$.reporting.timeEnd(TimingLabel.TOTAL);
this.$.syntaxLayer.process().then(() => {
this.$.reporting.timeEnd(TimingLabel.SYNTAX);
this.$.reporting.timeEnd(TimingLabel.TOTAL);
});
},
_handleNormalizeRange(event) {

66
polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.html
View File

@@ -60,7 +60,7 @@ limitations under the License.
suite('plugin layers', () => {
const pluginLayers = [{}, {}];
const pluginLayers = [{annotate: () => {}}, {annotate: () => {}}];
setup(() => {
stub('gr-js-api-interface', {
getDiffLayers() { return pluginLayers; },
@@ -303,6 +303,7 @@ limitations under the License.
});
test('ends content and starts syntax timer on render-content', done => {
element._diff = {content: []};
element.dispatchEvent(
new CustomEvent('render-content', {bubbles: true, composed: true}));
assert.isTrue(element.$.reporting.time.calledWithExactly(
@@ -312,14 +313,18 @@ limitations under the License.
done();
});
test('ends total and syntax timer on render-syntax', done => {
test('ends total and syntax timer after syntax layer processing', done => {
const processed = Promise.resolve();
sandbox.stub(element.$.syntaxLayer, 'process').returns(processed);
element.dispatchEvent(
new CustomEvent('render-syntax', {bubbles: true, composed: true}));
assert.isTrue(element.$.reporting.timeEnd.calledWithExactly(
'Diff Total Render'));
assert.isTrue(element.$.reporting.timeEnd.calledWithExactly(
'Diff Syntax Render'));
done();
new CustomEvent('render-content', {bubbles: true, composed: true}));
processed.then(() => {
assert.isTrue(element.$.reporting.timeEnd.calledWithExactly(
'Diff Total Render'));
assert.isTrue(element.$.reporting.timeEnd.calledWithExactly(
'Diff Syntax Render'));
done();
});
});
});
@@ -1285,5 +1290,50 @@ limitations under the License.
assert.deepEqual(element._filterThreadElsForLocation(threadEls, line,
Gerrit.DiffSide.RIGHT), [r]);
});
suite('syntax layer', () => {
setup(() => {
const prefs = {
line_length: 10,
show_tabs: true,
tab_size: 4,
context: -1,
syntax_highlighting: true,
};
element.prefs = prefs;
});
test('gr-diff-host provides syntax highlighting layer to gr-diff', () => {
element.patchRange = {};
element.reload();
assert.equal(element.$.diff.layers[0], element.$.syntaxLayer);
});
test('rendering normal-sized diff does not disable syntax', () => {
element._diff = {
content: [{
a: ['foo'],
}],
};
assert.isTrue(element.$.syntaxLayer.enabled);
});
test('rendering large diff disables syntax', () => {
// Before it renders, set the first diff line to 500 '*' characters.
element._diff = {
content: [{
a: [new Array(501).join('*')],
}],
};
assert.isFalse(element.$.syntaxLayer.enabled);
});
test('starts syntax layer processing on render-content event', () => {
sandbox.stub(element.$.syntaxLayer, 'process').returns(Promise.resolve());
element.dispatchEvent(
new CustomEvent('render-content', {bubbles: true, composed: true}));
assert.isTrue(element.$.syntaxLayer.process.called);
});
});
});
</script>

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

@@ -378,7 +378,7 @@ limitations under the License.
line-wrapping="[[lineWrapping]]"
is-image-diff="[[isImageDiff]]"
base-image="[[baseImage]]"
plugin-layers="[[pluginLayers]]"
layers="[[layers]]"
revision-image="[[revisionImage]]">
<table
id="diffTable"

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

@@ -271,7 +271,7 @@
/** Set by Polymer. */
isAttached: Boolean,
pluginLayers: Array,
layers: Array,
},
behaviors: [
@@ -724,7 +724,7 @@
_diffChanged(newValue) {
if (newValue) {
this._diffLength = this.$.diffBuilder.getDiffLength();
this._diffLength = this.getDiffLength(newValue);
this._debounceRenderDiffTable();
}
},
@@ -955,5 +955,23 @@
if (loading || !warning) { return 'newlineWarning hidden'; }
return 'newlineWarning';
},
/**
* Get the approximate length of the diff as the sum of the maximum
* length of the chunks.
* @param {Object} diff object
* @return {number}
*/
getDiffLength(diff) {
return diff.content.reduce((sum, sec) => {
if (sec.hasOwnProperty('ab')) {
return sum + sec.ab.length;
} else {
return sum + Math.max(
sec.hasOwnProperty('a') ? sec.a.length : 0,
sec.hasOwnProperty('b') ? sec.b.length : 0);
}
}, 0);
},
});
})();

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

@@ -767,7 +767,7 @@ limitations under the License.
new CustomEvent('render', {bubbles: true, composed: true}));
});
const mock = document.createElement('mock-diff-response');
sandbox.stub(element.$.diffBuilder, 'getDiffLength').returns(10000);
sandbox.stub(element, 'getDiffLength').returns(10000);
element.diff = mock.diffResponse;
element.noRenderOnPrefsChange = true;
});
@@ -1101,6 +1101,11 @@ limitations under the License.
));
});
});
test('getDiffLength', () => {
const diff = document.createElement('mock-diff-response').diffResponse;
assert.equal(element.getDiffLength(diff), 52);
});
});
a11ySuite('basic');

6
polygerrit-ui/app/elements/diff/gr-syntax-layer/gr-syntax-layer.js
View File

@@ -225,7 +225,7 @@
process() {
// Cancel any still running process() calls, because they append to the
// same _baseRanges and _revisionRanges fields.
this.cancel();
this._cancel();
// Discard existing ranges.
this._baseRanges = [];
@@ -295,7 +295,7 @@
/**
* Cancel any asynchronous syntax processing jobs.
*/
cancel() {
_cancel() {
if (this._processHandle != null) {
this.cancelAsync(this._processHandle);
this._processHandle = null;
@@ -306,7 +306,7 @@
},
_diffChanged() {
this.cancel();
this._cancel();
this._baseRanges = [];
this._revisionRanges = [];
},