Re-create change and diff view if and only if the change number changes
Change-Id: Ia795d3bea4ba3cfb92420daa5b71b314a46cb1d3
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 4f60823..807d0cd 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
@@ -1220,6 +1220,14 @@
return;
}
+ // Everything in the change view is tied to the change. It seems better to
+ // force the re-creation of the change view when the change number changes.
+ const changeChanged = this._changeNum !== value.changeNum;
+ if (this._changeNum !== undefined && changeChanged) {
+ fireEvent(this, EventType.RECREATE_CHANGE_VIEW);
+ return;
+ }
+
if (value.changeNum && value.project) {
this.restApiService.setInProjectLookup(value.changeNum, value.project);
}
@@ -1230,7 +1238,6 @@
value.basePatchNum !== undefined &&
(this._patchRange.patchNum !== value.patchNum ||
this._patchRange.basePatchNum !== value.basePatchNum);
- const changeChanged = this._changeNum !== value.changeNum;
let rightPatchNumChanged =
this._patchRange &&
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 06c3ded..877bdd6 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
@@ -380,7 +380,7 @@
stubRestApi('getDiffRobotComments').returns(Promise.resolve({}));
stubRestApi('getDiffDrafts').returns(Promise.resolve({}));
element = fixture.instantiate();
- element._changeNum = 1 as NumericChangeId;
+ element._changeNum = TEST_NUMERIC_CHANGE_ID;
sinon.stub(element.$.actions, 'reload').returns(Promise.resolve());
getPluginLoader().loadPlugins([]);
pluginApi.install(
@@ -518,7 +518,7 @@
suite('plugins adding to file tab', () => {
setup(done => {
- element._changeNum = 1 as NumericChangeId;
+ element._changeNum = TEST_NUMERIC_CHANGE_ID;
// Resolving it here instead of during setup() as other tests depend
// on flush() not being called during setup.
flush(() => done());
@@ -2530,23 +2530,6 @@
});
});
- test('_paramsChanged sets in projectLookup', () => {
- flush();
- const relatedChanges = element.shadowRoot!.querySelector(
- '#relatedChanges'
- ) as GrRelatedChangesList;
- sinon.stub(relatedChanges, 'reload');
- sinon.stub(element, 'loadData').returns(Promise.resolve([]));
- const setStub = stubRestApi('setInProjectLookup');
- element._paramsChanged({
- view: GerritNav.View.CHANGE,
- changeNum: 101 as NumericChangeId,
- project: TEST_PROJECT_NAME,
- });
- assert.isTrue(setStub.calledOnce);
- assert.isTrue(setStub.calledWith(101 as never, TEST_PROJECT_NAME as never));
- });
-
test('_handleToggleStar called when star is tapped', () => {
element._change = {
...createChangeViewChange(),
@@ -2605,7 +2588,7 @@
);
element._paramsChanged({
...createAppElementChangeViewParams(),
- changeNum: 101 as NumericChangeId,
+ changeNum: TEST_NUMERIC_CHANGE_ID,
project: TEST_PROJECT_NAME,
});
flush(() => {
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 8ead181..af6bbf7 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
@@ -94,7 +94,11 @@
getPatchRangeForCommentUrl,
} from '../../../utils/comment-util';
import {AppElementParams} from '../../gr-app-types';
-import {CustomKeyboardEvent, OpenFixPreviewEvent} from '../../../types/events';
+import {
+ CustomKeyboardEvent,
+ EventType,
+ OpenFixPreviewEvent,
+} from '../../../types/events';
import {fireAlert, fireEvent, fireTitleChange} from '../../../utils/event-util';
import {GerritView} from '../../../services/router/router-model';
import {assertIsDefined} from '../../../utils/common-util';
@@ -1002,6 +1006,14 @@
return;
}
+ // 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.
+ const changeChanged = this._changeNum !== value.changeNum;
+ if (this._changeNum !== undefined && changeChanged) {
+ fireEvent(this, EventType.RECREATE_DIFF_VIEW);
+ return;
+ }
+
this._change = undefined;
this._files = {sortedFileList: [], changeFilesByPath: {}};
this._path = undefined;
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js
index 5c3dfdc..244bafb 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.js
@@ -1735,19 +1735,6 @@
});
});
- test('_paramsChanged sets in projectLookup', () => {
- sinon.stub(element, '_initLineOfInterestAndCursor');
- const setStub = stubRestApi('setInProjectLookup');
- element._paramsChanged({
- view: GerritNav.View.DIFF,
- changeNum: 101,
- project: 'test-project',
- path: '',
- });
- assert.isTrue(setStub.calledOnce);
- assert.isTrue(setStub.calledWith(101, 'test-project'));
- });
-
test('shift+m navigates to next unreviewed file', () => {
element._files = getFilesFromFileList(['file1', 'file2', 'file3']);
element._reviewedFiles = new Set(['file1', 'file2']);
diff --git a/polygerrit-ui/app/elements/gr-app-element.ts b/polygerrit-ui/app/elements/gr-app-element.ts
index aadb534..6e20a55 100644
--- a/polygerrit-ui/app/elements/gr-app-element.ts
+++ b/polygerrit-ui/app/elements/gr-app-element.ts
@@ -67,18 +67,19 @@
import {GrSettingsView} from './settings/gr-settings-view/gr-settings-view';
import {
CustomKeyboardEvent,
+ DialogChangeEventDetail,
+ EventType,
LocationChangeEvent,
PageErrorEventDetail,
RpcLogEvent,
ShortcutTriggeredEvent,
TitleChangeEventDetail,
- DialogChangeEventDetail,
- EventType,
} from '../types/events';
import {ViewState} from '../types/types';
import {GerritView} from '../services/router/router-model';
import {LifeCycle} from '../constants/reporting';
import {fireIronAnnounce} from '../utils/event-util';
+import {assertIsDefined} from '../utils/common-util';
interface ErrorInfo {
text: string;
@@ -95,6 +96,10 @@
};
}
+type DomIf = PolymerElement & {
+ restamp: boolean;
+};
+
// TODO(TS): implement AppElement interface from gr-app-types.ts
@customElement('gr-app-element')
export class GrAppElement extends KeyboardShortcutMixin(PolymerElement) {
@@ -235,6 +240,12 @@
this.addEventListener('location-change', e =>
this._handleLocationChange(e)
);
+ this.addEventListener(EventType.RECREATE_CHANGE_VIEW, () =>
+ this.handleRecreateView(GerritView.CHANGE)
+ );
+ this.addEventListener(EventType.RECREATE_DIFF_VIEW, () =>
+ this.handleRecreateView(GerritView.DIFF)
+ );
document.addEventListener('gr-rpc-log', e => this._handleRpcLog(e));
this.addEventListener('shortcut-triggered', e =>
this._handleShortcutTriggered(e)
@@ -458,6 +469,33 @@
(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(view: GerritView.DIFF | GerritView.CHANGE) {
+ const isDiff = view === GerritView.DIFF;
+ const domId = isDiff ? '#dom-if-diff-view' : '#dom-if-change-view';
+ const domIf = this.root!.querySelector(domId) as DomIf;
+ assertIsDefined(domIf, '<dom-if> for the view');
+ // The rendering of DomIf is debounced, so just changing _show...View and
+ // restamp properties back and forth won't work. That is why we are using
+ // timeouts.
+ // The first timeout is needed, because the _viewChanged() observer also
+ // affects _show...View and would change _show...View=false directly back to
+ // _show...View=true.
+ setTimeout(() => {
+ this._showChangeView = false;
+ this._showDiffView = false;
+ domIf.restamp = true;
+ setTimeout(() => {
+ this._showChangeView = this.params?.view === GerritView.CHANGE;
+ this._showDiffView = this.params?.view === GerritView.DIFF;
+ domIf.restamp = false;
+ }, 1);
+ }, 1);
+ }
+
@observe('params.*')
_viewChanged() {
const view = this.params?.view;
diff --git a/polygerrit-ui/app/elements/gr-app-element_html.ts b/polygerrit-ui/app/elements/gr-app-element_html.ts
index 81a9988..a1e6ac9 100644
--- a/polygerrit-ui/app/elements/gr-app-element_html.ts
+++ b/polygerrit-ui/app/elements/gr-app-element_html.ts
@@ -131,7 +131,9 @@
view-state="{{_viewState.dashboardView}}"
></gr-dashboard-view>
</template>
- <template is="dom-if" if="[[_showChangeView]]" restamp="true">
+ <!-- Note that the change view does not have restamp="true" set, because we
+ want to re-use it as long as the change number does not change. -->
+ <template id="dom-if-change-view" is="dom-if" if="[[_showChangeView]]">
<gr-change-view
params="[[params]]"
view-state="{{_viewState.changeView}}"
@@ -141,7 +143,9 @@
<template is="dom-if" if="[[_showEditorView]]" restamp="true">
<gr-editor-view params="[[params]]"></gr-editor-view>
</template>
- <template is="dom-if" if="[[_showDiffView]]" restamp="true">
+ <!-- Note that the diff view does not have restamp="true" set, because we
+ want to re-use it as long as the change number does not change. -->
+ <template id="dom-if-diff-view" is="dom-if" if="[[_showDiffView]]">
<gr-diff-view
params="[[params]]"
change-view-state="{{_viewState.changeView}}"
diff --git a/polygerrit-ui/app/types/events.ts b/polygerrit-ui/app/types/events.ts
index 2612131..3a61437 100644
--- a/polygerrit-ui/app/types/events.ts
+++ b/polygerrit-ui/app/types/events.ts
@@ -34,6 +34,8 @@
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',
