opendev/gerrit
Code Issues Proposed changes

Make gr-diff.reload() listen for render event

Browse Source 
Before, it used the promise returned by the gr-diff-builder.render()
instead. Using the event is a prerequisite from moving away from
explicit render calls.

This comes with a small behavior change: Before, gr-diff would not
dispatch a `render` event when no rendering happened because prefs were
unset or the diff was too large and no safetyBypass was used. Now it
does because we still want reload to resolve the promise it returns.
This should not break anything though because currently only tests
listen for this event.

Bug: Issue 9623
Change-Id: I15dd07d706c21cb441f2bc597391e4620c9c12fc
This commit is contained in:
Ole Rehmsen
2018-08-27 14:27:53 +02:00
parent faf2c63ebf
commit ffb320ae65
2 changed files with 32 additions and 11 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -262,7 +262,14 @@
}
this.filesWeblinks = this._getFilesWeblinks(diff);
this._diff = diff;
return this._renderDiffTable();
return new Promise(resolve => {
const callback = () => {
resolve();
this.removeEventListener('render', callback);
};
this.addEventListener('render', callback);
this._renderDiffTable();
});
})
.catch(err => {
console.warn('Error encountered loading diff:', err);
@@ -715,16 +722,20 @@
},
_renderDiffTable() {
if (!this.prefs) { return Promise.resolve(); }
if (!this.prefs) {
this.dispatchEvent(new CustomEvent('render', {bubbles: true}));
return;
}
if (this.prefs.context === -1 &&
this._diffLength(this._diff) >= LARGE_DIFF_THRESHOLD_LINES &&
this._safetyBypass === null) {
this._showWarning = true;
return Promise.resolve();
this.dispatchEvent(new CustomEvent('render', {bubbles: true}));
return;
}
this._showWarning = false;
return this.$.diffBuilder.render(this.comments, this._getBypassPrefs());
this.$.diffBuilder.render(this.comments, this._getBypassPrefs());
},
/**

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

@@ -1120,39 +1120,49 @@ limitations under the License.
setup(() => {
element = fixture('basic');
renderStub = sandbox.stub(element.$.diffBuilder, 'render',
() => Promise.resolve());
() => {
Promise.resolve();
element.$.diffBuilder.dispatchEvent(
new CustomEvent('render', {bubbles: true}));
});
const mock = document.createElement('mock-diff-response');
element._diff = mock.diffResponse;
element.comments = {left: [], right: []};
element.noRenderOnPrefsChange = true;
});
test('lage render w/ context = 10', () => {
test('large render w/ context = 10', done => {
element.prefs = {context: 10};
sandbox.stub(element, '_diffLength', () => 10000);
return element._renderDiffTable().then(() => {
element.addEventListener('render', () => {
assert.isTrue(renderStub.called);
assert.isFalse(element._showWarning);
done();
});
element._renderDiffTable();
});
test('lage render w/ whole file and bypass', () => {
test('large render w/ whole file and bypass', done => {
element.prefs = {context: -1};
element._safetyBypass = 10;
sandbox.stub(element, '_diffLength', () => 10000);
return element._renderDiffTable().then(() => {
element.addEventListener('render', () => {
assert.isTrue(renderStub.called);
assert.isFalse(element._showWarning);
done();
});
element._renderDiffTable();
});
test('lage render w/ whole file and no bypass', () => {
test('large render w/ whole file and no bypass', done => {
element.prefs = {context: -1};
sandbox.stub(element, '_diffLength', () => 10000);
return element._renderDiffTable().then(() => {
element.addEventListener('render', () => {
assert.isFalse(renderStub.called);
assert.isTrue(element._showWarning);
done();
});
element._renderDiffTable();
});
});