opendev/gerrit
Code Issues Proposed changes

Debounce rerendering after inputs change

Browse Source 
The current implementation starts a render synchronously whenever one of the
inputs change. Often, multiple inputs change together, and the current
behavior causes unncessary rerenders (and cancels half way through).

This change instead schedules the rerendering asynchronously on the next
microtask, and debounces them so that multiple renders collapse into
one.

Change-Id: Id5e823a53ecfa9c542d81835bf67556dcab14d4a
This commit is contained in:
Ole Rehmsen
2019-03-07 17:40:10 +01:00
parent 4c25dfcfd9
commit 22ef403390
3 changed files with 62 additions and 26 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html
View File

@@ -1410,6 +1410,7 @@ limitations under the License.
ignore_whitespace: 'IGNORE_NONE',
};
diff._diff = mock.diffResponse;
diff.$.diff.flushDebouncer('renderDiffTable');
};
const renderAndGetNewDiffs = function(index) {

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

@@ -102,6 +102,8 @@
*/
const COMMIT_MSG_LINE_LENGTH = 72;
const RENDER_DIFF_TABLE_DEBOUNCE_NAME = 'renderDiffTable';
Polymer({
is: 'gr-diff',
@@ -392,6 +394,7 @@
/** Cancel any remaining diff builder rendering work. */
cancel() {
this.$.diffBuilder.cancel();
this.cancelDebouncer(RENDER_DIFF_TABLE_DEBOUNCE_NAME);
},
/** @return {!Array<!HTMLElement>} */
@@ -679,17 +682,32 @@
this.updateStyles(stylesToUpdate);
if (this.diff && !this.noRenderOnPrefsChange) {
this._renderDiffTable();
this._debounceRenderDiffTable();
}
},
_diffChanged(newValue) {
if (newValue) {
this._diffLength = this.$.diffBuilder.getDiffLength();
this._renderDiffTable();
this._debounceRenderDiffTable();
}
},
/**
* When called multiple times from the same microtask, will call
* _renderDiffTable only once, in the next microtask, unless it is cancelled
* before that microtask runs.
*
* This should be used instead of calling _renderDiffTable directly to
* render the diff in response to an input change, because there may be
* multiple inputs changing in the same microtask, but we only want to
* render once.
*/
_debounceRenderDiffTable() {
this.debounce(
RENDER_DIFF_TABLE_DEBOUNCE_NAME, () => this._renderDiffTable());
},
_renderDiffTable() {
this._unobserveIncrementalNodes();
if (!this.prefs) {
@@ -791,12 +809,12 @@
_handleFullBypass() {
this._safetyBypass = FULL_CONTEXT;
this._renderDiffTable();
this._debounceRenderDiffTable();
},
_handleLimitedBypass() {
this._safetyBypass = LIMITED_CONTEXT;
this._renderDiffTable();
this._debounceRenderDiffTable();
},
/** @return {string} */

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

@@ -40,6 +40,8 @@ limitations under the License.
let element;
let sandbox;
const MINIMAL_PREFS = {tab_size: 2, line_length: 80};
setup(() => {
sandbox = sinon.sandbox.create();
});
@@ -81,14 +83,14 @@ limitations under the License.
test('line limit with line_wrapping', () => {
element = fixture('basic');
element.prefs = {line_wrapping: true, line_length: 80, tab_size: 2};
element.prefs = Object.assign({}, MINIMAL_PREFS, {line_wrapping: true});
flushAsynchronousOperations();
assert.equal(element.customStyle['--line-limit'], '80ch');
});
test('line limit without line_wrapping', () => {
element = fixture('basic');
element.prefs = {line_wrapping: false, line_length: 80, tab_size: 2};
element.prefs = Object.assign({}, MINIMAL_PREFS, {line_wrapping: false});
flushAsynchronousOperations();
assert.isNotOk(element.customStyle['--line-limit']);
});
@@ -225,7 +227,7 @@ limitations under the License.
const mock = document.createElement('mock-diff-response');
element.$.diffBuilder._builder = element.$.diffBuilder._getDiffBuilder(
mock.diffResponse, {tab_size: 2, line_length: 80});
mock.diffResponse, Object.assign({}, MINIMAL_PREFS));
// No thread groups.
assert.isNotOk(element._getThreadGroupForLine(contentEl));
@@ -448,7 +450,7 @@ limitations under the License.
binary: true,
};
element.addEventListener('render', () => {
function rendered() {
// Recognizes that it should be an image diff.
assert.isTrue(element.isImageDiff);
assert.instanceOf(
@@ -460,7 +462,9 @@ limitations under the License.
assert.isNotOk(leftImage);
assert.isOk(rightImage);
done();
});
element.removeEventListener('render', rendered);
}
element.addEventListener('render', rendered);
element.revisionImage = mockFile2;
element.diff = mockDiff;
@@ -483,7 +487,7 @@ limitations under the License.
binary: true,
};
element.addEventListener('render', () => {
function rendered() {
// Recognizes that it should be an image diff.
assert.isTrue(element.isImageDiff);
assert.instanceOf(
@@ -495,7 +499,9 @@ limitations under the License.
assert.isOk(leftImage);
assert.isNotOk(rightImage);
done();
});
element.removeEventListener('render', rendered);
}
element.addEventListener('render', rendered);
element.baseImage = mockFile1;
element.diff = mockDiff;
@@ -519,7 +525,7 @@ limitations under the License.
};
mockFile1.type = 'image/jpeg-evil';
element.addEventListener('render', () => {
function rendered() {
// Recognizes that it should be an image diff.
assert.isTrue(element.isImageDiff);
assert.instanceOf(
@@ -527,7 +533,9 @@ limitations under the License.
const leftImage = element.$.diffTable.querySelector('td.left img');
assert.isNotOk(leftImage);
done();
});
element.removeEventListener('render', rendered);
}
element.addEventListener('render', rendered);
element.baseImage = mockFile1;
element.diff = mockDiff;
@@ -679,12 +687,14 @@ limitations under the License.
change_type: 'MODIFIED',
content: [{skip: 66}],
};
element.flushDebouncer('renderDiffTable');
});
test('change in preferences re-renders diff', () => {
sandbox.stub(element, '_renderDiffTable');
element.prefs = {};
element.prefs = {time_format: 'HHMM_12'};
element.prefs = Object.assign(
{}, MINIMAL_PREFS, {time_format: 'HHMM_12'});
element.flushDebouncer('renderDiffTable');
assert.isTrue(element._renderDiffTable.called);
});
@@ -692,8 +702,9 @@ limitations under the License.
'noRenderOnPrefsChange', () => {
sandbox.stub(element, '_renderDiffTable');
element.noRenderOnPrefsChange = true;
element.prefs = {};
element.prefs = {time_format: 'HHMM_12'};
element.prefs = Object.assign(
{}, MINIMAL_PREFS, {time_format: 'HHMM_12'});
element.flushDebouncer('renderDiffTable');
assert.isFalse(element._renderDiffTable.called);
});
});
@@ -760,33 +771,39 @@ limitations under the License.
});
test('large render w/ context = 10', done => {
element.prefs = {context: 10};
element.addEventListener('render', () => {
element.prefs = Object.assign({}, MINIMAL_PREFS, {context: 10});
function rendered() {
assert.isTrue(renderStub.called);
assert.isFalse(element._showWarning);
done();
});
element.removeEventListener('render', rendered);
}
element.addEventListener('render', rendered);
element._renderDiffTable();
});
test('large render w/ whole file and bypass', done => {
element.prefs = {context: -1};
element.prefs = Object.assign({}, MINIMAL_PREFS, {context: -1});
element._safetyBypass = 10;
element.addEventListener('render', () => {
function rendered() {
assert.isTrue(renderStub.called);
assert.isFalse(element._showWarning);
done();
});
element.removeEventListener('render', rendered);
}
element.addEventListener('render', rendered);
element._renderDiffTable();
});
test('large render w/ whole file and no bypass', done => {
element.prefs = {context: -1};
element.addEventListener('render', () => {
element.prefs = Object.assign({}, MINIMAL_PREFS, {context: -1});
function rendered() {
assert.isFalse(renderStub.called);
assert.isTrue(element._showWarning);
done();
});
element.removeEventListener('render', rendered);
}
element.addEventListener('render', rendered);
element._renderDiffTable();
});
});