Derive diff view properties directly from diff view model
Affected properties: path, lineNum, leftSide.
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.
The above properties are maintained by the view model already. Let's
just subscribe to them and set the props of gr-diff-view from them.
Release-Notes: skip
Change-Id: Icfbf73d9d0ce21d1ede04dd2fd50a1f5c1630db0
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 b54c406..5a6d57f 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
@@ -254,10 +254,15 @@
@state()
private allPatchSets?: PatchSet[] = [];
+ /** Directly reflects the view model property `diffView.lineNum`. */
// Private but used in tests.
@state()
focusLineNum?: number;
+ /** Directly reflects the view model property `diffView.leftSide`. */
+ @state()
+ leftSide = false;
+
// visible for testing
reviewedFiles = new Set<string>();
@@ -432,14 +437,24 @@
);
subscribe(
this,
- () => this.getChangeModel().diffPath$,
+ () => this.getViewModel().diffPath$,
path => (this.path = path)
);
subscribe(
this,
+ () => this.getViewModel().diffLine$,
+ line => (this.focusLineNum = line)
+ );
+ subscribe(
+ this,
+ () => this.getViewModel().diffLeftSide$,
+ leftSide => (this.leftSide = leftSide)
+ );
+ subscribe(
+ this,
() =>
combineLatest([
- this.getChangeModel().diffPath$,
+ this.getViewModel().diffPath$,
this.getChangeModel().reviewedFiles$,
]),
([path, files]) => {
@@ -454,7 +469,7 @@
subscribe(
this,
() =>
- this.getChangeModel().diffPath$.pipe(
+ this.getViewModel().diffPath$.pipe(
filter(diffPath => !!diffPath),
switchMap(() =>
combineLatest([
@@ -703,6 +718,12 @@
protected override updated(changedProperties: PropertyValues): void {
super.updated(changedProperties);
if (
+ changedProperties.has('focusLineNum') ||
+ changedProperties.has('leftSide')
+ ) {
+ this.initLineOfInterestAndCursor();
+ }
+ if (
changedProperties.has('changeComments') ||
changedProperties.has('path') ||
changedProperties.has('patchRange') ||
@@ -727,6 +748,9 @@
override render() {
if (this.viewState?.childView !== ChangeChildView.DIFF) return nothing;
+ if (!this.patchRange) return nothing;
+ if (!this.changeNum) return nothing;
+ if (!this.path) return nothing;
const file = this.getFileRange();
return html`
${this.renderStickyHeader()}
@@ -1326,10 +1350,10 @@
}
// Private but used in tests.
- initLineOfInterestAndCursor(leftSide: boolean) {
- assertIsDefined(this.diffHost, 'diffHost');
- this.diffHost.lineOfInterest = this.getLineOfInterest(leftSide);
- this.initCursor(leftSide);
+ initLineOfInterestAndCursor() {
+ if (!this.diffHost) return;
+ this.diffHost.lineOfInterest = this.getLineOfInterest();
+ this.initCursor();
}
private updateUrlToDiffUrl(lineNum?: number, leftSide?: boolean) {
@@ -1353,25 +1377,14 @@
// Private but used in tests.
initPatchRange() {
- let leftSide = false;
if (!this.change) return;
if (this.viewState?.childView !== ChangeChildView.DIFF) return;
- if (this.viewState.diffView?.path) {
- this.getChangeModel().updatePath(this.viewState.diffView.path);
- }
if (this.viewState.patchNum) {
this.patchRange = {
patchNum: this.viewState.patchNum,
basePatchNum: this.viewState.basePatchNum || PARENT,
};
}
- if (this.viewState.diffView?.lineNum) {
- this.focusLineNum = this.viewState.diffView.lineNum;
- leftSide = !!this.viewState.diffView?.leftSide;
- }
- assertIsDefined(this.patchRange, 'patchRange');
- this.initLineOfInterestAndCursor(leftSide);
-
this.commentMap = this.getPaths();
}
@@ -1430,11 +1443,7 @@
return;
}
- if (this.isConnected) {
- this.getChangeModel().updatePath(undefined);
- }
this.patchRange = undefined;
- this.focusLineNum = undefined;
this.changeNum = viewState.changeNum;
this.classList.remove('hideComments');
@@ -1486,30 +1495,22 @@
* If the params specify a diff address then configure the diff cursor.
* Private but used in tests.
*/
- initCursor(leftSide: boolean) {
- if (this.focusLineNum === undefined) {
- return;
- }
+ initCursor() {
+ if (!this.focusLineNum) return;
if (!this.cursor) return;
- if (leftSide) {
- this.cursor.side = Side.LEFT;
- } else {
- this.cursor.side = Side.RIGHT;
- }
+ this.cursor.side = this.leftSide ? Side.LEFT : Side.RIGHT;
this.cursor.initialLineNumber = this.focusLineNum;
}
// Private but used in tests.
- getLineOfInterest(leftSide: boolean): DisplayLine | undefined {
+ getLineOfInterest(): DisplayLine | undefined {
// If there is a line number specified, pass it along to the diff so that
// it will not get collapsed.
- if (!this.focusLineNum) {
- return undefined;
- }
+ if (!this.focusLineNum) return undefined;
return {
lineNum: this.focusLineNum,
- side: leftSide ? Side.LEFT : Side.RIGHT,
+ side: this.leftSide ? Side.LEFT : Side.RIGHT,
};
}
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 f01cd9b..03f4bb9 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
@@ -34,6 +34,7 @@
createRevision,
createFileInfo,
createDiffViewState,
+ TEST_NUMERIC_CHANGE_ID,
} from '../../../test/test-data-generators';
import {
BasePatchSetNum,
@@ -76,7 +77,10 @@
BrowserModel,
browserModelToken,
} from '../../../models/browser/browser-model';
-import {changeViewModelToken} from '../../../models/views/change';
+import {
+ ChangeViewModel,
+ changeViewModelToken,
+} from '../../../models/views/change';
import {FileNameToNormalizedFileInfoMap} from '../../../models/change/files-model';
function createComment(
@@ -104,6 +108,7 @@
let navToDiffStub: SinonStub;
let navToEditStub: SinonStub;
let changeModel: ChangeModel;
+ let viewModel: ChangeViewModel;
let commentsModel: CommentsModel;
let browserModel: BrowserModel;
let userModel: UserModel;
@@ -138,9 +143,9 @@
stubRestApi('getPortedComments').returns(Promise.resolve({}));
element = await fixture(html`<gr-diff-view></gr-diff-view>`);
- const viewModel = testResolver(changeViewModelToken);
+ viewModel = testResolver(changeViewModelToken);
viewModel.setState(createDiffViewState());
- element.changeNum = 42 as NumericChangeId;
+ await waitUntil(() => element.changeNum === TEST_NUMERIC_CHANGE_ID);
element.path = 'some/path.txt';
element.change = createParsedChange();
element.diff = {...createDiff(), content: []};
@@ -1062,7 +1067,6 @@
});
test('prev/up/next links', async () => {
- const viewModel = testResolver(changeViewModelToken);
viewModel.setState({
...createDiffViewState(),
});
@@ -1131,7 +1135,6 @@
});
test('prev/up/next links with patch range', async () => {
- const viewModel = testResolver(changeViewModelToken);
viewModel.setState({
...createDiffViewState(),
basePatchNum: 5 as BasePatchSetNum,
@@ -1154,6 +1157,7 @@
'wheatley.md',
]);
await waitUntil(() => element.path === 'glados.txt');
+ await waitUntil(() => element.patchRange?.patchNum === 10);
const linkEls = queryAll(element, '.navLink');
assert.equal(linkEls.length, 3);
@@ -1245,9 +1249,9 @@
manual_review: true,
};
userModel.setDiffPreferences(diffPreferences);
+ viewModel.updateState({diffView: {path: 'wheatley.md'}});
changeModel.setState({
change: createParsedChange(),
- diffPath: '/COMMIT_MSG',
reviewedFiles: [],
loadingStatus: LoadingStatus.LOADED,
});
@@ -1277,9 +1281,9 @@
manual_review: false,
};
userModel.setDiffPreferences(diffPreferences);
+ viewModel.updateState({diffView: {path: 'wheatley.md'}});
changeModel.setState({
change: createParsedChange(),
- diffPath: '/COMMIT_MSG',
reviewedFiles: [],
loadingStatus: LoadingStatus.LOADED,
});
@@ -1293,18 +1297,23 @@
const saveReviewedStub = sinon
.stub(changeModel, 'setReviewedFilesStatus')
.callsFake(() => Promise.resolve());
+ userModel.setDiffPreferences(createDefaultDiffPrefs());
+ viewModel.updateState({
+ patchNum: 1 as RevisionPatchSetNum,
+ basePatchNum: PARENT,
+ diffView: {path: '/COMMIT_MSG'},
+ });
changeModel.setState({
change: createParsedChange(),
- diffPath: '/COMMIT_MSG',
reviewedFiles: [],
loadingStatus: LoadingStatus.LOADED,
});
element.loggedIn = true;
+ await waitUntil(() => element.patchRange?.patchNum === 1);
+ await element.updateComplete;
assertIsDefined(element.diffHost);
sinon.stub(element.diffHost, 'reload');
- userModel.setDiffPreferences(createDefaultDiffPrefs());
-
await waitUntil(() => saveReviewedStub.called);
changeModel.updateStateFileReviewed('/COMMIT_MSG', true);
@@ -1312,7 +1321,7 @@
const reviewedStatusCheckBox = queryAndAssert<HTMLInputElement>(
element,
- 'input[type="checkbox"]'
+ 'input#reviewed'
);
assert.isTrue(reviewedStatusCheckBox.checked);
@@ -1379,13 +1388,7 @@
sinon.stub(element.diffHost, 'reload');
const initLineStub = sinon.stub(element, 'initLineOfInterestAndCursor');
- element.loggedIn = true;
- element.viewState = {
- ...createDiffViewState(),
- patchNum: 2 as RevisionPatchSetNum,
- basePatchNum: 1 as BasePatchSetNum,
- diffView: {path: '/COMMIT_MSG'},
- };
+ element.focusLineNum = 123;
await element.updateComplete;
await waitEventLoop();
@@ -1438,43 +1441,51 @@
assert.isNotOk(element.cursor.initialLineNumber);
// Does nothing when viewState specify no cursor address:
- element.initCursor(false);
+ element.leftSide = false;
+ element.initCursor();
assert.isNotOk(element.cursor.initialLineNumber);
// Does nothing when viewState specify side but no number:
- element.initCursor(true);
+ element.leftSide = true;
+ element.initCursor();
assert.isNotOk(element.cursor.initialLineNumber);
// Revision hash: specifies lineNum but not side.
element.focusLineNum = 234;
- element.initCursor(false);
+ element.leftSide = false;
+ element.initCursor();
assert.equal(element.cursor.initialLineNumber, 234);
assert.equal(element.cursor.side, Side.RIGHT);
// Base hash: specifies lineNum and side.
element.focusLineNum = 345;
- element.initCursor(true);
+ element.leftSide = true;
+ element.initCursor();
assert.equal(element.cursor.initialLineNumber, 345);
assert.equal(element.cursor.side, Side.LEFT);
// Specifies right side:
element.focusLineNum = 123;
- element.initCursor(false);
+ element.leftSide = false;
+ element.initCursor();
assert.equal(element.cursor.initialLineNumber, 123);
assert.equal(element.cursor.side, Side.RIGHT);
});
test('getLineOfInterest', () => {
- assert.isUndefined(element.getLineOfInterest(false));
+ element.leftSide = false;
+ assert.isUndefined(element.getLineOfInterest());
element.focusLineNum = 12;
- let result = element.getLineOfInterest(false);
+ element.leftSide = false;
+ let result = element.getLineOfInterest();
assert.isOk(result);
assert.equal(result!.lineNum, 12);
assert.equal(result!.side, Side.RIGHT);
- result = element.getLineOfInterest(true);
+ element.leftSide = true;
+ result = element.getLineOfInterest();
assert.isOk(result);
assert.equal(result!.lineNum, 12);
assert.equal(result!.side, Side.LEFT);
@@ -1559,6 +1570,7 @@
patchNum: 3 as RevisionPatchSetNum,
diffView: {path: 'abcd'},
};
+ await waitUntil(() => element.patchRange?.patchNum === 3);
await element.updateComplete;
});
test('empty', () => {
diff --git a/polygerrit-ui/app/models/change/change-model.ts b/polygerrit-ui/app/models/change/change-model.ts
index 7cbaabe..eb48590 100644
--- a/polygerrit-ui/app/models/change/change-model.ts
+++ b/polygerrit-ui/app/models/change/change-model.ts
@@ -60,12 +60,6 @@
loadingStatus: LoadingStatus;
change?: ParsedChangeInfo;
/**
- * The name of the file user is viewing in the diff view mode. File path is
- * specified in the url or derived from the commentId.
- * Does not apply to change-view or edit-view.
- */
- diffPath?: string;
- /**
* The list of reviewed files, kept in the model because we want changes made
* in one view to reflect on other views without re-rendering the other views.
* Undefined means it's still loading and empty set means no files reviewed.
@@ -170,11 +164,6 @@
changeState => changeState.loadingStatus
);
- public readonly diffPath$ = select(
- this.state$,
- changeState => changeState?.diffPath
- );
-
public readonly reviewedFiles$ = select(
this.state$,
changeState => changeState?.reviewedFiles
@@ -318,11 +307,6 @@
];
}
- // Temporary workaround until path is derived in the model itself.
- updatePath(diffPath?: string) {
- this.updateState({diffPath});
- }
-
updateStateReviewedFiles(reviewedFiles: string[]) {
this.updateState({reviewedFiles});
}
diff --git a/polygerrit-ui/app/models/views/change.ts b/polygerrit-ui/app/models/views/change.ts
index 317e511..e3570d1 100644
--- a/polygerrit-ui/app/models/views/change.ts
+++ b/polygerrit-ui/app/models/views/change.ts
@@ -209,6 +209,21 @@
state => state?.basePatchNum
);
+ public readonly diffPath$ = select(
+ this.state$,
+ state => state?.diffView?.path
+ );
+
+ public readonly diffLine$ = select(
+ this.state$,
+ state => state?.diffView?.lineNum
+ );
+
+ public readonly diffLeftSide$ = select(
+ this.state$,
+ state => state?.diffView?.leftSide ?? false
+ );
+
public readonly childView$ = select(this.state$, state => state?.childView);
public readonly tab$ = select(this.state$, state => state?.tab);
