Move annotation layer initialization from attached() to render()
Originally, the annotation layer initialization happens inside the
attached callback of the gr-diff-builder, and one problem with this
design is that in the diff view page, the attached() is usually called
before the plugins are installed, which means that the extra plugin
specific layers won't be picked up.
Moving the logic to render() would partially fix the issue because following
events usually happen in orders:
[diff-builder attached] -> [plugin installs] -> [diff-builder render].
However, above order is not garanteened because plugins are loaded one
by one, and they don't block diff loading and rendering. There may be a
case when plugin is loaded after diff rendering is started, so a better
solution needs to be implemented later.
Bug: Issue 10054
Change-Id: I944ee7ee5bdf68ee523f2428d692fb94bd5a7c47
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 098a4af..42a262a 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
@@ -124,26 +124,13 @@
'_groupsChanged(_groups.splices)',
],
- attached() {
- // Setup annotation layers.
- const layers = [
- this._createTrailingWhitespaceLayer(),
- this.$.syntaxLayer,
- this._createIntralineLayer(),
- this._createTabIndicatorLayer(),
- this.$.rangeLayer,
- ];
-
- // Get layers from plugins (if any).
- for (const pluginLayer of this.$.jsAPI.getDiffLayers(
- this.diffPath, this.changeNum, this.patchNum)) {
- layers.push(pluginLayer);
- }
-
- this._layers = layers;
- },
-
render(keyLocations, prefs) {
+ // Setting up annotation layers must happen after plugins are
+ // installed, and |render| satisfies the requirement, however,
+ // |attached| doesn't because in the diff view page, the element is
+ // attached before plugins are installed.
+ this._setupAnnotationLayers();
+
this.$.syntaxLayer.enabled = prefs.syntax_highlighting;
this._showTabs = !!prefs.show_tabs;
this._showTrailingWhitespace = !!prefs.show_whitespace_errors;
@@ -188,6 +175,24 @@
});
},
+ _setupAnnotationLayers() {
+ const layers = [
+ this._createTrailingWhitespaceLayer(),
+ this.$.syntaxLayer,
+ this._createIntralineLayer(),
+ this._createTabIndicatorLayer(),
+ this.$.rangeLayer,
+ ];
+
+ // Get layers from plugins (if any).
+ for (const pluginLayer of this.$.jsAPI.getDiffLayers(
+ this.diffPath, this.changeNum, this.patchNum)) {
+ layers.push(pluginLayer);
+ }
+
+ this._layers = layers;
+ },
+
getLineElByChild(node) {
while (node) {
if (node instanceof Element) {
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 1b0ba04..294d085 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
@@ -582,13 +582,14 @@
setup(() => {
element = fixture('basic');
element._showTrailingWhitespace = true;
+ element._setupAnnotationLayers();
initialLayersCount = element._layers.length;
});
test('no plugin layers', () => {
const getDiffLayersStub = sinon.stub(element.$.jsAPI, 'getDiffLayers')
.returns([]);
- element.attached();
+ element._setupAnnotationLayers();
assert.isTrue(getDiffLayersStub.called);
assert.equal(element._layers.length, initialLayersCount);
});
@@ -596,9 +597,9 @@
test('with plugin layers', () => {
const getDiffLayersStub = sinon.stub(element.$.jsAPI, 'getDiffLayers')
.returns([{}, {}]);
- element.attached();
+ element._setupAnnotationLayers();
assert.isTrue(getDiffLayersStub.called);
- assert.equal(element._layers.length, initialLayersCount+2);
+ assert.equal(element._layers.length, initialLayersCount + 2);
});
});
