Subscribe change-view to patchNum and basePatchNum And start using these model based states instead of the custom `patchRange` where it is safe. Add TODO to also migrate other usages. We cannot do this safely at the moment, because a lot of code in change-view is executed in response to `viewStateChanged()`. And we don't know in which order state is updated. Release-Notes: skip Google-Bug-Id: b/247042673 Change-Id: I9230ce556262437e3d3dcaf3bba86698ab37111f
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 3f09f74..e4a2cec 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
@@ -353,15 +353,22 @@ @state() private latestCommitMessage: string | null = ''; + @state() basePatchNum: BasePatchSetNum = PARENT; + + @state() patchNum?: RevisionPatchSetNum; + + // TODO: Migrate usages to this.patchNum and this.basePatchnum. // Use patchRange getter/setter. private _patchRange?: ChangeViewPatchRange; + // TODO: Migrate usages to this.patchNum and this.basePatchnum. // Private but used in tests. @state() get patchRange() { return this._patchRange; } + // TODO: Migrate usages to this.patchNum and this.basePatchnum. set patchRange(patchRange: ChangeViewPatchRange | undefined) { if (this._patchRange === patchRange) return; const oldPatchRange = this._patchRange; @@ -414,16 +421,8 @@ private updateCheckTimerHandle?: number | null; // Private but used in tests. - getEditMode() { - if (!this.patchRange || !this.viewState) { - return false; - } - - if (this.viewState.edit) { - return true; - } - - return this.patchRange.patchNum === EDIT; + getEditMode(): boolean { + return !!this.viewState?.edit || this.patchNum === EDIT; } isSubmitEnabled(): boolean { @@ -737,6 +736,16 @@ ); subscribe( this, + () => this.getChangeModel().patchNum$, + patchNum => (this.patchNum = patchNum) + ); + subscribe( + this, + () => this.getChangeModel().basePatchNum$, + basePatchNum => (this.basePatchNum = basePatchNum) + ); + subscribe( + this, () => this.getUserModel().account$, account => { this.account = account; @@ -2156,12 +2165,12 @@ // Private but used in tests. handleMessageAnchorTap(e: CustomEvent<{id: string}>) { assertIsDefined(this.change, 'change'); - assertIsDefined(this.patchRange, 'patchRange'); + assertIsDefined(this.patchNum, 'patchNum'); const hash = PREFIX + e.detail.id; const url = createChangeUrl({ change: this.change, - patchNum: this.patchRange.patchNum, - basePatchNum: this.patchRange.basePatchNum, + patchNum: this.patchNum, + basePatchNum: this.basePatchNum, edit: this.getEditMode(), messageHash: hash, }); @@ -2359,13 +2368,13 @@ // Private but used in tests. handleDiffAgainstBase() { assertIsDefined(this.change, 'change'); - assertIsDefined(this.patchRange, 'patchRange'); - if (this.patchRange.basePatchNum === PARENT) { + assertIsDefined(this.patchNum, 'patchNum'); + if (this.basePatchNum === PARENT) { fireAlert(this, 'Base is already selected.'); return; } this.getNavigation().setUrl( - createChangeUrl({change: this.change, patchNum: this.patchRange.patchNum}) + createChangeUrl({change: this.change, patchNum: this.patchNum}) ); } @@ -2373,16 +2382,16 @@ handleDiffBaseAgainstLeft() { if (this.viewState?.childView !== ChangeChildView.OVERVIEW) return; assertIsDefined(this.change, 'change'); - assertIsDefined(this.patchRange, 'patchRange'); + assertIsDefined(this.patchNum, 'patchNum'); - if (this.patchRange.basePatchNum === PARENT) { + if (this.basePatchNum === PARENT) { fireAlert(this, 'Left is already base.'); return; } this.getNavigation().setUrl( createChangeUrl({ change: this.change, - patchNum: this.patchRange.basePatchNum as RevisionPatchSetNum, + patchNum: this.basePatchNum as RevisionPatchSetNum, }) ); } @@ -2390,9 +2399,9 @@ // Private but used in tests. handleDiffAgainstLatest() { assertIsDefined(this.change, 'change'); - assertIsDefined(this.patchRange, 'patchRange'); + assertIsDefined(this.patchNum, 'patchNum'); const latestPatchNum = computeLatestPatchNum(this.allPatchSets); - if (this.patchRange.patchNum === latestPatchNum) { + if (this.patchNum === latestPatchNum) { fireAlert(this, 'Latest is already selected.'); return; } @@ -2400,7 +2409,7 @@ createChangeUrl({ change: this.change, patchNum: latestPatchNum, - basePatchNum: this.patchRange.basePatchNum, + basePatchNum: this.basePatchNum, }) ); } @@ -2408,9 +2417,9 @@ // Private but used in tests. handleDiffRightAgainstLatest() { assertIsDefined(this.change, 'change'); - assertIsDefined(this.patchRange, 'patchRange'); + assertIsDefined(this.patchNum, 'patchNum'); const latestPatchNum = computeLatestPatchNum(this.allPatchSets); - if (this.patchRange.patchNum === latestPatchNum) { + if (this.patchNum === latestPatchNum) { fireAlert(this, 'Right is already latest.'); return; } @@ -2418,7 +2427,7 @@ createChangeUrl({ change: this.change, patchNum: latestPatchNum, - basePatchNum: this.patchRange.patchNum as BasePatchSetNum, + basePatchNum: this.patchNum as BasePatchSetNum, }) ); } @@ -2426,12 +2435,9 @@ // Private but used in tests. handleDiffBaseAgainstLatest() { assertIsDefined(this.change, 'change'); - assertIsDefined(this.patchRange, 'patchRange'); + assertIsDefined(this.patchNum, 'patchNum'); const latestPatchNum = computeLatestPatchNum(this.allPatchSets); - if ( - this.patchRange.patchNum === latestPatchNum && - this.patchRange.basePatchNum === PARENT - ) { + if (this.patchNum === latestPatchNum && this.basePatchNum === PARENT) { fireAlert(this, 'Already diffing base against latest.'); return; } @@ -2550,7 +2556,7 @@ if ( !editRev && (changeIsMerged(change) || changeIsAbandoned(change)) && - this.getEditMode() + (this.patchRange?.patchNum === EDIT || this.viewState?.edit) ) { fireAlert( this, @@ -3048,7 +3054,7 @@ ); if (!controls) throw new Error('Missing edit controls'); assertIsDefined(this.change, 'change'); - assertIsDefined(this.patchRange, 'patchRange'); + assertIsDefined(this.patchNum, 'patchNum'); const path = e.detail.path; switch (e.detail.action) { @@ -3056,12 +3062,12 @@ controls.openDeleteDialog(path); break; case GrEditConstants.Actions.OPEN.id: - assertIsDefined(this.patchRange.patchNum, 'patchset number'); + assertIsDefined(this.patchNum, 'patchset number'); this.getNavigation().setUrl( createEditUrl({ changeNum: this.change._number, repo: this.change.project, - patchNum: this.patchRange.patchNum, + patchNum: this.patchNum, editView: {path}, }) ); @@ -3119,11 +3125,11 @@ private handleStopEditTap() { assertIsDefined(this.change, 'change'); - assertIsDefined(this.patchRange, 'patchRange'); + assertIsDefined(this.patchNum, 'patchNum'); this.getNavigation().setUrl( createChangeUrl({ change: this.change, - patchNum: this.patchRange.patchNum, + patchNum: this.patchNum, forceReload: true, }) );
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 21b163f..df48192 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
@@ -544,10 +544,7 @@ test('handleMessageAnchorTap', async () => { element.changeNum = 1 as NumericChangeId; - element.patchRange = { - basePatchNum: PARENT, - patchNum: 1 as RevisionPatchSetNum, - }; + element.patchNum = 1 as RevisionPatchSetNum; element.change = createChangeViewChange(); await element.updateComplete; const replaceStateStub = sinon.stub(history, 'replaceState'); @@ -563,10 +560,8 @@ ...createChangeViewChange(), revisions: createRevisions(10), }; - element.patchRange = { - patchNum: 3 as RevisionPatchSetNum, - basePatchNum: 1 as BasePatchSetNum, - }; + element.basePatchNum = 1 as BasePatchSetNum; + element.patchNum = 3 as RevisionPatchSetNum; element.handleDiffAgainstBase(); assert.isTrue(setUrlStub.called); assert.equal(setUrlStub.lastCall.firstArg, '/c/test-project/+/42/3'); @@ -577,10 +572,8 @@ ...createChangeViewChange(), revisions: createRevisions(10), }; - element.patchRange = { - basePatchNum: 1 as BasePatchSetNum, - patchNum: 3 as RevisionPatchSetNum, - }; + element.basePatchNum = 1 as BasePatchSetNum; + element.patchNum = 3 as RevisionPatchSetNum; element.handleDiffAgainstLatest(); assert.isTrue(setUrlStub.called); assert.equal(setUrlStub.lastCall.firstArg, '/c/test-project/+/42/1..10'); @@ -591,10 +584,8 @@ ...createChangeViewChange(), revisions: createRevisions(10), }; - element.patchRange = { - patchNum: 3 as RevisionPatchSetNum, - basePatchNum: 1 as BasePatchSetNum, - }; + element.basePatchNum = 1 as BasePatchSetNum; + element.patchNum = 3 as RevisionPatchSetNum; element.handleDiffBaseAgainstLeft(); assert.isTrue(setUrlStub.called); assert.equal(setUrlStub.lastCall.firstArg, '/c/test-project/+/42/1'); @@ -605,10 +596,8 @@ ...createChangeViewChange(), revisions: createRevisions(10), }; - element.patchRange = { - basePatchNum: 1 as BasePatchSetNum, - patchNum: 3 as RevisionPatchSetNum, - }; + element.basePatchNum = 1 as BasePatchSetNum; + element.patchNum = 3 as RevisionPatchSetNum; element.handleDiffRightAgainstLatest(); assert.isTrue(setUrlStub.called); assert.equal(setUrlStub.lastCall.firstArg, '/c/test-project/+/42/3..10'); @@ -619,10 +608,8 @@ ...createChangeViewChange(), revisions: createRevisions(10), }; - element.patchRange = { - basePatchNum: 1 as BasePatchSetNum, - patchNum: 3 as RevisionPatchSetNum, - }; + element.basePatchNum = 1 as BasePatchSetNum; + element.patchNum = 3 as RevisionPatchSetNum; element.handleDiffBaseAgainstLatest(); assert.isTrue(setUrlStub.called); assert.equal(setUrlStub.lastCall.firstArg, '/c/test-project/+/42/10'); @@ -1869,6 +1856,8 @@ test('computeEditMode', async () => { const callCompute = async (viewState: ChangeViewState) => { element.viewState = viewState; + element.patchNum = viewState.patchNum; + element.basePatchNum = viewState.basePatchNum ?? PARENT; await element.updateComplete; return element.getEditMode(); }; @@ -1932,10 +1921,7 @@ }); test('file-action-tap handling', async () => { - element.patchRange = { - basePatchNum: PARENT, - patchNum: 1 as RevisionPatchSetNum, - }; + element.patchNum = 1 as RevisionPatchSetNum; element.change = { ...createChangeViewChange(), }; @@ -2178,7 +2164,7 @@ assertIsDefined(element.actions); sinon.stub(element.metadata, 'computeLabelNames'); - element.patchRange = {patchNum: 1 as RevisionPatchSetNum}; + element.patchNum = 1 as RevisionPatchSetNum; element.actions.dispatchEvent( new CustomEvent('stop-edit-tap', {bubbles: false}) );
diff --git a/polygerrit-ui/app/utils/patch-set-util.ts b/polygerrit-ui/app/utils/patch-set-util.ts index 7e16ad9..7f3b6eb 100644 --- a/polygerrit-ui/app/utils/patch-set-util.ts +++ b/polygerrit-ui/app/utils/patch-set-util.ts
@@ -35,11 +35,6 @@ wip?: boolean; } -interface PatchRange { - patchNum?: RevisionPatchSetNum; - basePatchNum?: BasePatchSetNum; -} - /** * Whether the given patch is a numbered parent of a merge (i.e. a negative * number). @@ -294,10 +289,6 @@ return allPatchSets[0].num === EDIT; } -export function hasEditPatchsetLoaded(patchRange: PatchRange) { - return patchRange.patchNum === EDIT; -} - /** * @param revisions A sorted array of revisions. *