Merge "Use the `keyed()` directive for switching change and diff view"
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 371e5b5..72a6a3a 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
@@ -2080,13 +2080,7 @@
// Private but used in tests.
viewStateChanged() {
if (!this.viewState) return;
-
- if (this.isChangeObsolete()) {
- // Tell the app element that we are not going to handle the new change
- // number and that they have to create a new change view.
- fireEvent(this, EventType.RECREATE_CHANGE_VIEW);
- return;
- }
+ if (this.isChangeObsolete()) return;
if (this.viewState.basePatchNum === undefined)
this.viewState.basePatchNum = PARENT;
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 9b78e64..ac4c1f9 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
@@ -1557,21 +1557,6 @@
assert.isOk(element.patchRange?.patchNum);
});
- test('do not handle new change numbers', async () => {
- const recreateSpy = sinon.spy();
- element.addEventListener('recreate-change-view', recreateSpy);
-
- const value: ChangeViewState = createChangeViewState();
- element.viewState = value;
- await element.updateComplete;
- assert.isFalse(recreateSpy.calledOnce);
-
- value.changeNum = 555111333 as NumericChangeId;
- element.viewState = {...value};
- await element.updateComplete;
- assert.isTrue(recreateSpy.calledOnce);
- });
-
test('related changes are updated when loadData is called', async () => {
await element.updateComplete;
const relatedChanges = element.shadowRoot!.querySelector(
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 c74993f..c7d6948 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
@@ -53,11 +53,7 @@
import {CommentSide, DiffViewMode, Side} from '../../../constants/constants';
import {GrApplyFixDialog} from '../gr-apply-fix-dialog/gr-apply-fix-dialog';
import {CommentMap} from '../../../utils/comment-util';
-import {
- EventType,
- OpenFixPreviewEvent,
- ValueChangedEvent,
-} from '../../../types/events';
+import {OpenFixPreviewEvent, ValueChangedEvent} from '../../../types/events';
import {fireAlert, fireEvent, fireTitleChange} from '../../../utils/event-util';
import {assertIsDefined, queryAndAssert} from '../../../utils/common-util';
import {Key, toggleClass, whenVisible} from '../../../utils/dom-util';
@@ -417,14 +413,12 @@
changeNum => {
if (!changeNum || this.changeNum === changeNum) return;
- // We are only setting the changeNum of the diff view once!
+ // We are only setting the changeNum of the diff view once.
// Everything in the diff view is tied to the change. It seems better to
// force the re-creation of the diff view when the change number changes.
- if (!this.changeNum) {
- this.changeNum = changeNum;
- } else {
- fireEvent(this, EventType.RECREATE_DIFF_VIEW);
- }
+ // The parent element will make sure that a new change view is created
+ // when the change number changes (using the `keyed` directive).
+ if (!this.changeNum) this.changeNum = changeNum;
}
);
subscribe(
diff --git a/polygerrit-ui/app/elements/gr-app-element.ts b/polygerrit-ui/app/elements/gr-app-element.ts
index eceb05e..b096f76 100644
--- a/polygerrit-ui/app/elements/gr-app-element.ts
+++ b/polygerrit-ui/app/elements/gr-app-element.ts
@@ -32,7 +32,7 @@
import {navigationToken} from './core/gr-navigation/gr-navigation';
import {getAppContext} from '../services/app-context';
import {routerToken} from './core/gr-router/gr-router';
-import {AccountDetailInfo} from '../types/common';
+import {AccountDetailInfo, NumericChangeId} from '../types/common';
import {
constructServerErrorMsg,
GrErrorManager,
@@ -62,6 +62,7 @@
import {customElement, property, query, state} from 'lit/decorators.js';
import {Shortcut, ShortcutController} from './lit/shortcut-controller';
import {cache} from 'lit/directives/cache.js';
+import {keyed} from 'lit/directives/keyed.js';
import {assertIsDefined} from '../utils/common-util';
import './gr-css-mixins';
import {isDarkTheme, prefersDarkColorScheme} from '../utils/theme-util';
@@ -123,6 +124,9 @@
// TODO: Introduce a wrapper element for CHANGE, DIFF, EDIT view.
@state() private childView?: ChangeChildView;
+ // Used as a key for caching the CHANGE, DIFF, EDIT view.
+ @state() private changeNum?: NumericChangeId;
+
@state() private lastError?: ErrorInfo;
// private but used in test
@@ -148,12 +152,6 @@
// (e.g. shortcut dialog) is open
@state() private mainAriaHidden = false;
- // Triggers dom-if unsetting/setting restamp behaviour in lit
- @state() private invalidateChangeViewCache = false;
-
- // Triggers dom-if unsetting/setting restamp behaviour in lit
- @state() private invalidateDiffViewCache = false;
-
@state() private theme = AppTheme.AUTO;
readonly getRouter = resolve(this, routerToken);
@@ -189,12 +187,6 @@
document.addEventListener(EventType.LOCATION_CHANGE, () =>
this.handleLocationChange()
);
- this.addEventListener(EventType.RECREATE_CHANGE_VIEW, () =>
- this.handleRecreateView()
- );
- this.addEventListener(EventType.RECREATE_DIFF_VIEW, () =>
- this.handleRecreateView()
- );
document.addEventListener(EventType.GR_RPC_LOG, e => this.handleRpcLog(e));
this.shortcuts.addAbstract(Shortcut.OPEN_SHORTCUT_HELP_DIALOG, () =>
this.showKeyboardShortcuts()
@@ -246,8 +238,13 @@
subscribe(
this,
() => this.getChangeViewModel().childView$,
- childView => {
- this.childView = childView;
+ childView => (this.childView = childView)
+ );
+ subscribe(
+ this,
+ () => this.getChangeViewModel().changeNum$,
+ changeNum => {
+ this.changeNum = changeNum;
}
);
@@ -375,8 +372,19 @@
${this.renderHeader()}
<main ?aria-hidden=${this.mainAriaHidden}>
${this.renderMobileSearch()} ${this.renderChangeListView()}
- ${this.renderDashboardView()} ${this.renderChangeView()}
- ${this.renderEditorView()} ${this.renderDiffView()}
+ ${this.renderDashboardView()}
+ ${
+ // `keyed(this.changeNum, ...)` makes sure that these views are not
+ // re-used across changes, which is a precaution, because we have run
+ // into issue with that. That could be re-considered at some point.
+ keyed(
+ this.changeNum,
+ html`
+ ${this.renderChangeView()} ${this.renderEditorView()}
+ ${this.renderDiffView()}
+ `
+ )
+ }
${this.renderSettingsView()} ${this.renderAdminView()}
${this.renderPluginScreen()} ${this.renderCLAView()}
${this.renderDocumentationSearch()}
@@ -473,18 +481,15 @@
}
private renderChangeView() {
- if (this.invalidateChangeViewCache) {
- this.updateComplete.then(() => (this.invalidateChangeViewCache = false));
- return nothing;
- }
- return cache(this.isChangeView() ? this.changeViewTemplate() : nothing);
- }
-
- // Template as not to create duplicates, for renderChangeView() only.
- private changeViewTemplate() {
- return html`
- <gr-change-view .backPage=${this.lastSearchPage}></gr-change-view>
- `;
+ // The `cache()` is required for re-using the change view when switching
+ // back and forth between change, diff and editor views.
+ return cache(
+ this.isChangeView()
+ ? html`<gr-change-view
+ .backPage=${this.lastSearchPage}
+ ></gr-change-view>`
+ : nothing
+ );
}
private isChangeView() {
@@ -494,9 +499,11 @@
);
}
- private isDiffView() {
- return (
- this.view === GerritView.CHANGE && this.childView === ChangeChildView.DIFF
+ private renderEditorView() {
+ // The `cache()` is required for re-using the editor view when switching
+ // back and forth between change, diff and editor views.
+ return cache(
+ this.isEditorView() ? html`<gr-editor-view></gr-editor-view>` : nothing
);
}
@@ -506,21 +513,18 @@
);
}
- private renderEditorView() {
- if (!this.isEditorView()) return nothing;
- return html`<gr-editor-view></gr-editor-view>`;
- }
-
private renderDiffView() {
- if (this.invalidateDiffViewCache) {
- this.updateComplete.then(() => (this.invalidateDiffViewCache = false));
- return nothing;
- }
- return cache(this.isDiffView() ? this.diffViewTemplate() : nothing);
+ // The `cache()` is required for re-using the diff view when switching
+ // back and forth between change, diff and editor views.
+ return cache(
+ this.isDiffView() ? html`<gr-diff-view></gr-diff-view>` : nothing
+ );
}
- private diffViewTemplate() {
- return html`<gr-diff-view></gr-diff-view>`;
+ private isDiffView() {
+ return (
+ this.view === GerritView.CHANGE && this.childView === ChangeChildView.DIFF
+ );
}
private renderSettingsView() {
@@ -619,15 +623,6 @@
(this.account && this.account._account_id) || null;
}
- /**
- * Throws away the view and re-creates it. The view itself fires an event, if
- * it wants to be re-created.
- */
- private handleRecreateView() {
- this.invalidateChangeViewCache = true;
- this.invalidateDiffViewCache = true;
- }
-
private async viewChanged() {
if (
this.params &&
diff --git a/polygerrit-ui/app/types/events.ts b/polygerrit-ui/app/types/events.ts
index f642af7..2cc6742 100644
--- a/polygerrit-ui/app/types/events.ts
+++ b/polygerrit-ui/app/types/events.ts
@@ -28,8 +28,6 @@
OPEN_FIX_PREVIEW = 'open-fix-preview',
CLOSE_FIX_PREVIEW = 'close-fix-preview',
PAGE_ERROR = 'page-error',
- RECREATE_CHANGE_VIEW = 'recreate-change-view',
- RECREATE_DIFF_VIEW = 'recreate-diff-view',
RELOAD = 'reload',
REPLY = 'reply',
SERVER_ERROR = 'server-error',
