Move diffViewMode into model
Moving the diffViewMode property to the model allows any changes made
in the change page to the diffMode to reflect to the diff view
retained in the background.
Currently Gerrit has this feature where if the screen width is small
then we want to show Unified diff instead of Side by Side diff to the
user.
This is done by updating a default_diff_view property inside the
getPreferences() API call in gr-rest-api-interface. The implementation
runs into several issues which are fixed by this change.
1. The updated diff is only shown to the user if they reload the page
by requesting the preferences again.
2. Once the screen is narrowed and then expanded again, the diff view
mode remains Unified instead of reverting back to Side by Side(If the
user set that initially).
There was also the ability to temporarily change the diff mode until
the next reload but that has been removed for simplicity and now any
changes to the diff mode are reflected to the preference setting.
Change-Id: Ice95ed03faa5f05eaf650b06ba945206ca39cf6b
diff --git a/polygerrit-ui/app/constants/constants.ts b/polygerrit-ui/app/constants/constants.ts
index 645e770..6ff2894 100644
--- a/polygerrit-ui/app/constants/constants.ts
+++ b/polygerrit-ui/app/constants/constants.ts
@@ -256,7 +256,6 @@
export function createDefaultPreferences() {
return {
changes_per_page: 25,
- default_diff_view: DiffViewMode.SIDE_BY_SIDE,
diff_view: DiffViewMode.SIDE_BY_SIDE,
size_bar_in_change_table: true,
} as PreferencesInfo;
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 b635d3f..1a4b7a0 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
@@ -196,6 +196,7 @@
hasAttention,
} from '../../../utils/attention-set-util';
import {listen} from '../../../services/shortcuts/shortcuts-service';
+import {preferenceDiffViewMode$} from '../../../services/user/user-model';
const MIN_LINES_FOR_COMMIT_COLLAPSE = 18;
@@ -611,6 +612,10 @@
private lastStarredTimestamp?: number;
+ private readonly userService = appContext.userService;
+
+ private diffViewMode?: DiffViewMode;
+
override ready() {
super.ready();
aPluginHasRegistered$.pipe(takeUntil(this.disconnected$)).subscribe(b => {
@@ -622,6 +627,11 @@
drafts$.pipe(takeUntil(this.disconnected$)).subscribe(drafts => {
this._diffDrafts = {...drafts};
});
+ preferenceDiffViewMode$
+ .pipe(takeUntil(this.disconnected$))
+ .subscribe(diffViewMode => {
+ this.diffViewMode = diffViewMode;
+ });
changeComments$
.pipe(takeUntil(this.disconnected$))
.subscribe(changeComments => {
@@ -666,7 +676,6 @@
this._account = acct;
});
}
- this._setDiffViewMode();
});
this.replyDialogResizeObserver = new ResizeObserver(() =>
@@ -735,24 +744,6 @@
return this.shadowRoot!.querySelector<GrThreadList>('gr-thread-list');
}
- _setDiffViewMode(opt_reset?: boolean) {
- if (!opt_reset && this.viewState.diffViewMode) {
- return;
- }
-
- return this._getPreferences()
- .then(prefs => {
- if (!this.viewState.diffMode && prefs) {
- this.set('viewState.diffMode', prefs.default_diff_view);
- }
- })
- .then(() => {
- if (!this.viewState.diffMode) {
- this.set('viewState.diffMode', 'SIDE_BY_SIDE');
- }
- });
- }
-
_onOpenFixPreview(e: OpenFixPreviewEvent) {
this.$.applyFixDialog.open(e);
}
@@ -762,10 +753,12 @@
}
_handleToggleDiffMode() {
- if (this.viewState.diffMode === DiffViewMode.SIDE_BY_SIDE) {
- this.$.fileListHeader.setDiffViewMode(DiffViewMode.UNIFIED);
+ if (this.diffViewMode === DiffViewMode.SIDE_BY_SIDE) {
+ this.userService.updatePreferences({diff_view: DiffViewMode.UNIFIED});
} else {
- this.$.fileListHeader.setDiffViewMode(DiffViewMode.SIDE_BY_SIDE);
+ this.userService.updatePreferences({
+ diff_view: DiffViewMode.SIDE_BY_SIDE,
+ });
}
}
@@ -1416,9 +1409,6 @@
!!this.viewState.changeNum &&
this.viewState.changeNum !== this._changeNum
) {
- // Reset the diff mode to null when navigating from one change to
- // another, so that the user's preference is restored.
- this._setDiffViewMode(true);
this.set('_numFilesShown', DEFAULT_NUM_FILES_SHOWN);
}
this.set('viewState.changeNum', this._changeNum);
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_html.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_html.ts
index 0b77bc7..d42fc17 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_html.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_html.ts
@@ -533,7 +533,6 @@
server-config="[[_serverConfig]]"
shown-file-count="[[_shownFileCount]]"
diff-prefs="[[_diffPrefs]]"
- diff-view-mode="{{viewState.diffMode}}"
patch-num="{{_patchRange.patchNum}}"
base-patch-num="{{_patchRange.basePatchNum}}"
files-expanded="[[_filesExpanded]]"
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 6cbe59c..5fcc3b0 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
@@ -26,6 +26,7 @@
HttpMethod,
MessageTag,
PrimaryTab,
+ createDefaultPreferences,
} from '../../../constants/constants';
import {GrEditConstants} from '../../edit/gr-edit-constants';
import {_testOnly_resetEndpoints} from '../../shared/gr-js-api-interface/gr-plugin-endpoints';
@@ -35,7 +36,7 @@
import {EventType, PluginApi} from '../../../api/plugin';
import 'lodash/lodash';
-import {mockPromise, stubRestApi} from '../../../test/test-utils';
+import {mockPromise, stubRestApi, stubUsers} from '../../../test/test-utils';
import {
createAppElementChangeViewParams,
createApproval,
@@ -94,6 +95,7 @@
import {GrRelatedChangesList} from '../gr-related-changes-list/gr-related-changes-list';
import {appContext} from '../../../services/app-context';
import {ChangeStates} from '../../shared/gr-change-status/gr-change-status';
+import {_testOnly_setState} from '../../../services/user/user-model';
const pluginApi = _testOnly_initGerritPluginApi();
const fixture = fixtureFromElement('gr-change-view');
@@ -803,20 +805,30 @@
assert.isTrue(stub.called);
});
- test('m should toggle diff mode', () => {
- const setModeStub = sinon.stub(
- element.$.fileListHeader,
- 'setDiffViewMode'
+ test('m should toggle diff mode', async () => {
+ const updatePreferencesStub = stubUsers('updatePreferences');
+ await flush();
+
+ const prefs = {
+ ...createDefaultPreferences(),
+ diff_view: DiffViewMode.SIDE_BY_SIDE,
+ };
+ _testOnly_setState({preferences: prefs});
+ element._handleToggleDiffMode();
+ assert.isTrue(
+ updatePreferencesStub.calledWith({diff_view: DiffViewMode.UNIFIED})
);
- flush();
- element.viewState.diffMode = DiffViewMode.SIDE_BY_SIDE;
+ const newPrefs = {
+ ...createDefaultPreferences(),
+ diff_view: DiffViewMode.UNIFIED,
+ };
+ _testOnly_setState({preferences: newPrefs});
+ await flush();
element._handleToggleDiffMode();
- assert.isTrue(setModeStub.calledWith(DiffViewMode.UNIFIED));
-
- element.viewState.diffMode = DiffViewMode.UNIFIED;
- element._handleToggleDiffMode();
- assert.isTrue(setModeStub.calledWith(DiffViewMode.SIDE_BY_SIDE));
+ assert.isTrue(
+ updatePreferencesStub.calledWith({diff_view: DiffViewMode.SIDE_BY_SIDE})
+ );
});
});
@@ -1278,52 +1290,6 @@
assert.equal(element._numFilesShown, 200);
});
- test('_setDiffViewMode is called with reset when new change is loaded', () => {
- const setDiffViewModeStub = sinon.stub(element, '_setDiffViewMode');
- element.viewState = {changeNum: 1 as NumericChangeId};
- element._changeNum = 2 as NumericChangeId;
- element._resetFileListViewState();
- assert.isTrue(setDiffViewModeStub.calledWithExactly(true));
- });
-
- test('diffViewMode is propagated from file list header', () => {
- element.viewState = {diffMode: DiffViewMode.UNIFIED};
- element.$.fileListHeader.diffViewMode = DiffViewMode.SIDE_BY_SIDE;
- assert.equal(element.viewState.diffMode, DiffViewMode.SIDE_BY_SIDE);
- });
-
- test('diffMode defaults to side by side without preferences', async () => {
- stubRestApi('getPreferences').returns(Promise.resolve(createPreferences()));
- // No user prefs or diff view mode set.
-
- await element._setDiffViewMode()!;
- assert.equal(element.viewState.diffMode, DiffViewMode.SIDE_BY_SIDE);
- });
-
- test('diffMode defaults to preference when not already set', async () => {
- stubRestApi('getPreferences').returns(
- Promise.resolve({
- ...createPreferences(),
- default_diff_view: DiffViewMode.UNIFIED,
- })
- );
-
- await element._setDiffViewMode()!;
- assert.equal(element.viewState.diffMode, DiffViewMode.UNIFIED);
- });
-
- test('existing diffMode overrides preference', async () => {
- element.viewState.diffMode = DiffViewMode.SIDE_BY_SIDE;
- stubRestApi('getPreferences').returns(
- Promise.resolve({
- ...createPreferences(),
- default_diff_view: DiffViewMode.UNIFIED,
- })
- );
- await element._setDiffViewMode()!;
- assert.equal(element.viewState.diffMode, DiffViewMode.SIDE_BY_SIDE);
- });
-
test('don’t reload entire page when patchRange changes', async () => {
const reloadStub = sinon
.stub(element, 'loadData')
diff --git a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.ts b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.ts
index ae3eee5..1b44e35 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.ts
+++ b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.ts
@@ -41,7 +41,6 @@
import {DiffPreferencesInfo} from '../../../types/diff';
import {ChangeComments} from '../../diff/gr-comment-api/gr-comment-api';
import {GrDiffModeSelector} from '../../diff/gr-diff-mode-selector/gr-diff-mode-selector';
-import {DiffViewMode} from '../../../constants/constants';
import {GrButton} from '../../shared/gr-button/gr-button';
import {fireEvent} from '../../../utils/event-util';
import {
@@ -122,9 +121,6 @@
@property({type: Object})
diffPrefs?: DiffPreferencesInfo;
- @property({type: String, notify: true})
- diffViewMode?: DiffViewMode;
-
@property({type: String})
patchNum?: PatchSetNum;
@@ -144,10 +140,6 @@
private readonly shortcuts = appContext.shortcutsService;
- setDiffViewMode(mode: DiffViewMode) {
- this.$.modeSelect.setMode(mode);
- }
-
_expandAllDiffs() {
fireEvent(this, 'expand-diffs');
}
diff --git a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_html.ts b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_html.ts
index 73d0819..5a85531 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_html.ts
+++ b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header_html.ts
@@ -169,7 +169,6 @@
<span class="fileViewActionsLabel">Diff view:</span>
<gr-diff-mode-selector
id="modeSelect"
- mode="{{diffViewMode}}"
save-on-change="[[loggedIn]]"
></gr-diff-mode-selector>
<span
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 cc76145..050e86d 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
@@ -88,6 +88,7 @@
import {Subject} from 'rxjs';
import {takeUntil} from 'rxjs/operators';
import {listen} from '../../../services/shortcuts/shortcuts-service';
+import {diffViewMode$} from '../../../services/browser/browser-model';
export const DEFAULT_NUM_FILES_SHOWN = 200;
@@ -377,6 +378,10 @@
.subscribe(changeComments => {
this.changeComments = changeComments;
});
+ diffViewMode$
+ .pipe(takeUntil(this.disconnected$))
+ .subscribe(diffView => (this.diffViewMode = diffView));
+
getPluginLoader()
.awaitPluginsLoaded()
.then(() => {
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_html.ts b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_html.ts
index f7be36b..e8371e3 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_html.ts
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_html.ts
@@ -643,7 +643,6 @@
prefs="[[diffPrefs]]"
project-name="[[change.project]]"
no-render-on-prefs-change=""
- view-mode="[[diffViewMode]]"
></gr-diff-host>
</template>
</div>
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.js b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.js
index 7409be7..0db7690 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.js
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.js
@@ -820,10 +820,8 @@
MockInteractions.tap(row);
flush();
- const diffDisplay = element.diffs[0];
- element._userPrefs = {default_diff_view: 'SIDE_BY_SIDE'};
+ element._userPrefs = {diff_view: 'SIDE_BY_SIDE'};
element.set('diffViewMode', 'UNIFIED_DIFF');
- assert.equal(diffDisplay.viewMode, 'UNIFIED_DIFF');
assert.isTrue(element._updateDiffPreferences.called);
});
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 b1bad1c..a828b9c 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
@@ -88,10 +88,11 @@
import {TokenHighlightLayer} from '../gr-diff-builder/token-highlight-layer';
import {Timing} from '../../../constants/reporting';
import {changeComments$} from '../../../services/comments/comments-model';
-import {takeUntil} from 'rxjs/operators';
import {ChangeComments} from '../gr-comment-api/gr-comment-api';
import {Subject} from 'rxjs';
import {RenderPreferences} from '../../../api/diff';
+import {diffViewMode$} from '../../../services/browser/browser-model';
+import {takeUntil} from 'rxjs/operators';
const EMPTY_BLAME = 'No blame information for this diff.';
@@ -205,12 +206,12 @@
@property({type: Boolean})
lineWrapping = false;
- @property({type: String})
- viewMode = DiffViewMode.SIDE_BY_SIDE;
-
@property({type: Object})
lineOfInterest?: LineOfInterest;
+ @property({type: String})
+ viewMode = DiffViewMode.SIDE_BY_SIDE;
+
@property({type: Boolean})
showLoadFailure?: boolean;
@@ -312,6 +313,9 @@
override connectedCallback() {
super.connectedCallback();
+ diffViewMode$
+ .pipe(takeUntil(this.disconnected$))
+ .subscribe(diffView => (this.viewMode = diffView));
this._getLoggedIn().then(loggedIn => {
this._loggedIn = loggedIn;
});
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-mode-selector/gr-diff-mode-selector.ts b/polygerrit-ui/app/elements/diff/gr-diff-mode-selector/gr-diff-mode-selector.ts
index b47c51c..3d43ef3 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-mode-selector/gr-diff-mode-selector.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-mode-selector/gr-diff-mode-selector.ts
@@ -26,6 +26,9 @@
import {FixIronA11yAnnouncer} from '../../../types/types';
import {appContext} from '../../../services/app-context';
import {fireIronAnnounce} from '../../../utils/event-util';
+import {diffViewMode$} from '../../../services/browser/browser-model';
+import {Subject} from 'rxjs';
+import {takeUntil} from 'rxjs/operators';
@customElement('gr-diff-mode-selector')
export class GrDiffModeSelector extends PolymerElement {
@@ -34,7 +37,7 @@
}
@property({type: String, notify: true})
- mode?: DiffViewMode;
+ mode: DiffViewMode = DiffViewMode.SIDE_BY_SIDE;
/**
* If set to true, the user's preference will be updated every time a
@@ -48,11 +51,24 @@
private readonly userService = appContext.userService;
+ disconnected$ = new Subject();
+
+ constructor() {
+ super();
+ }
+
override connectedCallback() {
super.connectedCallback();
(
IronA11yAnnouncer as unknown as FixIronA11yAnnouncer
).requestAvailability();
+ diffViewMode$
+ .pipe(takeUntil(this.disconnected$))
+ .subscribe(diffView => (this.mode = diffView));
+ }
+
+ override disconnectedCallback() {
+ this.disconnected$.next();
}
/**
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-mode-selector/gr-diff-mode-selector_test.ts b/polygerrit-ui/app/elements/diff/gr-diff-mode-selector/gr-diff-mode-selector_test.ts
index 8b06c75..fe5f389 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-mode-selector/gr-diff-mode-selector_test.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-mode-selector/gr-diff-mode-selector_test.ts
@@ -20,6 +20,7 @@
import {GrDiffModeSelector} from './gr-diff-mode-selector';
import {DiffViewMode} from '../../../constants/constants';
import {stubUsers} from '../../../test/test-utils';
+import {_testOnly_setState} from '../../../services/browser/browser-model';
const basicFixture = fixtureFromElement('gr-diff-mode-selector');
@@ -47,8 +48,10 @@
});
test('setMode', () => {
+ _testOnly_setState({screenWidth: 0});
const saveStub = stubUsers('updatePreferences');
+ flush();
// Setting the mode initially does not save prefs.
element.saveOnChange = true;
element.setMode(DiffViewMode.SIDE_BY_SIDE);
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 0813900..d892c79 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
@@ -165,7 +165,7 @@
@property({type: Object, observer: '_paramsChanged'})
params?: AppElementParams;
- @property({type: Object, notify: true, observer: '_changeViewStateChanged'})
+ @property({type: Object, notify: true})
changeViewState: Partial<ChangeViewState> = {};
@property({type: Object})
@@ -222,12 +222,6 @@
@property({type: Object})
_userPrefs?: PreferencesInfo;
- @property({
- type: String,
- computed: '_getDiffViewMode(changeViewState.diffMode, _userPrefs)',
- })
- _diffMode?: string;
-
@property({type: Boolean})
_isImageDiff?: boolean;
@@ -349,6 +343,8 @@
private readonly restApiService = appContext.restApiService;
+ private readonly userService = appContext.userService;
+
private readonly commentsService = appContext.commentsService;
private readonly shortcuts = appContext.shortcutsService;
@@ -716,10 +712,13 @@
}
_handleToggleDiffMode() {
- if (this._getDiffViewMode() === DiffViewMode.SIDE_BY_SIDE) {
- this.$.modeSelect.setMode(DiffViewMode.UNIFIED);
+ if (!this._userPrefs) return;
+ if (this._userPrefs.diff_view === DiffViewMode.SIDE_BY_SIDE) {
+ this.userService.updatePreferences({diff_view: DiffViewMode.UNIFIED});
} else {
- this.$.modeSelect.setMode(DiffViewMode.SIDE_BY_SIDE);
+ this.userService.updatePreferences({
+ diff_view: DiffViewMode.SIDE_BY_SIDE,
+ });
}
}
@@ -1126,17 +1125,6 @@
});
}
- _changeViewStateChanged(changeViewState: Partial<ChangeViewState>) {
- if (changeViewState.diffMode === null) {
- // If screen size is small, always default to unified view.
- this.restApiService.getPreferences().then(prefs => {
- if (prefs) {
- this.set('changeViewState.diffMode', prefs.default_diff_view);
- }
- });
- }
- }
-
@observe('_path', '_prefs', '_reviewedFiles', '_patchRange')
_setReviewedObserver(
path?: string,
@@ -1351,29 +1339,6 @@
this.$.diffPreferencesDialog.open();
}
- /**
- * _getDiffViewMode: Get the diff view (side-by-side or unified) based on
- * the current state.
- *
- * The expected behavior is to use the mode specified in the user's
- * preferences unless they have manually chosen the alternative view or they
- * are on a mobile device. If the user navigates up to the change view, it
- * should clear this choice and revert to the preference the next time a
- * diff is viewed.
- *
- * Use side-by-side if the user is not logged in.
- */
- _getDiffViewMode() {
- if (this.changeViewState.diffMode) {
- return this.changeViewState.diffMode;
- } else if (this._userPrefs) {
- this.set('changeViewState.diffMode', this._userPrefs.default_diff_view);
- return this._userPrefs.default_diff_view;
- } else {
- return 'SIDE_BY_SIDE';
- }
- }
-
_computeModeSelectHideClass(diff?: DiffInfo) {
return !diff || diff.binary ? 'hide' : '';
}
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_html.ts b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_html.ts
index 308c353..16adb45 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_html.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_html.ts
@@ -339,7 +339,6 @@
<gr-diff-mode-selector
id="modeSelect"
save-on-change="[[_loggedIn]]"
- mode="{{changeViewState.diffMode}}"
show-tooltip-below=""
></gr-diff-mode-selector>
</div>
@@ -409,7 +408,6 @@
path="[[_path]]"
prefs="[[_prefs]]"
project-name="[[_change.project]]"
- view-mode="[[_diffMode]]"
is-blame-loaded="{{_isBlameLoaded}}"
on-comment-anchor-tap="_onLineSelected"
on-line-selected="_onLineSelected"
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 e4a8aa4..cc9aac0 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
@@ -18,8 +18,8 @@
import '../../../test/common-test-setup-karma.js';
import './gr-diff-view.js';
import {GerritNav} from '../../core/gr-navigation/gr-navigation.js';
-import {ChangeStatus} from '../../../constants/constants.js';
-import {stubRestApi} from '../../../test/test-utils.js';
+import {ChangeStatus, DiffViewMode} from '../../../constants/constants.js';
+import {stubRestApi, stubUsers} from '../../../test/test-utils.js';
import {ChangeComments} from '../gr-comment-api/gr-comment-api.js';
import {GerritView} from '../../../services/router/router-model.js';
import {
@@ -30,11 +30,10 @@
import {EditPatchSetNum} from '../../../types/common.js';
import {CursorMoveResult} from '../../../api/core.js';
import {EventType} from '../../../types/events.js';
+import {_testOnly_resetState, _testOnly_setState} from '../../../services/browser/browser-model.js';
const basicFixture = fixtureFromElement('gr-diff-view');
-const blankFixture = fixtureFromElement('div');
-
suite('gr-diff-view tests', () => {
suite('basic tests', () => {
let element;
@@ -70,6 +69,8 @@
stubRestApi('getDiffDrafts').returns(Promise.resolve({}));
stubRestApi('getPortedComments').returns(Promise.resolve({}));
+ _testOnly_resetState();
+
element = basicFixture.instantiate();
element._changeNum = '42';
element._path = 'some/path.txt';
@@ -432,6 +433,7 @@
test('keyboard shortcuts', () => {
element._changeNum = '42';
+ _testOnly_setState({screenWidth: 0});
element._patchRange = {
basePatchNum: PARENT,
patchNum: 10,
@@ -510,11 +512,11 @@
'_computeContainerClass');
MockInteractions.pressAndReleaseKeyOn(element, 74, null, 'j');
assert(computeContainerClassStub.lastCall.calledWithExactly(
- false, 'SIDE_BY_SIDE', true));
+ false, DiffViewMode.SIDE_BY_SIDE, true));
MockInteractions.pressAndReleaseKeyOn(element, 27, null, 'Escape');
assert(computeContainerClassStub.lastCall.calledWithExactly(
- false, 'SIDE_BY_SIDE', false));
+ false, DiffViewMode.SIDE_BY_SIDE, false));
// Note that stubbing _setReviewed means that the value of the
// `element.$.reviewed` checkbox is not flipped.
@@ -1308,47 +1310,23 @@
test('diff mode selector correctly toggles the diff', () => {
const select = element.$.modeSelect;
const diffDisplay = element.$.diffHost;
- element._userPrefs = {default_diff_view: 'SIDE_BY_SIDE'};
+ element._userPrefs = {diff_view: DiffViewMode.SIDE_BY_SIDE};
+ _testOnly_setState({screenWidth: 0});
+ const userStub = stubUsers('updatePreferences');
+
+ flush();
// The mode selected in the view state reflects the selected option.
- assert.equal(element._getDiffViewMode(), select.mode);
+ // assert.equal(element._userPrefs.diff_view, select.mode);
// The mode selected in the view state reflects the view rednered in the
// diff.
assert.equal(select.mode, diffDisplay.viewMode);
// We will simulate a user change of the selected mode.
- const newMode = 'UNIFIED_DIFF';
-
- // Set the mode, and simulate the change event.
- element.set('changeViewState.diffMode', newMode);
-
- // Make sure the handler was called and the state is still coherent.
- assert.equal(element._getDiffViewMode(), newMode);
- assert.equal(element._getDiffViewMode(), select.mode);
- assert.equal(element._getDiffViewMode(), diffDisplay.viewMode);
- });
-
- test('diff mode selector initializes from preferences', () => {
- let resolvePrefs;
- const prefsPromise = new Promise(resolve => {
- resolvePrefs = resolve;
- });
- stubRestApi('getPreferences')
- .callsFake(() => prefsPromise);
-
- // Attach a new gr-diff-view so we can intercept the preferences fetch.
- const view = document.createElement('gr-diff-view');
- blankFixture.instantiate().appendChild(view);
- flush();
-
- // At this point the diff mode doesn't yet have the user's preference.
- assert.equal(view._getDiffViewMode(), 'SIDE_BY_SIDE');
-
- // Receive the overriding preference.
- resolvePrefs({default_diff_view: 'UNIFIED'});
- flush();
- assert.equal(element._getDiffViewMode(), 'SIDE_BY_SIDE');
+ element._handleToggleDiffMode();
+ assert.isTrue(userStub.calledWithExactly({
+ diff_view: DiffViewMode.UNIFIED}));
});
test('diff mode selector should be hidden for binary', async () => {
@@ -1509,32 +1487,22 @@
assert.isTrue(getUrlStub.lastCall.args[6]);
});
- test('_getDiffViewMode', () => {
- // No user prefs or change view state set.
- assert.equal(element._getDiffViewMode(), 'SIDE_BY_SIDE');
-
- // User prefs but no change view state set.
- element.changeViewState.diffMode = undefined;
- element._userPrefs = {default_diff_view: 'UNIFIED_DIFF'};
- assert.equal(element._getDiffViewMode(), 'UNIFIED_DIFF');
-
- // User prefs and change view state set.
- element.changeViewState = {diffMode: 'SIDE_BY_SIDE'};
- assert.equal(element._getDiffViewMode(), 'SIDE_BY_SIDE');
- });
-
test('_handleToggleDiffMode', () => {
+ const userStub = stubUsers('updatePreferences');
const e = new CustomEvent('keydown', {
detail: {keyboardEvent: new KeyboardEvent('keydown'), key: 'x'},
});
- // Initial state.
- assert.equal(element._getDiffViewMode(), 'SIDE_BY_SIDE');
+ element._userPrefs = {diff_view: DiffViewMode.SIDE_BY_SIDE};
element._handleToggleDiffMode(e);
- assert.equal(element._getDiffViewMode(), 'UNIFIED_DIFF');
+ assert.deepEqual(userStub.lastCall.args[0], {
+ diff_view: DiffViewMode.UNIFIED});
+
+ element._userPrefs = {diff_view: DiffViewMode.UNIFIED};
element._handleToggleDiffMode(e);
- assert.equal(element._getDiffViewMode(), 'SIDE_BY_SIDE');
+ assert.deepEqual(userStub.lastCall.args[0], {
+ diff_view: DiffViewMode.SIDE_BY_SIDE});
});
suite('_initPatchRange', () => {
diff --git a/polygerrit-ui/app/elements/gr-app-element.ts b/polygerrit-ui/app/elements/gr-app-element.ts
index d88eeaf..7f7749a 100644
--- a/polygerrit-ui/app/elements/gr-app-element.ts
+++ b/polygerrit-ui/app/elements/gr-app-element.ts
@@ -212,6 +212,8 @@
private readonly restApiService = appContext.restApiService;
+ private readonly browserService = appContext.browserService;
+
override keyboardShortcuts(): ShortcutListener[] {
return [
listen(Shortcut.OPEN_SHORTCUT_HELP_DIALOG, _ =>
@@ -252,6 +254,8 @@
this.handleRecreateView(GerritView.DIFF)
);
document.addEventListener(EventType.GR_RPC_LOG, e => this._handleRpcLog(e));
+ const resizeObserver = this.browserService.observeWidth();
+ resizeObserver.observe(this);
}
override ready() {
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.ts b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.ts
index e75c41c..c2b0269 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.ts
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.ts
@@ -149,7 +149,6 @@
createDefaultDiffPrefs,
createDefaultEditPrefs,
createDefaultPreferences,
- DiffViewMode,
HttpMethod,
ReviewerState,
} from '../../../constants/constants';
@@ -159,8 +158,6 @@
import {FlagsService, KnownExperimentId} from '../../../services/flags/flags';
const MAX_PROJECT_RESULTS = 25;
-// This value is somewhat arbitrary and not based on research or calculations.
-const MAX_UNIFIED_DEFAULT_WINDOW_WIDTH_PX = 850;
const Requests = {
SEND_DIFF_DRAFT: 'sendDiffDraft',
@@ -977,13 +974,6 @@
return res;
}
const prefInfo = res as unknown as PreferencesInfo;
- if (this._isNarrowScreen()) {
- // Note that this can be problematic, because the diff will stay
- // unified even after increasing the window width.
- prefInfo.default_diff_view = DiffViewMode.UNIFIED;
- } else {
- prefInfo.default_diff_view = prefInfo.diff_view;
- }
return prefInfo;
});
}
@@ -1019,10 +1009,6 @@
});
}
- _isNarrowScreen() {
- return window.innerWidth < MAX_UNIFIED_DEFAULT_WINDOW_WIDTH_PX;
- }
-
getChanges(
changesPerPage?: number,
query?: string,
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.js b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.js
index a60a1ef..6a02985 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.js
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.js
@@ -333,26 +333,23 @@
stub.lastCall.args[0].errFn({});
});
- const preferenceSetup = function(testJSON, loggedIn, smallScreen) {
+ const preferenceSetup = function(testJSON, loggedIn) {
sinon.stub(element, 'getLoggedIn')
.callsFake(() => Promise.resolve(loggedIn));
- sinon.stub(element, '_isNarrowScreen').callsFake(() => smallScreen);
sinon.stub(
element._restApiHelper,
'fetchCacheURL')
.callsFake(() => Promise.resolve(testJSON));
};
- test('getPreferences returns correctly on small screens logged in',
+ test('getPreferences returns correctly logged in',
() => {
const testJSON = {diff_view: 'SIDE_BY_SIDE'};
const loggedIn = true;
- const smallScreen = true;
- preferenceSetup(testJSON, loggedIn, smallScreen);
+ preferenceSetup(testJSON, loggedIn);
return element.getPreferences().then(obj => {
- assert.equal(obj.default_diff_view, 'UNIFIED_DIFF');
assert.equal(obj.diff_view, 'SIDE_BY_SIDE');
});
});
@@ -361,12 +358,10 @@
() => {
const testJSON = {diff_view: 'UNIFIED_DIFF'};
const loggedIn = true;
- const smallScreen = false;
- preferenceSetup(testJSON, loggedIn, smallScreen);
+ preferenceSetup(testJSON, loggedIn);
return element.getPreferences().then(obj => {
- assert.equal(obj.default_diff_view, 'UNIFIED_DIFF');
assert.equal(obj.diff_view, 'UNIFIED_DIFF');
});
});
@@ -375,12 +370,10 @@
() => {
const testJSON = {diff_view: 'UNIFIED_DIFF'};
const loggedIn = false;
- const smallScreen = false;
- preferenceSetup(testJSON, loggedIn, smallScreen);
+ preferenceSetup(testJSON, loggedIn);
return element.getPreferences().then(obj => {
- assert.equal(obj.default_diff_view, 'SIDE_BY_SIDE');
assert.equal(obj.diff_view, 'SIDE_BY_SIDE');
});
});
diff --git a/polygerrit-ui/app/services/app-context-init.ts b/polygerrit-ui/app/services/app-context-init.ts
index 3a6f7c5..b9c4f49 100644
--- a/polygerrit-ui/app/services/app-context-init.ts
+++ b/polygerrit-ui/app/services/app-context-init.ts
@@ -28,6 +28,7 @@
import {UserService} from './user/user-service';
import {CommentsService} from './comments/comments-service';
import {ShortcutsService} from './shortcuts/shortcuts-service';
+import {BrowserService} from './browser/browser-service';
type ServiceName = keyof AppContext;
type ServiceCreator<T> = () => T;
@@ -84,5 +85,6 @@
configService: () => new ConfigService(),
userService: () => new UserService(appContext.restApiService),
shortcutsService: () => new ShortcutsService(appContext.reportingService),
+ browserService: () => new BrowserService(),
});
}
diff --git a/polygerrit-ui/app/services/app-context.ts b/polygerrit-ui/app/services/app-context.ts
index e5828d6..47da722 100644
--- a/polygerrit-ui/app/services/app-context.ts
+++ b/polygerrit-ui/app/services/app-context.ts
@@ -27,6 +27,7 @@
import {UserService} from './user/user-service';
import {CommentsService} from './comments/comments-service';
import {ShortcutsService} from './shortcuts/shortcuts-service';
+import {BrowserService} from './browser/browser-service';
export interface AppContext {
flagsService: FlagsService;
@@ -41,6 +42,7 @@
storageService: StorageService;
configService: ConfigService;
userService: UserService;
+ browserService: BrowserService;
shortcutsService: ShortcutsService;
}
diff --git a/polygerrit-ui/app/services/browser/browser-model.ts b/polygerrit-ui/app/services/browser/browser-model.ts
new file mode 100644
index 0000000..db790f6
--- /dev/null
+++ b/polygerrit-ui/app/services/browser/browser-model.ts
@@ -0,0 +1,74 @@
+/**
+ * @license
+ * Copyright (C) 2021 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import {BehaviorSubject, Observable, combineLatest} from 'rxjs';
+import {distinctUntilChanged, map} from 'rxjs/operators';
+import {preferenceDiffViewMode$} from '../user/user-model';
+import {DiffViewMode} from '../../api/diff';
+
+// This value is somewhat arbitrary and not based on research or calculations.
+const MAX_UNIFIED_DEFAULT_WINDOW_WIDTH_PX = 850;
+
+interface BrowserState {
+ /**
+ * We maintain the screen width in the state so that the app can react to
+ * changes in the width such as automatically changing to unified diff view
+ */
+ screenWidth?: number;
+}
+
+const initialState: BrowserState = {};
+
+// Mutable for testing
+let privateState$ = new BehaviorSubject(initialState);
+
+export function _testOnly_resetState() {
+ privateState$ = new BehaviorSubject(initialState);
+}
+
+export function _testOnly_setState(state: BrowserState) {
+ privateState$.next(state);
+}
+
+export function _testOnly_getState() {
+ return privateState$.getValue();
+}
+
+export const viewState$: Observable<BrowserState> = privateState$;
+
+export function updateStateScreenWidth(screenWidth: number) {
+ privateState$.next({...privateState$.getValue(), screenWidth});
+}
+
+export const isScreenTooSmall$ = viewState$.pipe(
+ map(
+ state =>
+ !!state.screenWidth &&
+ state.screenWidth < MAX_UNIFIED_DEFAULT_WINDOW_WIDTH_PX
+ ),
+ distinctUntilChanged()
+);
+
+export const diffViewMode$: Observable<DiffViewMode> = combineLatest([
+ isScreenTooSmall$,
+ preferenceDiffViewMode$,
+]).pipe(
+ map(([isScreenTooSmall, preferenceDiffViewMode]) => {
+ if (isScreenTooSmall) return DiffViewMode.UNIFIED;
+ else return preferenceDiffViewMode;
+ }, distinctUntilChanged())
+);
diff --git a/polygerrit-ui/app/services/browser/browser-service.ts b/polygerrit-ui/app/services/browser/browser-service.ts
new file mode 100644
index 0000000..d98f8f7
--- /dev/null
+++ b/polygerrit-ui/app/services/browser/browser-service.ts
@@ -0,0 +1,29 @@
+import {updateStateScreenWidth} from './browser-model';
+
+/**
+ * @license
+ * Copyright (C) 2021 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+export class BrowserService {
+ /* Observer the screen width so that the app can react to changes to it */
+ observeWidth() {
+ return new ResizeObserver(entries => {
+ entries.forEach(entry => {
+ updateStateScreenWidth(entry.contentRect.width);
+ });
+ });
+ }
+}
diff --git a/polygerrit-ui/app/services/user/user-model.ts b/polygerrit-ui/app/services/user/user-model.ts
index 72ce3e1..6d31f6c 100644
--- a/polygerrit-ui/app/services/user/user-model.ts
+++ b/polygerrit-ui/app/services/user/user-model.ts
@@ -17,7 +17,10 @@
import {AccountDetailInfo, PreferencesInfo} from '../../types/common';
import {BehaviorSubject, Observable} from 'rxjs';
import {map, distinctUntilChanged} from 'rxjs/operators';
-import {createDefaultPreferences} from '../../constants/constants';
+import {
+ createDefaultPreferences,
+ DiffViewMode,
+} from '../../constants/constants';
interface UserState {
/**
@@ -31,7 +34,20 @@
preferences: createDefaultPreferences(),
};
-const privateState$ = new BehaviorSubject(initialState);
+// Mutable for testing
+let privateState$ = new BehaviorSubject(initialState);
+
+export function _testOnly_resetState() {
+ privateState$ = new BehaviorSubject(initialState);
+}
+
+export function _testOnly_setState(state: UserState) {
+ privateState$.next(state);
+}
+
+export function _testOnly_getState() {
+ return privateState$.getValue();
+}
// Re-exporting as Observable so that you can only subscribe, but not emit.
export const userState$: Observable<UserState> = privateState$;
@@ -56,6 +72,11 @@
distinctUntilChanged()
);
+export const preferenceDiffViewMode$ = preferences$.pipe(
+ map(preference => preference.diff_view ?? DiffViewMode.SIDE_BY_SIDE),
+ distinctUntilChanged()
+);
+
export const myTopMenuItems$ = preferences$.pipe(
map(preferences => preferences?.my ?? []),
distinctUntilChanged()
diff --git a/polygerrit-ui/app/services/user/user-service.ts b/polygerrit-ui/app/services/user/user-service.ts
index 125d20c..0588a4a 100644
--- a/polygerrit-ui/app/services/user/user-service.ts
+++ b/polygerrit-ui/app/services/user/user-service.ts
@@ -14,11 +14,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-import {
- AccountDetailInfo,
- PreferencesInfo,
- PreferencesInput,
-} from '../../types/common';
+import {AccountDetailInfo, PreferencesInfo} from '../../types/common';
import {from, of} from 'rxjs';
import {account$, updateAccount, updatePreferences} from './user-model';
import {switchMap} from 'rxjs/operators';
@@ -44,7 +40,7 @@
});
}
- updatePreferences(prefs: PreferencesInput) {
+ updatePreferences(prefs: Partial<PreferencesInfo>) {
this.restApiService
.savePreferences(prefs)
.then((newPrefs: PreferencesInfo | undefined) => {
diff --git a/polygerrit-ui/app/test/test-utils.ts b/polygerrit-ui/app/test/test-utils.ts
index 39c30ad..49ecac8 100644
--- a/polygerrit-ui/app/test/test-utils.ts
+++ b/polygerrit-ui/app/test/test-utils.ts
@@ -25,6 +25,7 @@
import {ReportingService} from '../services/gr-reporting/gr-reporting';
import {CommentsService} from '../services/comments/comments-service';
import {UserService} from '../services/user/user-service';
+import {ShortcutsService} from '../services/shortcuts/shortcuts-service';
export {query, queryAll, queryAndAssert} from '../utils/common-util';
export interface MockPromise extends Promise<unknown> {
@@ -116,6 +117,10 @@
return sinon.stub(appContext.userService, method);
}
+export function stubShortcuts<K extends keyof ShortcutsService>(method: K) {
+ return sinon.stub(appContext.shortcutsService, method);
+}
+
export function stubStorage<K extends keyof StorageService>(method: K) {
return sinon.stub(appContext.storageService, method);
}
diff --git a/polygerrit-ui/app/types/common.ts b/polygerrit-ui/app/types/common.ts
index 1617aa3..3ac7c7b 100644
--- a/polygerrit-ui/app/types/common.ts
+++ b/polygerrit-ui/app/types/common.ts
@@ -1147,8 +1147,6 @@
work_in_progress_by_default?: boolean;
// The email_format doesn't mentioned in doc, but exists in Java class GeneralPreferencesInfo
email_format?: EmailFormat;
- // The following property doesn't exist in RestAPI, it is added by GrRestApiInterface
- default_diff_view?: DiffViewMode;
}
/**
diff --git a/polygerrit-ui/app/types/types.ts b/polygerrit-ui/app/types/types.ts
index 2d4d412..b90b12f 100644
--- a/polygerrit-ui/app/types/types.ts
+++ b/polygerrit-ui/app/types/types.ts
@@ -189,7 +189,6 @@
showDownloadDialog: boolean;
diffMode: DiffViewMode | null;
numFilesShown: number | null;
- diffViewMode?: boolean;
}
export interface ChangeListViewState {
