Skip to content

Commit 2fe254f

Browse files
sbattensbatten
committed
viewDescriptorService model cleanup
fixes microsoft#96791
1 parent 118da97 commit 2fe254f

1 file changed

Lines changed: 49 additions & 66 deletions

File tree

‎src/vs/workbench/services/views/browser/viewDescriptorService.ts‎

Lines changed: 49 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@ import { ViewContainerModel } from 'vs/workbench/services/views/common/viewConta
2121

2222
interface ICachedViewContainerInfo {
2323
containerId: string;
24-
location?: ViewContainerLocation;
2524
}
2625

2726
export class ViewDescriptorService extends Disposable implements IViewDescriptorService {
@@ -84,7 +83,8 @@ export class ViewDescriptorService extends Disposable implements IViewDescriptor
8483
}
8584
}
8685

87-
readonly onDidChangeViewContainers: Event<{ added: ReadonlyArray<{ container: ViewContainer, location: ViewContainerLocation }>, removed: ReadonlyArray<{ container: ViewContainer, location: ViewContainerLocation }> }>;
86+
private readonly _onDidChangeViewContainers = this._register(new Emitter<{ added: ReadonlyArray<{ container: ViewContainer, location: ViewContainerLocation }>, removed: ReadonlyArray<{ container: ViewContainer, location: ViewContainerLocation }> }>());
87+
readonly onDidChangeViewContainers = this._onDidChangeViewContainers.event;
8888
get viewContainers(): ReadonlyArray<ViewContainer> { return this.viewContainersRegistry.all; }
8989

