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/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', () => {
