From ea97c5b49af731a74408112726bd5d55faf7774b Mon Sep 17 00:00:00 2001 From: Wyatt Allen Date: Thu, 21 Jul 2016 22:19:25 -0700 Subject: [PATCH] Adds diff traversal helper and make diff object a builder property Adds a polymorphic method to GrDiffBuilder subclasses named `_getNextContentOnSide` which gets the a content element by traversing from its preceding content on the same side. This method is dramatically faster than the query-based method when diff sections are large. In preparation for the syntax highlighting change, the gr-diff-builder is refactored to use a property for the diff object, rather than accepting it as a parameter to the `render` function. Tests are included for new functions. Change-Id: Ifd0edb530a303de2626d55a691c3ba1eaed6167c --- .../gr-diff-builder-side-by-side.js | 12 +++ .../gr-diff-builder-unified.js | 14 ++++ .../diff/gr-diff-builder/gr-diff-builder.html | 7 +- .../diff/gr-diff-builder/gr-diff-builder.js | 20 ++++- .../gr-diff-builder/gr-diff-builder_test.html | 79 +++++++++++++++++-- .../app/elements/diff/gr-diff/gr-diff.html | 1 + .../app/elements/diff/gr-diff/gr-diff.js | 2 +- 7 files changed, 121 insertions(+), 14 deletions(-) diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-side-by-side.js b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-side-by-side.js index 0d9ecbc6d9..1cb8cc77aa 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-side-by-side.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-side-by-side.js @@ -66,5 +66,17 @@ } }; + GrDiffBuilderSideBySide.prototype._getNextContentOnSide = function( + content, side) { + var tr = content.parentElement.parentElement; + var content; + while (tr = tr.nextSibling) { + content = tr.querySelector( + 'td.content .contentText[data-side="' + side + '"]'); + if (content) { return content; } + } + return null; + }; + window.GrDiffBuilderSideBySide = GrDiffBuilderSideBySide; })(window, GrDiffBuilder); diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-unified.js b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-unified.js index d9c9b1b224..960bf461dc 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-unified.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-unified.js @@ -59,5 +59,19 @@ return row; }; + GrDiffBuilderUnified.prototype._getNextContentOnSide = function( + content, side) { + var tr = content.parentElement.parentElement; + var content; + while (tr = tr.nextSibling) { + if (tr.classList.contains('both') || ( + (side === 'left' && tr.classList.contains('remove')) || + (side === 'right' && tr.classList.contains('add')))) { + return tr.querySelector('.contentText'); + } + } + return null; + }; + window.GrDiffBuilderUnified = GrDiffBuilderUnified; })(window, GrDiffBuilder); 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 0aa5c94af5..3cad9d9c50 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 @@ -55,6 +55,7 @@ limitations under the License. */ properties: { + diff: Object, viewMode: String, comments: Object, isImageDiff: Boolean, @@ -81,11 +82,11 @@ limitations under the License. ]; }, - render: function(diff, comments, prefs) { + render: function(comments, prefs) { // Stop the processor (if it's running). this.$.processor.cancel(); - this._builder = this._getDiffBuilder(diff, comments, prefs); + this._builder = this._getDiffBuilder(this.diff, comments, prefs); this.$.processor.context = prefs.context; this.$.processor.keyLocations = this._getCommentLocations(comments); @@ -93,7 +94,7 @@ limitations under the License. this._clearDiffContent(); console.time('diff render'); - this.$.processor.process(diff.content).then(function() { + return this.$.processor.process(this.diff.content).then(function() { if (this.isImageDiff) { this._builder.renderDiffImages(); } diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js index 389d290ce6..816e3a4e59 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js @@ -139,6 +139,7 @@ var groups = this.getGroupsByLineRange(start, end, opt_side); groups.forEach(function(group) { + var content = null; group.lines.forEach(function(line) { if ((opt_side === 'left' && line.type === GrDiffLine.Type.ADD) || (opt_side === 'right' && line.type === GrDiffLine.Type.REMOVE)) { @@ -150,8 +151,12 @@ if (out_lines) { out_lines.push(line); } if (out_elements) { - var content = this.getContentByLine(lineNumber, opt_side, - group.element); + if (content) { + content = this._getNextContentOnSide(content, opt_side); + } else { + content = this.getContentByLine(lineNumber, opt_side, + group.element); + } if (content) { out_elements.push(content); } } }.bind(this)); @@ -537,5 +542,16 @@ this._renderContentByRange(start, end, side); }; + /** + * Finds the next DIV.contentText element following the given element, and on + * the same side. Will only search within a group. + * @param {HTMLElement} content + * @param {String} side Either 'left' or 'right' + * @return {HTMLElement} + */ + GrDiffBuilder.prototype._getNextContentOnSide = function(content, side) { + throw Error('Subclasses must implement _getNextContentOnSide'); + }; + window.GrDiffBuilder = GrDiffBuilder; })(window, GrDiffGroup, GrDiffLine); 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 f3d24f74aa..753ddaa75e 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 @@ -459,7 +459,8 @@ limitations under the License. }; return builder; }); - element.render({content: content}, {left: [], right: []}, prefs); + element.diff = {content: content}; + element.render({left: [], right: []}, prefs); }); test('renderSection', function() { @@ -494,25 +495,23 @@ limitations under the License. var element; var builder; var diff; + var prefs; setup(function(done) { element = fixture('mock-diff'); diff = document.createElement('mock-diff-response').diffResponse; + element.diff = diff; - var prefs = { + prefs = { line_length: 80, show_tabs: true, tab_size: 4, }; - function doneHandler() { - element.removeEventListener('render', doneHandler); + element.render({left: [], right: []}, prefs).then(function() { builder = element._builder; done(); - } - element.addEventListener('render', doneHandler); - - element.render(diff, {left: [], right: []}, prefs); + }); }); test('getContentByLine', function() { @@ -566,6 +565,70 @@ limitations under the License. spy.restore(); }); + + test('_getNextContentOnSide side-by-side left', function() { + var startElem = builder.getContentByLine(5, 'left', + element.$.diffTable); + var expectedStartString = diff.content[2].ab[0]; + var expectedNextString = diff.content[2].ab[1]; + assert.equal(startElem.textContent, expectedStartString); + + var nextElem = builder._getNextContentOnSide(startElem, + 'left'); + assert.equal(nextElem.textContent, expectedNextString); + }); + + test('_getNextContentOnSide side-by-side right', function() { + var startElem = builder.getContentByLine(5, 'right', + element.$.diffTable); + var expectedStartString = diff.content[1].b[0]; + var expectedNextString = diff.content[1].b[1]; + assert.equal(startElem.textContent, expectedStartString); + + var nextElem = builder._getNextContentOnSide(startElem, + 'right'); + assert.equal(nextElem.textContent, expectedNextString); + }); + + test('_getNextContentOnSide unified left', function(done) { + // Re-render as unified: + element.viewMode = 'UNIFIED_DIFF'; + element.render({left: [], right: []}, prefs).then(function() { + builder = element._builder; + + var startElem = builder.getContentByLine(5, 'left', + element.$.diffTable); + var expectedStartString = diff.content[2].ab[0]; + var expectedNextString = diff.content[2].ab[1]; + assert.equal(startElem.textContent, expectedStartString); + + var nextElem = builder._getNextContentOnSide(startElem, + 'left'); + assert.equal(nextElem.textContent, expectedNextString); + + done(); + }); + }); + + test('_getNextContentOnSide unified right', function(done) { + // Re-render as unified: + element.viewMode = 'UNIFIED_DIFF'; + element.render({left: [], right: []}, prefs).then(function() { + builder = element._builder; + + var startElem = builder.getContentByLine(5, 'right', + element.$.diffTable); + var expectedStartString = diff.content[1].b[0]; + var expectedNextString = diff.content[1].b[1]; + assert.equal(startElem.textContent, expectedStartString); + + var nextElem = builder._getNextContentOnSide(startElem, + 'right'); + assert.equal(nextElem.textContent, expectedNextString); + + done(); + }); + }); }); }); diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html index 2e7fd56975..33322df66b 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html +++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html @@ -151,6 +151,7 @@ limitations under the License.