Refactoring: Compute comment skips on the fly
We generally prefer either retrieving data from the model directly or
computing it on the fly while rendering. Maintaining additional state in
components by computing it in `update` or `willUpdate` adds substantial
complexity to the component.
Release-Notes: skip
Change-Id: I610d56e63f63535035e0e501a55b25c496ac73a7
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 0f5c62b..99d70b2 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
@@ -261,9 +261,6 @@
@state()
commentMap?: CommentMap;
- @state()
- private commentSkips?: CommentSkips;
-
// Private but used in tests.
@state()
isBlameLoaded?: boolean;
@@ -705,17 +702,6 @@
if (changedProperties.has('change')) {
this.allPatchSets = computeAllPatchSets(this.change);
}
- if (
- changedProperties.has('commentMap') ||
- changedProperties.has('files') ||
- changedProperties.has('path')
- ) {
- this.commentSkips = this.computeCommentSkips(
- this.commentMap,
- this.files?.sortedPaths,
- this.path
- );
- }
}
private reInitCursor() {
@@ -1111,13 +1097,14 @@
// Private but used in tests.
moveToPreviousFileWithComment() {
- if (!this.commentSkips) return;
+ const commentSkips = this.computeCommentSkips();
+ if (!commentSkips) return;
if (!this.change) return;
if (!this.patchRange?.patchNum) return;
// If there is no previous diff with comments, then return to the change
// view.
- if (!this.commentSkips.previous) {
+ if (!commentSkips.previous) {
this.navToChangeView();
return;
}
@@ -1127,19 +1114,20 @@
change: this.change,
patchNum: this.patchRange.patchNum,
basePatchNum: this.patchRange.basePatchNum,
- diffView: {path: this.commentSkips.previous},
+ diffView: {path: commentSkips.previous},
})
);
}
// Private but used in tests.
moveToNextFileWithComment() {
- if (!this.commentSkips) return;
+ const commentSkips = this.computeCommentSkips();
+ if (!commentSkips) return;
if (!this.change) return;
if (!this.patchRange?.patchNum) return;
// If there is no next diff with comments, then return to the change view.
- if (!this.commentSkips.next) {
+ if (!commentSkips.next) {
this.navToChangeView();
return;
}
@@ -1149,7 +1137,7 @@
change: this.change,
patchNum: this.patchRange.patchNum,
basePatchNum: this.patchRange.basePatchNum,
- diffView: {path: this.commentSkips.next},
+ diffView: {path: commentSkips.next},
})
);
}
@@ -1902,24 +1890,21 @@
}
// Private but used in tests.
- computeCommentSkips(
- commentMap?: CommentMap,
- fileList?: string[],
- path?: string
- ): CommentSkips | undefined {
- if (!commentMap) return undefined;
+ computeCommentSkips(): CommentSkips | undefined {
+ const fileList = this.files?.sortedPaths;
+ if (!this.commentMap) return undefined;
if (!fileList) return undefined;
- if (!path) return undefined;
+ if (!this.path) return undefined;
const skips: CommentSkips = {previous: null, next: null};
if (!fileList.length) {
return skips;
}
- const pathIndex = fileList.indexOf(path);
+ const pathIndex = fileList.indexOf(this.path);
// Scan backward for the previous file.
for (let i = pathIndex - 1; i >= 0; i--) {
- if (commentMap[fileList[i]]) {
+ if (this.commentMap[fileList[i]]) {
skips.previous = fileList[i];
break;
}
@@ -1927,7 +1912,7 @@
// Scan forward for the next file.
for (let i = pathIndex + 1; i < fileList.length; i++) {
- if (commentMap[fileList[i]]) {
+ if (this.commentMap[fileList[i]]) {
skips.next = fileList[i];
break;
}
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.ts b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.ts
index 65383f4..2c60b41 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.ts
@@ -1702,12 +1702,12 @@
suite('computeCommentSkips', () => {
test('empty file list', () => {
- const commentMap = {
+ element.commentMap = {
'path/one.jpg': true,
'path/three.wav': true,
};
- const path = 'path/two.m4v';
- const result = element.computeCommentSkips(commentMap, [], path);
+ element.path = 'path/two.m4v';
+ const result = element.computeCommentSkips();
assert.isOk(result);
assert.isNotOk(result!.previous);
assert.isNotOk(result!.next);
@@ -1715,34 +1715,36 @@
test('finds skips', () => {
const fileList = ['path/one.jpg', 'path/two.m4v', 'path/three.wav'];
- let path = fileList[1];
+ element.files = {sortedPaths: fileList, changeFilesByPath: {}};
+ element.path = fileList[1];
const commentMap: CommentMap = {};
+ element.commentMap = commentMap;
commentMap[fileList[0]] = true;
commentMap[fileList[1]] = false;
commentMap[fileList[2]] = true;
- let result = element.computeCommentSkips(commentMap, fileList, path);
+ let result = element.computeCommentSkips();
assert.isOk(result);
assert.equal(result!.previous, fileList[0]);
assert.equal(result!.next, fileList[2]);
commentMap[fileList[1]] = true;
- result = element.computeCommentSkips(commentMap, fileList, path);
+ result = element.computeCommentSkips();
assert.isOk(result);
assert.equal(result!.previous, fileList[0]);
assert.equal(result!.next, fileList[2]);
- path = fileList[0];
+ element.path = fileList[0];
- result = element.computeCommentSkips(commentMap, fileList, path);
+ result = element.computeCommentSkips();
assert.isOk(result);
assert.isNull(result!.previous);
assert.equal(result!.next, fileList[1]);
- path = fileList[2];
+ element.path = fileList[2];
- result = element.computeCommentSkips(commentMap, fileList, path);
+ result = element.computeCommentSkips();
assert.isOk(result);
assert.equal(result!.previous, fileList[1]);
assert.isNull(result!.next);
