Render inline diffs sequentially Changes with large numbers of files could overwhelm PolyGerrit when a user selects [Expand all] for inline diffs. This was because the asynchronous processing/rendering/annotating process would be kicked off for all unexpanded diffs simultaneously, resulting in browser lock-up and general slowness even after rendering had completed. With this change, inline diffs are rendered in serial rather than parallel. In this way the benefits of the async features of diff rendering extend to the file list, even for changes with many large diffs (such as the one in the linked issue). With this change, the `__expanded` property is removed from file objects in GR-FILE-LIST. Instead, that element maintains a list (`_expandedFilePaths`) which records the same information. Because the expanded files are recorded in a list, however, splices on the list can be observed, batch diff expansion can be handled sequentially. Tests are updated to respect the new expanded paths list. Bug: Issue 5396 Change-Id: Ib83ff5157177e1c890db8a82fbc25df8fecbe065
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html index 1ab8a77..553e1dd 100644 --- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html +++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
@@ -18,6 +18,7 @@ <link rel="import" href="../../../behaviors/gr-patch-set-behavior/gr-patch-set-behavior.html"> <link rel="import" href="../../../behaviors/gr-url-encoding-behavior.html"> <link rel="import" href="../../../bower_components/polymer/polymer.html"> +<link rel="import" href="../../core/gr-reporting/gr-reporting.html"> <link rel="import" href="../../diff/gr-diff/gr-diff.html"> <link rel="import" href="../../diff/gr-diff-cursor/gr-diff-cursor.html"> <link rel="import" href="../../shared/gr-button/gr-button.html"> @@ -243,7 +244,7 @@ aria-label$="[[_computeFileStatusLabel(file.status)]]"> [[_computeFileStatus(file.status)]] </div> - <a class$="[[_computePathClass(file.__expanded)]]" + <a class$="[[_computePathClass(file.__path, _expandedFilePaths.*)]]" href$="[[_computeDiffURL(changeNum, patchRange, file.__path)]]" on-click="_handleFileClick"> <div title$="[[_computeFileDisplayName(file.__path)]]" @@ -297,15 +298,16 @@ <div class="show-hide" hidden$="[[_userPrefs.expand_inline_diffs]]"> <label class="show-hide"> <input type="checkbox" class="show-hide" - checked$="[[!file.__expanded]]" data-path$="[[file.__path]]" + checked$="[[_isFileExpanded(file.__path, _expandedFilePaths.*)]]" + data-path$="[[file.__path]]" on-change="_handleHiddenChange"> - [[_computeShowHideText(file.__expanded)]] + [[_computeShowHideText(file.__path, _expandedFilePaths.*)]] </label> </div> </div> <gr-diff - hidden$="[[!file.__expanded]]" - expanded="[[file.__expanded]]" + no-auto-render + hidden$="[[!_isFileExpanded(file.__path, _expandedFilePaths.*)]]" project="[[change.project]]" commit="[[change.current_revision]]" change-num="[[changeNum]]" @@ -366,6 +368,7 @@ id="fileCursor" scroll-behavior="keep-visible" cursor-target-class="selected"></gr-cursor-manager> + <gr-reporting id="reporting"></gr-reporting> </template> <script src="gr-file-list.js"></script> </dom-module>
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js index 8016f98..a4992df 100644 --- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js +++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
@@ -104,6 +104,10 @@ readOnly: true, value: 50, }, + _expandedFilePaths: { + type: Array, + value: function() { return []; }, + }, }, behaviors: [ @@ -112,6 +116,10 @@ Gerrit.URLEncodingBehavior, ], + observers: [ + '_expandedPathsChanged(_expandedFilePaths.splices)', + ], + keyBindings: { 'shift+left': '_handleShiftLeftKey', 'shift+right': '_handleShiftRightKey', @@ -218,8 +226,18 @@ }, _handleHiddenChange: function(e) { - var model = e.model; - model.set('file.__expanded', !model.file.__expanded); + this._togglePathExpanded(e.model.file.__path); + }, + + _togglePathExpanded: function(path) { + // Is the path in the list of expanded diffs? IF so remove it, otherwise + // add it to the list. + var pathIndex = this._expandedFilePaths.indexOf(path); + if (pathIndex === -1) { + this.push('_expandedFilePaths', path); + } else { + this.splice('_expandedFilePaths', pathIndex, 1); + } }, _handlePatchChange: function(e) { @@ -236,25 +254,26 @@ } }, - /** - * Until upgrading to Polymer 2.0, manual management of reflection between - * _shownFiles and _files is necessary. Performance of linkPaths is very - * poor. - */ _expandAllDiffs: function(e) { this._showInlineDiffs = true; + + // Find the list of paths that are in the file list, but not in the + // expanded list. + var newPaths = []; + var path; for (var i = 0; i < this._shownFiles.length; i++) { - this.set(['_shownFiles', i, '__expanded'], true); - this.set(['_files', i, '__expanded'], true); + path = this._shownFiles[i].__path; + if (this._expandedFilePaths.indexOf(path) === -1) { + newPaths.push(path); + } } + + this.splice.apply(this, ['_expandedFilePaths', 0, 0].concat(newPaths)); }, _collapseAllDiffs: function(e) { this._showInlineDiffs = false; - for (var i = 0; i < this._shownFiles.length; i++) { - this.set(['_shownFiles', i, '__expanded'], false); - this.set(['_files', i, '__expanded'], false); - } + this._expandedFilePaths = []; this.$.diffCursor.handleDiffUpdate(); }, @@ -328,7 +347,6 @@ this.changeNum, this.patchRange).then(function(files) { // Append UI-specific properties. return files.map(function(file) { - file.__expanded = false; return file; }); }); @@ -365,12 +383,7 @@ this.$.fileCursor.index === -1) { return; } e.preventDefault(); - var expanded = this._files[this.$.fileCursor.index].__expanded; - // Until Polymer 2.0, manual management of reflection between _files - // and _shownFiles is necessary. - this.set(['_shownFiles', this.$.fileCursor.index, '__expanded'], - !expanded); - this.set(['_files', this.$.fileCursor.index, '__expanded'], !expanded); + this._togglePathExpanded(this.$.fileCursor.target.path); }, _handleCapitalIKey: function(e) { @@ -573,12 +586,13 @@ return classes.join(' '); }, - _computePathClass: function(expanded) { - return expanded ? 'path expanded' : 'path'; + _computePathClass: function(path, expandedFilesRecord) { + return this._isFileExpanded(path, expandedFilesRecord) ? 'path expanded' : + 'path'; }, - _computeShowHideText: function(expanded) { - return expanded ? '▼' : '◀'; + _computeShowHideText: function(path, expandedFilesRecord) { + return this._isFileExpanded(path, expandedFilesRecord) ? '▼' : '◀'; }, _computeFilesShown: function(numFilesShown, files) { @@ -663,5 +677,74 @@ return FileStatus.hasOwnProperty(statusCode) ? FileStatus[statusCode] : 'Status Unknown'; }, + + _isFileExpanded: function(path, expandedFilesRecord) { + return expandedFilesRecord.base.indexOf(path) !== -1; + }, + + /** + * Handle splices to the list of expanded file paths. If there are any new + * entries in the expanded list, then render each diff corresponding in + * order by waiting for the previous diff to finish before starting the next + * one. + * @param {splice} record The splice record in the expanded paths list. + */ + _expandedPathsChanged: function(record) { + if (!record) { return; } + + // Find the paths introduced by the new index splices: + var newPaths = record.indexSplices + .map(function(splice) { + return splice.object.slice(splice.index, + splice.index + splice.addedCount); + }) + .reduce(function(acc, paths) { return acc.concat(paths); }, []); + + var timerName = 'Expand ' + newPaths.length + ' diffs'; + this.$.reporting.time(timerName); + + this._renderInOrder(newPaths, this.diffs, newPaths.length) + .then(function() { + this.$.reporting.timeEnd(timerName); + this.$.diffCursor.handleDiffUpdate(); + }.bind(this)); + }, + + /** + * Given an array of paths and a NodeList of diff elements, render the diff + * for each path in order, awaiting the previous render to complete before + * continung. + * @param {!Array<!String>} paths + * @param {!NodeList<!GrDiffElement>} diffElements + * @param {Number} initialCount The total number of paths in the pass. This + * is used to generate log messages. + * @return {!Promise} + */ + _renderInOrder: function(paths, diffElements, initialCount) { + if (!paths.length) { + console.log('Finished expanding', initialCount, 'diff(s)'); + return Promise.resolve(); + } + console.log('Expanding diff', 1 + initialCount - paths.length, 'of', + initialCount, ':', paths[0]); + var diffElem = this._findDiffByPath(paths[0], diffElements); + return diffElem.reload().then(function() { + return this._renderInOrder(paths.slice(1), diffElements, initialCount); + }.bind(this)); + }, + + /** + * In the given NodeList of diff elements, find the diff for the given path. + * @param {!String} path + * @param {!NodeList<!GrDiffElement>} diffElements + * @return {!GrDiffElement} + */ + _findDiffByPath: function(path, diffElements) { + for (var i = 0; i < diffElements.length; i++) { + if (diffElements[i].path === path) { + return diffElements[i]; + } + } + }, }); })();
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html index 4c8b4f1..1fb9b60 100644 --- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html +++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html
@@ -80,19 +80,16 @@ lines_inserted: 9, lines_deleted: 0, __path: '/COMMIT_MSG', - __expanded: false, }); assert.deepEqual(files[1], { lines_inserted: 0, lines_deleted: 0, __path: 'about.txt', - __expanded: false, }); assert.deepEqual(files[2], { lines_inserted: 0, lines_deleted: 123, __path: 'tags.html', - __expanded: false, }); getChangeFilesStub.restore(); @@ -270,9 +267,9 @@ suite('keyboard shortcuts', function() { setup(function() { element._files = [ - {__path: '/COMMIT_MSG', __expanded: false}, - {__path: 'file_added_in_rev2.txt', __expanded: false}, - {__path: 'myfile.txt', __expanded: false}, + {__path: '/COMMIT_MSG'}, + {__path: 'file_added_in_rev2.txt'}, + {__path: 'myfile.txt'}, ]; element.changeNum = '42'; element.patchRange = { @@ -333,29 +330,31 @@ }); test('i key shows/hides selected inline diff', function() { + sandbox.stub(element, '_expandedPathsChanged'); flushAsynchronousOperations(); element.$.fileCursor.stops = element.diffs; element.$.fileCursor.setCursorAtIndex(0); MockInteractions.pressAndReleaseKeyOn(element, 73, null, 'i'); flushAsynchronousOperations(); - assert.isFalse(element.diffs[0].hasAttribute('hidden')); + assert.include(element._expandedFilePaths, element.diffs[0].path); MockInteractions.pressAndReleaseKeyOn(element, 73, null, 'i'); flushAsynchronousOperations(); - assert.isTrue(element.diffs[0].hasAttribute('hidden')); + assert.notInclude(element._expandedFilePaths, element.diffs[0].path); element.$.fileCursor.setCursorAtIndex(1); MockInteractions.pressAndReleaseKeyOn(element, 73, null, 'i'); flushAsynchronousOperations(); - assert.isFalse(element.diffs[1].hasAttribute('hidden')); + assert.include(element._expandedFilePaths, element.diffs[1].path); MockInteractions.pressAndReleaseKeyOn(element, 73, 'shift', 'i'); flushAsynchronousOperations(); for (var index in element.diffs) { - assert.isFalse(element.diffs[index].hasAttribute('hidden')); + assert.include(element._expandedFilePaths, element.diffs[index].path); } MockInteractions.pressAndReleaseKeyOn(element, 73, 'shift', 'i'); flushAsynchronousOperations(); for (var index in element.diffs) { - assert.isTrue(element.diffs[index].hasAttribute('hidden')); + assert.notInclude(element._expandedFilePaths, + element.diffs[index].path); } }); }); @@ -440,9 +439,9 @@ test('file review status', function() { element._files = [ - {__path: '/COMMIT_MSG', __expanded: false}, - {__path: 'file_added_in_rev2.txt', __expanded: false}, - {__path: 'myfile.txt', __expanded: false}, + {__path: '/COMMIT_MSG'}, + {__path: 'file_added_in_rev2.txt'}, + {__path: 'myfile.txt'}, ]; element._reviewed = ['/COMMIT_MSG', 'myfile.txt']; element.changeNum = '42'; @@ -537,7 +536,7 @@ test('checkbox shows/hides diff inline', function() { element._files = [ - {__path: 'myfile.txt', __expanded: false}, + {__path: 'myfile.txt'}, ]; element.changeNum = '42'; element.patchRange = { @@ -545,14 +544,16 @@ patchNum: '2', }; element.$.fileCursor.setCursorAtIndex(0); + sandbox.stub(element, '_expandedPathsChanged'); flushAsynchronousOperations(); var fileRows = Polymer.dom(element.root).querySelectorAll('.row:not(.header)'); var showHideCheck = fileRows[0].querySelector( 'input.show-hide[type="checkbox"]'); - assert.isTrue(showHideCheck.checked); + assert.isNotOk(showHideCheck.checked); MockInteractions.tap(showHideCheck); - assert.isFalse(element.diffs[0].hidden); + assert.isOk(showHideCheck.checked); + assert.notEqual(element._expandedFilePaths.indexOf('myfile.txt'), -1); }); test('path should be properly escaped', function() { @@ -575,7 +576,7 @@ test('diff mode correctly toggles the diffs', function() { element._files = [ - {__path: 'myfile.txt', __expanded: false}, + {__path: 'myfile.txt'}, ]; element.changeNum = '42'; element.patchRange = { @@ -628,7 +629,7 @@ assert.isTrue(computeSpy.lastCall.returnValue); var arr = []; _.times(element._maxFilesForBulkActions + 1, function() { - arr.push({__path: 'myfile.txt', __expanded: false}); + arr.push({__path: 'myfile.txt'}); }); element._files = arr; element._numFilesShown = arr.length; @@ -639,7 +640,7 @@ test('expanded attribute not set on path when not expanded', function() { element._files = [ - {__path: '/COMMIT_MSG', __expanded: false}, + {__path: '/COMMIT_MSG'}, ]; assert.isNotOk(element.$$('.expanded')); }); @@ -659,13 +660,14 @@ }); test('expand_inline_diffs user preference', function() { element._files = [ - {__path: '/COMMIT_MSG', __expanded: false}, + {__path: '/COMMIT_MSG'}, ]; element.changeNum = '42'; element.patchRange = { basePatchNum: 'PARENT', patchNum: '2', }; + sandbox.stub(element, '_expandedPathsChanged'); flushAsynchronousOperations(); var commitMsgFile = Polymer.dom(element.root) .querySelectorAll('.row:not(.header) a')[0]; @@ -686,5 +688,59 @@ assert(hiddenChangeSpy.calledOnce, 'file is expanded'); assert.isOk(element.$$('.expanded')); }); + + test('_togglePathExpanded', function() { + var path = 'path/to/my/file.txt'; + element.files = [{__path: path}]; + var expandedChangedStub = sandbox.stub(element, '_expandedPathsChanged'); + + assert.equal(element._expandedFilePaths.length, 0); + element._togglePathExpanded(path); + assert.equal(expandedChangedStub.callCount, 1); + assert.include(element._expandedFilePaths, path); + element._togglePathExpanded(path); + assert.equal(expandedChangedStub.callCount, 2); + assert.notInclude(element._expandedFilePaths, path); + }); + + test('_expandedPathsChanged', function(done) { + var path = 'path/to/my/file.txt'; + var diffs = [{ + path: path, + reload: function() { + done(); + return Promise.resolve(); + }, + }]; + var diffsStub = sinon.stub(element, 'diffs', { + get: function() { return diffs; }, + }); + element.push('_expandedFilePaths', path); + }); + + test('_renderInOrder', function(done) { + var callCount = 0; + var diffs = [{ + path: 'p0', + reload: function() { + assert.equal(callCount++, 2); + return Promise.resolve(); + }, + }, { + path: 'p1', + reload: function() { + assert.equal(callCount++, 1); + return Promise.resolve(); + }, + }, { + path: 'p2', + reload: function() { + assert.equal(callCount++, 0); + return Promise.resolve(); + }, + }]; + element._renderInOrder(['p2', 'p1', 'p0'], diffs, 3) + .then(function() { done(); }); + }); }); </script>
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 9c03d23..8631af7 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
@@ -149,11 +149,11 @@ reporting.timeEnd(TimingLabel.CONTENT); reporting.time(TimingLabel.SYNTAX); - this.$.syntaxLayer.process().then(function() { + return this.$.syntaxLayer.process().then(function() { reporting.timeEnd(TimingLabel.SYNTAX); reporting.timeEnd(TimingLabel.TOTAL); - }); - this.fire('render'); + this.fire('render'); + }.bind(this)); }.bind(this)); },
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js index 69eb000..697845e 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js +++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
@@ -34,10 +34,9 @@ properties: { changeNum: String, - expanded: { + noAutoRender: { type: Boolean, - value: true, - observer: '_handleShowDiff', + value: false, }, patchRange: Object, path: String, @@ -127,13 +126,14 @@ return Promise.all(promises).then(function() { if (this.prefs) { - this._render(); + return this._renderDiffTable(); } + return Promise.resolve(); }.bind(this)); }, getCursorStops: function() { - if (!this.expanded) { + if (this.noAutoRender) { return []; } @@ -166,14 +166,9 @@ this.toggleClass('no-left'); }, - _handleShowDiff: function() { - if (this._canRender()) { - this.reload(); - } - }, - _canRender: function() { - return this.changeNum && this.patchRange && this.path && this.expanded; + return this.changeNum && this.patchRange && this.path && + !this.noAutoRender; }, _getCommentThreads: function() { @@ -419,12 +414,12 @@ this.updateStyles(); if (this._diff && this._comments) { - this._render(); + this._renderDiffTable(); } }, - _render: function() { - this.$.diffBuilder.render(this._comments, this.prefs); + _renderDiffTable: function() { + return this.$.diffBuilder.render(this._comments, this.prefs); }, _clearDiffContent: function() {
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 158a9a2..0bbad2c 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
@@ -572,22 +572,6 @@ assert.equal(drafts.length, 1); assert.equal(drafts[0].id, id); }); - - test('_handleShowDiff reloads when expanded is made true', - function(done) { - element.expanded = false; - element.changeNum = element._comments.meta.changeNum; - element.patchRange = element._comments.meta.patchRange; - element.path = element._comments.meta.path; - - var stub = sandbox.stub(element, 'reload', function() { - assert.isTrue(stub.called); - done(); - }); - var spy = sinon.spy(element, '_handleShowDiff'); - element.set('expanded', true); - assert.isTrue(spy.called); - }); }); }); });