opendev/gerrit
Code Issues Proposed changes

Move annotation layer initialization from attached() to render()

Browse Source 
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
This commit is contained in:
Yuke Liao
2018-11-21 10:11:15 -08:00
parent ecdd280378
commit 2c9eb2d7cf
2 changed files with 28 additions and 22 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -124,26 +124,13 @@ limitations under the License.
'_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 @@ limitations under the License.
});
},
_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) {

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

@@ -582,13 +582,14 @@ limitations under the License.
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 @@ limitations under the License.
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);
});
});