diff --git a/src/vs/workbench/browser/panecomposite.ts b/src/vs/workbench/browser/panecomposite.ts index 341355e0efd546220c046e17167cd4bf61b51ebd..34de2b1a02c8a91d06d6a9ce2d8ef9da04cd15b4 100644 --- a/src/vs/workbench/browser/panecomposite.ts +++ b/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(id: string, focus?: boolean): T | undefined { + return this.viewPaneContainer.openView(id, focus) as T; } getViewPaneContainer(): ViewPaneContainer { diff --git a/src/vs/workbench/browser/parts/views/viewPaneContainer.ts b/src/vs/workbench/browser/parts/views/viewPaneContainer.ts index 57c20448adca4bcecccc4f435511afcbe88064b8..0430e16da41bdc510b6329f244633fc3456bb89a 100644 --- a/src/vs/workbench/browser/parts/views/viewPaneContainer.ts +++ b/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 { diff --git a/src/vs/workbench/browser/parts/views/views.ts b/src/vs/workbench/browser/parts/views/views.ts index 01424335718336e99d4b0e0c141cf598222a5ef6..2dd7417f4943ff6a9135f2a91d95e721a29b9e25 100644 --- a/src/vs/workbench/browser/parts/views/views.ts +++ b/src/vs/workbench/browser/parts/views/views.ts @@ -244,16 +244,22 @@ export class ViewsService extends Disposable implements IViewsService { async openView(id: string, focus: boolean): Promise { 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(id, focus) || null; + } else if (focus) { + paneComposite?.focus(); } } diff --git a/src/vs/workbench/common/panecomposite.ts b/src/vs/workbench/common/panecomposite.ts index e33f240f14780e27ebd7f70ad0236184737f8cd1..89ba70273052833d8eea3468bc2540ee36185674 100644 --- a/src/vs/workbench/common/panecomposite.ts +++ b/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(id: string, focus?: boolean): T | undefined; getViewPaneContainer(): IViewPaneContainer; saveState(): void; } diff --git a/src/vs/workbench/common/views.ts b/src/vs/workbench/common/views.ts index 8f4c462da5b800681300781e2a200b02773aa787..4a80422e378154258a168a9b49c693e8df710fe6 100644 --- a/src/vs/workbench/common/views.ts +++ b/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('viewsService'); export interface IViewsService { diff --git a/src/vs/workbench/services/progress/test/browser/progressIndicator.test.ts b/src/vs/workbench/services/progress/test/browser/progressIndicator.test.ts index 6893e8114cd0ad0bf7dffad955bf6c30c75dabbc..cdc53cda42d77bc850d57dd6fdfe233218f1adac 100644 --- a/src/vs/workbench/services/progress/test/browser/progressIndicator.test.ts +++ b/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(id: string, focus?: boolean): T | undefined { return undefined; } getViewPaneContainer(): IViewPaneContainer { return null!; } saveState(): void { } } diff --git a/src/vs/workbench/services/views/common/viewContainerModel.ts b/src/vs/workbench/services/views/common/viewContainerModel.ts index d24fd96abea0304b81e16c7e43401acc2c5d97e0..a6a3275a4c7d7707e3e097abc1379d28baf7aa7a 100644 --- a/src/vs/workbench/services/views/common/viewContainerModel.ts +++ b/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 { diff --git a/src/vs/workbench/services/views/test/browser/viewContainerModel.test.ts b/src/vs/workbench/services/views/test/browser/viewContainerModel.test.ts index e6d4fd83eab9fd212f5c9c304ced970c0b0db678..cf5401adde59e7baae9b4136793192203046d7de 100644 --- a/src/vs/workbench/services/views/test/browser/viewContainerModel.test.ts +++ b/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({}) }, 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({}) }, 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({}) }, 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); + }); + });