diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html
index f29a5da022..780301b525 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html
@@ -20,7 +20,6 @@ limitations under the License.
-
@@ -30,9 +29,6 @@ limitations under the License.
-
{
- 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);
- },
});
})();
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html
index 6831a8df12..45de81088a 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html
@@ -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;
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.html b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.html
index b70764b8dd..7d60f134bd 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.html
@@ -23,6 +23,7 @@ limitations under the License.
+
@@ -49,8 +50,13 @@ limitations under the License.
revision-image=[[_revisionImage]]
coverage-ranges="[[_coverageRanges]]"
blame="[[_blame]]"
- plugin-layers="[[pluginLayers]]"
- diff="[[diff]]">
+ layers="[[_layers]]"
+ diff="[[diff]]">
+
+
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js
index e2dec69079..a538dcfc75 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js
@@ -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';
/**
@@ -210,7 +217,13 @@
computed: '_computeParentIndex(patchRange.*)',
},
- pluginLayers: {
+ _syntaxHighlightingEnabled: {
+ type: Boolean,
+ computed:
+ '_isSyntaxHighlightingEnabled(prefs.syntax_highlighting, _diff)',
+ },
+
+ _layers: {
type: Array,
value: [],
},
@@ -235,7 +248,6 @@
'render-start': '_handleRenderStart',
'render-content': '_handleRenderContent',
- 'render-syntax': '_handleRenderSyntax',
'normalize-range': '_handleNormalizeRange',
},
@@ -263,13 +275,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;
@@ -312,8 +324,20 @@
}
this.filesWeblinks = this._getFilesWeblinks(diff);
return new Promise(resolve => {
- const callback = () => {
- resolve();
+ const callback = event => {
+ const needsSyntaxHighlighting = event.detail
+ && event.detail.contentRendered;
+ if (needsSyntaxHighlighting) {
+ this.$.reporting.time(TimingLabel.SYNTAX);
+ this.$.syntaxLayer.process().then(() => {
+ this.$.reporting.timeEnd(TimingLabel.SYNTAX);
+ this.$.reporting.timeEnd(TimingLabel.TOTAL);
+ resolve();
+ });
+ } else {
+ this.$.reporting.timeEnd(TimingLabel.TOTAL);
+ resolve();
+ }
this.removeEventListener('render', callback);
};
this.addEventListener('render', callback);
@@ -856,6 +880,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);
@@ -863,12 +906,6 @@
_handleRenderContent() {
this.$.reporting.timeEnd(TimingLabel.CONTENT);
- this.$.reporting.time(TimingLabel.SYNTAX);
- },
-
- _handleRenderSyntax() {
- this.$.reporting.timeEnd(TimingLabel.SYNTAX);
- this.$.reporting.timeEnd(TimingLabel.TOTAL);
},
_handleNormalizeRange(event) {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.html b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.html
index f87ef7af22..58edfdbfce 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.html
@@ -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; },
@@ -302,24 +302,81 @@ limitations under the License.
done();
});
- test('ends content and starts syntax timer on render-content', done => {
+ test('ends content timer on render-content', () => {
element.dispatchEvent(
new CustomEvent('render-content', {bubbles: true, composed: true}));
- assert.isTrue(element.$.reporting.time.calledWithExactly(
- 'Diff Syntax Render'));
assert.isTrue(element.$.reporting.timeEnd.calledWithExactly(
'Diff Content Render'));
- done();
});
- test('ends total and syntax timer on render-syntax', done => {
- 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();
+ test('ends total and syntax timer after syntax layer processing', done => {
+ let notifySyntaxProcessed;
+ sandbox.stub(element.$.syntaxLayer, 'process').returns(new Promise(
+ resolve => {
+ notifySyntaxProcessed = resolve;
+ }));
+ sandbox.stub(element.$.restAPI, 'getDiff').returns(
+ Promise.resolve({content: []}));
+ element.patchRange = {};
+ element.$.restAPI.getDiffPreferences().then(prefs => {
+ element.prefs = prefs;
+ return element.reload();
+ });
+ // Multiple cascading microtasks are scheduled.
+ setTimeout(() => {
+ notifySyntaxProcessed();
+ // Assert after the notification task is processed.
+ Promise.resolve().then(() => {
+ assert.isTrue(element.$.reporting.timeEnd.calledThrice);
+ assert.isTrue(element.$.reporting.timeEnd.calledWithExactly(
+ 'Diff Total Render'));
+ assert.isTrue(element.$.reporting.timeEnd.calledWithExactly(
+ 'Diff Syntax Render'));
+ done();
+ });
+ });
+ });
+
+ test('ends total timer w/ no syntax layer processing', done => {
+ sandbox.stub(element.$.restAPI, 'getDiff').returns(
+ Promise.resolve({content: []}));
+ element.patchRange = {};
+ element.reload();
+ // Multiple cascading microtasks are scheduled.
+ setTimeout(() => {
+ assert.isTrue(element.$.reporting.timeEnd.calledOnce);
+ assert.isTrue(element.$.reporting.timeEnd.calledWithExactly(
+ 'Diff Total Render'));
+ done();
+ });
+ });
+
+ test('completes reload promise after syntax layer processing', done => {
+ let notifySyntaxProcessed;
+ sandbox.stub(element.$.syntaxLayer, 'process').returns(new Promise(
+ resolve => {
+ notifySyntaxProcessed = resolve;
+ }));
+ sandbox.stub(element.$.restAPI, 'getDiff').returns(
+ Promise.resolve({content: []}));
+ element.patchRange = {};
+ let reloadComplete = false;
+ element.$.restAPI.getDiffPreferences().then(prefs => {
+ element.prefs = prefs;
+ return element.reload();
+ }).then(() => {
+ reloadComplete = true;
+ });
+ // Multiple cascading microtasks are scheduled.
+ setTimeout(() => {
+ assert.isFalse(reloadComplete);
+ notifySyntaxProcessed();
+ // Assert after the notification task is processed.
+ setTimeout(() => {
+ assert.isTrue(reloadComplete);
+ done();
+ });
+ });
});
});
@@ -1285,5 +1342,57 @@ 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 event', done => {
+ sandbox.stub(element.$.syntaxLayer, 'process').returns(Promise.resolve());
+ sandbox.stub(element.$.restAPI, 'getDiff').returns(
+ Promise.resolve({content: []}));
+ element.patchRange = {};
+ element.reload();
+ setTimeout(() => {
+ element.dispatchEvent(
+ new CustomEvent('render', {bubbles: true, composed: true}));
+ assert.isTrue(element.$.syntaxLayer.process.called);
+ done();
+ });
+ });
+ });
});
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
index 32fa7e52ed..374368cee2 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
@@ -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]]">
{
this.dispatchEvent(
- new CustomEvent('render', {bubbles: true, composed: true}));
+ new CustomEvent('render', {
+ bubbles: true,
+ composed: true,
+ detail: {contentRendered: true},
+ }));
});
},
@@ -955,5 +959,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);
+ },
});
})();
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
index 15deaff63a..5366664f6a 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
@@ -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,23 @@ limitations under the License.
));
});
});
+
+ test('getDiffLength', () => {
+ const diff = document.createElement('mock-diff-response').diffResponse;
+ assert.equal(element.getDiffLength(diff), 52);
+ });
+
+ test('`render` event has contentRendered field in detail', done => {
+ element = fixture('basic');
+ element.prefs = {};
+ renderStub = sandbox.stub(element.$.diffBuilder, 'render')
+ .returns(Promise.resolve());
+ element.addEventListener('render', event => {
+ assert.isTrue(event.detail.contentRendered);
+ done();
+ });
+ element._renderDiffTable();
+ });
});
a11ySuite('basic');
diff --git a/polygerrit-ui/app/elements/diff/gr-syntax-layer/gr-syntax-layer.js b/polygerrit-ui/app/elements/diff/gr-syntax-layer/gr-syntax-layer.js
index 627a279e69..fcaa696c6a 100644
--- a/polygerrit-ui/app/elements/diff/gr-syntax-layer/gr-syntax-layer.js
+++ b/polygerrit-ui/app/elements/diff/gr-syntax-layer/gr-syntax-layer.js
@@ -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 = [];
},