#96064 Fix model and view

- check for active view descriptors
- add model tests

src/vs/workbench/browser/panecomposite.ts

@@ -59,8 +59,8 @@ export class PaneComposite extends Composite implements IPaneComposite {
 		return this.viewPaneContainer.getOptimalWidth();
 	}
 
-	openView(id: string, focus?: boolean): IView {
-		return this.viewPaneContainer.openView(id, focus);
+	openView<T extends IView>(id: string, focus?: boolean): T | undefined {
+		return this.viewPaneContainer.openView(id, focus) as T;
 	}
 
 	getViewPaneContainer(): ViewPaneContainer {

src/vs/workbench/browser/parts/views/viewPaneContainer.ts

@@ -1140,15 +1140,17 @@ export class ViewPaneContainer extends Component implements IViewPaneContainer {
 		});
 	}
 
-	openView(id: string, focus?: boolean): IView {
+	openView(id: string, focus?: boolean): IView | undefined {
 		let view = this.getView(id);
 		if (!view) {
 			this.toggleViewVisibility(id);
 		}
-		view = this.getView(id)!;
-		view.setExpanded(true);
-		if (focus) {
-			view.focus();
+		view = this.getView(id);
+		if (view) {
+			view.setExpanded(true);
+			if (focus) {
+				view.focus();
+			}
 		}
 		return view;
 	}
@@ -1202,13 +1204,16 @@ export class ViewPaneContainer extends Component implements IViewPaneContainer {
 	}
 
 	protected toggleViewVisibility(viewId: string): void {
-		const visible = !this.viewContainerModel.isVisible(viewId);
-		type ViewsToggleVisibilityClassification = {
-			viewId: { classification: 'SystemMetaData', purpose: 'FeatureInsight' };
-			visible: { classification: 'SystemMetaData', purpose: 'FeatureInsight' };
-		};
-		this.telemetryService.publicLog2<{ viewId: String, visible: boolean }, ViewsToggleVisibilityClassification>('views.toggleVisibility', { viewId, visible });
-		this.viewContainerModel.setVisible(viewId, visible);
+		// Check if view is active
+		if (this.viewContainerModel.activeViewDescriptors.some(viewDescriptor => viewDescriptor.id === viewId)) {
+			const visible = !this.viewContainerModel.isVisible(viewId);
+			type ViewsToggleVisibilityClassification = {
+				viewId: { classification: 'SystemMetaData', purpose: 'FeatureInsight' };
+				visible: { classification: 'SystemMetaData', purpose: 'FeatureInsight' };
+			};
+			this.telemetryService.publicLog2<{ viewId: String, visible: boolean }, ViewsToggleVisibilityClassification>('views.toggleVisibility', { viewId, visible });
+			this.viewContainerModel.setVisible(viewId, visible);
+		}
 	}
 
 	private addPane(pane: ViewPane, size: number, index = this.paneItems.length - 1): void {

src/vs/workbench/browser/parts/views/views.ts

@@ -244,16 +244,22 @@ export class ViewsService extends Disposable implements IViewsService {
 
 	async openView<T extends IView>(id: string, focus: boolean): Promise<T | null> {
 		const viewContainer = this.viewDescriptorService.getViewContainerByViewId(id);
-		if (viewContainer) {
-			const location = this.viewDescriptorService.getViewContainerLocation(viewContainer);
-			const compositeDescriptor = this.getComposite(viewContainer.id, location!);
-			if (compositeDescriptor) {
-				const paneComposite = await this.openComposite(compositeDescriptor.id, location!) as IPaneComposite | undefined;
-				if (paneComposite && paneComposite.openView) {
-					return paneComposite.openView(id, focus) as T;
-				} else if (focus) {
-					paneComposite?.focus();
-				}
+		if (!viewContainer) {
+			return null;
+		}
+
+		if (!this.viewDescriptorService.getViewContainerModel(viewContainer).activeViewDescriptors.some(viewDescriptor => viewDescriptor.id === id)) {
+			return null;
+		}
+
+		const location = this.viewDescriptorService.getViewContainerLocation(viewContainer);
+		const compositeDescriptor = this.getComposite(viewContainer.id, location!);
+		if (compositeDescriptor) {
+			const paneComposite = await this.openComposite(compositeDescriptor.id, location!) as IPaneComposite | undefined;
+			if (paneComposite && paneComposite.openView) {
+				return paneComposite.openView<T>(id, focus) || null;
+			} else if (focus) {
+				paneComposite?.focus();
 			}
 		}
 

src/vs/workbench/common/panecomposite.ts

@@ -7,7 +7,7 @@ import { IView, IViewPaneContainer } from 'vs/workbench/common/views';
 import { IComposite } from 'vs/workbench/common/composite';
 
 export interface IPaneComposite extends IComposite {
-	openView(id: string, focus?: boolean): IView;
+	openView<T extends IView>(id: string, focus?: boolean): T | undefined;
 	getViewPaneContainer(): IViewPaneContainer;
 	saveState(): void;
 }

src/vs/workbench/common/views.ts

@@ -8,7 +8,6 @@ import { UriComponents, URI } from 'vs/base/common/uri';
 import { Event, Emitter } from 'vs/base/common/event';
 import { RawContextKey, ContextKeyExpression } from 'vs/platform/contextkey/common/contextkey';
 import { localize } from 'vs/nls';
-import { IViewlet } from 'vs/workbench/common/viewlet';
 import { createDecorator } from 'vs/platform/instantiation/common/instantiation';
 import { IDisposable, Disposable, toDisposable } from 'vs/base/common/lifecycle';
 import { ThemeIcon } from 'vs/platform/theme/common/themeService';
@@ -443,12 +442,6 @@ export interface IView {
 	getProgressIndicator(): IProgressIndicator | undefined;
 }
 
-export interface IViewsViewlet extends IViewlet {
-
-	openView(id: string, focus?: boolean): IView;
-
-}
-
 export const IViewsService = createDecorator<IViewsService>('viewsService');
 
 export interface IViewsService {

src/vs/workbench/services/progress/test/browser/progressIndicator.test.ts

@@ -30,7 +30,7 @@ class TestViewlet implements IViewlet {
 	getControl(): IEditorControl { return null!; }
 	focus(): void { }
 	getOptimalWidth(): number { return 10; }
-	openView(id: string, focus?: boolean): IView { return null!; }
+	openView<T extends IView>(id: string, focus?: boolean): T | undefined { return undefined; }
 	getViewPaneContainer(): IViewPaneContainer { return null!; }
 	saveState(): void { }
 }

src/vs/workbench/services/views/common/viewContainerModel.ts

@@ -384,8 +384,8 @@ export class ViewContainerModel extends Disposable implements IViewContainerMode
 				throw new Error(`Can't toggle this view's visibility`);
 			}
 
-			if (this.isViewDescriptorVisible(viewDescriptorItem) === visible) {
-				return;
+			if (this.isViewDescriptorVisibleWhenActive(viewDescriptorItem) === visible) {
+				continue;
 			}
 
 			if (viewDescriptor.workspace) {
@@ -398,6 +398,10 @@ export class ViewContainerModel extends Disposable implements IViewContainerMode
 				viewDescriptorItem.state.size = size;
 			}
 
+			if (this.isViewDescriptorVisible(viewDescriptorItem) !== visible) {
+				continue;
+			}
+
 			if (visible) {
 				added.push({ index: visibleIndex, viewDescriptor, size: viewDescriptorItem.state.size, collapsed: !!viewDescriptorItem.state.collapsed });
 			} else {

src/vs/workbench/services/views/test/browser/viewContainerModel.test.ts

@@ -382,4 +382,86 @@ suite('ViewContainerModel', () => {
 		assert.ok(!targetEvent.called, 'remove event should not be called since it is already hidden');
 	});
 
+	test('add event is not triggered if view was set visible (when visible) and not active', async function () {
+		container = ViewContainerRegistry.registerViewContainer({ id: 'test', name: 'test', ctorDescriptor: new SyncDescriptor(<any>{}) }, ViewContainerLocation.Sidebar);
+		const testObject = viewDescriptorService.getViewContainerModel(container);
+		const target = disposableStore.add(new ViewDescriptorSequence(testObject));
+		const viewDescriptor: IViewDescriptor = {
+			id: 'view1',
+			ctorDescriptor: null!,
+			name: 'Test View 1',
+			when: ContextKeyExpr.equals('showview1', true),
+			canToggleVisibility: true
+		};
+
+		const key = contextKeyService.createKey('showview1', true);
+		key.set(false);
+		ViewsRegistry.registerViews([viewDescriptor], container);
+
+		assert.equal(testObject.visibleViewDescriptors.length, 0);
+		assert.equal(target.elements.length, 0);
+
+		const targetEvent = sinon.spy(testObject.onDidAddVisibleViewDescriptors);
+		testObject.setVisible('view1', true);
+		assert.ok(!targetEvent.called, 'add event should not be called since it is already visible');
+		assert.equal(testObject.visibleViewDescriptors.length, 0);
+		assert.equal(target.elements.length, 0);
+	});
+
+	test('remove event is not triggered if view was hidden and not active', async function () {
+		container = ViewContainerRegistry.registerViewContainer({ id: 'test', name: 'test', ctorDescriptor: new SyncDescriptor(<any>{}) }, ViewContainerLocation.Sidebar);
+		const testObject = viewDescriptorService.getViewContainerModel(container);
+		const target = disposableStore.add(new ViewDescriptorSequence(testObject));
+		const viewDescriptor: IViewDescriptor = {
+			id: 'view1',
+			ctorDescriptor: null!,
+			name: 'Test View 1',
+			when: ContextKeyExpr.equals('showview1', true),
+			canToggleVisibility: true
+		};
+
+		const key = contextKeyService.createKey('showview1', true);
+		key.set(false);
+		ViewsRegistry.registerViews([viewDescriptor], container);
+
+		assert.equal(testObject.visibleViewDescriptors.length, 0);
+		assert.equal(target.elements.length, 0);
+
+		const targetEvent = sinon.spy(testObject.onDidAddVisibleViewDescriptors);
+		testObject.setVisible('view1', false);
+		assert.ok(!targetEvent.called, 'add event should not be called since it is disabled');
+		assert.equal(testObject.visibleViewDescriptors.length, 0);
+		assert.equal(target.elements.length, 0);
+	});
+
+	test('add event is not triggered if view was set visible (when not visible) and not active', async function () {
+		container = ViewContainerRegistry.registerViewContainer({ id: 'test', name: 'test', ctorDescriptor: new SyncDescriptor(<any>{}) }, ViewContainerLocation.Sidebar);
+		const testObject = viewDescriptorService.getViewContainerModel(container);
+		const target = disposableStore.add(new ViewDescriptorSequence(testObject));
+		const viewDescriptor: IViewDescriptor = {
+			id: 'view1',
+			ctorDescriptor: null!,
+			name: 'Test View 1',
+			when: ContextKeyExpr.equals('showview1', true),
+			canToggleVisibility: true
+		};
+
+		const key = contextKeyService.createKey('showview1', true);
+		key.set(false);
+		ViewsRegistry.registerViews([viewDescriptor], container);
+
+		assert.equal(testObject.visibleViewDescriptors.length, 0);
+		assert.equal(target.elements.length, 0);
+
+		testObject.setVisible('view1', false);
+		assert.equal(testObject.visibleViewDescriptors.length, 0);
+		assert.equal(target.elements.length, 0);
+
+		const targetEvent = sinon.spy(testObject.onDidAddVisibleViewDescriptors);
+		testObject.setVisible('view1', true);
+		assert.ok(!targetEvent.called, 'add event should not be called since it is disabled');
+		assert.equal(testObject.visibleViewDescriptors.length, 0);
+		assert.equal(target.elements.length, 0);
+	});
+
 });