Skip to content

Commit ece3baa

Browse files
sandy081sandy081
committed
microsoft#96064 Fix model and view
- check for active view descriptors - add model tests
1 parent 616882b commit ece3baa

8 files changed

Lines changed: 125 additions & 35 deletions

File tree

‎src/vs/workbench/browser/panecomposite.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,8 +59,8 @@ export class PaneComposite extends Composite implements IPaneComposite {
5959
return this.viewPaneContainer.getOptimalWidth();
6060
}
6161

62-
openView(id: string, focus?: boolean): IView {
63-
return this.viewPaneContainer.openView(id, focus);
62+
openView<T extends IView>(id: string, focus?: boolean): T | undefined {
63+
return this.viewPaneContainer.openView(id, focus) as T;
6464
}
6565

6666
getViewPaneContainer(): ViewPaneContainer {

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

Lines changed: 17 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1140,15 +1140,17 @@ export class ViewPaneContainer extends Component implements IViewPaneContainer {
11401140
});
11411141
}
11421142

1143-
openView(id: string, focus?: boolean): IView {
1143+
openView(id: string, focus?: boolean): IView | undefined {
11441144
let view = this.getView(id);
11451145
if (!view) {
11461146
this.toggleViewVisibility(id);
11471147
}
1148-
view = this.getView(id)!;
1149-
view.setExpanded(true);
1150-
if (focus) {
1151-
view.focus();
1148+
view = this.getView(id);
1149+
if (view) {
1150+
view.setExpanded(true);
1151+
if (focus) {
1152+
view.focus();
1153+
}
11521154
}
11531155
return view;
11541156
}
@@ -1202,13 +1204,16 @@ export class ViewPaneContainer extends Component implements IViewPaneContainer {
12021204
}
12031205

12041206
protected toggleViewVisibility(viewId: string): void {
1205-
const visible = !this.viewContainerModel.isVisible(viewId);
1206-
type ViewsToggleVisibilityClassification = {
1207-
viewId: { classification: 'SystemMetaData', purpose: 'FeatureInsight' };
1208-
visible: { classification: 'SystemMetaData', purpose: 'FeatureInsight' };
1209-
};
1210-
this.telemetryService.publicLog2<{ viewId: String, visible: boolean }, ViewsToggleVisibilityClassification>('views.toggleVisibility', { viewId, visible });
1211-
this.viewContainerModel.setVisible(viewId, visible);
1207+
// Check if view is active
1208+
if (this.viewContainerModel.activeViewDescriptors.some(viewDescriptor => viewDescriptor.id === viewId)) {
1209+
const visible = !this.viewContainerModel.isVisible(viewId);
1210+
type ViewsToggleVisibilityClassification = {
1211+
viewId: { classification: 'SystemMetaData', purpose: 'FeatureInsight' };
1212+
visible: { classification: 'SystemMetaData', purpose: 'FeatureInsight' };
1213+
};
1214+
this.telemetryService.publicLog2<{ viewId: String, visible: boolean }, ViewsToggleVisibilityClassification>('views.toggleVisibility', { viewId, visible });
1215+
this.viewContainerModel.setVisible(viewId, visible);
1216+
}
12121217
}
12131218

12141219
private addPane(pane: ViewPane, size: number, index = this.paneItems.length - 1): void {

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

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -244,16 +244,22 @@ export class ViewsService extends Disposable implements IViewsService {
244244

245245
async openView<T extends IView>(id: string, focus: boolean): Promise<T | null> {
246246
const viewContainer = this.viewDescriptorService.getViewContainerByViewId(id);
247-
if (viewContainer) {
248-
const location = this.viewDescriptorService.getViewContainerLocation(viewContainer);
249-
const compositeDescriptor = this.getComposite(viewContainer.id, location!);
250-
if (compositeDescriptor) {
251-
const paneComposite = await this.openComposite(compositeDescriptor.id, location!) as IPaneComposite | undefined;
252-
if (paneComposite && paneComposite.openView) {
253-
return paneComposite.openView(id, focus) as T;
254-
} else if (focus) {
255-
paneComposite?.focus();
256-
}
247+
if (!viewContainer) {
248+
return null;
249+
}
250+
251+
if (!this.viewDescriptorService.getViewContainerModel(viewContainer).activeViewDescriptors.some(viewDescriptor => viewDescriptor.id === id)) {
252+
return null;
253+
}
254+
255+
const location = this.viewDescriptorService.getViewContainerLocation(viewContainer);
256+
const compositeDescriptor = this.getComposite(viewContainer.id, location!);
257+
if (compositeDescriptor) {
258+
const paneComposite = await this.openComposite(compositeDescriptor.id, location!) as IPaneComposite | undefined;
259+
if (paneComposite && paneComposite.openView) {
260+
return paneComposite.openView<T>(id, focus) || null;
261+
} else if (focus) {
262+
paneComposite?.focus();
257263
}
258264
}
259265

‎src/vs/workbench/common/panecomposite.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import { IView, IViewPaneContainer } from 'vs/workbench/common/views';
77
import { IComposite } from 'vs/workbench/common/composite';
88

99
export interface IPaneComposite extends IComposite {
10-
openView(id: string, focus?: boolean): IView;
10+
openView<T extends IView>(id: string, focus?: boolean): T | undefined;
1111
getViewPaneContainer(): IViewPaneContainer;
1212
saveState(): void;
1313
}

‎src/vs/workbench/common/views.ts‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ import { UriComponents, URI } from 'vs/base/common/uri';
88
import { Event, Emitter } from 'vs/base/common/event';
99
import { RawContextKey, ContextKeyExpression } from 'vs/platform/contextkey/common/contextkey';
1010
import { localize } from 'vs/nls';
11-
import { IViewlet } from 'vs/workbench/common/viewlet';
1211
import { createDecorator } from 'vs/platform/instantiation/common/instantiation';
1312
import { IDisposable, Disposable, toDisposable } from 'vs/base/common/lifecycle';
1413
import { ThemeIcon } from 'vs/platform/theme/common/themeService';
@@ -443,12 +442,6 @@ export interface IView {
443442
getProgressIndicator(): IProgressIndicator | undefined;
444443
}
445444

446-
export interface IViewsViewlet extends IViewlet {
447-
448-
openView(id: string, focus?: boolean): IView;
449-
450-
}
451-
452445
export const IViewsService = createDecorator<IViewsService>('viewsService');
453446

454447
export interface IViewsService {

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ class TestViewlet implements IViewlet {
3030
getControl(): IEditorControl { return null!; }
3131
focus(): void { }
3232
getOptimalWidth(): number { return 10; }
33-
openView(id: string, focus?: boolean): IView { return null!; }
33+
openView<T extends IView>(id: string, focus?: boolean): T | undefined { return undefined; }
3434
getViewPaneContainer(): IViewPaneContainer { return null!; }
3535
saveState(): void { }
3636
}

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

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -384,8 +384,8 @@ export class ViewContainerModel extends Disposable implements IViewContainerMode
384384
throw new Error(`Can't toggle this view's visibility`);
385385
}
386386

387-
if (this.isViewDescriptorVisible(viewDescriptorItem) === visible) {
388-
return;
387+
if (this.isViewDescriptorVisibleWhenActive(viewDescriptorItem) === visible) {
388+
continue;
389389
}
390390

391391
if (viewDescriptor.workspace) {
@@ -398,6 +398,10 @@ export class ViewContainerModel extends Disposable implements IViewContainerMode
398398
viewDescriptorItem.state.size = size;
399399
}
400400

401+
if (this.isViewDescriptorVisible(viewDescriptorItem) !== visible) {
402+
continue;
403+
}
404+
401405
if (visible) {
402406
added.push({ index: visibleIndex, viewDescriptor, size: viewDescriptorItem.state.size, collapsed: !!viewDescriptorItem.state.collapsed });
403407
} else {

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

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -382,4 +382,86 @@ suite('ViewContainerModel', () => {
382382
assert.ok(!targetEvent.called, 'remove event should not be called since it is already hidden');
383383
});
384384

385+
test('add event is not triggered if view was set visible (when visible) and not active', async function () {
386+
container = ViewContainerRegistry.registerViewContainer({ id: 'test', name: 'test', ctorDescriptor: new SyncDescriptor(<any>{}) }, ViewContainerLocation.Sidebar);
387+
const testObject = viewDescriptorService.getViewContainerModel(container);
388+
const target = disposableStore.add(new ViewDescriptorSequence(testObject));
389+
const viewDescriptor: IViewDescriptor = {
390+
id: 'view1',
391+
ctorDescriptor: null!,
392+
name: 'Test View 1',
393+
when: ContextKeyExpr.equals('showview1', true),
394+
canToggleVisibility: true
395+
};
396+
397+
const key = contextKeyService.createKey('showview1', true);
398+
key.set(false);
399+
ViewsRegistry.registerViews([viewDescriptor], container);
400+
401+
assert.equal(testObject.visibleViewDescriptors.length, 0);
402+
assert.equal(target.elements.length, 0);
403+
404+
const targetEvent = sinon.spy(testObject.onDidAddVisibleViewDescriptors);
405+
testObject.setVisible('view1', true);
406+
assert.ok(!targetEvent.called, 'add event should not be called since it is already visible');
407+
assert.equal(testObject.visibleViewDescriptors.length, 0);
408+
assert.equal(target.elements.length, 0);
409+
});
410+
411+
test('remove event is not triggered if view was hidden and not active', async function () {
412+
container = ViewContainerRegistry.registerViewContainer({ id: 'test', name: 'test', ctorDescriptor: new SyncDescriptor(<any>{}) }, ViewContainerLocation.Sidebar);
413+
const testObject = viewDescriptorService.getViewContainerModel(container);
414+
const target = disposableStore.add(new ViewDescriptorSequence(testObject));
415+
const viewDescriptor: IViewDescriptor = {
416+
id: 'view1',
417+
ctorDescriptor: null!,
418+
name: 'Test View 1',
419+
when: ContextKeyExpr.equals('showview1', true),
420+
canToggleVisibility: true
421+
};
422+
423+
const key = contextKeyService.createKey('showview1', true);
424+
key.set(false);
425+
ViewsRegistry.registerViews([viewDescriptor], container);
426+
427+
assert.equal(testObject.visibleViewDescriptors.length, 0);
428+
assert.equal(target.elements.length, 0);
429+
430+
const targetEvent = sinon.spy(testObject.onDidAddVisibleViewDescriptors);
431+
testObject.setVisible('view1', false);
432+
assert.ok(!targetEvent.called, 'add event should not be called since it is disabled');
433+
assert.equal(testObject.visibleViewDescriptors.length, 0);
434+
assert.equal(target.elements.length, 0);
435+
});
436+
437+
test('add event is not triggered if view was set visible (when not visible) and not active', async function () {
438+
container = ViewContainerRegistry.registerViewContainer({ id: 'test', name: 'test', ctorDescriptor: new SyncDescriptor(<any>{}) }, ViewContainerLocation.Sidebar);
439+
const testObject = viewDescriptorService.getViewContainerModel(container);
440+
const target = disposableStore.add(new ViewDescriptorSequence(testObject));
441+
const viewDescriptor: IViewDescriptor = {
442+
id: 'view1',
443+
ctorDescriptor: null!,
444+
name: 'Test View 1',
445+
when: ContextKeyExpr.equals('showview1', true),
446+
canToggleVisibility: true
447+
};
448+
449+
const key = contextKeyService.createKey('showview1', true);
450+
key.set(false);
451+
ViewsRegistry.registerViews([viewDescriptor], container);
452+
453+
assert.equal(testObject.visibleViewDescriptors.length, 0);
454+
assert.equal(target.elements.length, 0);
455+
456+
testObject.setVisible('view1', false);
457+
assert.equal(testObject.visibleViewDescriptors.length, 0);
458+
assert.equal(target.elements.length, 0);
459+
460+
const targetEvent = sinon.spy(testObject.onDidAddVisibleViewDescriptors);
461+
testObject.setVisible('view1', true);
462+
assert.ok(!targetEvent.called, 'add event should not be called since it is disabled');
463+
assert.equal(testObject.visibleViewDescriptors.length, 0);
464+
assert.equal(target.elements.length, 0);
465+
});
466+
385467
});

0 commit comments

Comments
 (0)