opendev/gerrit
Code Issues Proposed changes

Support jumping to next/previous file with comments via shortcut

Browse Source 
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
This commit is contained in:
Wyatt Allen
2017-01-18 17:10:51 -08:00
parent d6d67bb740
commit 1bc4f2f565
3 changed files with 224 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -112,6 +112,20 @@ limitations under the License.
<td><span class="key">[</span></td>
<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>

113
polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
View File

@@ -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;
},
});
})();

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

@@ -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);
});
});
});
</script>