Remove `viewStateChanged()` method from gr-change-view The deed is done. This was the ultimate target of a large chain of refactorings: Do not let the change view handle view state updates in a complicated tree of paths, but instead let the change model react in specific independent subscriptions. And let the view just subscribe to model updates. The major missing change that was still needed, was the handling of firing and handling reload events. Unfortunately clearing the patchset and forcing a reload was also been handled by a `reload` event, but clearing the patchset is a view state change, so this should be a direct command to the model. So we have looked at all `fireReload()` and `fireReload(true)` instances and checked which one is more appropriate: Keeping firing the "soft" reload or requesting a new view state. One issue was that the `forceReload` URL parameter was only handled by the change view. We are handling this now in the change view model. We have unified the entire reload handling in the change view model: The `reload` event, the `reload()` method of the view model and the `forceReload` URL parameter all end up in the same place: We are always resetting the view state to `undefined` in order to trigger a reload. That is very robust and reliable and allows us to also get rid of some dedicated `reload$` observables that we don't need anymore. Stop logging `change-view-re-rendered`. We don't need that anymore. Release-Notes: skip Change-Id: Ie6319089dee7586c04c01b2ae91c18bc75f0d85d
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-bulk-abandon-flow/gr-change-list-bulk-abandon-flow.ts b/polygerrit-ui/app/elements/change-list/gr-change-list-bulk-abandon-flow/gr-change-list-bulk-abandon-flow.ts index df63780..2ca7f36 100644 --- a/polygerrit-ui/app/elements/change-list/gr-change-list-bulk-abandon-flow/gr-change-list-bulk-abandon-flow.ts +++ b/polygerrit-ui/app/elements/change-list/gr-change-list-bulk-abandon-flow/gr-change-list-bulk-abandon-flow.ts
@@ -147,7 +147,7 @@ private handleClose() { this.actionModal.close(); fireAlert(this, 'Reloading page..'); - fireReload(this, true); + fireReload(this); } }
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-bulk-vote-flow/gr-change-list-bulk-vote-flow.ts b/polygerrit-ui/app/elements/change-list/gr-change-list-bulk-vote-flow/gr-change-list-bulk-vote-flow.ts index db82523..6d7045a 100644 --- a/polygerrit-ui/app/elements/change-list/gr-change-list-bulk-vote-flow/gr-change-list-bulk-vote-flow.ts +++ b/polygerrit-ui/app/elements/change-list/gr-change-list-bulk-vote-flow/gr-change-list-bulk-vote-flow.ts
@@ -308,7 +308,7 @@ this.actionModal.close(); if (getOverallStatus(this.progressByChange) === ProgressStatus.NOT_STARTED) return; - fireReload(this, true); + fireReload(this); } private async handleConfirm() {
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts index 66f5d0c..7bbec62 100644 --- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts +++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
@@ -81,7 +81,6 @@ fireAlert, fireError, fireNoBubbleNoCompose, - fireReload, } from '../../../utils/event-util'; import { getApprovalInfo, @@ -1870,7 +1869,7 @@ // Hide rebase dialog only if the action succeeds this.actionsModal?.close(); this.hideAllDialogs(); - fireReload(this, true); + this.getChangeModel().navigateToChangeResetReload(); break; case ChangeActions.REVERT_SUBMISSION: { const revertSubmistionInfo = obj as unknown as RevertSubmissionInfo; @@ -1886,7 +1885,7 @@ break; } default: - fireReload(this, true); + this.getChangeModel().navigateToChangeResetReload(); break; } }); @@ -1954,7 +1953,7 @@ 'Cannot set label: a newer patch has been ' + 'uploaded to this change.', action: 'Reload', - callback: () => fireReload(this, true), + callback: () => this.getChangeModel().navigateToChangeResetReload(), }); // Because this is not a network error, call the cleanup function
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts index 0d51269..946191b 100644 --- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts +++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts
@@ -40,7 +40,7 @@ TopicName, } from '../../../types/common'; import {ActionType} from '../../../api/change-actions'; -import {SinonFakeTimers} from 'sinon'; +import {SinonFakeTimers, SinonStubbedMember} from 'sinon'; import {IronAutogrowTextareaElement} from '@polymer/iron-autogrow-textarea'; import {GrButton} from '../../shared/gr-button/gr-button'; import {GrDialog} from '../../shared/gr-dialog/gr-dialog'; @@ -56,10 +56,17 @@ import {testResolver} from '../../../test/common-test-setup'; import {storageServiceToken} from '../../../services/storage/gr-storage_impl'; import {pluginLoaderToken} from '../../shared/gr-js-api-interface/gr-plugin-loader'; +import { + ChangeModel, + changeModelToken, +} from '../../../models/change/change-model'; // TODO(dhruvsri): remove use of _populateRevertMessage as it's private suite('gr-change-actions tests', () => { let element: GrChangeActions; + let navigateResetStub: SinonStubbedMember< + ChangeModel['navigateToChangeResetReload'] + >; suite('basic tests', () => { setup(async () => { @@ -139,6 +146,10 @@ _account_id: 123 as AccountId, }; stubRestApi('getRepoBranches').returns(Promise.resolve([])); + navigateResetStub = sinon.stub( + testResolver(changeModelToken), + 'navigateToChangeResetReload' + ); await element.updateComplete; await element.reload(); @@ -612,13 +623,11 @@ }); test('rebase change fires reload event', async () => { - const eventStub = sinon.stub(element, 'dispatchEvent'); await element.handleResponse( {__key: 'rebase', __type: ActionType.CHANGE, label: 'l'}, new Response() ); - assert.isTrue(eventStub.called); - assert.equal(eventStub.lastCall.args[0].type, 'reload'); + assert.isTrue(navigateResetStub.called); }); test("rebase dialog gets recent changes each time it's opened", async () => {
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 dfdaff2..32e55c5 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
@@ -236,32 +236,18 @@ @query('gr-copy-links') private copyLinksDropdown?: GrCopyLinks; - private _viewState?: ChangeViewState; - - @property({type: Object}) - get viewState() { - return this._viewState; - } - - set viewState(viewState: ChangeViewState | undefined) { - if (this._viewState === viewState) return; - const oldViewState = this._viewState; - this._viewState = viewState; - this.viewStateChanged(); - this.requestUpdate('viewState', oldViewState); - } + @state() + viewState?: ChangeViewState; @property({type: String}) backPage?: string; - // Private but used in tests. @state() commentThreads?: CommentThread[]; // Don't use, use serverConfig instead. private _serverConfig?: ServerInfo; - // Private but used in tests. @state() get serverConfig() { return this._serverConfig; @@ -515,11 +501,7 @@ this.handleCommitMessageCancel() ); this.addEventListener('open-fix-preview', e => this.onOpenFixPreview(e)); - this.addEventListener('show-tab', e => this.setActiveTab(e)); - this.addEventListener('reload', e => { - this.loadData(/* clearPatchset= */ e.detail && e.detail.clearPatchset); - }); } private setupShortcuts() { @@ -527,7 +509,7 @@ this.shortcutsController.addAbstract(Shortcut.EMOJI_DROPDOWN, () => {}); // docOnly this.shortcutsController.addAbstract(Shortcut.MENTIONS_DROPDOWN, () => {}); // docOnly this.shortcutsController.addAbstract(Shortcut.REFRESH_CHANGE, () => - fireReload(this, true) + this.getChangeModel().navigateToChangeResetReload() ); this.shortcutsController.addAbstract(Shortcut.OPEN_REPLY_DIALOG, () => this.handleOpenReplyDialog() @@ -685,6 +667,15 @@ ); subscribe( this, + () => this.getChangeModel().changeNum$, + changeNum => { + // The change view is tied to a specific change number, so don't update + // changeNum to undefined and only set it once. + if (changeNum && !this.changeNum) this.changeNum = changeNum; + } + ); + subscribe( + this, () => this.getChangeModel().patchNum$, patchNum => (this.patchNum = patchNum) ); @@ -1732,7 +1723,7 @@ } this.editingCommitMessage = false; - fireReload(this, true); + this.getChangeModel().navigateToChangeResetReload(); }) .catch(() => { assertIsDefined(this.commitMessageEditor); @@ -1931,7 +1922,7 @@ handleReplySent() { assertIsDefined(this.replyModal); this.replyModal.close(); - fireReload(this); + this.getChangeModel().navigateToChangeResetReload(); } private handleReplyCancel() { @@ -1982,35 +1973,6 @@ } // Private but used in tests. - viewStateChanged() { - if (!this.viewState) return; - if (this.isChangeObsolete()) return; - - const forceReload = this.viewState.forceReload; - - // If changeNum is defined that means the change has already been - // rendered once before so a full reload is not required. - if (this.changeNum !== undefined && !forceReload) { - this.reporting.reportInteraction('change-view-re-rendered'); - return; - } - - // If the change was loaded before, then we are firing a 'reload' event - // instead of calling `loadData()` directly for two reasons: - // 1. We want to avoid code that is only relevant for the initial load of a - // change. - // 2. We have to somehow trigger the change-model reloading. Otherwise - // this.change is not updated. - if (this.changeNum) { - fireReload(this); - return; - } - - this.changeNum = this.viewState.changeNum; - this.loadData(); - } - - // Private but used in tests. handleMessageAnchorTap(e: CustomEvent<{id: string}>) { assertIsDefined(this.change, 'change'); assertIsDefined(this.patchNum, 'patchNum'); @@ -2079,6 +2041,9 @@ } } + /** + * This is the URL equivalent of changeModel.navigateToChangeResetReload(). + */ private computeChangeUrl(forceReload?: boolean) { if (!this.change) return undefined; return createChangeUrl({ @@ -2340,23 +2305,6 @@ } } - /** - * Reload the change. - * - * @param clearPatchset Reloads the change ignoring any patchset - * choice made. - * @return A promise that resolves when the core data has loaded. - * Some non-core data loading may still be in-flight when the core data - * promise resolves. - */ - loadData(clearPatchset?: boolean): void { - if (this.isChangeObsolete()) return; - if (clearPatchset && this.change) { - this.getChangeModel().navigateToChangeAndReset(); - return; - } - } - private async reportChangeDisplayed() { await waitUntil(() => !!this.metadata); await untilRendered(this.metadata!); @@ -2466,7 +2414,7 @@ dismissOnNavigation: true, showDismiss: true, action: 'Reload', - callback: () => fireReload(this, true), + callback: () => this.getChangeModel().navigateToChangeResetReload(), }); }); }, this.serverConfig.change.update_delay * 1000);
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 a661695..4349829 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
@@ -23,14 +23,12 @@ queryAndAssert, stubFlags, stubRestApi, - waitEventLoop, waitQueryAndAssert, waitUntil, waitUntilVisible, } from '../../../test/test-utils'; import { createChangeViewState, - createApproval, createChangeMessages, createRevision, createRevisions, @@ -38,7 +36,6 @@ createUserConfig, TEST_NUMERIC_CHANGE_ID, TEST_PROJECT_NAME, - createAccountWithIdNameAndEmail, createChangeViewChange, createAccountDetailWithId, createParsedChange, @@ -47,7 +44,6 @@ import {GrChangeView} from './gr-change-view'; import { AccountId, - ApprovalInfo, BasePatchSetNum, CommitId, EDIT, @@ -58,9 +54,7 @@ RobotCommentInfo, Timestamp, UrlEncodedCommentId, - DetailedLabelInfo, RepoName, - QuickLabelInfo, CommentThread, } from '../../../types/common'; import {GrEditControls} from '../../edit/gr-edit-controls/gr-edit-controls'; @@ -77,7 +71,6 @@ import {GrThreadList} from '../gr-thread-list/gr-thread-list'; import {assertIsDefined} from '../../../utils/common-util'; import {fixture, html, assert} from '@open-wc/testing'; -import {deepClone} from '../../../utils/deep-util'; import {Modifier} from '../../../utils/dom-util'; import {GrButton} from '../../shared/gr-button/gr-button'; import {GrCopyLinks} from '../gr-copy-links/gr-copy-links'; @@ -920,8 +913,6 @@ }, }, }; - sinon.stub(element, 'loadData'); - sinon.spy(element, 'viewStateChanged'); element.viewState = createChangeViewState(); }); }); @@ -1138,64 +1129,6 @@ assert.isTrue(element.isSubmitEnabled()); }); - test('reload is called when an approved label is removed', async () => { - const vote: ApprovalInfo = { - ...createApproval(), - _account_id: 1 as AccountId, - name: 'bojack', - value: 1, - }; - element.changeNum = TEST_NUMERIC_CHANGE_ID; - element.basePatchNum = PARENT; - element.patchNum = 1 as RevisionPatchSetNum; - const change = { - ...createParsedChange(), - owner: createAccountWithIdNameAndEmail(), - revisions: { - rev2: createRevision(2), - rev1: createRevision(1), - rev13: createRevision(13), - rev3: createRevision(3), - }, - current_revision: 'rev3' as CommitId, - status: ChangeStatus.NEW, - labels: { - test: { - all: [vote], - default_value: 0, - values: {}, - approved: {}, - }, - }, - }; - element.change = change; - await element.updateComplete; - const reloadStub = sinon.stub(element, 'loadData'); - const newChange = {...element.change}; - (newChange.labels!.test! as DetailedLabelInfo).all = []; - element.change = deepClone(newChange); - await element.updateComplete; - assert.isFalse(reloadStub.called); - - assert.isDefined(element.change); - const testLabels: DetailedLabelInfo & QuickLabelInfo = - newChange.labels!.test; - assertIsDefined(testLabels); - testLabels.all!.push(vote); - testLabels.all!.push(vote); - testLabels.approved = vote; - element.change = deepClone(newChange); - await element.updateComplete; - assert.isFalse(reloadStub.called); - - assert.isDefined(element.change); - (newChange.labels!.test! as DetailedLabelInfo).all = []; - element.change = deepClone(newChange); - await element.updateComplete; - assert.isTrue(reloadStub.called); - assert.isTrue(reloadStub.calledOnce); - }); - test('reply button has updated count when there are drafts', () => { const getLabel = (canReview: boolean) => { element.change!.actions!.ready = {enabled: canReview}; @@ -1219,74 +1152,6 @@ assert.equal(getLabel(true), 'Start Review (3)'); }); - test('don’t reload entire page when patchRange changes', async () => { - const reloadStub = sinon - .stub(element, 'loadData') - .callsFake(() => Promise.resolve()); - assertIsDefined(element.fileList); - await element.fileList.updateComplete; - const value: ChangeViewState = { - ...createChangeViewState(), - view: GerritView.CHANGE, - patchNum: 1 as RevisionPatchSetNum, - }; - element.changeNum = undefined; - element.viewState = value; - await element.updateComplete; - assert.isTrue(reloadStub.calledOnce); - - element.change = { - ...createChangeViewChange(), - revisions: { - rev1: createRevision(1), - rev2: createRevision(2), - }, - }; - - value.basePatchNum = 1 as BasePatchSetNum; - value.patchNum = 2 as RevisionPatchSetNum; - element.viewState = {...value}; - await element.updateComplete; - await waitEventLoop(); - assert.isFalse(reloadStub.calledTwice); - }); - - test('do not reload entire page when patchRange doesnt change', async () => { - assertIsDefined(element.fileList); - const reloadStub = sinon - .stub(element, 'loadData') - .callsFake(() => Promise.resolve()); - const collapseStub = sinon.stub(element.fileList, 'collapseAllDiffs'); - const value: ChangeViewState = createChangeViewState(); - element.viewState = value; - // change already loaded - assert.isOk(element.changeNum); - await element.updateComplete; - assert.isFalse(reloadStub.calledOnce); - element.viewState = {...value}; - await element.updateComplete; - assert.isFalse(reloadStub.calledTwice); - assert.isFalse(collapseStub.calledTwice); - }); - - test('forceReload updates the change', async () => { - assertIsDefined(element.fileList); - const getChangeStub = stubRestApi('getChangeDetail').returns( - Promise.resolve(createParsedChange()) - ); - const loadDataStub = sinon - .stub(element, 'loadData') - .callsFake(() => Promise.resolve()); - const collapseStub = sinon.stub(element.fileList, 'collapseAllDiffs'); - element.viewState = {...createChangeViewState(), forceReload: true}; - await element.updateComplete; - assert.isTrue(getChangeStub.called); - assert.isTrue(loadDataStub.called); - assert.isTrue(collapseStub.called); - // patchNum is set by changeChanged, so this verifies that change was set. - assert.isOk(element.patchNum); - }); - test('computeCopyTextForTitle', () => { element.change = { ...createChangeViewChange(), @@ -1326,6 +1191,7 @@ }); test('handleCommitMessageSave trims trailing whitespace', async () => { + element.changeNum = TEST_NUMERIC_CHANGE_ID; element.change = createChangeViewChange(); // Response code is 500, because we want to avoid window reloading const putStub = stubRestApi('putChangeCommitMessage').returns(
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts index 922f9c79..62693ea 100644 --- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts +++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
@@ -822,14 +822,6 @@ () => this.getChangeModel().basePatchNum$, x => (this.basePatchNum = x) ); - subscribe( - this, - () => this.getChangeModel().reload$, - () => { - this.resetFileState(); - this.collapseAllDiffs(); - } - ); } override willUpdate(changedProperties: PropertyValues): void {
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts index 0db08d1..0d9faf3 100644 --- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts +++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
@@ -90,7 +90,6 @@ fire, fireNoBubble, fireIronAnnounce, - fireReload, fireServerError, } from '../../../utils/event-util'; import {ErrorCallback} from '../../../api/rest'; @@ -1948,7 +1947,7 @@ } _reload() { - fireReload(this, true); + this.getChangeModel().navigateToChangeResetReload(); this.cancel(); }
diff --git a/polygerrit-ui/app/elements/change/gr-submit-requirement-hovercard/gr-submit-requirement-hovercard.ts b/polygerrit-ui/app/elements/change/gr-submit-requirement-hovercard/gr-submit-requirement-hovercard.ts index 2766114..98f65c0 100644 --- a/polygerrit-ui/app/elements/change/gr-submit-requirement-hovercard/gr-submit-requirement-hovercard.ts +++ b/polygerrit-ui/app/elements/change/gr-submit-requirement-hovercard/gr-submit-requirement-hovercard.ts
@@ -327,7 +327,7 @@ review ) .then(() => { - fireReload(this, true); + fireReload(this); }); }
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 902616e..26ae3aa 100644 --- a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts +++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
@@ -1257,10 +1257,10 @@ }; const queryMap = new URLSearchParams(ctx.querystring); - if (queryMap.has('forceReload')) state.forceReload = true; if (queryMap.has('openReplyDialog')) state.openReplyDialog = true; const tab = queryMap.get('tab'); + if (queryMap.has('forceReload')) state.forceReload = true; if (tab) state.tab = tab; const checksPatchset = Number(queryMap.get('checksPatchset')); if (Number.isInteger(checksPatchset) && checksPatchset > 0) { @@ -1353,6 +1353,8 @@ view: GerritView.CHANGE, childView: ChangeChildView.OVERVIEW, }; + const queryMap = new URLSearchParams(ctx.querystring); + if (queryMap.has('forceReload')) state.forceReload = true; assertIsDefined(state.repo); this.reporting.setRepoName(state.repo); this.reporting.setChangeId(changeNum); @@ -1374,6 +1376,8 @@ childView: ChangeChildView.DIFF, diffView: {path: ctx.params[8]}, }; + const queryMap = new URLSearchParams(ctx.querystring); + if (queryMap.has('forceReload')) state.forceReload = true; const address = this.parseLineAddress(ctx.hash); if (address) { state.diffView!.leftSide = address.leftSide; @@ -1424,6 +1428,8 @@ childView: ChangeChildView.EDIT, editView: {path: ctx.params[3], lineNum: Number(ctx.hash)}, }; + const queryMap = new URLSearchParams(ctx.querystring); + if (queryMap.has('forceReload')) state.forceReload = true; this.normalizePatchRangeParams(state); // Note that router model view must be updated before view models. this.setState(state); @@ -1447,14 +1453,7 @@ }; const tab = queryMap.get('tab'); if (tab) state.tab = tab; - if (queryMap.has('forceReload')) { - state.forceReload = true; - history.replaceState( - null, - '', - location.href.replace(/[?&]forceReload=true/, '') - ); - } + if (queryMap.has('forceReload')) state.forceReload = true; this.normalizePatchRangeParams(state); // Note that router model view must be updated before view models. this.setState(state);
diff --git a/polygerrit-ui/app/elements/edit/gr-edit-controls/gr-edit-controls.ts b/polygerrit-ui/app/elements/edit/gr-edit-controls/gr-edit-controls.ts index 0fc754c..5786112 100644 --- a/polygerrit-ui/app/elements/edit/gr-edit-controls/gr-edit-controls.ts +++ b/polygerrit-ui/app/elements/edit/gr-edit-controls/gr-edit-controls.ts
@@ -18,7 +18,7 @@ GrAutocomplete, } from '../../shared/gr-autocomplete/gr-autocomplete'; import {getAppContext} from '../../../services/app-context'; -import {fireAlert, fireReload} from '../../../utils/event-util'; +import {fireAlert} from '../../../utils/event-util'; import { assertIsDefined, query as queryUtil, @@ -34,6 +34,7 @@ import {modalStyles} from '../../../styles/gr-modal-styles'; import {whenVisible} from '../../../utils/dom-util'; import {throwingErrorCallback} from '../../shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper'; +import {changeModelToken} from '../../../models/change/change-model'; @customElement('gr-edit-controls') export class GrEditControls extends LitElement { @@ -77,6 +78,8 @@ private readonly restApiService = getAppContext().restApiService; + private readonly getChangeModel = resolve(this, changeModelToken); + private readonly getNavigation = resolve(this, navigationToken); static override get styles() { @@ -451,7 +454,7 @@ return; } this.closeDialog(this.openDialog); - fireReload(this, true); + this.getChangeModel().navigateToChangeResetReload(); }); } @@ -471,7 +474,7 @@ return; } this.closeDialog(dialog); - fireReload(this, true); + this.getChangeModel().navigateToChangeResetReload(); }); }; @@ -489,7 +492,7 @@ return; } this.closeDialog(dialog); - fireReload(this, true); + this.getChangeModel().navigateToChangeResetReload(); }); }; @@ -507,7 +510,7 @@ return; } this.closeDialog(dialog); - fireReload(this, true); + this.getChangeModel().navigateToChangeResetReload(); }); };
diff --git a/polygerrit-ui/app/elements/edit/gr-edit-controls/gr-edit-controls_test.ts b/polygerrit-ui/app/elements/edit/gr-edit-controls/gr-edit-controls_test.ts index 0729f21..0e6778a 100644 --- a/polygerrit-ui/app/elements/edit/gr-edit-controls/gr-edit-controls_test.ts +++ b/polygerrit-ui/app/elements/edit/gr-edit-controls/gr-edit-controls_test.ts
@@ -26,8 +26,12 @@ import {fixture, html, assert} from '@open-wc/testing'; import {GrButton} from '../../shared/gr-button/gr-button'; import '../../shared/gr-dialog/gr-dialog'; -import {waitForEventOnce} from '../../../utils/event-util'; import {testResolver} from '../../../test/common-test-setup'; +import { + ChangeModel, + changeModelToken, +} from '../../../models/change/change-model'; +import {SinonStubbedMember} from 'sinon'; suite('gr-edit-controls tests', () => { let element: GrEditControls; @@ -36,6 +40,9 @@ let closeDialogSpy: sinon.SinonSpy; let hideDialogStub: sinon.SinonStub; let queryStub: sinon.SinonStub; + let navigateResetStub: SinonStubbedMember< + ChangeModel['navigateToChangeResetReload'] + >; setup(async () => { element = await fixture<GrEditControls>(html` @@ -47,6 +54,10 @@ closeDialogSpy = sinon.spy(element, 'closeDialog'); hideDialogStub = sinon.stub(element, 'hideAllDialogs'); queryStub = stubRestApi('queryChangeFiles').returns(Promise.resolve([])); + navigateResetStub = sinon.stub( + testResolver(changeModelToken), + 'navigateToChangeResetReload' + ); await element.updateComplete; }); @@ -298,7 +309,7 @@ assert.isTrue(deleteStub.called); await deleteStub.lastCall.returnValue; assert.equal(element.path, ''); - assert.equal(eventStub.firstCall.args[0].type, 'reload'); + assert.equal(navigateResetStub.callCount, 1); assert.isTrue(closeDialogSpy.called); }); @@ -397,7 +408,7 @@ await renameStub.lastCall.returnValue; assert.equal(element.path, ''); - assert.equal(eventStub.firstCall.args[0].type, 'reload'); + assert.equal(navigateResetStub.callCount, 1); assert.isTrue(closeDialogSpy.called); }); @@ -485,7 +496,7 @@ assert.equal(restoreStub.lastCall.args[1], 'src/test.cpp'); return restoreStub.lastCall.returnValue.then(() => { assert.equal(element.path, ''); - assert.equal(eventStub.firstCall.args[0].type, 'reload'); + assert.equal(navigateResetStub.callCount, 1); assert.isTrue(closeDialogSpy.called); }); }); @@ -553,7 +564,8 @@ assert.equal(fileStub.lastCall.args[0], 1); assert.equal(fileStub.lastCall.args[1], 'test.php'); assert.equal(fileStub.lastCall.args[2], 'base64'); - await waitForEventOnce(element, 'reload'); + await waitUntil(() => navigateResetStub.called); + assert.equal(navigateResetStub.callCount, 1); }); });
diff --git a/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.ts b/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.ts index e822d85..f37a3d9 100644 --- a/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.ts +++ b/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.ts
@@ -16,7 +16,7 @@ } from '../../../types/common'; import {ParsedChangeInfo} from '../../../types/types'; import {HttpMethod, NotifyType} from '../../../constants/constants'; -import {fireAlert, fireReload} from '../../../utils/event-util'; +import {fireAlert} from '../../../utils/event-util'; import {getAppContext} from '../../../services/app-context'; import {ErrorCallback} from '../../../api/rest'; import {assertIsDefined} from '../../../utils/common-util'; @@ -490,13 +490,7 @@ ) .then(() => { assertIsDefined(this.change, 'change'); - // TODO: `forceReload: true` does not seem to work as expected: The patchset is not - // updated. Thus we are also calling `fireReload()` here. That can probably be - // cleaned up once the change-view was migrated to fully relying on the change model. - fireReload(this); - this.getNavigation().setUrl( - createChangeUrl({change: this.change, forceReload: true}) - ); + this.getChangeModel().navigateToChangeResetReload(); }); }); };
diff --git a/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account-contents.ts b/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account-contents.ts index 14c2c15..394015e 100644 --- a/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account-contents.ts +++ b/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account-contents.ts
@@ -43,7 +43,7 @@ import {configModelToken} from '../../../models/config/config-model'; import {createSearchUrl} from '../../../models/views/search'; import {createDashboardUrl} from '../../../models/views/dashboard'; -import {fire} from '../../../utils/event-util'; +import {fire, fireReload} from '../../../utils/event-util'; import {userModelToken} from '../../../models/user/user-model'; @customElement('gr-hovercard-account-contents') @@ -445,7 +445,7 @@ this.getReviewerState(this.change!) ); } - fire(this, 'reload', {clearPatchset: true}); + fireReload(this); }); } @@ -464,7 +464,7 @@ if (!response || !response.ok) { throw new Error('something went wrong when removing user'); } - fire(this, 'reload', {clearPatchset: true}); + fireReload(this); return response; }); }
diff --git a/polygerrit-ui/app/models/change/change-model.ts b/polygerrit-ui/app/models/change/change-model.ts index a21c5cd..6d111aa 100644 --- a/polygerrit-ui/app/models/change/change-model.ts +++ b/polygerrit-ui/app/models/change/change-model.ts
@@ -17,14 +17,8 @@ CommitId, } from '../../types/common'; import {ChangeStatus, DefaultBase} from '../../constants/constants'; -import {combineLatest, from, fromEvent, Observable, forkJoin, of} from 'rxjs'; -import { - map, - filter, - withLatestFrom, - startWith, - switchMap, -} from 'rxjs/operators'; +import {combineLatest, from, Observable, forkJoin, of} from 'rxjs'; +import {map, filter, withLatestFrom, switchMap} from 'rxjs/operators'; import { computeAllPatchSets, computeLatestPatchNum, @@ -350,12 +344,6 @@ getRevertCreatedChangeIds(messages ?? []) ); - // For usage in `combineLatest` we need `startWith` such that reload$ has an - // initial value. - readonly reload$: Observable<unknown> = fromEvent(document, 'reload').pipe( - startWith(undefined) - ); - constructor( private readonly navigation: NavigationService, private readonly viewModel: ChangeViewModel, @@ -459,7 +447,7 @@ if (!editRev) { const msg = 'Change edit not found. Please create a change edit.'; fireAlert(document, msg); - this.navigateToChangeAndReset(); + this.navigateToChangeResetReload(); } }); } @@ -487,7 +475,7 @@ 'Change edits cannot be created if change is merged ' + 'or abandoned. Redirecting to non edit mode.'; fireAlert(document, msg); - this.navigateToChangeAndReset(); + this.navigateToChangeResetReload(); } }); } @@ -568,9 +556,8 @@ } private loadChange() { - return combineLatest([this.viewModel.changeNum$, this.reload$]) + return this.viewModel.changeNum$ .pipe( - map(([changeNum, _]) => changeNum), switchMap(changeNum => { if (changeNum !== undefined) this.updateStateLoading(changeNum); const change = from(this.restApiService.getChangeDetail(changeNum)); @@ -696,13 +683,21 @@ }); } + // Mainly used for navigating from DIFF to OVERVIEW. navigateToChange(openReplyDialog = false) { const url = this.changeUrl(openReplyDialog); if (!url) return; this.navigation.setUrl(url); } - navigateToChangeAndReset() { + /** + * Wipes all URL parameters and other view state and goes to the change + * overview page, forcing a reload. + * + * This will also wipe the `patchNum`, so will always go to the latest + * patchset. + */ + navigateToChangeResetReload() { if (!this.change) return; const url = createChangeUrl({change: this.change, forceReload: true}); if (!url) return;
diff --git a/polygerrit-ui/app/models/change/change-model_test.ts b/polygerrit-ui/app/models/change/change-model_test.ts index aa48ebc..e5d2e36 100644 --- a/polygerrit-ui/app/models/change/change-model_test.ts +++ b/polygerrit-ui/app/models/change/change-model_test.ts
@@ -7,6 +7,7 @@ import {ChangeStatus} from '../../constants/constants'; import '../../test/common-test-setup'; import { + TEST_NUMERIC_CHANGE_ID, createChange, createChangeMessageInfo, createChangeViewState, @@ -269,16 +270,19 @@ testResolver(changeViewModelToken).setState(createChangeViewState()); promise.resolve(knownChange); state = await waitForLoadingStatus(LoadingStatus.LOADED); + assert.equal(stub.callCount, 1); // Reloading same change document.dispatchEvent(new CustomEvent('reload')); state = await waitForLoadingStatus(LoadingStatus.RELOADING); - assert.equal(stub.callCount, 2); + assert.equal(stub.callCount, 3); + assert.equal(stub.getCall(1).firstArg, undefined); + assert.equal(stub.getCall(2).firstArg, TEST_NUMERIC_CHANGE_ID); assert.deepEqual(state?.change, updateRevisionsWithCommitShas(knownChange)); promise.resolve(knownChange); state = await waitForLoadingStatus(LoadingStatus.LOADED); - assert.equal(stub.callCount, 2); + assert.equal(stub.callCount, 3); assert.deepEqual(state?.change, updateRevisionsWithCommitShas(knownChange)); });
diff --git a/polygerrit-ui/app/models/change/files-model.ts b/polygerrit-ui/app/models/change/files-model.ts index dbd3c85..c01b718 100644 --- a/polygerrit-ui/app/models/change/files-model.ts +++ b/polygerrit-ui/app/models/change/files-model.ts
@@ -209,13 +209,12 @@ filesToState: (files: NormalizedFileInfo[]) => Partial<FilesState> ) { return combineLatest([ - this.changeModel.reload$, this.changeModel.changeNum$, this.changeModel.basePatchNum$, this.changeModel.patchNum$, ]) .pipe( - switchMap(([_, changeNum, basePatchNum, patchNum]) => { + switchMap(([changeNum, basePatchNum, patchNum]) => { if (!changeNum || !patchNum) return of({}); const range = rangeChooser(basePatchNum, patchNum); if (!range) return of({});
diff --git a/polygerrit-ui/app/models/change/related-changes-model.ts b/polygerrit-ui/app/models/change/related-changes-model.ts index 901999c..9ae4295 100644 --- a/polygerrit-ui/app/models/change/related-changes-model.ts +++ b/polygerrit-ui/app/models/change/related-changes-model.ts
@@ -128,12 +128,11 @@ private loadRelatedChanges() { return combineLatest([ - this.changeModel.reload$, this.changeModel.changeNum$, this.changeModel.latestPatchNum$, ]) .pipe( - switchMap(([_, changeNum, latestPatchNum]) => { + switchMap(([changeNum, latestPatchNum]) => { if (!changeNum || !latestPatchNum) return of(undefined); return from( this.restApiService @@ -148,12 +147,9 @@ } private loadSubmittedTogether() { - return combineLatest([ - this.changeModel.reload$, - this.changeModel.changeNum$, - ]) + return this.changeModel.changeNum$ .pipe( - switchMap(([_, changeNum]) => { + switchMap(changeNum => { if (!changeNum) return of(undefined); return from( this.restApiService.getChangesSubmittedTogether(changeNum) @@ -167,13 +163,12 @@ private loadCherryPicks() { return combineLatest([ - this.changeModel.reload$, this.changeModel.changeNum$, this.changeModel.changeId$, this.changeModel.repo$, ]) .pipe( - switchMap(([_, changeNum, changeId, repo]) => { + switchMap(([changeNum, changeId, repo]) => { if (!changeNum || !changeId || !repo) return of(undefined); return from( this.restApiService.getChangeCherryPicks(repo, changeId, changeNum) @@ -187,13 +182,12 @@ private loadConflictingChanges() { return combineLatest([ - this.changeModel.reload$, this.changeModel.changeNum$, this.changeModel.status$, this.changeModel.mergeable$, ]) .pipe( - switchMap(([_, changeNum, status, mergeable]) => { + switchMap(([changeNum, status, mergeable]) => { if (!changeNum || !status || !mergeable) return of(undefined); if (status !== ChangeStatus.NEW) return of(undefined); return from(this.restApiService.getChangeConflicts(changeNum)); @@ -206,13 +200,12 @@ private loadSameTopicChanges() { return combineLatest([ - this.changeModel.reload$, this.changeModel.changeNum$, this.changeModel.topic$, this.configModel.serverConfig$, ]) .pipe( - switchMap(([_, changeNum, topic, config]) => { + switchMap(([changeNum, topic, config]) => { if (!changeNum || !topic || !config) return of(undefined); if (config.change.submit_whole_topic) return of(undefined); return from( @@ -229,12 +222,9 @@ } private loadRevertingChanges() { - return combineLatest([ - this.changeModel.reload$, - this.changeModel.revertingChangeIds$, - ]) + return this.changeModel.revertingChangeIds$ .pipe( - switchMap(([_, changeIds]) => { + switchMap(changeIds => { if (!changeIds?.length) return of([]); return forkJoin( changeIds.map(changeId =>
diff --git a/polygerrit-ui/app/models/checks/checks-model.ts b/polygerrit-ui/app/models/checks/checks-model.ts index 2aa4aa4..729d46f 100644 --- a/polygerrit-ui/app/models/checks/checks-model.ts +++ b/polygerrit-ui/app/models/checks/checks-model.ts
@@ -195,8 +195,6 @@ private readonly documentVisibilityChange$ = new BehaviorSubject(undefined); - private readonly reloadListener: () => void; - private readonly visibilityChangeListener: () => void; public checksSelectedPatchsetNumber$ = select( @@ -414,8 +412,6 @@ 'visibilitychange', this.visibilityChangeListener ); - this.reloadListener = () => this.reloadAll(); - document.addEventListener('reload', this.reloadListener); } private reportStats(state: {[name: string]: ChecksProviderState}) { @@ -459,7 +455,6 @@ } override finalize() { - document.removeEventListener('reload', this.reloadListener); document.removeEventListener( 'visibilitychange', this.visibilityChangeListener
diff --git a/polygerrit-ui/app/models/comments/comments-model.ts b/polygerrit-ui/app/models/comments/comments-model.ts index a98fd58..4872e86 100644 --- a/polygerrit-ui/app/models/comments/comments-model.ts +++ b/polygerrit-ui/app/models/comments/comments-model.ts
@@ -405,8 +405,6 @@ private patchNum?: PatchSetNum; - private readonly reloadListener: () => void; - private drafts: {[path: string]: DraftInfo[]} = {}; private draftToastTask?: DelayedTask; @@ -447,16 +445,6 @@ this.reloadAllPortedComments(); }) ); - this.reloadListener = () => { - this.reloadAllComments(); - this.reloadAllPortedComments(); - }; - document.addEventListener('reload', this.reloadListener); - } - - override finalize() { - document.removeEventListener('reload', this.reloadListener); - super.finalize(); } // Note that this does *not* reload ported comments.
diff --git a/polygerrit-ui/app/models/views/change.ts b/polygerrit-ui/app/models/views/change.ts index 5799bbd..713d401 100644 --- a/polygerrit-ui/app/models/views/change.ts +++ b/polygerrit-ui/app/models/views/change.ts
@@ -69,9 +69,16 @@ /** for scrolling a Change Log message into view in gr-change-view */ messageHash?: string; - /** for logging where the user came from */ + /** + * For logging where the user came from. This is handled by the router, so + * this is not inspected by the model. + */ usp?: string; - /** triggers all change related data to be reloaded */ + /** + * Triggers all change related data to be reloaded. This is implemented by + * intercepting change view state updates and `forceReload` causing the view + * state to be wiped clean as `undefined` in an intermediate update. + */ forceReload?: boolean; /** triggers opening the reply dialog */ openReplyDialog?: boolean; @@ -328,6 +335,39 @@ }); } }); + document.addEventListener('reload', this.reload); + } + + override finalize(): void { + document.removeEventListener('reload', this.reload); + } + + /** + * Calling this is the same as firing the 'reload' event. This is also the + * same as adding `forceReload` parameter in the URL. See below. + */ + reload = () => { + const state = this.getState(); + if (state !== undefined) this.forceLoad(state); + }; + + /** + * This is the destination of where the `reload()` method, the `reload` event + * and the `forceReload` URL parameter all end up. + */ + private forceLoad(state: ChangeViewState) { + this.setState(undefined); + // We have to do this in a timeout, because we need the `undefined` value to + // be processed by all observers first and thus have the "reset" completed. + setTimeout(() => this.setState({...state, forceReload: undefined})); + } + + override setState(state: ChangeViewState | undefined): void { + if (state?.forceReload) { + this.forceLoad(state); + } else { + super.setState(state); + } } toggleSelectedCheckRun(checkName: string) {
diff --git a/polygerrit-ui/app/types/events.ts b/polygerrit-ui/app/types/events.ts index 82b8520..52535e6a5 100644 --- a/polygerrit-ui/app/types/events.ts +++ b/polygerrit-ui/app/types/events.ts
@@ -52,7 +52,7 @@ 'open-fix-preview': OpenFixPreviewEvent; 'reply-to-comment': ReplyToCommentEvent; // prettier-ignore - 'reload': ReloadEvent; + 'reload': CustomEvent<{}>; 'remove-reviewer': RemoveReviewerEvent; 'show-alert': ShowAlertEvent; 'show-error': ShowErrorEvent; @@ -68,7 +68,7 @@ 'network-error': NetworkErrorEvent; 'page-error': PageErrorEvent; // prettier-ignore - 'reload': ReloadEvent; + 'reload': CustomEvent<{}>; 'server-error': ServerErrorEvent; 'show-alert': ShowAlertEvent; 'show-error': ShowErrorEvent; @@ -186,11 +186,6 @@ } export type PageErrorEvent = CustomEvent<PageErrorEventDetail>; -export interface ReloadEventDetail { - clearPatchset: boolean; -} -export type ReloadEvent = CustomEvent<ReloadEventDetail>; - export interface RemoveAccountEventDetail { account: AccountInfo; }
diff --git a/polygerrit-ui/app/utils/event-util.ts b/polygerrit-ui/app/utils/event-util.ts index bf60c8c..af545e7 100644 --- a/polygerrit-ui/app/utils/event-util.ts +++ b/polygerrit-ui/app/utils/event-util.ts
@@ -122,8 +122,8 @@ fire(target, 'show-tab', detail); } -export function fireReload(target: EventTarget, clearPatchset?: boolean) { - fire(target, 'reload', {clearPatchset: !!clearPatchset}); +export function fireReload(target: EventTarget) { + fire(target, 'reload', {}); } export function waitForEventOnce<K extends keyof HTMLElementEventMap>(