opendev/gerrit
Code Issues Proposed changes

[Diff view] Implement n/p and shift-n/p for jumping around

Browse Source 
+ Also update iron-test-helpers to 1.1.5 to be able to use
  modifiers in fake key events.

Bug: Issue 3925
Change-Id: I41ce2efe0b5df63a2a637e0942a97e9dafc432f9
This commit is contained in:
Andrew Bonventre
2016-02-25 18:19:51 -05:00
parent 46c5b47148
commit 15f796c7a4
7 changed files with 190 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
lib/js/BUCK
View File

@@ -229,10 +229,10 @@ bower_component(
bower_component(
name = 'iron-test-helpers',
package = 'polymerelements/iron-test-helpers',
version = '1.0.6',
version = '1.1.5',
deps = [':polymer'],
license = 'DO_NOT_DISTRIBUTE',
sha1 = 'c0f7c7f010ca3c63fb08ae0d9462e400380cde2c',
sha1 = '000e2256ae487e4d24edfb6d17dc98626bb8a8e2',
)
bower_component(

74
polygerrit-ui/app/elements/gr-diff-side.html
View File

@@ -170,6 +170,18 @@ limitations under the License.
value: '</hl>',
readOnly: true,
},
_diffChunkLineNums: {
type: Array,
value: function() { return []; },
},
_commentThreadLineNums: {
type: Array,
value: function() { return []; },
},
_focusedLineNum: {
type: Number,
value: 1,
},
},
listeners: {
@@ -183,6 +195,7 @@ limitations under the License.
rowInserted: function(index) {
this.renderLineIndexRange(index, index);
this._updateDOMIndices();
this._updateJumpIndices();
},
rowRemoved: function(index) {
@@ -192,6 +205,7 @@ limitations under the License.
removedEls[i].parentNode.removeChild(removedEls[i]);
}
this._updateDOMIndices();
this._updateJumpIndices();
},
rowUpdated: function(index) {
@@ -224,6 +238,22 @@ limitations under the License.
window.scrollTo(0, top - (window.innerHeight / 3) - el.offsetHeight);
},
scrollToNextDiffChunk: function() {
this._scrollToNextChunkOrThread(this._diffChunkLineNums);
},
scrollToPreviousDiffChunk: function() {
this._scrollToPreviousChunkOrThread(this._diffChunkLineNums);
},
scrollToNextCommentThread: function() {
this._scrollToNextChunkOrThread(this._commentThreadLineNums);
},
scrollToPreviousCommentThread: function() {
this._scrollToPreviousChunkOrThread(this._commentThreadLineNums);
},
renderLineIndexRange: function(startIndex, endIndex) {
this._render(this.content, startIndex, endIndex);
},
@@ -279,6 +309,49 @@ limitations under the License.
this.content[index].height = height;
},
_scrollToNextChunkOrThread: function(lineNums) {
for (var i = 0; i < lineNums.length; i++) {
if (lineNums[i] > this._focusedLineNum) {
this._focusedLineNum = lineNums[i];
this.scrollToLine(this._focusedLineNum);
return;
}
}
},
_scrollToPreviousChunkOrThread: function(lineNums) {
for (var i = lineNums.length - 1; i >= 0; i--) {
if (this._focusedLineNum > lineNums[i]) {
this._focusedLineNum = lineNums[i];
this.scrollToLine(this._focusedLineNum);
return;
}
}
},
_updateJumpIndices: function() {
this._commentThreadLineNums = [];
this._diffChunkLineNums = [];
var inHighlight = false;
for (var i = 0; i < this.content.length; i++) {
switch (this.content[i].type) {
case 'COMMENT_THREAD':
this._commentThreadLineNums.push(
this.content[i].comments[0].line);
break;
case 'CODE':
// Only grab the first line of the highlighted chunk.
if (!inHighlight && this.content[i].highlight) {
this._diffChunkLineNums.push(this.content[i].lineNum);
inHighlight = true;
} else if (!this.content[i].highlight) {
inHighlight = false;
}
break;
}
}
},
_updateDOMIndices: function() {
// There is no way to select elements with a data-index greater than a
// given value. For now, just update all DOM elements.
@@ -317,6 +390,7 @@ limitations under the License.
this._clearChildren(this.$.numbers);
this._clearChildren(this.$.content);
this._render(diff, 0, diff.length - 1);
this._updateJumpIndices();
},
_computeContainerClass: function(canComment) {

14
polygerrit-ui/app/elements/gr-diff-view.html
View File

@@ -276,6 +276,20 @@ limitations under the License.
e.preventDefault();
this._navToFile(this._fileList, 1);
break;
case 78: // 'n'
if (e.shiftKey) {
this.$.diff.scrollToNextCommentThread();
} else {
this.$.diff.scrollToNextDiffChunk();
}
break;
case 80: // 'p'
if (e.shiftKey) {
this.$.diff.scrollToPreviousCommentThread();
} else {
this.$.diff.scrollToPreviousDiffChunk();
}
break;
case 65: // 'a'
if (!this._loggedIn) { return; }

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

@@ -162,6 +162,7 @@ limitations under the License.
_baseDrafts: Array,
/**
* Base (left side) comments and drafts grouped by line number.
* Only used for initial rendering.
*/
_groupedBaseComments: {
type: Object,
@@ -169,6 +170,7 @@ limitations under the License.
},
/**
* Comments and drafts (right side) grouped by line number.
* Only used for initial rendering.
*/
_groupedComments: {
type: Object,
@@ -216,6 +218,22 @@ limitations under the License.
this.$.rightDiff.scrollToLine(lineNum);
},
scrollToNextDiffChunk: function() {
this.$.rightDiff.scrollToNextDiffChunk();
},
scrollToPreviousDiffChunk: function() {
this.$.rightDiff.scrollToPreviousDiffChunk();
},
scrollToNextCommentThread: function() {
this.$.rightDiff.scrollToNextCommentThread();
},
scrollToPreviousCommentThread: function() {
this.$.rightDiff.scrollToPreviousCommentThread();
},
reload: function(changeNum, patchRange, path) {
// If a diff takes a considerable amount of time to render, the previous
// diff can end up showing up while the DOM is constructed. Clear the

30
polygerrit-ui/app/elements/gr-keyboard-shortcuts-dialog.html
View File

@@ -58,6 +58,9 @@ limitations under the License.
padding: .1em .5em;
text-align: center;
}
.modifier {
font-weight: normal;
}
</style>
<header>
<h3>Keyboard shortcuts</h3>
@@ -78,6 +81,7 @@ limitations under the License.
<td>Show this dialog</td>
</tr>
</tbody>
<!-- Change View -->
<tbody hidden$="[[!_computeInView(view, 'gr-change-view')]]" hidden>
<tr>
<td></td><td class="header">Navigation</td>
@@ -95,6 +99,7 @@ limitations under the License.
<td>Up to change list</td>
</tr>
</tbody>
<!-- Diff View -->
<tbody hidden$="[[!_computeInView(view, 'gr-diff-view')]]" hidden>
<tr>
<td></td><td class="header">Navigation</td>
@@ -115,6 +120,7 @@ limitations under the License.
</table>
<table>
<!-- Change List and Dashboard -->
<tbody hidden$="[[!_computeInChangeListView(view)]]" hidden>
<tr>
<td></td><td class="header">Change list</td>
@@ -132,6 +138,7 @@ limitations under the License.
<td>Show selected change</td>
</tr>
</tbody>
<!-- Change View -->
<tbody hidden$="[[!_computeInView(view, 'gr-change-view')]]" hidden>
<tr>
<td></td><td class="header">Actions</td>
@@ -156,10 +163,33 @@ limitations under the License.
<td>Show selected file</td>
</tr>
</tbody>
<!-- Diff View -->
<tbody hidden$="[[!_computeInView(view, 'gr-diff-view')]]" hidden>
<tr>
<td></td><td class="header">Actions</td>
</tr>
<tr>
<td><span class="key">n</span></td>
<td>Show next diff chunk</td>
</tr>
<tr>
<td><span class="key">p</span></td>
<td>Show previous diff chunk</td>
</tr>
<tr>
<td>
<span class="key modifier">Shift</span>
<span class="key">n</span>
</td>
<td>Show next comment thread</td>
</tr>
<tr>
<td>
<span class="key modifier">Shift</span>
<span class="key">p</span>
</td>
<td>Show previous comment thread</td>
</tr>
<tr>
<td><span class="key">a</span></td>
<td>Review and publish comments</td>

32
polygerrit-ui/app/test/gr-diff-side-test.html
View File

@@ -264,5 +264,37 @@ limitations under the License.
MockInteractions.tap(lineEl);
assert.equal(numAddDraftEvents, 1);
});
test('jump to diff chunk/thread', function() {
element.content = [
{type: 'CODE', content: '', intraline: [], lineNum: 1, highlight: true},
{type: 'CODE', content: '', intraline: [], lineNum: 2, highlight: true},
{type: 'CODE', content: '', intraline: [], lineNum: 3 },
{type: 'CODE', content: '', intraline: [], lineNum: 4 },
{type: 'COMMENT_THREAD', comments: [ { line: 4 }]},
{type: 'CODE', content: '', intraline: [], lineNum: 5 },
{type: 'CODE', content: '', intraline: [], lineNum: 6, highlight: true},
{type: 'CODE', content: '', intraline: [], lineNum: 7, highlight: true},
{type: 'CODE', content: '', intraline: [], lineNum: 8 },
{type: 'COMMENT_THREAD', comments: [ { line: 8 }]},
{type: 'CODE', content: '', intraline: [], lineNum: 9 },
{type: 'CODE', content: '', intraline: [], lineNum: 10,
highlight: true},
];
var scrollToLineStub = sinon.stub(element, 'scrollToLine');
element.scrollToNextDiffChunk();
assert.isTrue(scrollToLineStub.lastCall.calledWithExactly(6));
element.scrollToPreviousDiffChunk();
assert.isTrue(scrollToLineStub.lastCall.calledWithExactly(1));
element.scrollToNextCommentThread();
assert.isTrue(scrollToLineStub.lastCall.calledWithExactly(4));
element.scrollToNextCommentThread();
assert.isTrue(scrollToLineStub.lastCall.calledWithExactly(8));
element.scrollToPreviousDiffChunk();
assert.isTrue(scrollToLineStub.lastCall.calledWithExactly(6));
scrollToLineStub.restore();
});
});
</script>

