Merge "Support jumping to next/previous file with comments via shortcut"
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>
