From 92f672f5cc5592d5cb61d6db5b80bcbbea2b12de Mon Sep 17 00:00:00 2001 From: Andrew Bonventre Date: Fri, 18 Dec 2015 19:52:28 -0500 Subject: [PATCH] Implement diff view context Only show N lines of context with a control to set the user's preferred context length. TODO in follow-up change: persist the context preference. Feature: Issue 3675 Change-Id: I7e98651459c80d4894c495f68b6f26f201b587ac --- polygerrit-ui/app/elements/gr-diff-side.html | 115 +++++++++-- polygerrit-ui/app/elements/gr-diff.html | 188 +++++++++++++++--- polygerrit-ui/app/test/gr-diff-side-test.html | 49 ++++- polygerrit-ui/app/test/gr-diff-test.html | 76 +++++++ 4 files changed, 376 insertions(+), 52 deletions(-) diff --git a/polygerrit-ui/app/elements/gr-diff-side.html b/polygerrit-ui/app/elements/gr-diff-side.html index 0120d3b4aa..642a02fd0c 100644 --- a/polygerrit-ui/app/elements/gr-diff-side.html +++ b/polygerrit-ui/app/elements/gr-diff-side.html @@ -65,8 +65,23 @@ limitations under the License. /* >> character */ content: '\00BB'; } - .filler { - background: #eee; + .numbers .filler { + background-color: #eee; + } + .contextControl { + background-color: #fef; + } + .contextControl a:link, + .contextControl a:visited { + display: block; + text-decoration: none; + } + .numbers .contextControl { + padding: 0 .75em; + text-align: right; + } + .content .contextControl { + text-align: center; }
@@ -90,6 +105,12 @@ limitations under the License. Polymer({ is: 'gr-diff-side', + /** + * Fired when an expand context control is clicked. + * + * @event expand-context + */ + properties: { canComment: { type: Boolean, @@ -98,7 +119,7 @@ limitations under the License. content: { type: Array, notify: true, - observer: '_render', + observer: '_contentChanged', }, width: { type: Number, @@ -143,10 +164,28 @@ limitations under the License. window.scrollTo(0, top - (window.innerHeight / 3) - el.offsetHeight); }, + renderLineIndexRange: function(startIndex, endIndex) { + this._render(this.content, startIndex, endIndex); + }, + + hideElementsWithIndex: function(index) { + var els = Polymer.dom(this.root).querySelectorAll( + '[data-index="' + index + '"]'); + for (var i = 0; i < els.length; i++) { + els[i].setAttribute('hidden', true); + } + }, + _widthChanged: function(width) { this.$.content.style.width = width + 'ch'; }, + _contentChanged: function(diff) { + this._clearChildren(this.$.numbers); + this._clearChildren(this.$.content); + this._render(diff, 0, diff.length - 1); + }, + _computeContainerClass: function(canComment) { return 'container' + (canComment ? ' canComment' : ''); }, @@ -157,24 +196,61 @@ limitations under the License. } }, - _render: function(diff) { - this._clearChildren(this.$.numbers); - this._clearChildren(this.$.content); - for (var i = 0; i < diff.length; i++) { + _handleContextControlClick: function(context, e) { + e.preventDefault(); + this.fire('expand-context', {bubbles: false, context: context}); + }, + + _render: function(diff, startIndex, endIndex) { + var beforeLineEl; + var beforeContentEl; + if (endIndex != diff.length - 1) { + beforeLineEl = this.$$('.numbers [data-index="' + endIndex + '"]'); + beforeContentEl = this.$$('.content [data-index="' + endIndex + '"]'); + } + + for (var i = startIndex; i <= endIndex; i++) { + if (diff[i].hidden) { continue; } + switch (diff[i].type) { case 'CODE': - this._renderCode(diff[i]); + this._renderCode(diff[i], i, beforeLineEl, beforeContentEl); break; case 'FILLER': - this._renderFiller(diff[i]); + this._renderFiller(diff[i], i, beforeLineEl, beforeContentEl); + break; + case 'CONTEXT_CONTROL': + this._renderContextControl(diff[i], i, beforeLineEl, + beforeContentEl); break; } } }, - _renderFiller: function(filler) { + _renderContextControl: function(control, index, beforeLineEl, + beforeContentEl) { + var lineEl = this._createElement('div', 'contextControl'); + lineEl.setAttribute('data-index', index); + lineEl.textContent = '@@'; + var contentEl = this._createElement('div', 'contextControl'); + contentEl.setAttribute('data-index', index); + var a = this._createElement('a'); + a.href = '#'; + a.textContent = 'Show ' + control.numLines + ' common ' + + (control.numLines == 1 ? 'line' : 'lines') + '...'; + a.addEventListener('click', + this._handleContextControlClick.bind(this, control)); + contentEl.appendChild(a); + + this.$.numbers.insertBefore(lineEl, beforeLineEl); + this.$.content.insertBefore(contentEl, beforeContentEl); + }, + + _renderFiller: function(filler, index, beforeLineEl, beforeContentEl) { var lineFillerEl = this._createElement('div', 'filler'); + lineFillerEl.setAttribute('data-index', index); var fillerEl = this._createElement('div', 'filler'); + fillerEl.setAttribute('data-index', index); var numLines = filler.numLines || 1; lineFillerEl.textContent = '\n'.repeat(numLines); @@ -182,18 +258,22 @@ limitations under the License. var newlineEl = this._createElement('span', 'br'); fillerEl.appendChild(newlineEl); } - this.$.numbers.appendChild(lineFillerEl); - this.$.content.appendChild(fillerEl); + + this.$.numbers.insertBefore(lineFillerEl, beforeLineEl); + this.$.content.insertBefore(fillerEl, beforeContentEl); }, - _renderCode: function(code) { + _renderCode: function(code, index, beforeLineEl, + beforeContentEl) { var lineNumEl = this._createElement('div', 'lineNum'); lineNumEl.setAttribute('data-line-num', code.lineNum); + lineNumEl.setAttribute('data-index', index); var numLines = code.numLines || 1; lineNumEl.textContent = code.lineNum + '\n'.repeat(numLines); var contentEl = this._createElement('div', 'code'); contentEl.setAttribute('data-line-num', code.lineNum); + contentEl.setAttribute('data-index', index); if (code.highlight) { contentEl.classList.add(code.intraline.length > 0 ? @@ -218,8 +298,8 @@ limitations under the License. contentEl.innerHTML = html; } - this.$.numbers.appendChild(lineNumEl); - this.$.content.appendChild(contentEl); + this.$.numbers.insertBefore(lineNumEl, beforeLineEl); + this.$.content.insertBefore(contentEl, beforeContentEl); }, // Advance `index` by the appropriate number of characters that would @@ -321,7 +401,10 @@ limitations under the License. // Since the Polymer DOM utility functions (which would do this // automatically) are not being used for performance reasons, this is // done manually. - el.classList.add('style-scope', 'gr-diff-side', className); + el.classList.add('style-scope', 'gr-diff-side'); + if (!!className) { + el.classList.add(className); + } return el; }, }); diff --git a/polygerrit-ui/app/elements/gr-diff.html b/polygerrit-ui/app/elements/gr-diff.html index ebc67b991f..5462c9c5f8 100644 --- a/polygerrit-ui/app/elements/gr-diff.html +++ b/polygerrit-ui/app/elements/gr-diff.html @@ -23,9 +23,14 @@ limitations under the License. diff --git a/polygerrit-ui/app/test/gr-diff-test.html b/polygerrit-ui/app/test/gr-diff-test.html index b3a56d8e37..d1e5ad906e 100644 --- a/polygerrit-ui/app/test/gr-diff-test.html +++ b/polygerrit-ui/app/test/gr-diff-test.html @@ -264,5 +264,81 @@ limitations under the License. } ]); }); + + test('context', function() { + element._context = 3; + element._diffResponse = { + content: [ + { + ab: [ + '', + '', + 'My great page', + '', + '
', + ] + }, + { + a: [ + ' Welcome ', + ' to the wooorld of tomorrow!', + ], + b: [ + ' Hello, world!', + ], + }, + { + ab: [ + '
', + '', + 'Leela: This is the only place the ship can’t hear us, so ', + 'everyone pretend to shower.', + 'Fry: Same as every day. Got it.', + ] + }, + ] + }; + element._processContent(); + + // First eight lines should be hidden on both sides. + for (var i = 0; i < 8; i++) { + assert.isTrue(element._diff.leftSide[i].hidden); + assert.isTrue(element._diff.rightSide[i].hidden); + } + // A context control should be at index 8 on both sides. + var leftContext = element._diff.leftSide[8]; + var rightContext = element._diff.rightSide[8]; + assert.deepEqual(leftContext, rightContext); + assert.equal(leftContext.numLines, 8); + assert.equal(leftContext.start, 0); + assert.equal(leftContext.end, 8); + + // Line indices 9-16 should be shown. + for (var i = 9; i <= 16; i++) { + // notOk (falsy) because the `hidden` attribute may not be present. + assert.notOk(element._diff.leftSide[i].hidden); + assert.notOk(element._diff.rightSide[i].hidden); + } + + // Lines at indices 17 and 18 should be hidden. + assert.isTrue(element._diff.leftSide[17].hidden); + assert.isTrue(element._diff.rightSide[17].hidden); + assert.isTrue(element._diff.leftSide[18].hidden); + assert.isTrue(element._diff.rightSide[18].hidden); + + // Context control at index 19. + leftContext = element._diff.leftSide[19]; + rightContext = element._diff.rightSide[19]; + assert.deepEqual(leftContext, rightContext); + assert.equal(leftContext.numLines, 2); + assert.equal(leftContext.start, 17); + assert.equal(leftContext.end, 19); + }); });