Use new files model in `gr-file-list` element
Instead of calling the Rest API for getting file information
`<gr-file-list>` just subscribes to the new files model.
Remove the `reload()` method from `<gr-file-list>`. The model loads
new file information reactively. No need for telling the file list to
reload directly.
The files model deals with proper normalization of file infos, and with
adding files for unmodified (but commented on) files.
Release-Notes: skip
Change-Id: Ia5d59c176e2740b14a4c484933f035d5ec03e419
diff --git a/polygerrit-ui/app/api/rest-api.ts b/polygerrit-ui/app/api/rest-api.ts
index b62da8d..5ec0836 100644
--- a/polygerrit-ui/app/api/rest-api.ts
+++ b/polygerrit-ui/app/api/rest-api.ts
@@ -614,8 +614,8 @@
old_path?: string;
lines_inserted?: number;
lines_deleted?: number;
- size_delta: number; // in bytes
- size: number; // in bytes
+ size_delta?: number; // in bytes
+ size?: number; // in bytes
}
/**
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 84a52d3..64d1146 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
@@ -176,6 +176,7 @@
import {FilesExpandedState} from '../gr-file-list-constants';
import {subscribe} from '../../lit/subscription-controller';
import {configModelToken} from '../../../models/config/config-model';
+import {filesModelToken} from '../../../models/change/files-model';
const MIN_LINES_FOR_COMMIT_COLLAPSE = 18;
@@ -553,6 +554,8 @@
private readonly getConfigModel = resolve(this, configModelToken);
+ private readonly getFilesModel = resolve(this, filesModelToken);
+
private readonly getShortcutsService = resolve(this, shortcutsServiceToken);
private replyRefitTask?: DelayedTask;
@@ -2887,13 +2890,6 @@
// have loaded.
coreDataPromise = Promise.all([patchResourcesLoaded, loadingFlagSet]);
} else {
- // Resolves when the file list has loaded.
- const fileListReload = loadingFlagSet.then(async () => {
- assertIsDefined(this.fileList);
- await this.fileList.reload();
- });
- allDataPromises.push(fileListReload);
-
const latestCommitMessageLoaded = loadingFlagSet.then(() => {
// If the latest commit message is known, there is nothing to do.
if (this.latestCommitMessage) {
@@ -2942,6 +2938,7 @@
return this.getRelatedChangesList()?.reload(relatedChangesPromise);
});
allDataPromises.push(relatedChangesLoaded);
+ allDataPromises.push(this.filesLoaded());
Promise.all(allDataPromises).then(() => {
// Loading of commments data is no longer part of this reporting
@@ -2954,6 +2951,11 @@
return coreDataPromise;
}
+ private async filesLoaded() {
+ if (!this.isConnected) await until(this.connected$, connected => connected);
+ await until(this.getFilesModel().files$, f => f.length > 0);
+ }
+
/**
* Determines whether or not the given change has a parent change. If there
* is a relation chain, and the change id is not the last item of the
@@ -2978,12 +2980,7 @@
reloadPatchNumDependentResources(patchNumChanged?: boolean) {
assertIsDefined(this.changeNum, 'changeNum');
if (!this.patchRange?.patchNum) throw new Error('missing patchNum');
- // Filelist might not be rendered yet, so we wait for it to be rendered
- // before we ask it to reload.
- const promises = [
- this.loadAndSetCommitInfo(),
- this.updateComplete.then(() => this.fileList!.reload()),
- ];
+ const promises = [this.loadAndSetCommitInfo()];
if (patchNumChanged) {
promises.push(
this.getCommentsModel().reloadPortedComments(
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 d3ed34f..32feb6a 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
@@ -1408,7 +1408,6 @@
assertIsDefined(element.fileList);
sinon.stub(element, 'loadData').callsFake(() => Promise.resolve());
sinon.stub(element, 'loadAndSetCommitInfo');
- sinon.stub(element.fileList, 'reload');
await element.updateComplete;
const reloadPortedCommentsStub = sinon.stub(
element.getCommentsModel(),
@@ -2312,6 +2311,9 @@
});
test('report changeDisplayed on paramsChanged', async () => {
+ stubRestApi('getChangeOrEditFiles').resolves({
+ 'a-file.js': {},
+ });
const changeDisplayStub = sinon.stub(
element.reporting,
'changeDisplayed'
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 9d6714e..74080c2 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
@@ -38,18 +38,15 @@
toggleClass,
} from '../../../utils/dom-util';
import {
- addUnmodifiedFiles,
computeDisplayPath,
computeTruncatedPath,
isMagicPath,
- specialFilePathCompare,
} from '../../../utils/path-list-util';
import {customElement, property, query, state} from 'lit/decorators';
import {
BasePatchSetNum,
EDIT,
FileInfo,
- FileNameToFileInfoMap,
NumericChangeId,
PARENT,
PatchRange,
@@ -68,6 +65,7 @@
import {browserModelToken} from '../../../models/browser/browser-model';
import {commentsModelToken} from '../../../models/comments/comments-model';
import {changeModelToken} from '../../../models/change/change-model';
+import {filesModelToken} from '../../../models/change/files-model';
import {ShortcutController} from '../../lit/shortcut-controller';
import {css, html, LitElement, nothing, PropertyValues} from 'lit';
import {Shortcut} from '../../../services/shortcuts/shortcuts-config';
@@ -214,10 +212,6 @@
// Private but used in tests.
@state()
- filesByPath?: FileNameToFileInfoMap;
-
- // Private but used in tests.
- @state()
files: NormalizedFileInfo[] = [];
// Private but used in tests.
@@ -292,6 +286,8 @@
private readonly getChangeModel = resolve(this, changeModelToken);
+ private readonly getFilesModel = resolve(this, filesModelToken);
+
private readonly getCommentsModel = resolve(this, commentsModelToken);
private readonly getBrowserModel = resolve(this, browserModelToken);
@@ -684,6 +680,13 @@
);
subscribe(
this,
+ () => this.getFilesModel().filesWithUnmodified$,
+ files => {
+ this.files = [...files];
+ }
+ );
+ subscribe(
+ this,
() => this.getBrowserModel().diffViewMode$,
diffView => {
this.diffViewMode = diffView;
@@ -724,23 +727,12 @@
}
override willUpdate(changedProperties: PropertyValues): void {
- if (changedProperties.has('filesByPath')) {
- this.updateCleanlyMergedPaths();
- }
if (
changedProperties.has('diffPrefs') ||
changedProperties.has('diffViewMode')
) {
this.updateDiffPreferences();
}
- if (
- changedProperties.has('filesByPath') ||
- changedProperties.has('changeComments') ||
- changedProperties.has('patchRange')
- ) {
- changedProperties.set('files', this.files);
- this.computeFiles();
- }
if (changedProperties.has('files')) {
this.filesChanged();
}
@@ -1402,20 +1394,14 @@
</div>`;
}
- async reload() {
- if (!this.changeNum || !this.patchRange?.patchNum) return;
- this.collapseAllDiffs();
-
- this.filesByPath = await this.restApiService.getChangeOrEditFiles(
- this.changeNum,
- this.patchRange
- );
-
+ protected override firstUpdated(): void {
this.detectChromiteButler();
this.reporting.fileListDisplayed();
}
- private async updateCleanlyMergedPaths() {
+ // TODO: Move into files-model.
+ // visible for testing
+ async updateCleanlyMergedPaths() {
// When viewing Auto Merge base vs a patchset, add an additional row that
// knows how many files were cleanly merged. This requires an additional RPC
// for the diffs between target parent and the patch set. The cleanly merged
@@ -1436,8 +1422,8 @@
patchNum: this.patchRange.patchNum,
}
);
- if (!allFilesByPath || !this.filesByPath) return;
- const conflictingPaths = Object.keys(this.filesByPath);
+ if (!allFilesByPath) return;
+ const conflictingPaths = this.files.map(f => f.__path);
this.cleanlyMergedPaths = Object.keys(allFilesByPath).filter(
path => !conflictingPaths.includes(path)
);
@@ -1486,9 +1472,9 @@
const deleted = obj.lines_deleted ? obj.lines_deleted : 0;
const total_size = obj.size && obj.binary ? obj.size : 0;
const size_delta_inserted =
- obj.binary && obj.size_delta > 0 ? obj.size_delta : 0;
+ obj.binary && obj.size_delta && obj.size_delta > 0 ? obj.size_delta : 0;
const size_delta_deleted =
- obj.binary && obj.size_delta < 0 ? obj.size_delta : 0;
+ obj.binary && obj.size_delta && obj.size_delta < 0 ? obj.size_delta : 0;
return {
inserted: acc.inserted + inserted,
@@ -1645,22 +1631,6 @@
);
}
- private normalizeChangeFilesResponse(
- response: FileNameToFileInfoMap
- ): NormalizedFileInfo[] {
- const paths = Object.keys(response).sort(specialFilePathCompare);
- const files: NormalizedFileInfo[] = [];
- for (let i = 0; i < paths.length; i++) {
- const info = {...response[paths[i]]} as NormalizedFileInfo;
- info.__path = paths[i];
- info.lines_inserted = info.lines_inserted || 0;
- info.lines_deleted = info.lines_deleted || 0;
- info.size_delta = info.size_delta || 0;
- files.push(info);
- }
- return files;
- }
-
/**
* Returns true if the event e is a click on an element.
*
@@ -2008,22 +1978,6 @@
});
}
- private computeFiles() {
- if (
- this.filesByPath === undefined ||
- this.changeComments === undefined ||
- this.patchRange === undefined
- ) {
- return;
- }
- // Await all promises resolving from reload. @See Issue 9057
- if (!this.changeComments) return;
- const commentedPaths = this.changeComments.getPaths(this.patchRange);
- const files = {...this.filesByPath};
- addUnmodifiedFiles(files, commentedPaths);
- this.files = this.normalizeChangeFilesResponse(files);
- }
-
private computeFilesShown(): NormalizedFileInfo[] {
const previousNumFilesShown = this.shownFiles ? this.shownFiles.length : 0;
@@ -2057,6 +2011,8 @@
}
async filesChanged() {
+ if (this.expandedFiles.length > 0) this.expandedFiles = [];
+ this.updateCleanlyMergedPaths();
if (!this.files || this.files.length === 0) return;
await this.updateComplete;
this.fileCursor.stops = Array.from(
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.ts b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.ts
index 6a1baf4..8d9afb1 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.ts
@@ -44,10 +44,12 @@
queryAll,
queryAndAssert,
} from '../../../utils/common-util';
-import {GrFileList} from './gr-file-list';
+import {GrFileList, NormalizedFileInfo} from './gr-file-list';
+import {FileInfo} from '../../../api/rest-api';
import {GrButton} from '../../shared/gr-button/gr-button';
import * as MockInteractions from '@polymer/iron-test-helpers/mock-interactions';
import {ParsedChangeInfo} from '../../../types/types';
+import {normalize} from '../../../models/change/files-model';
import {GrDiffHost} from '../../diff/gr-diff-host/gr-diff-host';
import {IronIconElement} from '@polymer/iron-icon';
import {GrEditFileControls} from '../../edit/gr-edit-file-controls/gr-edit-file-controls';
@@ -60,13 +62,12 @@
});
});
-function createFilesByPath(count: number) {
- return Array(count)
- .fill(0)
- .reduce((filesByPath, _, idx) => {
- filesByPath[`'/file${idx}`] = {lines_inserted: 9};
- return filesByPath;
- }, {});
+function createFiles(
+ count: number,
+ fileInfo: FileInfo = {}
+): NormalizedFileInfo[] {
+ const files = Array(count).fill({});
+ return files.map((_, idx) => normalize(fileInfo, `'/file${idx}`));
}
suite('gr-file-list tests', () => {
@@ -164,7 +165,7 @@
});
test('renders file row', async () => {
- element.filesByPath = createFilesByPath(1);
+ element.files = createFiles(1, {lines_inserted: 9});
await element.updateComplete;
const fileRows = queryAll<HTMLDivElement>(element, '.file-row');
expect(fileRows?.[0]).dom.equal(/* HTML */ `<div
@@ -255,7 +256,7 @@
test('correct number of files are shown', async () => {
element.fileListIncrement = 300;
- element.filesByPath = createFilesByPath(500);
+ element.files = createFiles(500);
await element.updateComplete;
await flush();
@@ -288,7 +289,7 @@
test('rendering each row calls the reportRenderedRow method', async () => {
const renderedStub = sinon.stub(element, 'reportRenderedRow');
- element.filesByPath = createFilesByPath(10);
+ element.files = createFiles(10);
await element.updateComplete;
assert.equal(queryAll<HTMLDivElement>(element, '.file-row').length, 10);
@@ -296,30 +297,34 @@
});
test('calculate totals for patch number', async () => {
- element.filesByPath = {
- '/COMMIT_MSG': {
+ element.files = [
+ {
+ __path: '/COMMIT_MSG',
lines_inserted: 9,
size: 0,
size_delta: 0,
},
- '/MERGE_LIST': {
+ {
+ __path: '/MERGE_LIST',
lines_inserted: 9,
size: 0,
size_delta: 0,
},
- 'file_added_in_rev2.txt': {
+ {
+ __path: 'file_added_in_rev2.txt',
lines_inserted: 1,
lines_deleted: 1,
size_delta: 10,
size: 100,
},
- 'myfile.txt': {
+ {
+ __path: 'myfile.txt',
lines_inserted: 1,
lines_deleted: 1,
size_delta: 10,
size: 100,
},
- };
+ ];
await element.updateComplete;
let patchChange = element.calculatePatchChange();
@@ -334,30 +339,34 @@
assert.isFalse(element.shouldHideChangeTotals(patchChange));
// Test with a commit message that isn't the first file.
- element.filesByPath = {
- 'file_added_in_rev2.txt': {
+ element.files = [
+ {
+ __path: 'file_added_in_rev2.txt',
lines_inserted: 1,
lines_deleted: 1,
size: 0,
size_delta: 0,
},
- '/COMMIT_MSG': {
+ {
+ __path: '/COMMIT_MSG',
lines_inserted: 9,
size: 0,
size_delta: 0,
},
- '/MERGE_LIST': {
+ {
+ __path: '/MERGE_LIST',
lines_inserted: 9,
size: 0,
size_delta: 0,
},
- 'myfile.txt': {
+ {
+ __path: 'myfile.txt',
lines_inserted: 1,
lines_deleted: 1,
size: 0,
size_delta: 0,
},
- };
+ ];
await element.updateComplete;
patchChange = element.calculatePatchChange();
@@ -372,20 +381,22 @@
assert.isFalse(element.shouldHideChangeTotals(patchChange));
// Test with no commit message.
- element.filesByPath = {
- 'file_added_in_rev2.txt': {
+ element.files = [
+ {
+ __path: 'file_added_in_rev2.txt',
lines_inserted: 1,
lines_deleted: 1,
size: 0,
size_delta: 0,
},
- 'myfile.txt': {
+ {
+ __path: 'myfile.txt',
lines_inserted: 1,
lines_deleted: 1,
size: 0,
size_delta: 0,
},
- };
+ ];
await element.updateComplete;
patchChange = element.calculatePatchChange();
@@ -400,18 +411,20 @@
assert.isFalse(element.shouldHideChangeTotals(patchChange));
// Test with files missing either lines_inserted or lines_deleted.
- element.filesByPath = {
- 'file_added_in_rev2.txt': {
+ element.files = [
+ {
+ __path: 'file_added_in_rev2.txt',
lines_inserted: 1,
size: 0,
size_delta: 0,
},
- 'myfile.txt': {
+ {
+ __path: 'myfile.txt',
lines_deleted: 1,
size: 0,
size_delta: 0,
},
- };
+ ];
await element.updateComplete;
patchChange = element.calculatePatchChange();
@@ -427,15 +440,26 @@
});
test('binary only files', async () => {
- element.filesByPath = {
- '/COMMIT_MSG': {
+ element.files = [
+ {
+ __path: '/COMMIT_MSG',
lines_inserted: 9,
size: 0,
size_delta: 0,
},
- file_binary_1: {binary: true, size_delta: 10, size: 100},
- file_binary_2: {binary: true, size_delta: -5, size: 120},
- };
+ {
+ __path: 'file_binary_1',
+ binary: true,
+ size_delta: 10,
+ size: 100,
+ },
+ {
+ __path: 'file_binary_2',
+ binary: true,
+ size_delta: -5,
+ size: 120,
+ },
+ ];
await element.updateComplete;
const patchChange = element.calculatePatchChange();
@@ -451,21 +475,38 @@
});
test('binary and regular files', async () => {
- element.filesByPath = {
- '/COMMIT_MSG': {
+ element.files = [
+ {
+ __path: '/COMMIT_MSG',
lines_inserted: 9,
size: 0,
size_delta: 0,
},
- file_binary_1: {binary: true, size_delta: 10, size: 100},
- file_binary_2: {binary: true, size_delta: -5, size: 120},
- 'myfile.txt': {lines_deleted: 5, size_delta: -10, size: 100},
- 'myfile2.txt': {
+ {
+ __path: 'file_binary_1',
+ binary: true,
+ size_delta: 10,
+ size: 100,
+ },
+ {
+ __path: 'file_binary_2',
+ binary: true,
+ size_delta: -5,
+ size: 120,
+ },
+ {
+ __path: 'myfile.txt',
+ lines_deleted: 5,
+ size_delta: -10,
+ size: 100,
+ },
+ {
+ __path: 'myfile2.txt',
lines_inserted: 10,
size: 0,
size_delta: 0,
},
- };
+ ];
await element.updateComplete;
const patchChange = element.calculatePatchChange();
@@ -791,11 +832,11 @@
suite('keyboard shortcuts', () => {
setup(async () => {
- element.filesByPath = {
- '/COMMIT_MSG': {size: 0, size_delta: 0},
- 'file_added_in_rev2.txt': {size: 0, size_delta: 0},
- 'myfile.txt': {size: 0, size_delta: 0},
- };
+ element.files = [
+ normalize({}, '/COMMIT_MSG'),
+ normalize({}, 'file_added_in_rev2.txt'),
+ normalize({}, 'myfile.txt'),
+ ];
element.changeNum = 42 as NumericChangeId;
element.patchRange = {
basePatchNum: PARENT,
@@ -881,7 +922,7 @@
});
test('i key shows/hides selected inline diff', async () => {
- const paths = Object.keys(element.filesByPath!);
+ const paths = element.files.map(f => f.__path);
sinon.stub(element, 'expandedFilesChanged');
const files = [...queryAll<HTMLDivElement>(element, '.file-row')];
element.fileCursor.stops = files;
@@ -1037,11 +1078,11 @@
test('file review status', async () => {
element.reviewed = ['/COMMIT_MSG', 'myfile.txt'];
- element.filesByPath = {
- '/COMMIT_MSG': {size: 0, size_delta: 0},
- 'file_added_in_rev2.txt': {size: 0, size_delta: 0},
- 'myfile.txt': {size: 0, size_delta: 0},
- };
+ element.files = [
+ normalize({}, '/COMMIT_MSG'),
+ normalize({}, 'file_added_in_rev2.txt'),
+ normalize({}, 'myfile.txt'),
+ ];
element.changeNum = 42 as NumericChangeId;
element.patchRange = {
basePatchNum: PARENT,
@@ -1102,11 +1143,11 @@
});
test('handleFileListClick', async () => {
- element.filesByPath = {
- '/COMMIT_MSG': {size: 0, size_delta: 0},
- 'f1.txt': {size: 0, size_delta: 0},
- 'f2.txt': {size: 0, size_delta: 0},
- };
+ element.files = [
+ normalize({}, '/COMMIT_MSG'),
+ normalize({}, 'f1.txt'),
+ normalize({}, 'f2.txt'),
+ ];
element.changeNum = 42 as NumericChangeId;
element.patchRange = {
basePatchNum: PARENT,
@@ -1142,11 +1183,11 @@
});
test('handleFileListClick editMode', async () => {
- element.filesByPath = {
- '/COMMIT_MSG': {size: 0, size_delta: 0},
- 'f1.txt': {size: 0, size_delta: 0},
- 'f2.txt': {size: 0, size_delta: 0},
- };
+ element.files = [
+ normalize({}, '/COMMIT_MSG'),
+ normalize({}, 'f1.txt'),
+ normalize({}, 'f2.txt'),
+ ];
element.changeNum = 42 as NumericChangeId;
element.patchRange = {
basePatchNum: PARENT,
@@ -1167,9 +1208,7 @@
});
test('checkbox shows/hides diff inline', async () => {
- element.filesByPath = {
- 'myfile.txt': {size: 0, size_delta: 0},
- };
+ element.files = [normalize({}, 'myfile.txt')];
element.changeNum = 42 as NumericChangeId;
element.patchRange = {
basePatchNum: PARENT,
@@ -1197,9 +1236,7 @@
});
test('diff mode correctly toggles the diffs', async () => {
- element.filesByPath = {
- 'myfile.txt': {size: 0, size_delta: 0},
- };
+ element.files = [normalize({}, 'myfile.txt')];
element.changeNum = 42 as NumericChangeId;
element.patchRange = {
basePatchNum: PARENT,
@@ -1221,16 +1258,12 @@
});
test('expanded attribute not set on path when not expanded', () => {
- element.filesByPath = {
- '/COMMIT_MSG': {size: 0, size_delta: 0},
- };
+ element.files = [normalize({}, '/COMMIT_MSG')];
assert.isNotOk(query(element, 'expanded'));
});
test('tapping row ignores links', async () => {
- element.filesByPath = {
- '/COMMIT_MSG': {size: 0, size_delta: 0},
- };
+ element.files = [normalize({}, '/COMMIT_MSG')];
element.changeNum = 42 as NumericChangeId;
element.patchRange = {
basePatchNum: PARENT,
@@ -1259,7 +1292,7 @@
test('toggleFileExpanded', async () => {
const path = 'path/to/my/file.txt';
- element.filesByPath = {[path]: {size: 0, size_delta: 0}};
+ element.files = [normalize({}, path)];
await element.updateComplete;
// Wait for expandedFilesChanged to finish.
await flush();
@@ -1305,7 +1338,7 @@
const reInitStub = sinon.stub(element.diffCursor, 'reInitAndUpdateStops');
const path = 'path/to/my/file.txt';
- element.filesByPath = {[path]: {size: 0, size_delta: 0}};
+ element.files = [normalize({}, path)];
// Wait for diffs to be computed.
await element.updateComplete;
await flush();
@@ -1373,10 +1406,7 @@
});
test('filesExpanded value updates to correct enum', async () => {
- element.filesByPath = {
- 'foo.bar': {size: 0, size_delta: 0},
- 'baz.bar': {size: 0, size_delta: 0},
- };
+ element.files = [normalize({}, 'foo.bar'), normalize({}, 'baz.bar')];
await element.updateComplete;
assert.equal(element.filesExpanded, FilesExpandedState.NONE);
element.expandedFiles.push({path: 'baz.bar'});
@@ -1504,10 +1534,11 @@
let filesStub: sinon.SinonStub;
setup(async () => {
+ element.files = [
+ normalize({size: 0, size_delta: 0}, 'conflictingFile.js'),
+ ];
filesStub = stubRestApi('getChangeOrEditFiles')
.onFirstCall()
- .resolves({'conflictingFile.js': {size: 0, size_delta: 0}})
- .onSecondCall()
.resolves({
'conflictingFile.js': {size: 0, size_delta: 0},
'cleanlyMergedFile.js': {size: 0, size_delta: 0},
@@ -1540,7 +1571,6 @@
});
test('displays cleanly merged file count', async () => {
- await element.reload();
await waitUntil(() => !!query(element, '.cleanlyMergedText'));
const message = queryAndAssert<HTMLSpanElement>(
@@ -1554,14 +1584,13 @@
filesStub.restore();
stubRestApi('getChangeOrEditFiles')
.onFirstCall()
- .resolves({'conflictingFile.js': {size: 0, size_delta: 0}})
- .onSecondCall()
.resolves({
'conflictingFile.js': {size: 0, size_delta: 0},
'cleanlyMergedFile.js': {size: 0, size_delta: 0},
'anotherCleanlyMergedFile.js': {size: 0, size_delta: 0},
});
- await element.reload();
+ await element.updateCleanlyMergedPaths();
+ await element.updateComplete;
await waitUntil(() => !!query(element, '.cleanlyMergedText'));
const message = queryAndAssert(
@@ -1572,7 +1601,6 @@
});
test('displays button for navigating to parent 1 base', async () => {
- await element.reload();
await waitUntil(() => !!query(element, '.showParentButton'));
queryAndAssert(element, '.showParentButton');
@@ -1582,8 +1610,6 @@
filesStub.restore();
stubRestApi('getChangeOrEditFiles')
.onFirstCall()
- .resolves({'conflictingFile.js': {size: 0, size_delta: 0}})
- .onSecondCall()
.resolves({
'conflictingFile.js': {size: 0, size_delta: 0},
'cleanlyMergedFile.js': {
@@ -1592,8 +1618,7 @@
size_delta: 0,
},
});
- await element.reload();
- await element.updateComplete;
+ await element.updateCleanlyMergedPaths();
assert.deepEqual(element.cleanlyMergedOldPaths, [
'cleanlyMergedFileOldName.js',
@@ -1605,7 +1630,7 @@
basePatchNum: 1 as BasePatchSetNum,
patchNum: 2 as RevisionPatchSetNum,
};
- await element.reload();
+ await element.updateCleanlyMergedPaths();
await element.updateComplete;
assert.notOk(query(element, '.cleanlyMergedText'));
@@ -1617,7 +1642,7 @@
basePatchNum: 1 as BasePatchSetNum,
patchNum: EDIT,
};
- await element.reload();
+ await element.updateCleanlyMergedPaths();
await element.updateComplete;
assert.notOk(query(element, '.cleanlyMergedText'));
@@ -1970,21 +1995,23 @@
element.numFilesShown = 75;
element.selectedIndex = 0;
- element.filesByPath = {
- '/COMMIT_MSG': {lines_inserted: 9, size: 0, size_delta: 0},
- 'file_added_in_rev2.txt': {
+ element.files = [
+ {__path: '/COMMIT_MSG', lines_inserted: 9, size: 0, size_delta: 0},
+ {
+ __path: 'file_added_in_rev2.txt',
lines_inserted: 1,
lines_deleted: 1,
size_delta: 10,
size: 100,
},
- 'myfile.txt': {
+ {
+ __path: 'myfile.txt',
lines_inserted: 1,
lines_deleted: 1,
size_delta: 10,
size: 100,
},
- };
+ ];
element.reviewed = ['/COMMIT_MSG', 'myfile.txt'];
element.changeNum = 42 as NumericChangeId;
element.patchRange = {
@@ -2152,15 +2179,15 @@
});
test('openSelectedFile behavior', async () => {
- const filesByPath = element.filesByPath;
- element.filesByPath = {};
+ const files = element.files;
+ element.files = [];
await element.updateComplete;
const navStub = sinon.stub(GerritNav, 'navigateToDiff');
// Noop when there are no files.
element.openSelectedFile();
assert.isFalse(navStub.called);
- element.filesByPath = filesByPath;
+ element.files = files;
await element.updateComplete;
// Navigates when a file is selected.
element.openSelectedFile();
diff --git a/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.ts b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.ts
index 40d3ff8..bdb7db7 100644
--- a/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.ts
+++ b/polygerrit-ui/app/elements/diff/gr-comment-api/gr-comment-api.ts
@@ -109,6 +109,8 @@
for (const [path, comments] of Object.entries(response)) {
// If don't care about patch range, we know that the path exists.
if (comments.some(c => !patchRange || isInPatchRange(c, patchRange))) {
+ // TODO: Replace the CommentMap type with just an array or set. We
+ // never set the value to false.
commentMap[path] = true;
}
}
diff --git a/polygerrit-ui/app/models/change/change-model.ts b/polygerrit-ui/app/models/change/change-model.ts
index fb1a9a4..19fabda 100644
--- a/polygerrit-ui/app/models/change/change-model.ts
+++ b/polygerrit-ui/app/models/change/change-model.ts
@@ -187,25 +187,27 @@
* Note that this selector can emit without the change being available!
*/
public readonly patchNum$: Observable<RevisionPatchSetNum | undefined> =
- /**
- * If you depend on both, router and change state, then you want to filter
- * out inconsistent state, e.g. router changeNum already updated, change not
- * yet reset to undefined.
- */
- combineLatest([this.routerModel.state$, this.state$])
- .pipe(
- filter(([routerState, changeState]) => {
+ select(
+ combineLatest([
+ this.routerModel.state$,
+ this.state$,
+ this.routerModel.routerPatchNum$,
+ this.latestPatchNum$,
+ ]).pipe(
+ /**
+ * If you depend on both, router and change state, then you want to filter
+ * out inconsistent state, e.g. router changeNum already updated, change not
+ * yet reset to undefined.
+ */
+ filter(([routerState, changeState, _routerPatchN, _latestPatchN]) => {
const changeNum = changeState.change?._number;
const routerChangeNum = routerState.changeNum;
return changeNum === undefined || changeNum === routerChangeNum;
- }),
- distinctUntilChanged()
- )
- .pipe(
- withLatestFrom(this.routerModel.routerPatchNum$, this.latestPatchNum$),
- map(([_, routerPatchN, latestPatchN]) => routerPatchN || latestPatchN),
- distinctUntilChanged()
- );
+ })
+ ),
+ ([_routerState, _changeState, routerPatchN, latestPatchN]) =>
+ routerPatchN || latestPatchN
+ );
/**
* Emits the base patchset number. This is identical to the
@@ -245,10 +247,9 @@
// For usage in `combineLatest` we need `startWith` such that reload$ has an
// initial value.
- private readonly reload$: Observable<unknown> = fromEvent(
- document,
- 'reload'
- ).pipe(startWith(undefined));
+ readonly reload$: Observable<unknown> = fromEvent(document, 'reload').pipe(
+ startWith(undefined)
+ );
constructor(
readonly routerModel: RouterModel,
diff --git a/polygerrit-ui/app/models/change/files-model.ts b/polygerrit-ui/app/models/change/files-model.ts
index 2bafa04..fa02c47 100644
--- a/polygerrit-ui/app/models/change/files-model.ts
+++ b/polygerrit-ui/app/models/change/files-model.ts
@@ -9,32 +9,67 @@
import {RestApiService} from '../../services/gr-rest-api/gr-rest-api';
import {Finalizable} from '../../services/registry';
import {select} from '../../utils/observable-util';
+import {FileInfoStatus, SpecialFilePath} from '../../constants/constants';
import {specialFilePathCompare} from '../../utils/path-list-util';
import {Model} from '../model';
import {define} from '../dependency';
import {ChangeModel} from './change-model';
+import {CommentsModel} from '../comments/comments-model';
export interface NormalizedFileInfo extends FileInfo {
__path: string;
+ // Compared to `FileInfo` these four props are required here.
+ lines_inserted: number;
+ lines_deleted: number;
+ size_delta: number; // in bytes
+ size: number; // in bytes
+}
+
+export function normalize(file: FileInfo, path: string): NormalizedFileInfo {
+ return {
+ __path: path,
+ // These 4 props are required in NormalizedFileInfo, but optional in
+ // FileInfo. So let's set a default value, if not already set.
+ lines_inserted: 0,
+ lines_deleted: 0,
+ size_delta: 0,
+ size: 0,
+ ...file,
+ };
}
function mapToList(map?: FileNameToFileInfoMap): NormalizedFileInfo[] {
const list: NormalizedFileInfo[] = [];
for (const [key, value] of Object.entries(map ?? {})) {
- list.push({
- ...value,
- __path: key,
- // These 3 props are required in NormalizedFileInfo, but optional in
- // FileInfo. So let's set a default value, if not already set.
- lines_inserted: value.lines_inserted ?? 0,
- lines_deleted: value.lines_deleted ?? 0,
- size_delta: value.size_delta ?? 0,
- });
+ list.push(normalize(value, key));
}
list.sort((f1, f2) => specialFilePathCompare(f1.__path, f2.__path));
return list;
}
+export function addUnmodified(
+ files: NormalizedFileInfo[],
+ commentedPaths: string[]
+) {
+ const combined = [...files];
+ for (const commentedPath of commentedPaths) {
+ if (commentedPath === SpecialFilePath.PATCHSET_LEVEL_COMMENTS) continue;
+ if (files.some(f => f.__path === commentedPath)) continue;
+ if (
+ files.some(
+ f => f.status === FileInfoStatus.RENAMED && f.old_path === commentedPath
+ )
+ ) {
+ continue;
+ }
+ combined.push(
+ normalize({status: FileInfoStatus.UNMODIFIED}, commentedPath)
+ );
+ }
+ combined.sort((f1, f2) => specialFilePathCompare(f1.__path, f2.__path));
+ return combined;
+}
+
export interface FilesState {
// TODO: Move reviewed files from change model into here. Change model is
// already large and complex, so the files model is a better fit.
@@ -51,21 +86,28 @@
export class FilesModel extends Model<FilesState> implements Finalizable {
public readonly files$ = select(this.state$, state => state.files);
+ public readonly filesWithUnmodified$ = select(
+ combineLatest([this.files$, this.commentsModel.commentedPaths$]),
+ ([files, commentedPaths]) => addUnmodified(files, commentedPaths)
+ );
+
private subscriptions: Subscription[] = [];
constructor(
readonly changeModel: ChangeModel,
+ readonly commentsModel: CommentsModel,
readonly restApiService: RestApiService
) {
super(initialState);
this.subscriptions = [
combineLatest([
+ this.changeModel.reload$,
this.changeModel.changeNum$,
this.changeModel.basePatchNum$,
this.changeModel.patchNum$,
])
.pipe(
- switchMap(([changeNum, basePatchNum, patchNum]) => {
+ switchMap(([_, changeNum, basePatchNum, patchNum]) => {
if (!changeNum || !patchNum) return of({});
return from(
this.restApiService.getChangeOrEditFiles(changeNum, {
diff --git a/polygerrit-ui/app/models/comments/comments-model.ts b/polygerrit-ui/app/models/comments/comments-model.ts
index d827f12..7beb2fb 100644
--- a/polygerrit-ui/app/models/comments/comments-model.ts
+++ b/polygerrit-ui/app/models/comments/comments-model.ts
@@ -259,6 +259,19 @@
changeComments.getAllThreadsForChange()
);
+ public readonly commentedPaths$ = select(
+ combineLatest([
+ this.changeComments$,
+ this.changeModel.basePatchNum$,
+ this.changeModel.patchNum$,
+ ]),
+ ([changeComments, basePatchNum, patchNum]) => {
+ if (!patchNum) return [];
+ const pathsMap = changeComments.getPaths({basePatchNum, patchNum});
+ return Object.keys(pathsMap);
+ }
+ );
+
public thread$(id: UrlEncodedCommentId) {
return select(this.threads$, threads => threads.find(t => t.rootId === id));
}
diff --git a/polygerrit-ui/app/services/app-context-init.ts b/polygerrit-ui/app/services/app-context-init.ts
index 0c090e5..ec58698 100644
--- a/polygerrit-ui/app/services/app-context-init.ts
+++ b/polygerrit-ui/app/services/app-context-init.ts
@@ -86,9 +86,6 @@
);
dependencies.set(changeModelToken, changeModel);
- const filesModel = new FilesModel(changeModel, appContext.restApiService);
- dependencies.set(filesModelToken, filesModel);
-
const commentsModel = new CommentsModel(
appContext.routerModel,
changeModel,
@@ -97,6 +94,13 @@
);
dependencies.set(commentsModelToken, commentsModel);
+ const filesModel = new FilesModel(
+ changeModel,
+ commentsModel,
+ appContext.restApiService
+ );
+ dependencies.set(filesModelToken, filesModel);
+
const configModel = new ConfigModel(changeModel, appContext.restApiService);
dependencies.set(configModelToken, configModel);
diff --git a/polygerrit-ui/app/test/test-app-context-init.ts b/polygerrit-ui/app/test/test-app-context-init.ts
index e4f499b..4f5504e 100644
--- a/polygerrit-ui/app/test/test-app-context-init.ts
+++ b/polygerrit-ui/app/test/test-app-context-init.ts
@@ -96,10 +96,6 @@
);
dependencies.set(changeModelToken, changeModelCreator);
- const filesModelCreator = () =>
- new FilesModel(resolver(changeModelToken), appContext.restApiService);
- dependencies.set(filesModelToken, filesModelCreator);
-
const commentsModelCreator = () =>
new CommentsModel(
appContext.routerModel,
@@ -109,6 +105,14 @@
);
dependencies.set(commentsModelToken, commentsModelCreator);
+ const filesModelCreator = () =>
+ new FilesModel(
+ resolver(changeModelToken),
+ resolver(commentsModelToken),
+ appContext.restApiService
+ );
+ dependencies.set(filesModelToken, filesModelCreator);
+
const configModelCreator = () =>
new ConfigModel(resolver(changeModelToken), appContext.restApiService);
dependencies.set(configModelToken, configModelCreator);
diff --git a/polygerrit-ui/app/utils/comment-util.ts b/polygerrit-ui/app/utils/comment-util.ts
index 8aab00e..c5cbe64 100644
--- a/polygerrit-ui/app/utils/comment-util.ts
+++ b/polygerrit-ui/app/utils/comment-util.ts
@@ -50,6 +50,7 @@
export type Comment = UnsavedInfo | DraftInfo | CommentInfo | RobotCommentInfo;
+// TODO: Replace the CommentMap type with just an array of paths.
export type CommentMap = {[path: string]: boolean};
export function isRobot<T extends CommentBasics>(