9090
constructor(
@@ -112,21 +112,22 @@ export class ViewDescriptorService extends Disposable implements IViewDescriptor
112112

113113
// Register all containers that were registered before this ctor
114114
this.viewContainers.forEach(viewContainer => this.onDidRegisterViewContainer(viewContainer));
115-
this.onDidChangeViewContainers = Event.any<{ added: ReadonlyArray<{ container: ViewContainer, location: ViewContainerLocation }>, removed: ReadonlyArray<{ container: ViewContainer, location: ViewContainerLocation }> }>(
116-
Event.map(this.viewContainersRegistry.onDidRegister, e => ({ added: [{ container: e.viewContainer, location: e.viewContainerLocation }], removed: [] })),
117-
Event.map(this.viewContainersRegistry.onDidDeregister, e => ({ added: [], removed: [{ container: e.viewContainer, location: e.viewContainerLocation }] }))
118-
);
119-
120-
// Try generating all generated containers that don't need extensions
121-
this.tryGenerateContainers();
122115

123116
this._register(this.viewsRegistry.onViewsRegistered(views => this.onDidRegisterViews(views)));
124117
this._register(this.viewsRegistry.onViewsDeregistered(({ views, viewContainer }) => this.onDidDeregisterViews(views, viewContainer)));
125118

126119
this._register(this.viewsRegistry.onDidChangeContainer(({ views, from, to }) => this.moveViews(views, from, to)));
127120

128-
this._register(this.viewContainersRegistry.onDidRegister(({ viewContainer }) => this.onDidRegisterViewContainer(viewContainer)));
129-
this._register(this.viewContainersRegistry.onDidDeregister(({ viewContainer }) => this.onDidDeregisterViewContainer(viewContainer)));
121+
this._register(this.viewContainersRegistry.onDidRegister(({ viewContainer }) => {
122+
this.onDidRegisterViewContainer(viewContainer);
123+
this._onDidChangeViewContainers.fire({ added: [{ container: viewContainer, location: this.getViewContainerLocation(viewContainer) }], removed: [] });
124+
}));
125+
126+
this._register(this.viewContainersRegistry.onDidDeregister(({ viewContainer }) => {
127+
this.onDidDeregisterViewContainer(viewContainer);
128+
this._onDidChangeViewContainers.fire({ removed: [{ container: viewContainer, location: this.getViewContainerLocation(viewContainer) }], added: [] });
129+
}));
130+
130131
this._register(toDisposable(() => {
131132
this.viewContainerModels.forEach(({ disposable }) => disposable.dispose());
132133
this.viewContainerModels.clear();
@@ -145,14 +146,13 @@ export class ViewDescriptorService extends Disposable implements IViewDescriptor
145146

146147
// The container has not been registered yet
147148
if (!viewContainer || !this.viewContainerModels.has(viewContainer)) {
148-
if (containerData.cachedContainerInfo && this.shouldGenerateContainer(containerData.cachedContainerInfo)) {
149-
const containerInfo = containerData.cachedContainerInfo;
150-
149+
if (containerData.cachedContainerInfo && this.isGeneratedContainerId(containerData.cachedContainerInfo.containerId)) {
151150
if (!this.viewContainersRegistry.get(containerId)) {
152-
this.registerGeneratedViewContainer(containerInfo.location!, containerId);
151+
this.registerGeneratedViewContainer(this.cachedViewContainerInfo.get(containerId)!, containerId);
153152
}
154153
}
155154

155+
// Registration of a generated container handles registration of its views
156156
continue;
157157
}
158158

@@ -174,7 +174,7 @@ export class ViewDescriptorService extends Disposable implements IViewDescriptor
174174
}
175175
}
176176

177-
private tryGenerateContainers(fallbackToDefault?: boolean): void {
177+
private fallbackOrphanedViews(): void {
178178
for (const [viewId, containerInfo] of this.cachedViewInfo.entries()) {
179179
const containerId = containerInfo.containerId;
180180

@@ -183,34 +183,18 @@ export class ViewDescriptorService extends Disposable implements IViewDescriptor
183183
continue;
184184
}
185185

186-
// check if we should generate this container
187-
if (this.shouldGenerateContainer(containerInfo)) {
188-
this.registerGeneratedViewContainer(containerInfo.location!, containerId);
189-
continue;
190-
}
191-
192-
if (fallbackToDefault) {
193-
// check if view has been registered to default location
194-
const viewContainer = this.viewsRegistry.getViewContainer(viewId);
195-
const viewDescriptor = this.getViewDescriptorById(viewId);
196-
if (viewContainer && viewDescriptor) {
197-
this.addViews(viewContainer, [viewDescriptor]);
198-
199-
const newLocation = this.getViewContainerLocation(viewContainer);
200-
if (containerInfo.location && containerInfo.location !== newLocation) {
201-
this._onDidChangeLocation.fire({ views: [viewDescriptor], from: containerInfo.location, to: newLocation });
202-
}
203-
}
186+
// check if view has been registered to default location
187+
const viewContainer = this.viewsRegistry.getViewContainer(viewId);
188+
const viewDescriptor = this.getViewDescriptorById(viewId);
189+
if (viewContainer && viewDescriptor) {
190+
this.addViews(viewContainer, [viewDescriptor]);
204191
}
205192
}
206-
207-
if (fallbackToDefault) {
208-
this.saveViewPositionsToCache();
209-
}
210193
}
211194

212195
private onDidRegisterExtensions(): void {
213-
this.tryGenerateContainers(true);
196+
// If an extension is uninstalled, this method will handle resetting views to default locations
197+
this.fallbackOrphanedViews();
214198
}
215199

216200
private onDidRegisterViews(views: { views: IViewDescriptor[], viewContainer: ViewContainer }[]): void {
@@ -227,8 +211,8 @@ export class ViewDescriptorService extends Disposable implements IViewDescriptor
227211
});
228212
}
229213

230-
private shouldGenerateContainer(containerInfo: ICachedViewContainerInfo): boolean {
231-
return containerInfo.containerId.startsWith(ViewDescriptorService.COMMON_CONTAINER_ID_PREFIX) && containerInfo.location !== undefined;
214+
private isGeneratedContainerId(id: string): boolean {
215+
return id.startsWith(ViewDescriptorService.COMMON_CONTAINER_ID_PREFIX);
232216
}
233217

234218
private onDidDeregisterViews(views: IViewDescriptor[], viewContainer: ViewContainer): void {
@@ -258,17 +242,8 @@ export class ViewDescriptorService extends Disposable implements IViewDescriptor
258242
}
259243

260244
getViewLocationById(viewId: string): ViewContainerLocation | null {
261-
const cachedInfo = this.cachedViewInfo.get(viewId);
262-
263-
if (cachedInfo && cachedInfo.location) {
264-
return cachedInfo.location;
265-
}
266-
267-
const container = cachedInfo?.containerId ?
268-
this.viewContainersRegistry.get(cachedInfo.containerId) ?? null :
269-
this.viewsRegistry.getViewContainer(viewId);
270-
271-
if (!container) {
245+
const container = this.getViewContainerByViewId(viewId);
246+
if (container === null) {
272247
return null;
273248
}
274249

@@ -316,11 +291,7 @@ export class ViewDescriptorService extends Disposable implements IViewDescriptor
316291
const from = this.getViewContainerLocation(viewContainer);
317292
const to = location;
318293
if (from !== to) {
319-
if (this.getDefaultViewContainerLocation(viewContainer) === to) {
320-
this.cachedViewContainerInfo.delete(viewContainer.id);
321-
} else {
322-
this.cachedViewContainerInfo.set(viewContainer.id, to);
323-
}
294+
this.cachedViewContainerInfo.set(viewContainer.id, to);
324295

325296
this._onDidChangeContainerLocation.fire({ viewContainer, from, to });
326297

@@ -407,14 +378,22 @@ export class ViewDescriptorService extends Disposable implements IViewDescriptor
407378
private registerGeneratedViewContainer(location: ViewContainerLocation, existingId?: string): ViewContainer {
408379
const id = existingId || this.generateContainerId(location);
409380

410-
return this.viewContainersRegistry.registerViewContainer({
381+
const container = this.viewContainersRegistry.registerViewContainer({
411382
id,
412383
ctorDescriptor: new SyncDescriptor(ViewPaneContainer, [id, { mergeViewWithContainerWhenSingleView: true, donotShowContainerTitleWhenMergedWithContainer: true }]),
413384
name: 'Custom Views', // we don't want to see this, so no need to localize
414385
icon: location === ViewContainerLocation.Sidebar ? 'codicon-window' : undefined,
415386
storageId: `${id}.state`,
416387
hideIfEmpty: true
417388
}, location);
389+
390+
const cachedInfo = this.cachedViewContainerInfo.get(container.id);
391+
if (cachedInfo !== location) {
392+
this.cachedViewContainerInfo.set(container.id, location);
393+
this.saveViewContainerLocationsToCache();
394+
}
395+
396+
return container;
418397
}
419398

420399
private getCachedViewPositions(): Map<string, ICachedViewContainerInfo> {
@@ -451,7 +430,10 @@ export class ViewDescriptorService extends Disposable implements IViewDescriptor
451430
const newViewContainerInfo = newCachedPositions.get(viewId)!;
452431
// Verify if we need to create the destination container
453432
if (!this.viewContainersRegistry.get(newViewContainerInfo.containerId)) {
454-
this.registerGeneratedViewContainer(newViewContainerInfo.location!, newViewContainerInfo.containerId);
433+
const location = this.cachedViewContainerInfo.get(newViewContainerInfo.containerId);
434+
if (location) {
435+
this.registerGeneratedViewContainer(location, newViewContainerInfo.containerId);
436+
}
455437
}
456438

457439
// Try moving to the new container
@@ -541,10 +523,8 @@ export class ViewDescriptorService extends Disposable implements IViewDescriptor
541523
this.viewContainers.forEach(viewContainer => {
542524
const viewContainerModel = this.getViewContainerModel(viewContainer);
543525
viewContainerModel.allViewDescriptors.forEach(viewDescriptor => {
544-
const containerLocation = this.getViewContainerLocation(viewContainer);
545526
this.cachedViewInfo.set(viewDescriptor.id, {
546-
containerId: viewContainer.id,
547-
location: containerLocation
527+
containerId: viewContainer.id
548528
});
549529
});
550530
});
@@ -565,7 +545,7 @@ export class ViewDescriptorService extends Disposable implements IViewDescriptor
565545
private saveViewContainerLocationsToCache(): void {
566546
for (const [containerId, location] of this.cachedViewContainerInfo) {
567547
const container = this.getViewContainerById(containerId);
568-
if (container && location === this.getDefaultViewContainerLocation(container)) {
548+
if (container && location === this.getDefaultViewContainerLocation(container) && !this.isGeneratedContainerId(containerId)) {
569549
this.cachedViewContainerInfo.delete(containerId);
570550
}
571551
}
@@ -613,7 +593,11 @@ export class ViewDescriptorService extends Disposable implements IViewDescriptor
613593

614594
this.viewContainerModels.set(viewContainer, { viewContainerModel: viewContainerModel, disposable: disposables });
615595

616-
const viewsToRegister = this.getViewsByContainer(viewContainer);
596+
// Register all views that were statically registered to this container
597+
this.onDidRegisterViews([{ views: this.viewsRegistry.getViews(viewContainer), viewContainer }]);
598+
599+
// Add views that were registered prior to this view container
600+
const viewsToRegister = this.getViewsByContainer(viewContainer).filter(view => this.getDefaultContainerById(view.id) !== viewContainer);
617601
if (viewsToRegister.length) {
618602
this.addViews(viewContainer, viewsToRegister);
619603
viewsToRegister.forEach(viewDescriptor => this.getOrCreateMovableViewContextKey(viewDescriptor).set(!!viewDescriptor.canMoveView));
@@ -638,9 +622,8 @@ export class ViewDescriptorService extends Disposable implements IViewDescriptor
638622

639623
private addViews(container: ViewContainer, views: IViewDescriptor[]): void {
640624
// Update in memory cache
641-
const location = this.getViewContainerLocation(container);
642625
views.forEach(view => {
643-
this.cachedViewInfo.set(view.id, { containerId: container.id, location });
626+
this.cachedViewInfo.set(view.id, { containerId: container.id });
644627
this.getOrCreateDefaultViewLocationContextKey(view).set(this.getDefaultContainerById(view.id) === container);
645628
});
646629

0 commit comments

Comments
 (0)