Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for view initial state (collapsed or hidden) #102002

Merged
merged 4 commits into from Jul 17, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions extensions/npm/package.json
Expand Up @@ -56,8 +56,9 @@
{
"id": "npm",
"name": "%view.name%",
"when": "npm:showScriptExplorer || config.npm.enableScriptExplorer",
"icon": "images/code.svg"
"when": "npm:showScriptExplorer",
"icon": "images/code.svg",
"visibility": "hidden"
}
]
},
Expand Down
38 changes: 36 additions & 2 deletions src/vs/workbench/api/browser/viewsExtensionPoint.ts
Expand Up @@ -83,12 +83,19 @@ interface IUserFriendlyViewDescriptor {

icon?: string;
contextualTitle?: string;
visibility?: string;

// From 'remoteViewDescriptor' type
group?: string;
remoteName?: string | string[];
}

enum InitialVisibility {
Visible = 'visible',
Hidden = 'hidden',
Collapsed = 'collapsed'
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you thought about making initialState an object type that can take visibility and collapsed states as two separate properties?

"hidden":  false,
"expanded": false

Allows to have a view that is visible and collapsed by default. This also aligns with existing default?

Copy link
Member Author

@alexr00 alexr00 Jul 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me. I would name them hidden and collapsed or visible and expanded, so that the default would be either all true or all false. Do you have a preference? (I will also take it to the API sync)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allows to have a view that is visible and collapsed by default.

The current implementation already allows this, just set the initialState to collapsed.

I thought of one concern with having visible and expanded separate:
The NPM view is not visible by default. The user goes to the ... menu and makes the NPM view visible. At this point, they have said that they want the view. The view should show up as visible and expanded, since they have just taken explicit action that they want the view. In what scenario would we want a view to to be hidden and collapsed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In what scenario would we want a view to to be hidden and collapsed?

Yeah good point. I do not think it is an useful combi to have hidden and collapsed by default. But other three combinations are useful.

hidden collapsed
true true
true false
false true
false false

Yeah, lets see what does API sync team suggest.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "hidden":true and "collapsed":false is also not meaningful, since the view is hidden, who cased whether it is collapsed?

At the API sync, an enum was preferred. The property would be called visibility and could be set to visible(default), hidden, or collapsed. @sandy081 what do you think of those options?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Liked it. 👍

const viewDescriptor: IJSONSchema = {
type: 'object',
properties: {
Expand All @@ -112,6 +119,20 @@ const viewDescriptor: IJSONSchema = {
description: localize('vscode.extension.contributes.view.contextualTitle', "Human-readable context for when the view is moved out of its original location. By default, the view's container name will be used. Will be shown"),
type: 'string'
},
visibility: {
description: localize('vscode.extension.contributes.view.initialState', "Initial state of the view when the extension is first installed. Once the user has changed the view state by collapsing, moving, or hiding the view, the initial state will not be used again."),
type: 'string',
enum: [
'visible',
'hidden',
'collapsed'
],
enumDescriptions: [
localize('vscode.extension.contributes.view.initialState.visible', "The default initial state for view. The view will be expanded. This may have different behavior when the view container that the view is in is built in."),
localize('vscode.extension.contributes.view.initialState.hidden', "The view will not be shown in the view container, but will be discoverable through the views menu and other view entry points and can be un-hidden by the user."),
localize('vscode.extension.contributes.view.initialState.collapsed', "The view will show in the view container, but will be collapsed.")
]
}
}
};

Expand Down Expand Up @@ -419,6 +440,7 @@ class ViewsExtensionHandler implements IWorkbenchContribution {
: undefined;

const icon = item.icon ? resources.joinPath(extension.description.extensionLocation, item.icon) : undefined;
const initialVisibility = this.convertInitialVisibility(item.visibility);
const viewDescriptor = <ICustomViewDescriptor>{
id: item.id,
name: item.name,
Expand All @@ -429,12 +451,13 @@ class ViewsExtensionHandler implements IWorkbenchContribution {
canToggleVisibility: true,
canMoveView: true,
treeView: this.instantiationService.createInstance(CustomTreeView, item.id, item.name),
collapsed: this.showCollapsed(container),
collapsed: this.showCollapsed(container) || initialVisibility === InitialVisibility.Collapsed,
order: order,
extensionId: extension.description.identifier,
originalContainerId: entry.key,
group: item.group,
remoteAuthority: item.remoteName || (<any>item).remoteAuthority // TODO@roblou - delete after remote extensions are updated
remoteAuthority: item.remoteName || (<any>item).remoteAuthority, // TODO@roblou - delete after remote extensions are updated
hideByDefault: initialVisibility === InitialVisibility.Hidden
};

viewIds.add(viewDescriptor.id);
Expand Down Expand Up @@ -463,6 +486,13 @@ class ViewsExtensionHandler implements IWorkbenchContribution {
}
}

private convertInitialVisibility(value: any): InitialVisibility | undefined {
if (Object.values(InitialVisibility).includes(value)) {
return value;
}
return undefined;
}

private isValidViewDescriptors(viewDescriptors: IUserFriendlyViewDescriptor[], collector: ExtensionMessageCollector): boolean {
if (!Array.isArray(viewDescriptors)) {
collector.error(localize('requirearray', "views must be an array"));
Expand Down Expand Up @@ -490,6 +520,10 @@ class ViewsExtensionHandler implements IWorkbenchContribution {
collector.error(localize('optstring', "property `{0}` can be omitted or must be of type `string`", 'contextualTitle'));
return false;
}
if (descriptor.visibility && !this.convertInitialVisibility(descriptor.visibility)) {
collector.error(localize('optenum', "property `{0}` can be omitted or must be one of {1}", 'visibility', Object.values(InitialVisibility).join(', ')));
return false;
}
}

return true;
Expand Down
2 changes: 2 additions & 0 deletions src/vs/workbench/browser/parts/views/viewPaneContainer.ts
Expand Up @@ -982,6 +982,8 @@ export class ViewPaneContainer extends Component implements IViewPaneContainer {
this.updateViewHeaders();
}
});

this._register(this.viewContainerModel.onDidChangeActiveViewDescriptors(() => this._onTitleAreaUpdate.fire()));
sandy081 marked this conversation as resolved.
Show resolved Hide resolved
}

getTitle(): string {
Expand Down
2 changes: 1 addition & 1 deletion src/vs/workbench/browser/viewlet.ts
Expand Up @@ -45,7 +45,7 @@ export abstract class Viewlet extends PaneComposite implements IViewlet {
@IConfigurationService protected configurationService: IConfigurationService
) {
super(id, viewPaneContainer, telemetryService, storageService, instantiationService, themeService, contextMenuService, extensionService, contextService);
this._register(Event.any(viewPaneContainer.onDidAddViews, viewPaneContainer.onDidRemoveViews)(() => {
this._register(Event.any(viewPaneContainer.onDidAddViews, viewPaneContainer.onDidRemoveViews, viewPaneContainer.onTitleAreaUpdate)(() => {
sandy081 marked this conversation as resolved.
Show resolved Hide resolved
// Update title area since there is no better way to update secondary actions
this.updateTitleArea();
}));
Expand Down