Support jumping to next/previous file with comments via shortcut
Adds keyboard shortcuts to the diff view to navigate to the next or
previous file in the change's file list that has comments in the current
patch range.
Feature: Issue 5235
Change-Id: I1ad39089c1ac227e335093f25b72311f7e98b3f7
diff --git a/polygerrit-ui/app/elements/core/gr-keyboard-shortcuts-dialog/gr-keyboard-shortcuts-dialog.html b/polygerrit-ui/app/elements/core/gr-keyboard-shortcuts-dialog/gr-keyboard-shortcuts-dialog.html
index 7a7e761..7ab74a5 100644
--- a/polygerrit-ui/app/elements/core/gr-keyboard-shortcuts-dialog/gr-keyboard-shortcuts-dialog.html
+++ b/polygerrit-ui/app/elements/core/gr-keyboard-shortcuts-dialog/gr-keyboard-shortcuts-dialog.html
@@ -113,6 +113,20 @@
<td>Show previous file</td>
</tr>
<tr>
+ <td>
+ <span class="key modifier">Shift</span>
+ <span class="key">j</span>
+ </td>
+ <td>Show next file that has comments</td>
+ </tr>
+ <tr>
+ <td>
+ <span class="key modifier">Shift</span>
+ <span class="key">k</span>
+ </td>
+ <td>Show previous file that has comments</td>
+ </tr>
+ <tr>
<td><span class="key">u</span></td>
<td>Up to change</td>
</tr>
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
index 600415a..63e3574 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
@@ -80,6 +80,21 @@
},
_isImageDiff: Boolean,
_filesWeblinks: Object,
+
+ /**
+ * Map of paths in the current chnage and patch range that have comments
+ * or drafts or robot comments.
+ */
+ _commentMap: Object,
+
+ /**
+ * Object to contain the path of the next and previous file in the current
+ * change and patch range that has comments.
+ */
+ _commentSkips: {
+ type: Object,
+ computed: '_computeCommentSkips(_commentMap, _fileList, _path)',
+ },
},
behaviors: [
@@ -210,8 +225,13 @@
},
_handleUpKey: function(e) {
- if (this.shouldSuppressKeyboardShortcut(e) ||
- this.modifierPressed(e)) { return; }
+ if (this.shouldSuppressKeyboardShortcut(e)) { return; }
+ if (e.detail.keyboardEvent.shiftKey &&
+ e.detail.keyboardEvent.keyCode === 75) { // 'K'
+ this._moveToPreviousFileWithComment();
+ return;
+ }
+ if (this.modifierPressed(e)) { return; }
e.preventDefault();
this.$.diff.displayLine = true;
@@ -219,14 +239,33 @@
},
_handleDownKey: function(e) {
- if (this.shouldSuppressKeyboardShortcut(e) ||
- this.modifierPressed(e)) { return; }
+ if (this.shouldSuppressKeyboardShortcut(e)) { return; }
+ if (e.detail.keyboardEvent.shiftKey &&
+ e.detail.keyboardEvent.keyCode === 74) { // 'J'
+ this._moveToNextFileWithComment();
+ return;
+ }
+ if (this.modifierPressed(e)) { return; }
e.preventDefault();
this.$.diff.displayLine = true;
this.$.cursor.moveDown();
},
+ _moveToPreviousFileWithComment: function() {
+ if (this._commentSkips && this._commentSkips.previous) {
+ page.show(this._getDiffURL(this._changeNum, this._patchRange,
+ this._commentSkips.previous));
+ }
+ },
+
+ _moveToNextFileWithComment: function() {
+ if (this._commentSkips && this._commentSkips.next) {
+ page.show(this._getDiffURL(this._changeNum, this._patchRange,
+ this._commentSkips.next));
+ }
+ },
+
_handleCKey: function(e) {
if (this.shouldSuppressKeyboardShortcut(e)) { return; }
if (this.$.diff.isRangeSelected()) { return; }
@@ -409,6 +448,10 @@
Promise.all(promises)
.then(function() { return this.$.diff.reload(); }.bind(this))
.then(function() { this._loading = false; }.bind(this));
+
+ this._loadCommentMap().then(function(commentMap) {
+ this._commentMap = commentMap;
+ }.bind(this));
},
/**
@@ -596,5 +639,67 @@
url += '/patch?zip&path=' + encodeURIComponent(path);
return url;
},
+
+ /**
+ * Request all comments (and drafts and robot comments) for the current
+ * change and construct the map of file paths that have comments for the
+ * current patch range.
+ * @return {Promise} A promise that yields a comment map object.
+ */
+ _loadCommentMap: function() {
+ function filterByRange(comment) {
+ var patchNum = comment.patch_set + '';
+ return patchNum === this._patchRange.patchNum ||
+ patchNum === this._patchRange.basePatchNum;
+ };
+
+ return Promise.all([
+ this.$.restAPI.getDiffComments(this._changeNum),
+ this._getDiffDrafts(),
+ this.$.restAPI.getDiffRobotComments(this._changeNum),
+ ]).then(function(results) {
+ var commentMap = {};
+ results.forEach(function(response) {
+ for (var path in response) {
+ if (response.hasOwnProperty(path) &&
+ response[path].filter(filterByRange.bind(this)).length) {
+ commentMap[path] = true;
+ }
+ }
+ }.bind(this));
+ return commentMap;
+ }.bind(this));
+ },
+
+ _getDiffDrafts: function() {
+ return this._getLoggedIn().then(function(loggedIn) {
+ if (!loggedIn) { return Promise.resolve({}); }
+ return this.$.restAPI.getDiffDrafts(this._changeNum);
+ }.bind(this));
+ },
+
+ _computeCommentSkips: function(commentMap, fileList, path) {
+ var skips = {previous: null, next: null};
+ if (!fileList.length) { return skips; }
+ var pathIndex = fileList.indexOf(path);
+
+ // Scan backward for the previous file.
+ for (var i = pathIndex - 1; i >= 0; i--) {
+ if (commentMap[fileList[i]]) {
+ skips.previous = fileList[i];
+ break;
+ }
+ }
+
+ // Scan forward for the next file.
+ for (i = pathIndex + 1; i < fileList.length; i++) {
+ if (commentMap[fileList[i]]) {
+ skips.next = fileList[i];
+ break;
+ }
+ }
+
+ return skips;
+ },
});
})();
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html
index f6e4765..99dcd41 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.html
@@ -584,5 +584,106 @@
element.changeViewState = {diffMode: 'SIDE_BY_SIDE'};
assert.equal(element._getDiffViewMode(), 'SIDE_BY_SIDE');
});
+
+ suite('_loadCommentMap', function() {
+ test('empty', function(done) {
+ stub('gr-rest-api-interface', {
+ getDiffRobotComments: function() { return Promise.resolve({}); },
+ getDiffComments: function() { return Promise.resolve({}); },
+ });
+ element._loadCommentMap().then(function(map) {
+ assert.equal(Object.keys(map).length, 0);
+ done();
+ });
+ });
+
+ test('paths in patch range', function(done) {
+ stub('gr-rest-api-interface', {
+ getDiffRobotComments: function() { return Promise.resolve({}); },
+ getDiffComments: function() {
+ return Promise.resolve({
+ 'path/to/file/one.cpp': [{patch_set: 3, message: 'lorem'}],
+ 'path-to/file/two.py': [{patch_set: 5, message: 'ipsum'}],
+ });
+ },
+ });
+ element._changeNum = '42';
+ element._patchRange = {
+ basePatchNum: '3',
+ patchNum: '5',
+ };
+ element._loadCommentMap().then(function(map) {
+ assert.deepEqual(Object.keys(map),
+ ['path/to/file/one.cpp', 'path-to/file/two.py']);
+ done();
+ });
+ });
+
+ test('empty for paths outside patch range', function(done) {
+ stub('gr-rest-api-interface', {
+ getDiffRobotComments: function() { return Promise.resolve({}); },
+ getDiffComments: function() {
+ return Promise.resolve({
+ 'path/to/file/one.cpp': [{patch_set: 'PARENT', message: 'lorem'}],
+ 'path-to/file/two.py': [{patch_set: 2, message: 'ipsum'}],
+ });
+ },
+ });
+ element._changeNum = '42';
+ element._patchRange = {
+ basePatchNum: '3',
+ patchNum: '5',
+ };
+ element._loadCommentMap().then(function(map) {
+ assert.equal(Object.keys(map).length, 0);
+ done();
+ });
+ });
+ });
+
+ suite('_computeCommentSkips', function() {
+ test('empty file list', function() {
+ var commentMap = {
+ 'path/one.jpg': true,
+ 'path/three.wav': true,
+ };
+ var path = 'path/two.m4v';
+ var fileList = [];
+ var result = element._computeCommentSkips(commentMap, fileList, path);
+ assert.isNull(result.previous);
+ assert.isNull(result.next);
+ });
+
+ test('finds skips', function() {
+ var fileList = ['path/one.jpg', 'path/two.m4v', 'path/three.wav'];
+ var path = fileList[1];
+ var commentMap = {};
+ commentMap[fileList[0]] = true;
+ commentMap[fileList[1]] = false;
+ commentMap[fileList[2]] = true;
+
+ var result = element._computeCommentSkips(commentMap, fileList, path);
+ assert.equal(result.previous, fileList[0]);
+ assert.equal(result.next, fileList[2]);
+
+ commentMap[fileList[1]] = true;
+
+ result = element._computeCommentSkips(commentMap, fileList, path);
+ assert.equal(result.previous, fileList[0]);
+ assert.equal(result.next, fileList[2]);
+
+ path = fileList[0];
+
+ result = element._computeCommentSkips(commentMap, fileList, path);
+ assert.isNull(result.previous);
+ assert.equal(result.next, fileList[1]);
+
+ path = fileList[2];
+
+ result = element._computeCommentSkips(commentMap, fileList, path);
+ assert.equal(result.previous, fileList[1]);
+ assert.isNull(result.next);
+ });
+ });
});
</script>
