Navigate to next file with comment thread
Shift + n keyboard shortcut currently navigates to next comment thread
but does not navigate to the next file when end of file is reached.
Add the behaviour and navigate to change page when there are no more
files with a comment.
Change-Id: Ie92d62a0515767822b2ce297132b8814ab6c7326
diff --git a/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.ts b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.ts
index 1085fbc..5aecc75 100644
--- a/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.ts
+++ b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.ts
@@ -599,6 +599,8 @@
export const _testOnly_findCommentById =
ChangeComments.prototype.findCommentById;
+export const _testOnly_getCommentsForPath =
+ ChangeComments.prototype.getCommentsForPath;
@customElement('gr-comment-api')
export class GrCommentApi extends GestureEventListeners(
LegacyElementMixin(PolymerElement)
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor.ts b/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor.ts
index 035c1a3..5ac37dc 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor.ts
@@ -235,7 +235,11 @@
return result;
}
- moveToNextCommentThread(): CursorMoveResult {
+ moveToNextCommentThread(): CursorMoveResult | undefined {
+ if (this.isAtEnd()) {
+ fireEvent(this, 'navigate-to-next-file-with-comments');
+ return;
+ }
const result = this.$.cursorManager.next({
filter: (row: HTMLElement) => this._rowHasThread(row),
});
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
index 1e9b1cc..dc7eeba 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
@@ -712,7 +712,12 @@
);
}
- _navToFile(path: string, fileList: string[], direction: -1 | 1) {
+ _navToFile(
+ path: string,
+ fileList: string[],
+ direction: -1 | 1,
+ navigateToFirstComment?: boolean
+ ) {
const newPath = this._getNavLinkPath(path, fileList, direction);
if (!newPath) return;
if (!this._change) return;
@@ -728,11 +733,18 @@
}
if (!newPath.path) return;
+ let lineNum;
+ if (navigateToFirstComment)
+ lineNum = this._changeComments?.getCommentsForPath(
+ newPath.path,
+ this._patchRange
+ )?.[0].line;
GerritNav.navigateToDiff(
this._change,
newPath.path,
this._patchRange.patchNum,
- this._patchRange.basePatchNum
+ this._patchRange.basePatchNum,
+ lineNum
);
}
@@ -1759,6 +1771,23 @@
this._navToFile(this._path, unreviewedFiles, 1);
}
+ _navigateToNextFileWithCommentThread() {
+ if (!this._path) return;
+ if (!this._fileList) return;
+ if (!this._patchRange) return;
+ if (!this._change) return;
+ const hasComment = (path: string) => {
+ return (
+ this._changeComments?.getCommentsForPath(path, this._patchRange!)
+ ?.length ?? 0 > 0
+ );
+ };
+ const filesWithComments = this._fileList.filter(
+ file => file === this._path || hasComment(file)
+ );
+ this._navToFile(this._path, filesWithComments, 1, true);
+ }
+
_handleReloadingDiffPreference() {
this._getDiffPreferences();
}
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_html.ts b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_html.ts
index a00927c..bfb0cd8 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_html.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_html.ts
@@ -428,6 +428,7 @@
<gr-diff-cursor
id="cursor"
on-navigate-to-next-unreviewed-file="_handleNextUnreviewedFile"
+ on-navigate-to-next-file-with-comments="_navigateToNextFileWithCommentThread"
></gr-diff-cursor>
<gr-comment-api id="commentAPI"></gr-comment-api>
`;
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js
index 8fe06f8..e166f18 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js
@@ -21,7 +21,7 @@
import {ChangeStatus} from '../../../constants/constants.js';
import {TestKeyboardShortcutBinder} from '../../../test/test-utils.js';
import {Shortcut} from '../../../mixins/keyboard-shortcut-mixin/keyboard-shortcut-mixin.js';
-import {_testOnly_findCommentById} from '../gr-comment-api/gr-comment-api.js';
+import {ChangeComments, _testOnly_findCommentById, _testOnly_getCommentsForPath} from '../gr-comment-api/gr-comment-api.js';
import {GerritView} from '../../../services/router/router-model.js';
import {
createChange,
@@ -137,6 +137,7 @@
computeUnresolvedNum: () => {},
getPaths: () => {},
getThreadsBySideForFile: () => [],
+ getCommentsForPath: _testOnly_getCommentsForPath,
findCommentById: _testOnly_findCommentById,
}));
@@ -465,6 +466,48 @@
assert.equal(element._setReviewed.lastCall.args[0], true);
});
+ test('moveToNextCommentThread navigates to next file', () => {
+ const diffNavStub = sinon.stub(GerritNav, 'navigateToDiff');
+ const diffChangeStub = sinon.stub(element, '_navigateToChange');
+ sinon.stub(element.$.cursor, 'isAtEnd').returns(true);
+ element._changeNum = '42';
+ const comment = {
+ 'wheatley.md': [{
+ ...createComment(),
+ patch_set: 10,
+ line: 21,
+ }],
+ };
+ element._changeComments = new ChangeComments(comment);
+ element._patchRange = {
+ basePatchNum: PARENT,
+ patchNum: 10,
+ };
+ element._change = {
+ _number: 42,
+ revisions: {
+ a: {_number: 10, commit: {parents: []}},
+ },
+ };
+ element._files = getFilesFromFileList(
+ ['chell.go', 'glados.txt', 'wheatley.md']);
+ element._path = 'glados.txt';
+ element.changeViewState.selectedFileIndex = 1;
+ element._loggedIn = true;
+
+ MockInteractions.pressAndReleaseKeyOn(element, 78, 'shift', 'n');
+ flush();
+ assert.isTrue(diffNavStub.calledWithExactly(
+ element._change, 'wheatley.md', 10, PARENT, 21));
+
+ element._path = 'wheatley.md'; // navigated to next file
+
+ MockInteractions.pressAndReleaseKeyOn(element, 78, 'shift', 'n');
+ flush();
+
+ assert.isTrue(diffChangeStub.called);
+ });
+
test('shift+x shortcut expands all diff context', () => {
const expandStub = sinon.stub(element.$.diffHost, 'expandAllContext');
MockInteractions.pressAndReleaseKeyOn(element, 88, 'shift', 'x');
@@ -621,14 +664,14 @@
MockInteractions.pressAndReleaseKeyOn(element, 221, null, ']');
assert.isTrue(element._loading);
assert(diffNavStub.lastCall.calledWithExactly(element._change,
- 'wheatley.md', 10, 5),
+ 'wheatley.md', 10, 5, undefined),
'Should navigate to /c/42/5..10/wheatley.md');
element._path = 'wheatley.md';
MockInteractions.pressAndReleaseKeyOn(element, 219, null, '[');
assert.isTrue(element._loading);
assert(diffNavStub.lastCall.calledWithExactly(element._change,
- 'glados.txt', 10, 5),
+ 'glados.txt', 10, 5, undefined),
'Should navigate to /c/42/5..10/glados.txt');
element._path = 'glados.txt';
@@ -638,7 +681,8 @@
element._change,
'chell.go',
10,
- 5),
+ 5,
+ undefined),
'Should navigate to /c/42/5..10/chell.go');
element._path = 'chell.go';
@@ -692,13 +736,13 @@
MockInteractions.pressAndReleaseKeyOn(element, 221, null, ']');
assert(diffNavStub.lastCall.calledWithExactly(element._change,
- 'wheatley.md', 1, PARENT),
+ 'wheatley.md', 1, PARENT, undefined),
'Should navigate to /c/42/1/wheatley.md');
element._path = 'wheatley.md';
MockInteractions.pressAndReleaseKeyOn(element, 219, null, '[');
assert(diffNavStub.lastCall.calledWithExactly(element._change,
- 'glados.txt', 1, PARENT),
+ 'glados.txt', 1, PARENT, undefined),
'Should navigate to /c/42/1/glados.txt');
element._path = 'glados.txt';
@@ -707,7 +751,8 @@
element._change,
'chell.go',
1,
- PARENT), 'Should navigate to /c/42/1/chell.go');
+ PARENT,
+ undefined), 'Should navigate to /c/42/1/chell.go');
element._path = 'chell.go';
changeNavStub.reset();
