Compute comment map in diff view on-the-fly
The goal is to pick apart the `viewStateChanged()` method one by one.
Relying on that method to initialize and update correctly has proven
to be extremely error prone. Each piece of data should have a more
direct way of how it is initialized or updated. Computing things
on-the-fly and removing lines of code from `viewStateChanged()` is the
easiest we can do.
Release-Notes: skip
Change-Id: I82acb501165fce6fc5c584c477834e0151e196df
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 5a6d57f..ac1c1c8 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
@@ -242,10 +242,6 @@
// Private but used in tests.
@state()
- commentMap?: CommentMap;
-
- // Private but used in tests.
- @state()
isBlameLoaded?: boolean;
@state()
@@ -1385,7 +1381,6 @@
basePatchNum: this.viewState.basePatchNum || PARENT,
};
}
- this.commentMap = this.getPaths();
}
private isSameDiffLoaded(value: ChangeViewState) {
@@ -1681,22 +1676,17 @@
}
// Private but used in tests.
- getPaths(): CommentMap {
- if (!this.changeComments) return {};
- return this.changeComments.getPaths(this.patchRange);
- }
-
- // Private but used in tests.
findFileWithComment(direction: -1 | 1): string | undefined {
const fileList = this.files?.sortedPaths;
- if (!this.commentMap) return undefined;
+ const commentMap: CommentMap =
+ this.changeComments?.getPaths(this.patchRange) ?? {};
if (!fileList || fileList.length === 0) return undefined;
if (!this.path) return undefined;
const pathIndex = fileList.indexOf(this.path);
const stopIndex = direction === 1 ? fileList.length : -1;
for (let i = pathIndex + direction; i !== stopIndex; i += direction) {
- if (this.commentMap[fileList[i]]) return fileList[i];
+ if (commentMap[fileList[i]]) return fileList[i];
}
return undefined;
}
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 03f4bb9..fc4157d 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
@@ -60,7 +60,6 @@
ChangeModel,
LoadingStatus,
} from '../../../models/change/change-model';
-import {CommentMap} from '../../../utils/comment-util';
import {assertIsDefined} from '../../../utils/common-util';
import {GrDiffModeSelector} from '../../../embed/diff/gr-diff-mode-selector/gr-diff-mode-selector';
import {fixture, html, assert} from '@open-wc/testing';
@@ -1562,47 +1561,12 @@
});
});
- suite('initPatchRange', () => {
- setup(async () => {
- getDiffRestApiStub.returns(Promise.resolve(createDiff()));
- element.viewState = {
- ...createDiffViewState(),
- patchNum: 3 as RevisionPatchSetNum,
- diffView: {path: 'abcd'},
- };
- await waitUntil(() => element.patchRange?.patchNum === 3);
- await element.updateComplete;
- });
- test('empty', () => {
- sinon.stub(element, 'getPaths').returns({});
- element.initPatchRange();
- assert.equal(Object.keys(element.commentMap ?? {}).length, 0);
- });
-
- test('has paths', () => {
- sinon.stub(element, 'getPaths').returns({
- 'path/to/file/one.cpp': true,
- 'path-to/file/two.py': true,
- });
- element.changeNum = 42 as NumericChangeId;
- element.patchRange = {
- basePatchNum: 3 as BasePatchSetNum,
- patchNum: 5 as RevisionPatchSetNum,
- };
- element.initPatchRange();
- assert.deepEqual(Object.keys(element.commentMap ?? {}), [
- 'path/to/file/one.cpp',
- 'path-to/file/two.py',
- ]);
- });
- });
-
suite('findFileWithComment', () => {
test('empty file list', () => {
- element.commentMap = {
- 'path/one.jpg': true,
- 'path/three.wav': true,
- };
+ element.changeComments = new ChangeComments({
+ 'path/one.jpg': [createComment('c1', 1, 1, 'path/one.jpg')],
+ 'path/three.wav': [createComment('c1', 1, 1, 'path/three.wav')],
+ });
element.path = 'path/two.m4v';
assert.isUndefined(element.findFileWithComment(-1));
assert.isUndefined(element.findFileWithComment(1));
@@ -1612,16 +1576,19 @@
const fileList = ['path/one.jpg', 'path/two.m4v', 'path/three.wav'];
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;
+ element.changeComments = new ChangeComments({
+ 'path/one.jpg': [createComment('c1', 1, 1, 'path/one.jpg')],
+ 'path/three.wav': [createComment('c1', 1, 1, 'path/three.wav')],
+ });
assert.equal(element.findFileWithComment(-1), fileList[0]);
assert.equal(element.findFileWithComment(1), fileList[2]);
- commentMap[fileList[1]] = true;
+ element.changeComments = new ChangeComments({
+ 'path/one.jpg': [createComment('c1', 1, 1, 'path/one.jpg')],
+ 'path/two.m4v': [createComment('c1', 1, 1, 'path/two.m4v')],
+ 'path/three.wav': [createComment('c1', 1, 1, 'path/three.wav')],
+ });
assert.equal(element.findFileWithComment(-1), fileList[0]);
assert.equal(element.findFileWithComment(1), fileList[2]);
@@ -1652,11 +1619,9 @@
suite('moveToFileWithComment previous', () => {
test('no previous', async () => {
- const commentMap: CommentMap = {};
- commentMap[element.files.sortedPaths[0]!] = false;
- commentMap[element.files.sortedPaths[1]!] = false;
- commentMap[element.files.sortedPaths[2]!] = true;
- element.commentMap = commentMap;
+ element.changeComments = new ChangeComments({
+ 'path/three.wav': [createComment('c1', 1, 1, 'path/three.wav')],
+ });
element.path = element.files.sortedPaths[1];
await element.updateComplete;
@@ -1666,11 +1631,10 @@
});
test('w/ previous', async () => {
- const commentMap: CommentMap = {};
- commentMap[element.files.sortedPaths[0]!] = true;
- commentMap[element.files.sortedPaths[1]!] = false;
- commentMap[element.files.sortedPaths[2]!] = true;
- element.commentMap = commentMap;
+ element.changeComments = new ChangeComments({
+ 'path/one.jpg': [createComment('c1', 1, 1, 'path/one.jpg')],
+ 'path/three.wav': [createComment('c1', 1, 1, 'path/three.wav')],
+ });
element.path = element.files.sortedPaths[1];
await element.updateComplete;
@@ -1682,11 +1646,9 @@
suite('moveToFileWithComment next', () => {
test('no previous', async () => {
- const commentMap: CommentMap = {};
- commentMap[element.files.sortedPaths[0]!] = true;
- commentMap[element.files.sortedPaths[1]!] = false;
- commentMap[element.files.sortedPaths[2]!] = false;
- element.commentMap = commentMap;
+ element.changeComments = new ChangeComments({
+ 'path/one.jpg': [createComment('c1', 1, 1, 'path/one.jpg')],
+ });
element.path = element.files.sortedPaths[1];
await element.updateComplete;
@@ -1696,11 +1658,10 @@
});
test('w/ previous', async () => {
- const commentMap: CommentMap = {};
- commentMap[element.files.sortedPaths[0]!] = true;
- commentMap[element.files.sortedPaths[1]!] = false;
- commentMap[element.files.sortedPaths[2]!] = true;
- element.commentMap = commentMap;
+ element.changeComments = new ChangeComments({
+ 'path/one.jpg': [createComment('c1', 1, 1, 'path/one.jpg')],
+ 'path/three.wav': [createComment('c1', 1, 1, 'path/three.wav')],
+ });
element.path = element.files.sortedPaths[1];
await element.updateComplete;
