Refactoring: Navigate to diff through change model, not from diff view
We are only touching `gr-diff-view` here, but we intend to do a similar
change to `gr-change-view`. The change model has the current context, so
there is no need for the views to do this kind of "work". That allows
better re-use and simplifies the very large gr-*-view files.
Release-Notes: skip
Change-Id: I535293f80ea3e28f4e30980d00a7a8cf38c0b682
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 71bfc0e..70e6b41 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
@@ -20,11 +20,9 @@
import '../gr-diff-preferences-dialog/gr-diff-preferences-dialog';
import '../gr-patch-range-select/gr-patch-range-select';
import '../../change/gr-download-dialog/gr-download-dialog';
-import {navigationToken} from '../../core/gr-navigation/gr-navigation';
import {getAppContext} from '../../../services/app-context';
import {
computeAllPatchSets,
- computeLatestPatchNum,
PatchSet,
isMergeParent,
getParentIndex,
@@ -43,7 +41,6 @@
import {ChangeComments} from '../../diff/gr-comment-api/gr-comment-api';
import {
BasePatchSetNum,
- ChangeInfo,
CommitId,
EDIT,
NumericChangeId,
@@ -52,17 +49,11 @@
PatchSetNumber,
PreferencesInfo,
RepoName,
- RevisionInfo,
RevisionPatchSetNum,
ServerInfo,
} from '../../../types/common';
import {DiffInfo, DiffPreferencesInfo} from '../../../types/diff';
-import {
- CommitRange,
- EditRevisionInfo,
- FileRange,
- ParsedChangeInfo,
-} from '../../../types/types';
+import {CommitRange, FileRange, ParsedChangeInfo} from '../../../types/types';
import {FilesWebLinks} from '../gr-patch-range-select/gr-patch-range-select';
import {GrDiffCursor} from '../../../embed/diff/gr-diff-cursor/gr-diff-cursor';
import {CommentSide, DiffViewMode, Side} from '../../../constants/constants';
@@ -107,9 +98,7 @@
ChangeChildView,
changeViewModelToken,
ChangeViewState,
- createChangeUrl,
} from '../../../models/views/change';
-import {createEditUrl} from '../../../models/views/edit';
import {GeneratedWebLink} from '../../../utils/weblink-util';
import {userModelToken} from '../../../models/user/user-model';
import {modalStyles} from '../../../styles/gr-modal-styles';
@@ -194,6 +183,9 @@
@state()
change?: ParsedChangeInfo;
+ @state()
+ latestPatchNum?: PatchSetNumber;
+
// Private but used in tests.
@state()
changeComments?: ChangeComments;
@@ -299,8 +291,6 @@
private readonly shortcutsController = new ShortcutController(this);
- private readonly getNavigation = resolve(this, navigationToken);
-
constructor() {
super();
this.setupKeyboardShortcuts();
@@ -350,7 +340,9 @@
listen(Shortcut.OPEN_REPLY_DIALOG, _ => this.handleOpenReplyDialog());
listen(Shortcut.TOGGLE_LEFT_PANE, _ => this.handleToggleLeftPane());
listen(Shortcut.OPEN_DOWNLOAD_DIALOG, _ => this.handleOpenDownloadDialog());
- listen(Shortcut.UP_TO_CHANGE, _ => this.handleUpToChange());
+ listen(Shortcut.UP_TO_CHANGE, _ =>
+ this.getChangeModel().navigateToChange()
+ );
listen(Shortcut.OPEN_DIFF_PREFS, _ => this.handleCommaKey());
listen(Shortcut.TOGGLE_DIFF_MODE, _ => this.handleToggleDiffMode());
listen(Shortcut.TOGGLE_FILE_REVIEWED, e => {
@@ -433,6 +425,11 @@
);
subscribe(
this,
+ () => this.getChangeModel().latestPatchNum$,
+ latestPatchNum => (this.latestPatchNum = latestPatchNum)
+ );
+ subscribe(
+ this,
() => this.getChangeModel().reviewedFiles$,
reviewedFiles => {
this.reviewedFiles = new Set(reviewedFiles) ?? new Set();
@@ -790,7 +787,8 @@
const fileNum = this.computeFileNum(formattedFiles);
const fileNumClass = this.computeFileNumClass(fileNum, formattedFiles);
return html` <div>
- <a href=${this.getChangePath()}>${this.changeNum}</a
+ <a href=${ifDefined(this.getChangeModel().changeUrl())}
+ >${this.changeNum}</a
><span class="changeNumberColon">:</span>
<span class="headerSubject">${this.change?.subject}</span>
<input
@@ -834,7 +832,7 @@
Shortcut.UP_TO_CHANGE,
ShortcutSection.NAVIGATION
)}
- href=${this.getChangePath()}
+ href=${ifDefined(this.getChangeModel().changeUrl())}
>Up</a
>
<span class="separator"></span>
@@ -1093,23 +1091,12 @@
// Private but used in tests.
moveToFileWithComment(direction: -1 | 1) {
- if (!this.change) return;
- if (!this.patchRange?.patchNum) return;
-
- const file = this.findFileWithComment(direction);
- if (!file) {
- this.navToChangeView();
- return;
+ const path = this.findFileWithComment(direction);
+ if (!path) {
+ this.getChangeModel().navigateToChange();
+ } else {
+ this.getChangeModel().navigateToDiff({path});
}
-
- this.getNavigation().setUrl(
- createDiffUrl({
- change: this.change,
- patchNum: this.patchRange.patchNum,
- basePatchNum: this.patchRange.basePatchNum,
- diffView: {path: file},
- })
- );
}
private handleNewComment() {
@@ -1198,7 +1185,7 @@
fireEvent(this, 'show-auth-required');
return;
}
- this.navToChangeView(true);
+ this.getChangeModel().navigateToChange(true);
}
private handleToggleLeftPane() {
@@ -1234,10 +1221,6 @@
this.downloadModal.close();
}
- private handleUpToChange() {
- this.navToChangeView();
- }
-
private handleCommaKey() {
if (!this.loggedIn) return;
assertIsDefined(this.diffPreferencesDialog, 'diffPreferencesDialog');
@@ -1257,19 +1240,6 @@
}
// Private but used in tests.
- navToChangeView(openReplyDialog = false) {
- if (!this.changeNum || !this.patchRange?.patchNum) {
- return;
- }
- this.navigateToChange(
- this.change,
- this.patchRange,
- this.change && this.change.revisions,
- openReplyDialog
- );
- }
-
- // Private but used in tests.
navToFile(
fileList: string[],
direction: -1 | 1,
@@ -1277,15 +1247,10 @@
) {
const newPath = this.getNavLinkPath(fileList, direction);
if (!newPath) return;
- if (!this.change) return;
if (!this.patchRange) return;
if (newPath.up) {
- this.navigateToChange(
- this.change,
- this.patchRange,
- this.change && this.change.revisions
- );
+ this.getChangeModel().navigateToChange();
return;
}
@@ -1296,14 +1261,7 @@
newPath.path,
this.patchRange
)?.[0].line;
- this.getNavigation().setUrl(
- createDiffUrl({
- change: this.change,
- patchNum: this.patchRange.patchNum,
- basePatchNum: this.patchRange.basePatchNum,
- diffView: {path: newPath.path, lineNum},
- })
- );
+ this.getChangeModel().navigateToDiff({path: newPath.path, lineNum});
}
/**
@@ -1318,30 +1276,22 @@
if (!direction) return;
const newPath = this.getNavLinkPath(this.files.sortedPaths, direction);
- if (!newPath) {
- return;
- }
-
- if (newPath.up) {
- return this.getChangePath();
- }
- return this.getDiffUrl(this.change, this.patchRange, newPath.path);
+ if (!newPath) return;
+ if (newPath.up) return this.getChangeModel().changeUrl();
+ if (!newPath.path) return;
+ return this.getChangeModel().diffUrl({path: newPath.path});
}
private goToEditFile() {
- if (!this.change) return;
- if (!this.path) return;
- if (!this.patchRange) return;
+ assertIsDefined(this.path, 'path');
+ assertIsDefined(this.patchRange, 'patchRange');
// TODO(taoalpha): add a shortcut for editing
const cursorAddress = this.cursor?.getAddress();
- const editUrl = createEditUrl({
- changeNum: this.change._number,
- repo: this.change.project,
- patchNum: this.patchRange.patchNum,
- editView: {path: this.path, lineNum: cursorAddress?.number},
+ this.getChangeModel().navigateToEdit({
+ path: this.path,
+ lineNum: cursorAddress?.number,
});
- this.getNavigation().setUrl(editUrl);
}
/**
@@ -1363,7 +1313,6 @@
if (!this.path || !fileList || fileList.length === 0) {
return null;
}
-
let idx = fileList.indexOf(this.path);
if (idx === -1) {
const file = direction > 0 ? fileList[0] : fileList[fileList.length - 1];
@@ -1598,83 +1547,6 @@
}
}
- private getDiffUrl(
- change?: ChangeInfo | ParsedChangeInfo,
- patchRange?: PatchRange,
- path?: string
- ) {
- if (!change || !patchRange || !path) return '';
- return createDiffUrl({
- changeNum: change._number,
- repo: change.project,
- patchNum: patchRange.patchNum,
- basePatchNum: patchRange.basePatchNum,
- diffView: {path},
- });
- }
-
- /**
- * When the latest patch of the change is selected (and there is no base
- * patch) then the patch range need not appear in the URL. Return a patch
- * range object with undefined values when a range is not needed.
- */
- private getChangeUrlRange(
- patchRange?: PatchRange,
- revisions?: {[revisionId: string]: RevisionInfo | EditRevisionInfo}
- ) {
- let patchNum = undefined;
- let basePatchNum = undefined;
- let latestPatchNum = -1;
- for (const rev of Object.values(revisions || {})) {
- if (typeof rev._number === 'number') {
- latestPatchNum = Math.max(latestPatchNum, rev._number);
- }
- }
- if (!patchRange) return {patchNum, basePatchNum};
- if (
- patchRange.basePatchNum !== PARENT ||
- patchRange.patchNum !== latestPatchNum
- ) {
- patchNum = patchRange.patchNum;
- basePatchNum = patchRange.basePatchNum;
- }
- return {patchNum, basePatchNum};
- }
-
- private getChangePath() {
- if (!this.change) return '';
- if (!this.patchRange) return '';
-
- const range = this.getChangeUrlRange(
- this.patchRange,
- this.change.revisions
- );
- return createChangeUrl({
- change: this.change,
- patchNum: range.patchNum,
- basePatchNum: range.basePatchNum,
- });
- }
-
- // Private but used in tests.
- navigateToChange(
- change?: ChangeInfo | ParsedChangeInfo,
- patchRange?: PatchRange,
- revisions?: {[revisionId: string]: RevisionInfo | EditRevisionInfo},
- openReplyDialog?: boolean
- ) {
- if (!change) return;
- const range = this.getChangeUrlRange(patchRange, revisions);
- this.getNavigation().setUrl(
- createChangeUrl({
- change,
- patchNum: range.patchNum,
- basePatchNum: range.basePatchNum,
- openReplyDialog: !!openReplyDialog,
- })
- );
- }
-
// Private but used in tests
formatFilesForDropdown(): DropdownItem[] {
if (!this.files) return [];
@@ -1702,28 +1574,13 @@
// Private but used in tests.
handleFileChange(e: CustomEvent) {
- if (!this.change) return;
- if (!this.patchRange) return;
-
- // This is when it gets set initially.
const path = e.detail.value;
- if (path === this.path) {
- return;
- }
-
- this.getNavigation().setUrl(
- createDiffUrl({
- change: this.change,
- patchNum: this.patchRange.patchNum,
- basePatchNum: this.patchRange.basePatchNum,
- diffView: {path},
- })
- );
+ if (path === this.path) return;
+ this.getChangeModel().navigateToDiff(path);
}
// Private but used in tests.
handlePatchChange(e: CustomEvent) {
- if (!this.change) return;
if (!this.path) return;
if (!this.patchRange) return;
@@ -1734,13 +1591,10 @@
) {
return;
}
- this.getNavigation().setUrl(
- createDiffUrl({
- change: this.change,
- patchNum,
- basePatchNum,
- diffView: {path: this.path},
- })
+ this.getChangeModel().navigateToDiff(
+ {path: this.path},
+ patchNum,
+ basePatchNum
);
}
@@ -1919,107 +1773,88 @@
// Private but used in tests.
handleDiffAgainstBase() {
- if (!this.change) return;
- if (!this.path) return;
- if (!this.patchRange) return;
+ assertIsDefined(this.path, 'path');
+ assertIsDefined(this.patchRange, 'patchRange');
if (this.patchRange.basePatchNum === PARENT) {
fireAlert(this, 'Base is already selected.');
return;
}
- this.getNavigation().setUrl(
- createDiffUrl({
- change: this.change,
- patchNum: this.patchRange.patchNum,
- diffView: {path: this.path},
- })
+ this.getChangeModel().navigateToDiff(
+ {path: this.path},
+ this.patchRange.patchNum,
+ PARENT
);
}
// Private but used in tests.
handleDiffBaseAgainstLeft() {
if (this.viewState?.childView !== ChangeChildView.DIFF) return;
- if (!this.change) return;
- if (!this.path) return;
- if (!this.patchRange) return;
+ assertIsDefined(this.path, 'path');
+ assertIsDefined(this.patchRange, 'patchRange');
if (this.patchRange.basePatchNum === PARENT) {
fireAlert(this, 'Left is already base.');
return;
}
- this.getNavigation().setUrl(
- createDiffUrl({
- change: this.change,
- patchNum: this.patchRange.basePatchNum as RevisionPatchSetNum,
- basePatchNum: PARENT,
- diffView: {path: this.path},
- })
+ this.getChangeModel().navigateToDiff(
+ {path: this.path},
+ this.patchRange.basePatchNum as RevisionPatchSetNum,
+ PARENT
);
}
// Private but used in tests.
handleDiffAgainstLatest() {
- if (!this.change) return;
- if (!this.path) return;
- if (!this.patchRange) return;
+ assertIsDefined(this.path, 'path');
+ assertIsDefined(this.patchRange, 'patchRange');
- const latestPatchNum = computeLatestPatchNum(this.allPatchSets);
- if (this.patchRange.patchNum === latestPatchNum) {
+ if (this.patchRange.patchNum === this.latestPatchNum) {
fireAlert(this, 'Latest is already selected.');
return;
}
- this.getNavigation().setUrl(
- createDiffUrl({
- change: this.change,
- patchNum: latestPatchNum,
- basePatchNum: this.patchRange.basePatchNum,
- diffView: {path: this.path},
- })
+ this.getChangeModel().navigateToDiff(
+ {path: this.path},
+ this.latestPatchNum,
+ this.patchRange.basePatchNum
);
}
// Private but used in tests.
handleDiffRightAgainstLatest() {
- if (!this.change) return;
- if (!this.path) return;
- if (!this.patchRange) return;
+ assertIsDefined(this.path, 'path');
+ assertIsDefined(this.patchRange, 'patchRange');
- const latestPatchNum = computeLatestPatchNum(this.allPatchSets);
- if (this.patchRange.patchNum === latestPatchNum) {
+ if (this.patchRange.patchNum === this.latestPatchNum) {
fireAlert(this, 'Right is already latest.');
return;
}
- this.getNavigation().setUrl(
- createDiffUrl({
- change: this.change,
- patchNum: latestPatchNum,
- basePatchNum: this.patchRange.patchNum as BasePatchSetNum,
- diffView: {path: this.path},
- })
+
+ this.getChangeModel().navigateToDiff(
+ {path: this.path},
+ this.latestPatchNum,
+ this.patchRange.patchNum as BasePatchSetNum
);
}
// Private but used in tests.
handleDiffBaseAgainstLatest() {
- if (!this.change) return;
- if (!this.path) return;
- if (!this.patchRange) return;
+ assertIsDefined(this.path, 'path');
+ assertIsDefined(this.patchRange, 'patchRange');
- const latestPatchNum = computeLatestPatchNum(this.allPatchSets);
if (
- this.patchRange.patchNum === latestPatchNum &&
+ this.patchRange.patchNum === this.latestPatchNum &&
this.patchRange.basePatchNum === PARENT
) {
fireAlert(this, 'Already diffing base against latest.');
return;
}
- this.getNavigation().setUrl(
- createDiffUrl({
- change: this.change,
- patchNum: latestPatchNum,
- diffView: {path: this.path},
- })
+
+ this.getChangeModel().navigateToDiff(
+ {path: this.path},
+ this.latestPatchNum,
+ PARENT
);
}
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 603bd05..2465539 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
@@ -5,7 +5,6 @@
*/
import '../../../test/common-test-setup';
import './gr-diff-view';
-import {navigationToken} from '../../core/gr-navigation/gr-navigation';
import {
ChangeStatus,
DiffViewMode,
@@ -104,7 +103,9 @@
let clock: SinonFakeTimers;
let diffCommentsStub;
let getDiffRestApiStub: SinonStub;
- let setUrlStub: SinonStub;
+ let navToChangeStub: SinonStub;
+ let navToDiffStub: SinonStub;
+ let navToEditStub: SinonStub;
let changeModel: ChangeModel;
let commentsModel: CommentsModel;
let browserModel: BrowserModel;
@@ -122,7 +123,6 @@
}
setup(async () => {
- setUrlStub = sinon.stub(testResolver(navigationToken), 'setUrl');
stubRestApi('getConfig').returns(Promise.resolve(createServerInfo()));
stubRestApi('getLoggedIn').returns(Promise.resolve(false));
stubRestApi('getProjectConfig').returns(Promise.resolve(createConfig()));
@@ -162,6 +162,9 @@
changeModel = testResolver(changeModelToken);
browserModel = testResolver(browserModelToken);
userModel = testResolver(userModelToken);
+ navToChangeStub = sinon.stub(changeModel, 'navigateToChange');
+ navToDiffStub = sinon.stub(changeModel, 'navigateToDiff');
+ navToEditStub = sinon.stub(changeModel, 'navigateToEdit');
commentsModel.setState({
comments: {},
@@ -228,20 +231,19 @@
});
test('renders', async () => {
- clock = sinon.useFakeTimers();
- element.changeNum = 42 as NumericChangeId;
browserModel.setScreenWidth(0);
element.patchRange = {
basePatchNum: PARENT,
patchNum: 10 as RevisionPatchSetNum,
};
- element.change = {
+ const change = {
...createParsedChange(),
_number: 42 as NumericChangeId,
revisions: {
a: createRevision(10),
},
};
+ changeModel.updateStateChange(change);
element.files = getFilesFromFileList([
'chell.go',
'glados.txt',
@@ -430,49 +432,45 @@
element.path = 'glados.txt';
element.loggedIn = true;
await element.updateComplete;
- setUrlStub.reset();
+ navToChangeStub.reset();
pressKey(element, 'u');
- assert.equal(setUrlStub.callCount, 1);
- assert.equal(setUrlStub.lastCall.firstArg, '/c/test-project/+/42');
+ assert.isTrue(navToChangeStub.calledOnce);
await element.updateComplete;
pressKey(element, ']');
- assert.equal(setUrlStub.callCount, 2);
- assert.equal(
- setUrlStub.lastCall.firstArg,
- '/c/test-project/+/42/10/wheatley.md'
- );
+ assert.equal(navToDiffStub.callCount, 1);
+ 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(setUrlStub.callCount, 3);
- assert.equal(
- setUrlStub.lastCall.firstArg,
- '/c/test-project/+/42/10/glados.txt'
- );
+ 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(setUrlStub.callCount, 4);
- assert.equal(
- setUrlStub.lastCall.firstArg,
- '/c/test-project/+/42/10/chell.go'
- );
+ assert.equal(navToDiffStub.callCount, 3);
+ assert.deepEqual(navToDiffStub.lastCall.args, [
+ {path: 'chell.go', lineNum: undefined},
+ ]);
+
element.path = 'chell.go';
await element.updateComplete;
assert.isTrue(element.loading);
pressKey(element, '[');
- assert.equal(setUrlStub.callCount, 5);
- assert.equal(setUrlStub.lastCall.firstArg, '/c/test-project/+/42');
+ assert.equal(navToChangeStub.callCount, 2);
await element.updateComplete;
assert.isTrue(element.loading);
@@ -574,23 +572,21 @@
element.path = 'glados.txt';
element.loggedIn = true;
await element.updateComplete;
- setUrlStub.reset();
+ navToDiffStub.reset();
pressKey(element, 'N');
await element.updateComplete;
- assert.equal(setUrlStub.callCount, 1);
- assert.equal(
- setUrlStub.lastCall.firstArg,
- '/c/test-project/+/42/10/wheatley.md#21'
- );
+ assert.equal(navToDiffStub.callCount, 1);
+ assert.deepEqual(navToDiffStub.lastCall.args, [
+ {path: 'wheatley.md', lineNum: 21},
+ ]);
element.path = 'wheatley.md'; // navigated to next file
pressKey(element, 'N');
await element.updateComplete;
- assert.equal(setUrlStub.callCount, 2);
- assert.equal(setUrlStub.lastCall.firstArg, '/c/test-project/+/42');
+ assert.equal(navToChangeStub.callCount, 1);
});
test('shift+x shortcut toggles all diff context', async () => {
@@ -608,36 +604,26 @@
};
await element.updateComplete;
element.handleDiffAgainstBase();
- assert.equal(
- setUrlStub.lastCall.firstArg,
- '/c/test-project/+/42/10/some/path.txt'
- );
+ const expected = [{path: 'some/path.txt'}, 10, PARENT];
+ assert.deepEqual(navToDiffStub.lastCall.args, expected);
});
test('diff against latest', async () => {
element.path = 'foo';
- element.change = {
- ...createParsedChange(),
- revisions: createRevisions(12),
- };
+ element.latestPatchNum = 12 as PatchSetNumber;
element.patchRange = {
basePatchNum: 5 as BasePatchSetNum,
patchNum: 10 as RevisionPatchSetNum,
};
await element.updateComplete;
element.handleDiffAgainstLatest();
- assert.equal(
- setUrlStub.lastCall.firstArg,
- '/c/test-project/+/42/5..12/foo'
- );
+ const expected = [{path: 'foo'}, 12, 5];
+ assert.deepEqual(navToDiffStub.lastCall.args, expected);
});
test('handleDiffBaseAgainstLeft', async () => {
element.path = 'foo';
- element.change = {
- ...createParsedChange(),
- revisions: createRevisions(10),
- };
+ element.latestPatchNum = 10 as PatchSetNumber;
element.patchRange = {
patchNum: 3 as RevisionPatchSetNum,
basePatchNum: 1 as BasePatchSetNum,
@@ -650,42 +636,33 @@
};
await element.updateComplete;
element.handleDiffBaseAgainstLeft();
- assert.equal(setUrlStub.lastCall.firstArg, '/c/test-project/+/42/1/foo');
+ const expected = [{path: 'foo'}, 1, PARENT];
+ assert.deepEqual(navToDiffStub.lastCall.args, expected);
});
test('handleDiffRightAgainstLatest', async () => {
element.path = 'foo';
- element.change = {
- ...createParsedChange(),
- revisions: createRevisions(10),
- };
+ element.latestPatchNum = 10 as PatchSetNumber;
element.patchRange = {
basePatchNum: 1 as BasePatchSetNum,
patchNum: 3 as RevisionPatchSetNum,
};
await element.updateComplete;
element.handleDiffRightAgainstLatest();
- assert.equal(
- setUrlStub.lastCall.firstArg,
- '/c/test-project/+/42/3..10/foo'
- );
+ const expected = [{path: 'foo'}, 10, 3];
+ assert.deepEqual(navToDiffStub.lastCall.args, expected);
});
test('handleDiffBaseAgainstLatest', async () => {
- element.change = {
- ...createParsedChange(),
- revisions: createRevisions(10),
- };
+ element.latestPatchNum = 10 as PatchSetNumber;
element.patchRange = {
basePatchNum: 1 as BasePatchSetNum,
patchNum: 3 as RevisionPatchSetNum,
};
await element.updateComplete;
element.handleDiffBaseAgainstLatest();
- assert.equal(
- setUrlStub.lastCall.firstArg,
- '/c/test-project/+/42/10/some/path.txt'
- );
+ const expected = [{path: 'some/path.txt'}, 10, PARENT];
+ assert.deepEqual(navToDiffStub.lastCall.args, expected);
});
test('A fires an error event when not logged in', async () => {
@@ -694,7 +671,7 @@
element.addEventListener('show-auth-required', loggedInErrorSpy);
pressKey(element, 'a');
await element.updateComplete;
- assert.isFalse(setUrlStub.calledOnce);
+ assert.isFalse(navToDiffStub.calledOnce);
assert.isTrue(loggedInErrorSpy.called);
});
@@ -716,16 +693,13 @@
await element.updateComplete;
const loggedInErrorSpy = sinon.spy();
element.addEventListener('show-auth-required', loggedInErrorSpy);
- setUrlStub.reset();
+ navToDiffStub.reset();
pressKey(element, 'a');
await element.updateComplete;
- assert.equal(setUrlStub.callCount, 1);
- assert.equal(
- setUrlStub.lastCall.firstArg,
- '/c/test-project/+/42/5..10?openReplyDialog=true'
- );
+ assert.isTrue(navToChangeStub.calledOnce);
+ assert.deepEqual(navToChangeStub.lastCall.args, [true]);
assert.isFalse(loggedInErrorSpy.called);
});
@@ -748,11 +722,8 @@
element.addEventListener('show-auth-required', loggedInErrorSpy);
pressKey(element, 'a');
await element.updateComplete;
- assert.isTrue(setUrlStub.calledOnce);
- assert.equal(
- setUrlStub.lastCall.firstArg,
- '/c/test-project/+/42/1?openReplyDialog=true'
- );
+ assert.isTrue(navToChangeStub.calledOnce);
+ assert.deepEqual(navToChangeStub.lastCall.args, [true]);
assert.isFalse(loggedInErrorSpy.called);
});
@@ -778,40 +749,35 @@
element.path = 'glados.txt';
pressKey(element, 'u');
- assert.equal(setUrlStub.callCount, 1);
- assert.equal(setUrlStub.lastCall.firstArg, '/c/test-project/+/42/5..10');
+ assert.equal(navToChangeStub.callCount, 1);
pressKey(element, ']');
assert.isTrue(element.loading);
- assert.equal(setUrlStub.callCount, 2);
- assert.equal(
- setUrlStub.lastCall.firstArg,
- '/c/test-project/+/42/5..10/wheatley.md'
- );
+ assert.equal(navToDiffStub.callCount, 1);
+ assert.deepEqual(navToDiffStub.lastCall.args, [
+ {path: 'wheatley.md', lineNum: undefined},
+ ]);
element.path = 'wheatley.md';
pressKey(element, '[');
assert.isTrue(element.loading);
- assert.equal(setUrlStub.callCount, 3);
- assert.equal(
- setUrlStub.lastCall.firstArg,
- '/c/test-project/+/42/5..10/glados.txt'
- );
+ assert.equal(navToDiffStub.callCount, 2);
+ assert.deepEqual(navToDiffStub.lastCall.args, [
+ {path: 'glados.txt', lineNum: undefined},
+ ]);
element.path = 'glados.txt';
pressKey(element, '[');
assert.isTrue(element.loading);
- assert.equal(setUrlStub.callCount, 4);
- assert.equal(
- setUrlStub.lastCall.firstArg,
- '/c/test-project/+/42/5..10/chell.go'
- );
+ assert.equal(navToDiffStub.callCount, 3);
+ assert.deepEqual(navToDiffStub.lastCall.args, [
+ {path: 'chell.go', lineNum: undefined},
+ ]);
element.path = 'chell.go';
pressKey(element, '[');
assert.isTrue(element.loading);
- assert.equal(setUrlStub.callCount, 5);
- assert.equal(setUrlStub.lastCall.firstArg, '/c/test-project/+/42/5..10');
+ assert.equal(navToChangeStub.callCount, 2);
assertIsDefined(element.downloadModal);
const downloadModalStub = sinon.stub(element.downloadModal, 'showModal');
@@ -819,7 +785,7 @@
assert.isTrue(downloadModalStub.called);
});
- test('keyboard shortcuts with old patch number', () => {
+ test('keyboard shortcuts with old patch number', async () => {
element.changeNum = 42 as NumericChangeId;
element.patchRange = {
basePatchNum: PARENT,
@@ -841,34 +807,30 @@
element.path = 'glados.txt';
pressKey(element, 'u');
- assert.isTrue(setUrlStub.calledOnce);
- assert.equal(setUrlStub.lastCall.firstArg, '/c/test-project/+/42/1');
+ assert.isTrue(navToChangeStub.calledOnce);
pressKey(element, ']');
- assert.equal(
- setUrlStub.lastCall.firstArg,
- '/c/test-project/+/42/1/wheatley.md'
- );
+ assert.deepEqual(navToDiffStub.lastCall.args, [
+ {path: 'wheatley.md', lineNum: undefined},
+ ]);
element.path = 'wheatley.md';
pressKey(element, '[');
- assert.equal(
- setUrlStub.lastCall.firstArg,
- '/c/test-project/+/42/1/glados.txt'
- );
+ assert.deepEqual(navToDiffStub.lastCall.args, [
+ {path: 'glados.txt', lineNum: undefined},
+ ]);
element.path = 'glados.txt';
pressKey(element, '[');
- assert.equal(
- setUrlStub.lastCall.firstArg,
- '/c/test-project/+/42/1/chell.go'
- );
- element.path = 'chell.go';
+ assert.deepEqual(navToDiffStub.lastCall.args, [
+ {path: 'chell.go', lineNum: undefined},
+ ]);
- setUrlStub.reset();
+ element.path = 'chell.go';
+ await element.updateComplete;
+ navToDiffStub.reset();
pressKey(element, '[');
- assert.isTrue(setUrlStub.calledOnce);
- assert.equal(setUrlStub.lastCall.firstArg, '/c/test-project/+/42/1');
+ assert.equal(navToChangeStub.callCount, 2);
});
test('edit should redirect to edit page', async () => {
@@ -878,16 +840,6 @@
basePatchNum: PARENT,
patchNum: 1 as RevisionPatchSetNum,
};
- element.change = {
- ...createParsedChange(),
- _number: 42 as NumericChangeId,
- project: 'gerrit' as RepoName,
- status: ChangeStatus.NEW,
- revisions: {
- a: createRevision(1),
- b: createRevision(2),
- },
- };
await element.updateComplete;
const editBtn = queryAndAssert<GrButton>(
element,
@@ -895,8 +847,10 @@
);
assert.isTrue(!!editBtn);
editBtn.click();
- assert.equal(setUrlStub.callCount, 1);
- assert.equal(setUrlStub.lastCall.firstArg, '/c/gerrit/+/42/1/t.txt,edit');
+ assert.equal(navToEditStub.callCount, 1);
+ assert.deepEqual(navToEditStub.lastCall.args, [
+ {path: 't.txt', lineNum: undefined},
+ ]);
});
test('edit should redirect to edit page with line number', async () => {
@@ -907,16 +861,6 @@
basePatchNum: PARENT,
patchNum: 1 as RevisionPatchSetNum,
};
- element.change = {
- ...createParsedChange(),
- _number: 42 as NumericChangeId,
- project: 'gerrit' as RepoName,
- status: ChangeStatus.NEW,
- revisions: {
- a: createRevision(1),
- b: createRevision(2),
- },
- };
assertIsDefined(element.cursor);
sinon
.stub(element.cursor, 'getAddress')
@@ -928,11 +872,10 @@
);
assert.isTrue(!!editBtn);
editBtn.click();
- assert.equal(setUrlStub.callCount, 1);
- assert.equal(
- setUrlStub.lastCall.firstArg,
- '/c/gerrit/+/42/1/t.txt,edit#42'
- );
+ assert.equal(navToEditStub.callCount, 1);
+ assert.deepEqual(navToEditStub.lastCall.args, [
+ {path: 't.txt', lineNum: 42},
+ ]);
});
async function isEditVisibile({
@@ -1122,18 +1065,20 @@
});
test('prev/up/next links', async () => {
- element.changeNum = 42 as NumericChangeId;
- element.patchRange = {
- basePatchNum: PARENT,
- patchNum: 10 as RevisionPatchSetNum,
- };
- element.change = {
+ const viewModel = testResolver(changeViewModelToken);
+ viewModel.setState({
+ ...createDiffViewState(),
+ });
+ const change = {
...createParsedChange(),
_number: 42 as NumericChangeId,
revisions: {
a: createRevision(10),
},
};
+ changeModel.updateStateChange(change);
+ await element.updateComplete;
+
element.files = getFilesFromFileList([
'chell.go',
'glados.txt',
@@ -1153,24 +1098,30 @@
linkEls[2].getAttribute('href'),
'/c/test-project/+/42/10/wheatley.md'
);
+
element.path = 'wheatley.md';
await element.updateComplete;
+
assert.equal(
linkEls[0].getAttribute('href'),
'/c/test-project/+/42/10/glados.txt'
);
assert.equal(linkEls[1].getAttribute('href'), '/c/test-project/+/42');
assert.equal(linkEls[2].getAttribute('href'), '/c/test-project/+/42');
+
element.path = 'chell.go';
await element.updateComplete;
+
assert.equal(linkEls[0].getAttribute('href'), '/c/test-project/+/42');
assert.equal(linkEls[1].getAttribute('href'), '/c/test-project/+/42');
assert.equal(
linkEls[2].getAttribute('href'),
'/c/test-project/+/42/10/glados.txt'
);
+
element.path = 'not_a_real_file';
await element.updateComplete;
+
assert.equal(
linkEls[0].getAttribute('href'),
'/c/test-project/+/42/10/wheatley.md'
@@ -1183,26 +1134,30 @@
});
test('prev/up/next links with patch range', async () => {
- element.changeNum = 42 as NumericChangeId;
- element.patchRange = {
+ const viewModel = testResolver(changeViewModelToken);
+ viewModel.setState({
+ ...createDiffViewState(),
basePatchNum: 5 as BasePatchSetNum,
patchNum: 10 as RevisionPatchSetNum,
- };
- element.change = {
+ diffView: {path: 'glados.txt'},
+ });
+ const change = {
...createParsedChange(),
_number: 42 as NumericChangeId,
revisions: {
a: createRevision(5),
b: createRevision(10),
+ c: createRevision(12),
},
};
+ changeModel.updateStateChange(change);
element.files = getFilesFromFileList([
'chell.go',
'glados.txt',
'wheatley.md',
]);
- element.path = 'glados.txt';
- await element.updateComplete;
+ await waitUntil(() => element.path === 'glados.txt');
+
const linkEls = queryAll(element, '.navLink');
assert.equal(linkEls.length, 3);
assert.equal(
@@ -1217,8 +1172,10 @@
linkEls[2].getAttribute('href'),
'/c/test-project/+/42/5..10/wheatley.md'
);
- element.path = 'wheatley.md';
- await element.updateComplete;
+
+ viewModel.updateState({diffView: {path: 'wheatley.md'}});
+ await waitUntil(() => element.path === 'wheatley.md');
+
assert.equal(
linkEls[0].getAttribute('href'),
'/c/test-project/+/42/5..10/glados.txt'
@@ -1231,8 +1188,10 @@
linkEls[2].getAttribute('href'),
'/c/test-project/+/42/5..10'
);
- element.path = 'chell.go';
- await element.updateComplete;
+
+ viewModel.updateState({diffView: {path: 'chell.go'}});
+ await waitUntil(() => element.path === 'chell.go');
+
assert.equal(
linkEls[0].getAttribute('href'),
'/c/test-project/+/42/5..10'
@@ -1249,13 +1208,7 @@
});
test('handlePatchChange calls setUrl correctly', async () => {
- element.change = {
- ...createParsedChange(),
- _number: 321 as NumericChangeId,
- project: 'foo/bar' as RepoName,
- };
element.path = 'path/to/file.txt';
-
element.patchRange = {
basePatchNum: PARENT,
patchNum: 3 as RevisionPatchSetNum,
@@ -1266,15 +1219,15 @@
basePatchNum: PARENT,
patchNum: 1 as RevisionPatchSetNum,
};
-
queryAndAssert(element, '#rangeSelect').dispatchEvent(
new CustomEvent('patch-range-change', {detail, bubbles: false})
);
- assert.equal(
- setUrlStub.lastCall.firstArg,
- '/c/foo/bar/+/321/1/path/to/file.txt'
- );
+ assert.deepEqual(navToDiffStub.lastCall.args, [
+ {path: element.path},
+ detail.patchNum,
+ detail.basePatchNum,
+ ]);
});
test(
@@ -1741,10 +1694,7 @@
});
suite('skip next/previous', () => {
- let navToChangeStub: SinonStub;
-
setup(() => {
- navToChangeStub = sinon.stub(element, 'navToChangeView');
element.files = getFilesFromFileList([
'path/one.jpg',
'path/two.m4v',
@@ -1768,7 +1718,7 @@
element.moveToFileWithComment(-1);
assert.isTrue(navToChangeStub.calledOnce);
- assert.isFalse(setUrlStub.called);
+ assert.isFalse(navToDiffStub.called);
});
test('w/ previous', async () => {
@@ -1782,7 +1732,7 @@
element.moveToFileWithComment(-1);
assert.isFalse(navToChangeStub.called);
- assert.isTrue(setUrlStub.calledOnce);
+ assert.isTrue(navToDiffStub.calledOnce);
});
});
@@ -1798,7 +1748,7 @@
element.moveToFileWithComment(1);
assert.isTrue(navToChangeStub.calledOnce);
- assert.isFalse(setUrlStub.called);
+ assert.isFalse(navToDiffStub.called);
});
test('w/ previous', async () => {
@@ -1812,7 +1762,7 @@
element.moveToFileWithComment(1);
assert.isFalse(navToChangeStub.called);
- assert.isTrue(setUrlStub.calledOnce);
+ assert.isTrue(navToDiffStub.calledOnce);
});
});
});
@@ -2076,13 +2026,13 @@
revisions: createRevisions(1),
};
await element.updateComplete;
- assert.isFalse(setUrlStub.called);
+ assert.isFalse(navToDiffStub.called);
// Switch to file2
element.handleFileChange(
new CustomEvent('value-change', {detail: {value: 'file2'}})
);
- assert.isTrue(setUrlStub.calledOnce);
+ assert.isTrue(navToDiffStub.calledOnce);
// This is to mock the param change triggered by above navigate
element.viewState = {
@@ -2097,7 +2047,7 @@
};
// No extra call
- assert.isTrue(setUrlStub.calledOnce);
+ assert.isTrue(navToDiffStub.calledOnce);
});
test('_computeDownloadDropdownLinks', () => {
diff --git a/polygerrit-ui/app/models/change/change-model.ts b/polygerrit-ui/app/models/change/change-model.ts
index eaaf697..7cbaabe 100644
--- a/polygerrit-ui/app/models/change/change-model.ts
+++ b/polygerrit-ui/app/models/change/change-model.ts
@@ -12,6 +12,7 @@
PatchSetNum,
PreferencesInfo,
RevisionPatchSetNum,
+ PatchSetNumber,
} from '../../types/common';
import {DefaultBase} from '../../constants/constants';
import {combineLatest, from, fromEvent, Observable, forkJoin, of} from 'rxjs';
@@ -37,7 +38,10 @@
import {UserModel} from '../user/user-model';
import {define} from '../dependency';
import {isOwner} from '../../utils/change-util';
-import {ChangeViewModel} from '../views/change';
+import {ChangeViewModel, createChangeUrl} from '../views/change';
+import {createDiffUrl} from '../views/diff';
+import {NavigationService} from '../../elements/core/gr-navigation/gr-navigation';
+import {createEditUrl} from '../views/edit';
export enum LoadingStatus {
NOT_LOADED = 'NOT_LOADED',
@@ -150,7 +154,11 @@
export class ChangeModel extends Model<ChangeState> {
private change?: ParsedChangeInfo;
- private patchNum?: PatchSetNum;
+ private patchNum?: RevisionPatchSetNum;
+
+ private basePatchNum?: BasePatchSetNum;
+
+ private latestPatchNum?: PatchSetNumber;
public readonly change$ = select(
this.state$,
@@ -257,6 +265,7 @@
);
constructor(
+ private readonly navigation: NavigationService,
private readonly viewModel: ChangeViewModel,
private readonly restApiService: RestApiService,
private readonly userModel: UserModel
@@ -289,6 +298,12 @@
}),
this.change$.subscribe(change => (this.change = change)),
this.patchNum$.subscribe(patchNum => (this.patchNum = patchNum)),
+ this.basePatchNum$.subscribe(
+ basePatchNum => (this.basePatchNum = basePatchNum)
+ ),
+ this.latestPatchNum$.subscribe(
+ latestPatchNum => (this.latestPatchNum = latestPatchNum)
+ ),
combineLatest([this.patchNum$, this.changeNum$, this.userModel.loggedIn$])
.pipe(
switchMap(([patchNum, changeNum, loggedIn]) => {
@@ -372,6 +387,65 @@
return this.getState().change;
}
+ diffUrl(
+ diffView: {path: string; lineNum?: number},
+ patchNum = this.patchNum,
+ basePatchNum = this.basePatchNum
+ ) {
+ if (!this.change) return;
+ if (!this.patchNum) return;
+ return createDiffUrl({
+ change: this.change,
+ patchNum,
+ basePatchNum,
+ diffView,
+ });
+ }
+
+ navigateToDiff(
+ diffView: {path: string; lineNum?: number},
+ patchNum = this.patchNum,
+ basePatchNum = this.basePatchNum
+ ) {
+ const url = this.diffUrl(diffView, patchNum, basePatchNum);
+ if (!url) return;
+ this.navigation.setUrl(url);
+ }
+
+ changeUrl(openReplyDialog = false) {
+ if (!this.change) return;
+ const isLatest = this.latestPatchNum === this.patchNum;
+ return createChangeUrl({
+ change: this.change,
+ patchNum:
+ isLatest && this.basePatchNum === PARENT ? undefined : this.patchNum,
+ basePatchNum: this.basePatchNum,
+ openReplyDialog,
+ });
+ }
+
+ navigateToChange(openReplyDialog = false) {
+ const url = this.changeUrl(openReplyDialog);
+ if (!url) return;
+ this.navigation.setUrl(url);
+ }
+
+ editUrl(editView: {path: string; lineNum?: number}) {
+ if (!this.change) return;
+ return createEditUrl({
+ changeNum: this.change._number,
+ repo: this.change.project,
+ patchNum: this.patchNum,
+ editView,
+ });
+ }
+
+ navigateToEdit(editView: {path: string; lineNum?: number}) {
+ const url = this.editUrl(editView);
+ if (!url) return;
+ this.navigation.setUrl(url);
+ }
+
/**
* Check whether there is no newer patch than the latest patch that was
* available when this change was loaded.
diff --git a/polygerrit-ui/app/models/change/change-model_test.ts b/polygerrit-ui/app/models/change/change-model_test.ts
index 4bc23d8..c11c15b 100644
--- a/polygerrit-ui/app/models/change/change-model_test.ts
+++ b/polygerrit-ui/app/models/change/change-model_test.ts
@@ -35,6 +35,7 @@
import {testResolver} from '../../test/common-test-setup';
import {userModelToken} from '../user/user-model';
import {changeViewModelToken} from '../views/change';
+import {navigationToken} from '../../elements/core/gr-navigation/gr-navigation';
suite('updateChangeWithEdit() tests', () => {
test('undefined change', async () => {
@@ -84,6 +85,7 @@
setup(() => {
changeModel = new ChangeModel(
+ testResolver(navigationToken),
testResolver(changeViewModelToken),
getAppContext().restApiService,
testResolver(userModelToken)
diff --git a/polygerrit-ui/app/services/app-context-init.ts b/polygerrit-ui/app/services/app-context-init.ts
index 3a15de9..14fb253 100644
--- a/polygerrit-ui/app/services/app-context-init.ts
+++ b/polygerrit-ui/app/services/app-context-init.ts
@@ -148,6 +148,7 @@
changeModelToken,
() =>
new ChangeModel(
+ resolver(navigationToken),
resolver(changeViewModelToken),
appContext.restApiService,
resolver(userModelToken)
