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
diff --git a/polygerrit-ui/app/elements/gr-diff-side.html b/polygerrit-ui/app/elements/gr-diff-side.html index 0120d3b..642a02f 100644 --- a/polygerrit-ui/app/elements/gr-diff-side.html +++ b/polygerrit-ui/app/elements/gr-diff-side.html
@@ -65,8 +65,23 @@ /* >> 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; } </style> <div class$="[[_computeContainerClass(canComment)]]"> @@ -90,6 +105,12 @@ Polymer({ is: 'gr-diff-side', + /** + * Fired when an expand context control is clicked. + * + * @event expand-context + */ + properties: { canComment: { type: Boolean, @@ -98,7 +119,7 @@ content: { type: Array, notify: true, - observer: '_render', + observer: '_contentChanged', }, width: { type: Number, @@ -143,10 +164,28 @@ 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 @@ } }, - _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 @@ 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 @@ 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 @@ // 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 ebc67b9..5462c9c 100644 --- a/polygerrit-ui/app/elements/gr-diff.html +++ b/polygerrit-ui/app/elements/gr-diff.html
@@ -23,9 +23,14 @@ <dom-module id="gr-diff"> <template> <style> - gr-patch-range-select { + .header { + display: flex; margin: 0 var(--default-horizontal-margin) .75em; } + .contextControl { + flex: 1; + text-align: right; + } .diffContainer { border-bottom: 1px solid #eee; border-top: 1px solid #eee; @@ -34,12 +39,12 @@ white-space: pre; } gr-diff-side:first-of-type { - --light-highlight-color: #ffecec; - --dark-highlight-color: #faa; + --light-highlight-color: #fee; + --dark-highlight-color: #ffd4d4; } gr-diff-side:last-of-type { - --light-highlight-color: #eaffea; - --dark-highlight-color: #9f9; + --light-highlight-color: #efe; + --dark-highlight-color: #d4ffd4; border-right: 1px solid #ddd; } </style> @@ -56,20 +61,37 @@ <gr-ajax id="draftsXHR" url="[[_computeDraftsPath(changeNum, patchRange.patchNum)]]"></gr-ajax> - <gr-patch-range-select - path="[[path]]" - change-num="[[changeNum]]" - patch-range="[[patchRange]]" - available-patches="[[availablePatches]]"></gr-patch-range-select> + <div class="header"> + <gr-patch-range-select + path="[[path]]" + change-num="[[changeNum]]" + patch-range="[[patchRange]]" + available-patches="[[availablePatches]]"></gr-patch-range-select> + <div class="contextControl"> + Context: + <select id="contextSelect" on-change="_handleContextSelectChange"> + <option value="3">3 lines</option> + <option value="10">10 lines</option> + <option value="25">25 lines</option> + <option value="50">50 lines</option> + <option value="75">75 lines</option> + <option value="100">100 lines</option> + <option value="ALL">Whole file</option> + </select> + </div> + </div> + <div class="diffContainer"> <gr-diff-side id="leftDiff" content="{{_diff.leftSide}}" width="[[sideWidth]]" - can-comment="[[_loggedIn]]"></gr-diff-side> + can-comment="[[_loggedIn]]" + on-expand-context="_handleExpandContext"></gr-diff-side> <gr-diff-side id="rightDiff" content="{{_diff.rightSide}}" width="[[sideWidth]]" - can-comment="[[_loggedIn]]"></gr-diff-side> + can-comment="[[_loggedIn]]" + on-expand-context="_handleExpandContext"></gr-diff-side> </div> </template> <script> @@ -104,6 +126,11 @@ value: 80, }, _comments: Array, + _context: { + type: Number, + value: 10, + observer: '_contextChanged', + }, _baseComments: Array, _drafts: Array, _baseDrafts: Array, @@ -134,6 +161,14 @@ this.$.rightDiff.scrollToLine(lineNum); }, + _contextChanged: function(context) { + if (context == Infinity) { + this.$.contextSelect.value = 'ALL'; + } else { + this.$.contextSelect.value = context; + } + }, + _diffOptionsChanged: function(changeNum, patchRange, path) { if (!this.auto) { return; } @@ -145,7 +180,7 @@ app.accountReady.then(function() { promises.push(this._getCommentsAndDrafts(basePatchNum, app.loggedIn)); this._diffRequestsPromise = Promise.all(promises).then(function() { - this._allDataReceived(); + this._render(); }.bind(this)).catch(function(err) { alert('Oops. Something went wrong. Check the console and bug the ' + 'PolyGerrit team for assistance.'); @@ -154,7 +189,7 @@ }.bind(this)); }, - _allDataReceived: function() { + _render: function() { this._processContent(); // Allow for the initial rendering to complete before firing the event. @@ -237,11 +272,43 @@ return params; }, + _handleContextSelectChange: function(e) { + if (e.target.value == 'ALL') { + this._context = Infinity; + } else { + this._context = parseInt(e.target.value, 10); + } + this._render(); + }, + + _handleExpandContext: function(e) { + var ctx = e.detail.context; + var contextControlIndex = -1; + for (var i = ctx.start; i <= ctx.end; i++) { + this._diff.leftSide[i].hidden = false; + this._diff.rightSide[i].hidden = false; + if (this._diff.leftSide[i].type == 'CONTEXT_CONTROL' && + this._diff.rightSide[i].type == 'CONTEXT_CONTROL') { + contextControlIndex = i; + } + } + this._diff.leftSide[contextControlIndex].hidden = true; + this._diff.rightSide[contextControlIndex].hidden = true; + + this.$.leftDiff.hideElementsWithIndex(contextControlIndex); + this.$.rightDiff.hideElementsWithIndex(contextControlIndex); + + this.$.leftDiff.renderLineIndexRange(ctx.start, ctx.end); + this.$.rightDiff.renderLineIndexRange(ctx.start, ctx.end); + }, + _processContent: function() { var leftSide = []; var rightSide = []; var initialLineNum = 0 + (this._diffResponse.content.skip || 0); var ctx = { + hidingLines: false, + lastNumLinesHidden: 0, left: { lineNum: initialLineNum, }, @@ -249,9 +316,17 @@ lineNum: initialLineNum, } }; - for (var i = 0; i < this._diffResponse.content.length; i++) { - this._addDiffChunk(ctx, this._diffResponse.content[i], leftSide, - rightSide); + var content = this._diffResponse.content; + for (var i = 0; i < content.length; i++) { + if (i == 0) { + ctx.skipRange = [0, this._context]; + } else if (i == content.length - 1) { + ctx.skipRange = [this._context, 0]; + } else { + ctx.skipRange = [this._context, this._context]; + } + ctx.diffChunkIndex = i; + this._addDiffChunk(ctx, content[i], leftSide, rightSide); } this._diff = { leftSide: leftSide, @@ -259,24 +334,71 @@ }; }, + _addContextControl: function(ctx, leftSide, rightSide) { + var numLinesHidden = ctx.lastNumLinesHidden; + var leftStart = leftSide.length - numLinesHidden; + var leftEnd = leftSide.length; + var rightStart = rightSide.length - numLinesHidden; + var rightEnd = rightSide.length; + if (leftStart != rightStart || leftEnd != rightEnd) { + throw Error( + 'Left and right ranges for context control should be equal:' + + 'Left: [' + leftStart + ', ' + leftEnd + '] ' + + 'Right: ['+ rightStart + ', ' + rightEnd + ']'); + } + var obj = { + type: 'CONTEXT_CONTROL', + numLines: numLinesHidden, + start: leftStart, + end: leftEnd, + }; + // NOTE: Be careful, here. This object is meant to be immutable. If the + // object is altered within one side's array it will reflect the + // alterations in another. + leftSide.push(obj); + rightSide.push(obj); + }, + + _addCommonDiffChunk: function(ctx, chunk, leftSide, rightSide) { + for (var i = 0; i < chunk.ab.length; i++) { + var numLines = Math.ceil(chunk.ab[i].length / this.sideWidth); + var hidden = i >= ctx.skipRange[0] && + i < chunk.ab.length - ctx.skipRange[1]; + if (ctx.hidingLines && hidden == false) { + // No longer hiding lines. Add a context control. + this._addContextControl(ctx, leftSide, rightSide); + ctx.lastNumLinesHidden = 0; + } + ctx.hidingLines = hidden; + if (hidden) { + ctx.lastNumLinesHidden++; + } + + // Blank lines within a diff content array indicate a newline. + leftSide.push({ + type: 'CODE', + hidden: hidden, + content: chunk.ab[i] || '\n', + numLines: numLines, + lineNum: ++ctx.left.lineNum, + }); + rightSide.push({ + type: 'CODE', + hidden: hidden, + content: chunk.ab[i] || '\n', + numLines: numLines, + lineNum: ++ctx.right.lineNum, + }); + } + if (ctx.lastNumLinesHidden > 0) { + this._addContextControl(ctx, leftSide, rightSide); + } + }, + _addDiffChunk: function(ctx, chunk, leftSide, rightSide) { if (chunk.ab) { - for (var i = 0; i < chunk.ab.length; i++) { - var numLines = Math.ceil(chunk.ab[i].length / this.sideWidth); - // Blank lines within a diff content array indicate a newline. - leftSide.push({ - type: 'CODE', - content: chunk.ab[i] || '\n', - numLines: numLines, - lineNum: ++ctx.left.lineNum, - }); - rightSide.push({ - type: 'CODE', - content: chunk.ab[i] || '\n', - numLines: numLines, - lineNum: ++ctx.right.lineNum, - }); - } + this._addCommonDiffChunk(ctx, chunk, leftSide, rightSide); + return; } var leftHighlights = [];
diff --git a/polygerrit-ui/app/test/gr-diff-side-test.html b/polygerrit-ui/app/test/gr-diff-side-test.html index b54461b..36eed2f 100644 --- a/polygerrit-ui/app/test/gr-diff-side-test.html +++ b/polygerrit-ui/app/test/gr-diff-side-test.html
@@ -65,7 +65,7 @@ intraline: [], }); } - element._render(content); + element.content = content; window.scrollTo(0, 0); element.scrollToLine(-12849); @@ -113,7 +113,7 @@ highlight: false, intraline: [], }]; - element._render(content); + element.content = content; var lineEl = element.$$('.numbers .lineNum[data-line-num="1"]'); assert.ok(lineEl); @@ -129,7 +129,7 @@ highlight: false, intraline: [], }]; - element._render(content); + element.content = content; lineEl = element.$$('.numbers .lineNum[data-line-num="1"]'); assert.ok(lineEl); @@ -140,5 +140,48 @@ 'A'.repeat(20) + element._lineFeedHTML); }); + + test('diff context', function() { + var content = [ + {type: 'CODE', hidden: true, content: '<!DOCTYPE html>'}, + {type: 'CODE', hidden: true, content: '<meta charset="utf-8">'}, + {type: 'CODE', hidden: true, content: '<title>My great page</title>'}, + {type: 'CODE', hidden: true, content: '<style>'}, + {type: 'CODE', hidden: true, content: ' *,'}, + {type: 'CODE', hidden: true, content: ' *:before,'}, + {type: 'CODE', hidden: true, content: ' *:after {'}, + {type: 'CODE', hidden: true, content: ' box-sizing: border-box;'}, + {type: 'CONTEXT_CONTROL', numLines: 8, start: 0, end: 8}, + {type: 'CODE', hidden: false, content: ' }'}, + ]; + element.content = content; + + // Only the context elements and the following code line elements should + // be present in the DOM. + var contextEls = + Polymer.dom(element.root).querySelectorAll('.contextControl'); + assert.equal(contextEls.length, 2); + var codeLineEls = + Polymer.dom(element.root).querySelectorAll('.lineNum, .code'); + assert.equal(codeLineEls.length, 2); + + for (var i = 0; i <= 8; i++) { + element.content[i].hidden = false; + } + element.renderLineIndexRange(0, 8); + element.hideElementsWithIndex(8); + + contextEls = + Polymer.dom(element.root).querySelectorAll('.contextControl'); + for (var i = 0; i < contextEls.length; i++) { + assert.isTrue(contextEls[i].hasAttribute('hidden')); + } + + codeLineEls = + Polymer.dom(element.root).querySelectorAll('.lineNum, .code'); + + // Nine lines should now be present in the DOM. + assert.equal(codeLineEls.length, 9 * 2); + }); }); </script>
diff --git a/polygerrit-ui/app/test/gr-diff-test.html b/polygerrit-ui/app/test/gr-diff-test.html index b3a56d8..d1e5ad9 100644 --- a/polygerrit-ui/app/test/gr-diff-test.html +++ b/polygerrit-ui/app/test/gr-diff-test.html
@@ -264,5 +264,81 @@ } ]); }); + + test('context', function() { + element._context = 3; + element._diffResponse = { + content: [ + { + ab: [ + '<!DOCTYPE html>', + '<meta charset="utf-8">', + '<title>My great page</title>', + '<style>', + ' *,', + ' *:before,', + ' *:after {', + ' box-sizing: border-box;', + ' }', + '</style>', + '<header>', + ] + }, + { + a: [ + ' Welcome ', + ' to the wooorld of tomorrow!', + ], + b: [ + ' Hello, world!', + ], + }, + { + ab: [ + '</header>', + '<body>', + '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); + }); }); </script>