Move basic change state from router to change view model
Release-Notes: skip
Google-Bug-Id: b/247042673
Change-Id: I3c59896837b18709fbc4bb8584d3abb7354492f1
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
index 8d5a372..5fa959b4 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
@@ -134,7 +134,6 @@
fireReload,
fireTitleChange,
} from '../../../utils/event-util';
-import {routerModelToken} from '../../../services/router/router-model';
import {
debounce,
DelayedTask,
@@ -537,8 +536,6 @@
private readonly getChangeModel = resolve(this, changeModelToken);
- private readonly getRouterModel = resolve(this, routerModelToken);
-
private readonly getCommentsModel = resolve(this, commentsModelToken);
private readonly getConfigModel = resolve(this, configModelToken);
@@ -571,7 +568,7 @@
/** Simply reflects the router-model value. */
// visible for testing
- routerPatchNum?: PatchSetNum;
+ viewModelPatchNum?: PatchSetNum;
private readonly shortcutsController = new ShortcutController(this);
@@ -705,9 +702,9 @@
);
subscribe(
this,
- () => this.getRouterModel().routerPatchNum$,
+ () => this.getViewModel().patchNum$,
patchNum => {
- this.routerPatchNum = patchNum;
+ this.viewModelPatchNum = patchNum;
}
);
subscribe(
@@ -2649,7 +2646,7 @@
// is under change-model control. `patchRange.patchNum` should eventually
// also be model managed, so we can reconcile these two code snippets into
// one location.
- if (!this.routerPatchNum && latestPsNum === editParentRev._number) {
+ if (!this.viewModelPatchNum && latestPsNum === editParentRev._number) {
this.patchRange = {...this.patchRange, patchNum: EDIT};
// The file list is not reactive (yet) with regards to patch range
// changes, so we have to actively trigger it.
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts
index cd8b9af..9b78e64 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts
@@ -1987,7 +1987,7 @@
// When edit is set, but patchNum as well, then keep patchNum.
element.patchRange.patchNum = 5 as RevisionPatchSetNum;
- element.routerPatchNum = 5 as RevisionPatchSetNum;
+ element.viewModelPatchNum = 5 as RevisionPatchSetNum;
element.processEdit(change);
assert.equal(element.patchRange.patchNum, 5 as RevisionPatchSetNum);
});
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
index 5d09694..28b7e3b 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
@@ -360,13 +360,14 @@
if ('repo' in state && state.repo !== undefined && 'changeNum' in state)
this.restApiService.setInProjectLookup(state.changeNum, state.repo);
- this.routerModel.setState({
- view: state.view,
- changeNum: 'changeNum' in state ? state.changeNum : undefined,
- patchNum: 'patchNum' in state ? state.patchNum ?? undefined : undefined,
- basePatchNum:
- 'basePatchNum' in state ? state.basePatchNum ?? undefined : undefined,
- });
+ this.routerModel.setState({view: state.view});
+ // We are trying to reset the change (view) model when navigating to other
+ // views, because we don't trust our reset logic at the moment. The models
+ // singletons and might unintentionally keep state from one change to
+ // another. TODO: Let's find some way to avoid that.
+ if (state.view !== GerritView.CHANGE) {
+ this.changeViewModel.setState(undefined);
+ }
this.appElement().params = state;
}
diff --git a/polygerrit-ui/app/models/change/change-model.ts b/polygerrit-ui/app/models/change/change-model.ts
index 8282a3f..eaaf697 100644
--- a/polygerrit-ui/app/models/change/change-model.ts
+++ b/polygerrit-ui/app/models/change/change-model.ts
@@ -22,7 +22,6 @@
startWith,
switchMap,
} from 'rxjs/operators';
-import {RouterModel} from '../../services/router/router-model';
import {
computeAllPatchSets,
computeLatestPatchNum,
@@ -38,6 +37,7 @@
import {UserModel} from '../user/user-model';
import {define} from '../dependency';
import {isOwner} from '../../utils/change-util';
+import {ChangeViewModel} from '../views/change';
export enum LoadingStatus {
NOT_LOADED = 'NOT_LOADED',
@@ -76,7 +76,7 @@
export function updateChangeWithEdit(
change?: ParsedChangeInfo,
edit?: EditInfo,
- routerPatchNum?: PatchSetNum
+ viewModelPatchNum?: PatchSetNum
): ParsedChangeInfo | undefined {
if (!change || !edit) return change;
assertIsDefined(edit.commit.commit, 'edit.commit.commit');
@@ -95,7 +95,7 @@
// which is still done in change-view. `_patchRange.patchNum` should
// eventually also be model managed, so we can reconcile these two code
// snippets into one location.
- if (routerPatchNum === undefined) {
+ if (viewModelPatchNum === undefined) {
change.current_revision = edit.commit.commit;
}
return change;
@@ -103,20 +103,20 @@
/**
* Derives the base patchset number from all the data that can potentially
- * influence it. Mostly just returns `routerBasePatchNum` or PARENT, but has
+ * influence it. Mostly just returns `viewModelBasePatchNum` or PARENT, but has
* some special logic when looking at merge commits.
*
- * NOTE: At the moment this returns just `routerBasePatchNum ?? PARENT`, see
+ * NOTE: At the moment this returns just `viewModelBasePatchNum ?? PARENT`, see
* TODO below.
*/
function computeBase(
- routerBasePatchNum: BasePatchSetNum | undefined,
+ viewModelBasePatchNum: BasePatchSetNum | undefined,
patchNum: RevisionPatchSetNum | undefined,
change: ParsedChangeInfo | undefined,
preferences: PreferencesInfo
): BasePatchSetNum {
- if (routerBasePatchNum && routerBasePatchNum !== PARENT) {
- return routerBasePatchNum;
+ if (viewModelBasePatchNum && viewModelBasePatchNum !== PARENT) {
+ return viewModelBasePatchNum;
}
if (!change || !patchNum) return PARENT;
@@ -129,7 +129,7 @@
// but we are not sure whether this was ever 100% working correctly. A
// major challenge is being able to select PARENT explicitly even if your
// preference for the default choice is FIRST_PARENT. <gr-file-list-header>
- // just uses `navigation.setUrl()` and the router does not have any
+ // just uses `navigation.setUrl()` and the view model does not have any
// way of forcing the basePatchSetNum to stick to PARENT without being
// altered back to FIRST_PARENT here.
// See also corresponding TODO in gr-settings-view.
@@ -192,57 +192,57 @@
public readonly patchNum$: Observable<RevisionPatchSetNum | undefined> =
select(
combineLatest([
- this.routerModel.state$,
+ this.viewModel.state$,
this.state$,
this.latestPatchNum$,
]).pipe(
/**
- * If you depend on both, router and change state, then you want to
- * filter out inconsistent state, e.g. router changeNum already updated,
- * change not yet reset to undefined.
+ * If you depend on both, view model and change state, then you want to
+ * filter out inconsistent state, e.g. view model changeNum already
+ * updated, change not yet reset to undefined.
*/
- filter(([routerState, changeState, _latestPatchN]) => {
+ filter(([viewModelState, changeState, _latestPatchN]) => {
const changeNum = changeState.change?._number;
- const routerChangeNum = routerState.changeNum;
- return changeNum === undefined || changeNum === routerChangeNum;
+ const viewModelChangeNum = viewModelState?.changeNum;
+ return changeNum === undefined || changeNum === viewModelChangeNum;
})
),
- ([routerState, _changeState, latestPatchN]) =>
- routerState?.patchNum || latestPatchN
+ ([viewModelState, _changeState, latestPatchN]) =>
+ viewModelState?.patchNum || latestPatchN
);
/**
* Emits the base patchset number. This is identical to the
- * `routerBasePatchNum$`, but has some special logic for merges.
+ * `viewModel.basePatchNum$`, but has some special logic for merges.
*
* Note that this selector can emit without the change being available!
*/
public readonly basePatchNum$: Observable<BasePatchSetNum> =
/**
- * If you depend on both, router and change state, then you want to filter
- * out inconsistent state, e.g. router changeNum already updated, change not
- * yet reset to undefined.
+ * If you depend on both, view model and change state, then you want to
+ * filter out inconsistent state, e.g. view model changeNum already
+ * updated, change not yet reset to undefined.
*/
select(
combineLatest([
- this.routerModel.state$,
+ this.viewModel.state$,
this.state$,
this.userModel.state$,
]).pipe(
- filter(([routerState, changeState, _]) => {
+ filter(([viewModelState, changeState, _]) => {
const changeNum = changeState.change?._number;
- const routerChangeNum = routerState.changeNum;
- return changeNum === undefined || changeNum === routerChangeNum;
+ const viewModelChangeNum = viewModelState?.changeNum;
+ return changeNum === undefined || changeNum === viewModelChangeNum;
}),
withLatestFrom(
- this.routerModel.routerBasePatchNum$,
+ this.viewModel.basePatchNum$,
this.patchNum$,
this.change$,
this.userModel.preferences$
)
),
- ([_, routerBasePatchNum, patchNum, change, preferences]) =>
- computeBase(routerBasePatchNum, patchNum, change, preferences)
+ ([_, viewModelBasePatchNum, patchNum, change, preferences]) =>
+ computeBase(viewModelBasePatchNum, patchNum, change, preferences)
);
public readonly isOwner$: Observable<boolean> = select(
@@ -257,13 +257,13 @@
);
constructor(
- private readonly routerModel: RouterModel,
+ private readonly viewModel: ChangeViewModel,
private readonly restApiService: RestApiService,
private readonly userModel: UserModel
) {
super(initialState);
this.subscriptions = [
- combineLatest([this.routerModel.routerChangeNum$, this.reload$])
+ combineLatest([this.viewModel.changeNum$, this.reload$])
.pipe(
map(([changeNum, _]) => changeNum),
switchMap(changeNum => {
@@ -272,7 +272,7 @@
const edit = from(this.restApiService.getChangeEdit(changeNum));
return forkJoin([change, edit]);
}),
- withLatestFrom(this.routerModel.routerPatchNum$),
+ withLatestFrom(this.viewModel.patchNum$),
map(([[change, edit], patchNum]) =>
updateChangeWithEdit(change, edit, patchNum)
)
diff --git a/polygerrit-ui/app/models/change/change-model_test.ts b/polygerrit-ui/app/models/change/change-model_test.ts
index a2fc7c9..4bc23d8 100644
--- a/polygerrit-ui/app/models/change/change-model_test.ts
+++ b/polygerrit-ui/app/models/change/change-model_test.ts
@@ -9,6 +9,7 @@
import {
createChange,
createChangeMessageInfo,
+ createChangeViewState,
createEditInfo,
createParsedChange,
createRevision,
@@ -28,12 +29,12 @@
} from '../../types/common';
import {ParsedChangeInfo} from '../../types/types';
import {getAppContext} from '../../services/app-context';
-import {GerritView, routerModelToken} from '../../services/router/router-model';
import {ChangeState, LoadingStatus, updateChangeWithEdit} from './change-model';
import {ChangeModel} from './change-model';
import {assert} from '@open-wc/testing';
import {testResolver} from '../../test/common-test-setup';
import {userModelToken} from '../user/user-model';
+import {changeViewModelToken} from '../views/change';
suite('updateChangeWithEdit() tests', () => {
test('undefined change', async () => {
@@ -83,7 +84,7 @@
setup(() => {
changeModel = new ChangeModel(
- testResolver(routerModelToken),
+ testResolver(changeViewModelToken),
getAppContext().restApiService,
testResolver(userModelToken)
);
@@ -121,10 +122,7 @@
assert.equal(stub.callCount, 0);
assert.isUndefined(state?.change);
- testResolver(routerModelToken).setState({
- view: GerritView.CHANGE,
- changeNum: knownChange._number,
- });
+ testResolver(changeViewModelToken).setState(createChangeViewState());
state = await waitForLoadingStatus(LoadingStatus.LOADING);
assert.equal(stub.callCount, 1);
assert.isUndefined(state?.change);
@@ -140,10 +138,7 @@
const promise = mockPromise<ParsedChangeInfo | undefined>();
const stub = stubRestApi('getChangeDetail').callsFake(() => promise);
let state: ChangeState;
- testResolver(routerModelToken).setState({
- view: GerritView.CHANGE,
- changeNum: knownChange._number,
- });
+ testResolver(changeViewModelToken).setState(createChangeViewState());
promise.resolve(knownChange);
state = await waitForLoadingStatus(LoadingStatus.LOADED);
@@ -164,10 +159,7 @@
let promise = mockPromise<ParsedChangeInfo | undefined>();
const stub = stubRestApi('getChangeDetail').callsFake(() => promise);
let state: ChangeState;
- testResolver(routerModelToken).setState({
- view: GerritView.CHANGE,
- changeNum: knownChange._number,
- });
+ testResolver(changeViewModelToken).setState(createChangeViewState());
promise.resolve(knownChange);
state = await waitForLoadingStatus(LoadingStatus.LOADED);
@@ -178,8 +170,8 @@
_number: 123 as NumericChangeId,
};
promise = mockPromise<ParsedChangeInfo | undefined>();
- testResolver(routerModelToken).setState({
- view: GerritView.CHANGE,
+ testResolver(changeViewModelToken).setState({
+ ...createChangeViewState(),
changeNum: otherChange._number,
});
state = await waitForLoadingStatus(LoadingStatus.LOADING);
@@ -197,10 +189,7 @@
let promise = mockPromise<ParsedChangeInfo | undefined>();
const stub = stubRestApi('getChangeDetail').callsFake(() => promise);
let state: ChangeState;
- testResolver(routerModelToken).setState({
- view: GerritView.CHANGE,
- changeNum: knownChange._number,
- });
+ testResolver(changeViewModelToken).setState(createChangeViewState());
promise.resolve(knownChange);
state = await waitForLoadingStatus(LoadingStatus.LOADED);
@@ -208,10 +197,7 @@
promise = mockPromise<ParsedChangeInfo | undefined>();
promise.resolve(undefined);
- testResolver(routerModelToken).setState({
- view: GerritView.CHANGE,
- changeNum: undefined,
- });
+ testResolver(changeViewModelToken).setState(undefined);
state = await waitForLoadingStatus(LoadingStatus.NOT_LOADED);
assert.equal(stub.callCount, 2);
assert.isUndefined(state?.change);
@@ -220,10 +206,7 @@
promise = mockPromise<ParsedChangeInfo | undefined>();
promise.resolve(knownChange);
- testResolver(routerModelToken).setState({
- view: GerritView.CHANGE,
- changeNum: knownChange._number,
- });
+ testResolver(changeViewModelToken).setState(createChangeViewState());
state = await waitForLoadingStatus(LoadingStatus.LOADED);
assert.equal(stub.callCount, 3);
assert.equal(state?.change, knownChange);
@@ -299,7 +282,7 @@
assert.equal(spy.lastCall.firstArg, PARENT);
// test update
- testResolver(routerModelToken).updateState({
+ testResolver(changeViewModelToken).updateState({
basePatchNum: 1 as PatchSetNumber,
});
assert.equal(spy.callCount, 2);
diff --git a/polygerrit-ui/app/models/comments/comments-model.ts b/polygerrit-ui/app/models/comments/comments-model.ts
index 51b4591..1fdf342 100644
--- a/polygerrit-ui/app/models/comments/comments-model.ts
+++ b/polygerrit-ui/app/models/comments/comments-model.ts
@@ -29,7 +29,6 @@
} from '../../utils/comment-util';
import {deepEqual} from '../../utils/deep-util';
import {select} from '../../utils/observable-util';
-import {RouterModel} from '../../services/router/router-model';
import {define} from '../dependency';
import {combineLatest, forkJoin, from, Observable, of} from 'rxjs';
import {fire, fireAlert, fireEvent} from '../../utils/event-util';
@@ -54,6 +53,7 @@
switchMap,
} from 'rxjs/operators';
import {isDefined} from '../../types/types';
+import {ChangeViewModel} from '../views/change';
export interface CommentState {
/** undefined means 'still loading' */
@@ -415,7 +415,7 @@
private discardedDrafts: DraftInfo[] = [];
constructor(
- private readonly routerModel: RouterModel,
+ private readonly changeViewModel: ChangeViewModel,
private readonly changeModel: ChangeModel,
private readonly accountsModel: AccountsModel,
private readonly restApiService: RestApiService,
@@ -432,7 +432,7 @@
this.changeModel.patchNum$.subscribe(x => (this.patchNum = x))
);
this.subscriptions.push(
- this.routerModel.routerChangeNum$.subscribe(changeNum => {
+ this.changeViewModel.changeNum$.subscribe(changeNum => {
this.changeNum = changeNum;
this.setState({...initialState});
this.reloadAllComments();
diff --git a/polygerrit-ui/app/models/comments/comments-model_test.ts b/polygerrit-ui/app/models/comments/comments-model_test.ts
index cab36d2..a689e42 100644
--- a/polygerrit-ui/app/models/comments/comments-model_test.ts
+++ b/polygerrit-ui/app/models/comments/comments-model_test.ts
@@ -6,6 +6,7 @@
import '../../test/common-test-setup';
import {
createAccountWithEmail,
+ createChangeViewState,
createDraft,
} from '../../test/test-data-generators';
import {
@@ -20,17 +21,16 @@
import {
createComment,
createParsedChange,
- TEST_NUMERIC_CHANGE_ID,
} from '../../test/test-data-generators';
import {stubRestApi, waitUntil, waitUntilCalled} from '../../test/test-utils';
import {getAppContext} from '../../services/app-context';
-import {GerritView, routerModelToken} from '../../services/router/router-model';
import {PathToCommentsInfoMap} from '../../types/common';
import {changeModelToken} from '../change/change-model';
import {assert} from '@open-wc/testing';
import {testResolver} from '../../test/common-test-setup';
import {accountsModelToken} from '../accounts-model/accounts-model';
import {ChangeComments} from '../../elements/diff/gr-comment-api/gr-comment-api';
+import {changeViewModelToken} from '../views/change';
suite('comments model tests', () => {
test('updateStateDeleteDraft', () => {
@@ -72,7 +72,7 @@
test('loads comments', async () => {
const model = new CommentsModel(
- testResolver(routerModelToken),
+ testResolver(changeViewModelToken),
testResolver(changeModelToken),
testResolver(accountsModelToken),
getAppContext().restApiService,
@@ -100,10 +100,7 @@
model.portedComments$.subscribe(c => (portedComments = c ?? {}))
);
- testResolver(routerModelToken).setState({
- view: GerritView.CHANGE,
- changeNum: TEST_NUMERIC_CHANGE_ID,
- });
+ testResolver(changeViewModelToken).setState(createChangeViewState());
testResolver(changeModelToken).updateStateChange(createParsedChange());
await waitUntilCalled(diffCommentsSpy, 'diffCommentsSpy');
@@ -133,7 +130,7 @@
};
stubRestApi('getAccountDetails').returns(Promise.resolve(account));
const model = new CommentsModel(
- testResolver(routerModelToken),
+ testResolver(changeViewModelToken),
testResolver(changeModelToken),
testResolver(accountsModelToken),
getAppContext().restApiService,
@@ -161,7 +158,7 @@
};
stubRestApi('getAccountDetails').returns(Promise.resolve(account));
const model = new CommentsModel(
- testResolver(routerModelToken),
+ testResolver(changeViewModelToken),
testResolver(changeModelToken),
testResolver(accountsModelToken),
getAppContext().restApiService,
@@ -199,7 +196,7 @@
})
);
const model = new CommentsModel(
- testResolver(routerModelToken),
+ testResolver(changeViewModelToken),
testResolver(changeModelToken),
testResolver(accountsModelToken),
getAppContext().restApiService,
diff --git a/polygerrit-ui/app/models/views/change.ts b/polygerrit-ui/app/models/views/change.ts
index 9701b54..3f3c76b 100644
--- a/polygerrit-ui/app/models/views/change.ts
+++ b/polygerrit-ui/app/models/views/change.ts
@@ -200,6 +200,15 @@
define<ChangeViewModel>('change-view-model');
export class ChangeViewModel extends Model<ChangeViewState | undefined> {
+ public readonly changeNum$ = select(this.state$, state => state?.changeNum);
+
+ public readonly patchNum$ = select(this.state$, state => state?.patchNum);
+
+ public readonly basePatchNum$ = select(
+ this.state$,
+ state => state?.basePatchNum
+ );
+
public readonly childView$ = select(this.state$, state => state?.childView);
public readonly tab$ = select(this.state$, state => state?.tab);
diff --git a/polygerrit-ui/app/services/app-context-init.ts b/polygerrit-ui/app/services/app-context-init.ts
index 93dc131..3a15de9 100644
--- a/polygerrit-ui/app/services/app-context-init.ts
+++ b/polygerrit-ui/app/services/app-context-init.ts
@@ -148,7 +148,7 @@
changeModelToken,
() =>
new ChangeModel(
- resolver(routerModelToken),
+ resolver(changeViewModelToken),
appContext.restApiService,
resolver(userModelToken)
),
@@ -157,7 +157,7 @@
commentsModelToken,
() =>
new CommentsModel(
- resolver(routerModelToken),
+ resolver(changeViewModelToken),
resolver(changeModelToken),
resolver(accountsModelToken),
appContext.restApiService,
diff --git a/polygerrit-ui/app/services/router/router-model.ts b/polygerrit-ui/app/services/router/router-model.ts
index 18372b3..c3c1cb6 100644
--- a/polygerrit-ui/app/services/router/router-model.ts
+++ b/polygerrit-ui/app/services/router/router-model.ts
@@ -4,11 +4,6 @@
* SPDX-License-Identifier: Apache-2.0
*/
import {Observable} from 'rxjs';
-import {
- NumericChangeId,
- RevisionPatchSetNum,
- BasePatchSetNum,
-} from '../../types/common';
import {Model} from '../../models/model';
import {select} from '../../utils/observable-util';
import {define} from '../../models/dependency';
@@ -29,12 +24,6 @@
export interface RouterState {
// Note that this router model view must be updated before view model state.
view?: GerritView;
- // TODO: Move into ChangeViewState.
- changeNum?: NumericChangeId;
- // TODO: Move into ChangeViewState.
- patchNum?: RevisionPatchSetNum;
- // TODO: Move into ChangeViewState.
- basePatchNum?: BasePatchSetNum;
}
export const routerModelToken = define<RouterModel>('router-model');
@@ -44,20 +33,6 @@
state => state.view
);
- // TODO: Move into ChangeViewState.
- readonly routerChangeNum$: Observable<NumericChangeId | undefined> = select(
- this.state$,
- state => state.changeNum
- );
-
- // TODO: Move into ChangeViewState.
- readonly routerPatchNum$: Observable<RevisionPatchSetNum | undefined> =
- select(this.state$, state => state.patchNum);
-
- // TODO: Move into ChangeViewState.
- readonly routerBasePatchNum$: Observable<BasePatchSetNum | undefined> =
- select(this.state$, state => state.basePatchNum);
-
constructor() {
super({});
}
