Remove `viewStateChanged()` method from gr-diff-view
Instead introduce two dedicated methods:
1. reloadDiff() for (re-)loading the entire diff by calling into
gr-diff-host, which actually manages the entire loading process.
2. initializePositions() for when we just want to reset the view
without reloading the diff, e.g. when coming back from the change
page to the same diff. Then we just need to reset the scroll
position and re-initialize the cursor.
Release-Notes: skip
Change-Id: Ib41b393f16b71449daa0355336d44eeeafbf9e46
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
index 274d0fb..de92b16 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
@@ -599,7 +599,11 @@
this.hasReloadBeenCalledOnce = true;
this.reporting.time(Timing.DIFF_TOTAL);
this.reporting.time(Timing.DIFF_LOAD);
+ // TODO: Find better names for these 3 clear/cancel methods. Ideally the
+ // <gr-diff-host> should not re-used at all for another diff rendering pass.
this.clear();
+ this.cancel();
+ this.clearDiffContent();
assertIsDefined(this.path, 'path');
assertIsDefined(this.changeNum, 'changeNum');
this.diff = undefined;
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 160c9aa..65a5a23 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
@@ -62,7 +62,7 @@
import {assertIsDefined, queryAndAssert} from '../../../utils/common-util';
import {Key, toggleClass, whenVisible} from '../../../utils/dom-util';
import {CursorMoveResult} from '../../../api/core';
-import {isFalse, throttleWrap, until} from '../../../utils/async-util';
+import {throttleWrap} from '../../../utils/async-util';
import {filter, take, switchMap} from 'rxjs/operators';
import {combineLatest} from 'rxjs';
import {
@@ -70,13 +70,11 @@
ShortcutSection,
shortcutsServiceToken,
} from '../../../services/shortcuts/shortcuts-service';
-import {LoadingStatus} from '../../../models/change/change-model';
import {DisplayLine} from '../../../api/diff';
import {GrDownloadDialog} from '../../change/gr-download-dialog/gr-download-dialog';
import {commentsModelToken} from '../../../models/comments/comments-model';
import {changeModelToken} from '../../../models/change/change-model';
import {resolve} from '../../../models/dependency';
-import {BehaviorSubject} from 'rxjs';
import {css, html, LitElement, nothing, PropertyValues} from 'lit';
import {ShortcutController} from '../../lit/shortcut-controller';
import {subscribe} from '../../lit/subscription-controller';
@@ -90,7 +88,6 @@
import {
ChangeChildView,
changeViewModelToken,
- ChangeViewState,
} from '../../../models/views/change';
import {GeneratedWebLink} from '../../../utils/weblink-util';
import {userModelToken} from '../../../models/user/user-model';
@@ -149,21 +146,6 @@
@query('#diffPreferencesDialog')
diffPreferencesDialog?: GrDiffPreferencesDialog;
- private _viewState: ChangeViewState | undefined;
-
- @state()
- get viewState(): ChangeViewState | undefined {
- 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);
- }
-
// Private but used in tests.
@state()
get patchRange(): PatchRange | undefined {
@@ -221,13 +203,13 @@
this.requestUpdate('path', oldPath);
}
+ /** Allows us to react when the user switches to the DIFF view. */
// Private but used in tests.
- @state()
- loggedIn = false;
+ @state() isActiveChildView = false;
// Private but used in tests.
@state()
- loading = true;
+ loggedIn = false;
@property({type: Object})
prefs?: DiffPreferencesInfo;
@@ -288,8 +270,6 @@
@state()
cursor?: GrDiffCursor;
- private connected$ = new BehaviorSubject(false);
-
private readonly shortcutsController = new ShortcutController(this);
constructor() {
@@ -298,11 +278,6 @@
this.setupSubscriptions();
subscribe(
this,
- () => this.getViewModel().state$,
- x => (this.viewState = x)
- );
- subscribe(
- this,
() => this.getFilesModel().filesIncludingUnmodified$,
files => {
const filesByPath: FileNameToNormalizedFileInfoMap = {};
@@ -454,6 +429,11 @@
);
subscribe(
this,
+ () => this.getViewModel().childView$,
+ childView => (this.isActiveChildView = childView === ChangeChildView.DIFF)
+ );
+ subscribe(
+ this,
() => this.getViewModel().diffPath$,
path => (this.path = path)
);
@@ -501,14 +481,14 @@
switchMap(() =>
combineLatest([
this.getChangeModel().patchNum$,
- this.getViewModel().state$,
+ this.getViewModel().childView$,
this.getUserModel().diffPreferences$,
this.getChangeModel().reviewedFiles$,
]).pipe(
filter(
- ([patchNum, viewState, diffPrefs, reviewedFiles]) =>
+ ([patchNum, childView, diffPrefs, reviewedFiles]) =>
!!patchNum &&
- viewState?.childView === ChangeChildView.DIFF &&
+ childView === ChangeChildView.DIFF &&
!!diffPrefs &&
!!reviewedFiles
),
@@ -714,7 +694,6 @@
override connectedCallback() {
super.connectedCallback();
- this.connected$.next(true);
this.throttledToggleFileReviewed = throttleWrap(_ =>
this.handleToggleFileReviewed()
);
@@ -725,7 +704,6 @@
override disconnectedCallback() {
this.cursor?.dispose();
- this.connected$.next(false);
super.disconnectedCallback();
}
@@ -738,19 +716,33 @@
protected override updated(changedProperties: PropertyValues): void {
super.updated(changedProperties);
if (
+ changedProperties.has('change') ||
+ changedProperties.has('path') ||
+ changedProperties.has('patchNum') ||
+ changedProperties.has('basePatchNum')
+ ) {
+ this.reloadDiff();
+ } else if (
+ changedProperties.has('isActiveChildView') &&
+ this.isActiveChildView
+ ) {
+ this.initializePositions();
+ }
+ if (
changedProperties.has('focusLineNum') ||
changedProperties.has('leftSide')
) {
this.initLineOfInterestAndCursor();
}
if (
+ changedProperties.has('change') ||
changedProperties.has('changeComments') ||
changedProperties.has('path') ||
changedProperties.has('patchNum') ||
changedProperties.has('basePatchNum') ||
changedProperties.has('files')
) {
- if (this.changeComments && this.path && this.patchRange) {
+ if (this.change && this.changeComments && this.path && this.patchRange) {
assertIsDefined(this.diffHost, 'diffHost');
const file = this.files?.changeFilesByPath?.[this.path];
this.diffHost.updateComplete.then(() => {
@@ -768,18 +760,16 @@
}
override render() {
- if (this.viewState?.childView !== ChangeChildView.DIFF) return nothing;
- if (!this.patchNum) return nothing;
- if (!this.changeNum) return nothing;
- if (!this.path) return nothing;
+ if (!this.isActiveChildView) return nothing;
+ if (!this.patchNum || !this.changeNum || !this.change || !this.path) {
+ return html`<div class="loading">Loading...</div>`;
+ }
const file = this.getFileRange();
return html`
${this.renderStickyHeader()}
- <div class="loading" ?hidden=${!this.loading}>Loading...</div>
<h2 class="assistive-tech-only">Diff view</h2>
<gr-diff-host
id="diffHost"
- ?hidden=${this.loading}
.changeNum=${this.changeNum}
.change=${this.change}
.patchRange=${this.patchRange}
@@ -1386,96 +1376,27 @@
history.replaceState(null, '', url);
}
- // TODO: This is probably not working anymore as expected, because we don't
- // know in which order the view model subscriptions fire.
- private isSameDiffLoaded(value: ChangeViewState) {
- return (
- this.basePatchNum === value.basePatchNum &&
- this.patchNum === value.patchNum &&
- this.path === value.diffView?.path
- );
+ async reloadDiff() {
+ if (!this.diffHost) return;
+ await this.diffHost.reload(true);
+ this.reporting.diffViewDisplayed();
+ if (this.isBlameLoaded) this.loadBlame();
}
- private async untilModelLoaded() {
- // NOTE: Wait until this page is connected before determining whether the
- // model is loaded. This can happen when params are changed when setting up
- // this view. It's unclear whether this issue is related to Polymer
- // specifically.
- if (!this.isConnected) {
- await until(this.connected$, connected => connected);
- }
- await until(
- this.getChangeModel().changeLoadingStatus$,
- status => status === LoadingStatus.LOADED
- );
- }
-
- // Private but used in tests.
- viewStateChanged() {
- if (this.viewState?.childView !== ChangeChildView.DIFF) return;
- const viewState = this.viewState;
-
+ /**
+ * (Re-initialize) the diff view without actually reloading the diff. The
+ * typical user journey is that the user comes back from the change page.
+ */
+ initializePositions() {
// The diff view is kept in the background once created. If the user
// scrolls in the change page, the scrolling is reflected in the diff view
// as well, which means the diff is scrolled to a random position based
// on how much the change view was scrolled.
// Hence, reset the scroll position here.
document.documentElement.scrollTop = 0;
-
- if (this.changeNum !== undefined && this.isSameDiffLoaded(viewState)) {
- // changeNum has not changed, so check if there are changes in patchRange
- // path. If no changes then we can simply render the view as is.
- this.reporting.reportInteraction('diff-view-re-rendered');
- // Make sure to re-initialize the cursor because this is typically
- // done on the 'render' event which doesn't fire in this path as
- // rerendering is avoided.
- this.reInitCursor();
- this.diffHost?.initLayers();
- return;
- }
-
+ this.reInitCursor();
+ this.diffHost?.initLayers();
this.classList.remove('hideComments');
-
- // When navigating away from the page, there is a possibility that the
- // patch number is no longer a part of the URL (say when navigating to
- // the top-level change info view) and therefore undefined in `params`.
- if (!viewState.patchNum) {
- this.reporting.error(
- 'GrDiffView',
- new Error(`Invalid diff view URL, no patchNum found: ${this.viewState}`)
- );
- return;
- }
-
- const promises: Promise<unknown>[] = [];
- if (!this.change) {
- promises.push(this.untilModelLoaded());
- }
- promises.push(this.waitUntilCommentsLoaded());
-
- if (this.diffHost) {
- this.diffHost.cancel();
- this.diffHost.clearDiffContent();
- }
- this.loading = true;
- return Promise.all(promises)
- .then(() => {
- this.loading = false;
- return this.updateComplete.then(() => this.diffHost!.reload(true));
- })
- .then(() => {
- this.reporting.diffViewDisplayed();
- })
- .then(() => {
- // If the blame was loaded for a previous file and user navigates to
- // another file, then we load the blame for this file too
- if (this.isBlameLoaded) this.loadBlame();
- });
- }
-
- private async waitUntilCommentsLoaded() {
- await until(this.connected$, c => c);
- await until(this.getCommentsModel().commentsLoading$, isFalse);
}
/**
@@ -1722,6 +1643,7 @@
// Private but used in tests.
handleDiffAgainstBase() {
+ if (!this.isActiveChildView) return;
assertIsDefined(this.path, 'path');
assertIsDefined(this.patchNum, 'patchNum');
@@ -1738,7 +1660,7 @@
// Private but used in tests.
handleDiffBaseAgainstLeft() {
- if (this.viewState?.childView !== ChangeChildView.DIFF) return;
+ if (!this.isActiveChildView) return;
assertIsDefined(this.path, 'path');
assertIsDefined(this.patchNum, 'patchNum');
@@ -1755,6 +1677,7 @@
// Private but used in tests.
handleDiffAgainstLatest() {
+ if (!this.isActiveChildView) return;
assertIsDefined(this.path, 'path');
assertIsDefined(this.patchNum, 'patchNum');
@@ -1772,6 +1695,7 @@
// Private but used in tests.
handleDiffRightAgainstLatest() {
+ if (!this.isActiveChildView) return;
assertIsDefined(this.path, 'path');
assertIsDefined(this.patchNum, 'patchNum');
@@ -1789,6 +1713,7 @@
// Private but used in tests.
handleDiffBaseAgainstLatest() {
+ if (!this.isActiveChildView) return;
assertIsDefined(this.path, 'path');
assertIsDefined(this.patchNum, 'patchNum');
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.ts b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.ts
index c35df68..1999a82 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.ts
@@ -17,7 +17,6 @@
query,
queryAll,
queryAndAssert,
- stubReporting,
stubRestApi,
waitEventLoop,
waitUntil,
@@ -181,48 +180,6 @@
sinon.restore();
});
- test('viewState change triggers diffViewDisplayed()', () => {
- const diffViewDisplayedStub = stubReporting('diffViewDisplayed');
- assertIsDefined(element.diffHost);
- sinon.stub(element.diffHost, 'reload').returns(Promise.resolve());
- const viewStateChangedSpy = sinon.spy(element, 'viewStateChanged');
- element.viewState = {
- ...createDiffViewState(),
- patchNum: 2 as RevisionPatchSetNum,
- basePatchNum: 1 as BasePatchSetNum,
- diffView: {path: '/COMMIT_MSG'},
- };
- element.path = '/COMMIT_MSG';
- element.patchNum = 1 as RevisionPatchSetNum;
- element.basePatchNum = PARENT;
- return viewStateChangedSpy.returnValues[0]?.then(() => {
- assert.isTrue(diffViewDisplayedStub.calledOnce);
- });
- });
-
- test('viewState change causes blame to load if it was set to true', () => {
- // Blame loads for subsequent files if it was loaded for one file
- element.isBlameLoaded = true;
- stubReporting('diffViewDisplayed');
- const loadBlameStub = sinon.stub(element, 'loadBlame');
- assertIsDefined(element.diffHost);
- sinon.stub(element.diffHost, 'reload').returns(Promise.resolve());
- const viewStateChangedSpy = sinon.spy(element, 'viewStateChanged');
- element.viewState = {
- ...createDiffViewState(),
- patchNum: 2 as RevisionPatchSetNum,
- basePatchNum: 1 as BasePatchSetNum,
- diffView: {path: '/COMMIT_MSG'},
- };
- element.path = '/COMMIT_MSG';
- element.patchNum = 1 as RevisionPatchSetNum;
- element.basePatchNum = PARENT;
- return viewStateChangedSpy.returnValues[0]!.then(() => {
- assert.isTrue(element.isBlameLoaded);
- assert.isTrue(loadBlameStub.calledOnce);
- });
- });
-
test('toggle left diff with a hotkey', () => {
assertIsDefined(element.diffHost);
const toggleLeftDiffStub = sinon.stub(element.diffHost, 'toggleLeftDiff');
@@ -393,9 +350,8 @@
</a>
</div>
</div>
- <div class="loading">Loading...</div>
<h2 class="assistive-tech-only">Diff view</h2>
- <gr-diff-host hidden="" id="diffHost"> </gr-diff-host>
+ <gr-diff-host id="diffHost"> </gr-diff-host>
<gr-apply-fix-dialog id="applyFixDialog"> </gr-apply-fix-dialog>
<gr-diff-preferences-dialog id="diffPreferencesDialog">
</gr-diff-preferences-dialog>
@@ -439,21 +395,19 @@
assert.deepEqual(navToDiffStub.lastCall.args, [
{path: 'wheatley.md', lineNum: undefined},
]);
+
element.path = 'wheatley.md';
await element.updateComplete;
- assert.isTrue(element.loading);
-
pressKey(element, '[');
assert.equal(navToDiffStub.callCount, 2);
assert.deepEqual(navToDiffStub.lastCall.args, [
{path: 'glados.txt', lineNum: undefined},
]);
+
element.path = 'glados.txt';
await element.updateComplete;
- assert.isTrue(element.loading);
-
pressKey(element, '[');
assert.equal(navToDiffStub.callCount, 3);
assert.deepEqual(navToDiffStub.lastCall.args, [
@@ -463,12 +417,9 @@
element.path = 'chell.go';
await element.updateComplete;
- assert.isTrue(element.loading);
-
pressKey(element, '[');
assert.equal(navToChangeStub.callCount, 2);
await element.updateComplete;
- assert.isTrue(element.loading);
assertIsDefined(element.diffPreferencesDialog);
const showPrefsStub = sinon
@@ -616,12 +567,12 @@
element.latestPatchNum = 10 as PatchSetNumber;
element.patchNum = 3 as RevisionPatchSetNum;
element.basePatchNum = 1 as BasePatchSetNum;
- element.viewState = {
+ viewModel.setState({
...createDiffViewState(),
patchNum: 3 as RevisionPatchSetNum,
basePatchNum: 1 as BasePatchSetNum,
diffView: {path: 'foo'},
- };
+ });
await element.updateComplete;
element.handleDiffBaseAgainstLeft();
const expected = [{path: 'foo'}, 1, PARENT];
@@ -730,7 +681,6 @@
assert.equal(navToChangeStub.callCount, 1);
pressKey(element, ']');
- assert.isTrue(element.loading);
assert.equal(navToDiffStub.callCount, 1);
assert.deepEqual(navToDiffStub.lastCall.args, [
{path: 'wheatley.md', lineNum: undefined},
@@ -738,7 +688,6 @@
element.path = 'wheatley.md';
pressKey(element, '[');
- assert.isTrue(element.loading);
assert.equal(navToDiffStub.callCount, 2);
assert.deepEqual(navToDiffStub.lastCall.args, [
{path: 'glados.txt', lineNum: undefined},
@@ -746,7 +695,6 @@
element.path = 'glados.txt';
pressKey(element, '[');
- assert.isTrue(element.loading);
assert.equal(navToDiffStub.callCount, 3);
assert.deepEqual(navToDiffStub.lastCall.args, [
{path: 'chell.go', lineNum: undefined},
@@ -754,7 +702,6 @@
element.path = 'chell.go';
pressKey(element, '[');
- assert.isTrue(element.loading);
assert.equal(navToChangeStub.callCount, 2);
assertIsDefined(element.downloadModal);
@@ -809,6 +756,26 @@
assert.equal(navToChangeStub.callCount, 2);
});
+ test('reloadDiff is called when patchNum changes', async () => {
+ const reloadStub = sinon.stub(element, 'reloadDiff');
+ element.patchNum = 5 as RevisionPatchSetNum;
+ await element.updateComplete;
+ assert.isTrue(reloadStub.called);
+ });
+
+ test('initializePositions is called when view becomes active', async () => {
+ const reloadStub = sinon.stub(element, 'reloadDiff');
+ const initializeStub = sinon.stub(element, 'initializePositions');
+
+ element.isActiveChildView = false;
+ await element.updateComplete;
+ element.isActiveChildView = true;
+ await element.updateComplete;
+
+ assert.isTrue(initializeStub.calledOnce);
+ assert.isFalse(reloadStub.called);
+ });
+
test('edit should redirect to edit page', async () => {
element.loggedIn = true;
element.path = 't.txt';
@@ -1319,10 +1286,10 @@
const callCount = saveReviewedStub.callCount;
- element.viewState = {
+ viewModel.setState({
...createDiffViewState(),
repo: 'test' as RepoName,
- };
+ });
await element.updateComplete;
// saveReviewedState observer observes viewState, but should not fire when
@@ -1855,12 +1822,12 @@
sinon.stub(element, 'initLineOfInterestAndCursor');
// Load file1
- element.viewState = {
+ viewModel.setState({
...createDiffViewState(),
patchNum: 1 as RevisionPatchSetNum,
repo: 'test-project' as RepoName,
diffView: {path: 'file1'},
- };
+ });
element.patchNum = 1 as RevisionPatchSetNum;
element.basePatchNum = PARENT;
element.change = {
@@ -1877,12 +1844,12 @@
assert.isTrue(navToDiffStub.calledOnce);
// This is to mock the param change triggered by above navigate
- element.viewState = {
+ viewModel.setState({
...createDiffViewState(),
patchNum: 1 as RevisionPatchSetNum,
repo: 'test-project' as RepoName,
diffView: {path: 'file2'},
- };
+ });
element.patchNum = 1 as RevisionPatchSetNum;
element.basePatchNum = PARENT;
