From 1bc4f2f5655e82e15cf6fd90720977b9aa1ae568 Mon Sep 17 00:00:00 2001 From: Wyatt Allen Date: Wed, 18 Jan 2017 17:10:51 -0800 Subject: [PATCH] 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 --- .../gr-keyboard-shortcuts-dialog.html | 14 +++ .../diff/gr-diff-view/gr-diff-view.js | 113 +++++++++++++++++- .../diff/gr-diff-view/gr-diff-view_test.html | 101 ++++++++++++++++ 3 files changed, 224 insertions(+), 4 deletions(-) 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 7a7e761901..7ab74a588d 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 @@ -112,6 +112,20 @@ limitations under the License. [ Show previous file + + + Shift + j + + Show next file that has comments + + + + Shift + k + + Show previous file that has comments + u Up to change 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 600415a59f..63e357470b 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 f6e4765628..99dcd41ecb 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 @@ limitations under the License. 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); + }); + }); });