20
polygerrit-ui/app/test/gr-diff-view-test.html
View File

@@ -117,6 +117,26 @@ limitations under the License.
MockInteractions.pressAndReleaseKeyOn(element, 188); // ','
assert(showPrefsStub.calledOnce);
var scrollStub = sinon.stub(element.$.diff, 'scrollToNextDiffChunk');
MockInteractions.pressAndReleaseKeyOn(element, 78); // 'n'
assert(scrollStub.calledOnce);
scrollStub.restore();
scrollStub = sinon.stub(element.$.diff, 'scrollToPreviousDiffChunk');
MockInteractions.pressAndReleaseKeyOn(element, 80); // 'p'
assert(scrollStub.calledOnce);
scrollStub.restore();
scrollStub = sinon.stub(element.$.diff, 'scrollToNextCommentThread');
MockInteractions.pressAndReleaseKeyOn(element, 78, ['shift']); // 'N'
assert(scrollStub.calledOnce);
scrollStub.restore();
scrollStub = sinon.stub(element.$.diff, 'scrollToPreviousCommentThread');
MockInteractions.pressAndReleaseKeyOn(element, 80, ['shift']); // 'P'
assert(scrollStub.calledOnce);
scrollStub.restore();
showPrefsStub.restore();
showStub.restore();
});