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

Declarative contribution of custom Tree Explorer #14048

Merged
merged 126 commits into from
Nov 10, 2016
Merged
Show file tree
Hide file tree
Changes from 75 commits
Commits
Show all changes
126 commits
Select commit Hold shift + click to select a range
2657a9d
Add contributes.explorer section and add it to ViewletRegistry
octref Sep 26, 2016
aa1ffcd
Add icon to activity bar (loaded separately than stock Viewlets)
octref Sep 26, 2016
c2bdd6a
Refactor and put id as property of TreeContentProvider
octref Sep 28, 2016
d03ea81
Make TreeViewlet render correctly
octref Sep 30, 2016
d8c0af7
Hook up provider mechanism and add onClick
octref Sep 30, 2016
5276b19
Clean up
octref Oct 3, 2016
8081e2e
async loading children
octref Oct 3, 2016
10af4d7
Add click behavior to Controller
octref Oct 4, 2016
6f4de8b
Remove isChildrenResolved from ITreeNode
octref Oct 4, 2016
61639c0
Clean up
octref Oct 5, 2016
c8bbaaa
Remove JSON as ITreeNode is all primitives
octref Oct 5, 2016
f364538
Reduce calls to refresh tree to one call
octref Oct 5, 2016
154cdf9
Keep a local tree at extHostExplorer side
octref Oct 5, 2016
ec1cc0d
Better naming and some doc
octref Oct 6, 2016
e88e3f0
More consistent naming and clean up random strings
octref Oct 6, 2016
373829a
Allow tree label to be set
octref Oct 6, 2016
7f5e03f
Avoid same order for multiple viewlets
octref Oct 6, 2016
230ac46
Allow icon to be set via explorer contribution
octref Oct 6, 2016
eced815
Descriptor id needs to correspond to Viewlet's
octref Oct 7, 2016
8e9982c
isExpanded -> shouldInitiallyExpand
octref Oct 7, 2016
fb032eb
Remove unused go outline stuff
octref Oct 7, 2016
29c03ea
Remove children on ExtHost side and refactor
octref Oct 11, 2016
f366857
Completely remove children
octref Oct 11, 2016
09703d2
Add initial onClickCommand
octref Oct 11, 2016
c7f2d3d
Resolving onClickCommand and clean up
octref Oct 11, 2016
30e3be2
Rename treeContentProviderId to treeExplorerNodeProviderId
octref Oct 12, 2016
b8ce9ad
Make nodeProvider generic and add contentProvider
octref Oct 13, 2016
668aa65
onClickCommand -> clickCommand
octref Oct 13, 2016
f660ca6
Update API
octref Oct 13, 2016
f659fdd
Add actions
octref Oct 13, 2016
6bb8278
Hook up hasChildren
octref Oct 14, 2016
b474369
Give default value to label, hasChildren, etc
octref Oct 14, 2016
0cc3fec
Placeholder for toggle external actions
octref Oct 14, 2016
c24338d
Fix API
octref Oct 14, 2016
af5af72
Accomondate new non-promise provideRootNode etc
octref Oct 14, 2016
6300fbf
Remove unused svg files
octref Oct 17, 2016
13d5690
Add license
octref Oct 17, 2016
4eab0df
Split explorer's extension point to a separate file
octref Oct 17, 2016
944101c
Let ViewletRegistry handle batch registration and notify ActivityBarPart
octref Oct 17, 2016
1c28702
Read config from global storage
octref Oct 17, 2016
940a7fa
Only display whitelisted viewlet icons on activity bar
octref Oct 18, 2016
2e0f8d0
Populate command with enabled/disabled viewlets
octref Oct 18, 2016
1364159
Retrieve list of registered viewletes to populate toggle custom explo…
octref Oct 18, 2016
88e869a
Refactor activitybar and clean up
octref Oct 18, 2016
0031078
Clean up
octref Oct 18, 2016
0471662
Add back pre-commit hygiene
octref Oct 18, 2016
5b1ba3c
Fix lint error
octref Oct 18, 2016
31afc03
Clean up format issues
octref Oct 18, 2016
09eebc5
Use providerId instead of 'pineTree'!
octref Oct 19, 2016
857411d
Make naming consistent
octref Oct 19, 2016
0c18979
Handle errors caused by provider
octref Oct 19, 2016
045aedd
ContentProvider -> TreeExplorerNodeProvider
octref Oct 19, 2016
f19446e
Avoid re-rendering the tree when icon is clicked
octref Oct 19, 2016
ec7f077
Naming
octref Oct 19, 2016
7750596
Remove shouldInitiallyExpand
octref Oct 19, 2016
c49e7bc
Fix active viewlet highlighting and double click hide for sidebar
octref Oct 19, 2016
7642976
Open file explorer if last viewlet before close is external
octref Oct 19, 2016
b654b53
Update doc
octref Oct 19, 2016
4cc76f5
Remember order of external viewlets
octref Oct 20, 2016
16dd5f9
Clean up
octref Oct 20, 2016
313df77
Polish doc language
octref Oct 20, 2016
b141507
Minor clean up
octref Oct 20, 2016
6fd1c54
Move VIEWLET_ID_ROOT to one place
octref Oct 20, 2016
267aab6
Correct ID for toggle action
octref Oct 20, 2016
6e868d6
Add refresh button to toolBar
octref Oct 20, 2016
a5adf50
Always preserve stock viewlets order
octref Oct 20, 2016
d579cb8
Hook refresh button to update input
octref Oct 20, 2016
0b84a56
Consistent naming
octref Oct 21, 2016
37398ea
Handle all error by showing message
octref Oct 21, 2016
bc573a7
Let user contributed explorer icon be a string
octref Oct 21, 2016
0e9aecd
Clarify some code
octref Oct 21, 2016
60574ba
Add progress indication for long running resolveChildren
octref Oct 21, 2016
a21ac50
Fix use before initialization
octref Oct 21, 2016
39ecfd3
Restore external viewlet on startup
octref Oct 21, 2016
e2f4a6b
Coerce external viewlet icons into style similar to stock ones
octref Oct 21, 2016
794ad90
Address feedback
octref Oct 24, 2016
8f777ab
Merge remote-tracking branch 'origin/master' into treeExplorerAPI
octref Oct 24, 2016
4ed359a
Update dts doc
octref Oct 24, 2016
a3d577c
Address feedback
octref Oct 24, 2016
a196d7f
Move static css rules to css file
octref Oct 24, 2016
70be684
Use treeLabel in toggle command
octref Oct 24, 2016
f5d4ede
Merge remote-tracking branch 'origin/master' into treeExplorerAPI
octref Oct 31, 2016
09c11fe
Move API to proposed dts
octref Oct 31, 2016
f18fa6e
Fix build error
octref Oct 31, 2016
c476937
Set registerTreeExplorerNodeProvider as proposed API
octref Nov 1, 2016
31960a7
Move error handling to main side
octref Nov 1, 2016
84dd43b
Execute onClickCommand on Main side
octref Nov 1, 2016
a52cfbb
Move explorerExtensionPoint from platform to workbench
octref Nov 1, 2016
d153d96
Display external viewlets on all extensions loaded
octref Nov 1, 2016
894c3cf
Refactor and consistent naming
octref Nov 2, 2016
355f115
Use JSON/interface over the wire
octref Nov 2, 2016
1ed2912
Clean up unused external viewlet order
octref Nov 2, 2016
68284bb
Access modifiers
octref Nov 2, 2016
a04f317
Merge remote-tracking branch 'upstream/master' into treeExplorerAPI
bpasero Nov 3, 2016
4660455
Move explorer entry point to parts/explorers
octref Nov 3, 2016
50eb471
External Viewlet -> ExtViewlet
octref Nov 3, 2016
3870173
Put actions for tree explorers to common layer
octref Nov 3, 2016
a6f8b94
nls and consistent naming for toggle tree explorer action
octref Nov 3, 2016
3b5e3ea
Clean up treeExplorer's ID
octref Nov 3, 2016
9cf01e8
For the sake of browser compatibility and linting
octref Nov 3, 2016
b205a4c
Vertical centering text in tree item label
octref Nov 3, 2016
1cdbe70
Make activity bar icon class unique
octref Nov 3, 2016
13f2dad
Validate user provided viewletId
octref Nov 3, 2016
5342705
Better naming
octref Nov 3, 2016
65f43cc
nls
octref Nov 3, 2016
347c2e9
External -> Extension
octref Nov 3, 2016
ff63a4d
Merge remote-tracking branch 'origin/master' into treeExplorerAPI
octref Nov 3, 2016
11486b8
Rename confusing service
octref Nov 4, 2016
29346ad
Merge remote-tracking branch 'origin/master' into treeExplorerAPI
octref Nov 4, 2016
307037c
Add ViewletService as IViewletService
octref Nov 7, 2016
15bf2ec
Put Viewlet related things to ViewletService
octref Nov 7, 2016
e2b7b52
Restore enable/disable extensions & preserve enabling order
octref Nov 7, 2016
6793e08
Remove awkward workaround for extension viewletid
octref Nov 8, 2016
c57973f
Remove unused event
octref Nov 8, 2016
85f4204
Fix restoring last viewlet
octref Nov 8, 2016
0b15cbb
Preserve badge on add/remove viewlet
octref Nov 9, 2016
b9cf15d
Merge remote-tracking branch 'upstream/master' into treeExplorerAPI
octref Nov 9, 2016
902137c
Fix compile error, will write test later
octref Nov 9, 2016
06b82f5
Split didExtensionViewletLoad from didViewletToggle
octref Nov 9, 2016
4d25517
Open viewlet/switch to default viewlet when toggling extension
octref Nov 9, 2016
dd19d59
Various cleanup
octref Nov 9, 2016
d111426
Naming
octref Nov 9, 2016
25885c6
Remove unused services
octref Nov 9, 2016
80a0cbf
Naming and clean up
octref Nov 9, 2016
ae720e9
Merge remote-tracking branch 'upstream/master' into treeExplorerAPI
octref Nov 9, 2016
a585a33
Move viewlet restore logic to viewletService
octref Nov 10, 2016
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 71 additions & 0 deletions src/vs/platform/explorers/browser/explorerExtensionPoint.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
/*---------------------------------------------------------------------------------------------
* Copyright (c) Microsoft Corporation. All rights reserved.
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/
'use strict';

import { localize } from 'vs/nls';
import { join } from 'vs/base/common/paths';
import { createCSSRule } from 'vs/base/browser/dom';
import { IJSONSchema } from 'vs/base/common/jsonSchema';
import { ExtensionsRegistry } from 'vs/platform/extensions/common/extensionsRegistry';
import { Registry } from 'vs/platform/platform';
import { ViewletRegistry, Extensions as ViewletExtensions, ViewletDescriptor } from 'vs/workbench/browser/viewlet';
import { VIEWLET_ID_ROOT } from 'vs/workbench/parts/explorers/common/treeExplorer';

namespace schema {

export interface IExplorer {
Copy link
Member

Choose a reason for hiding this comment

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

Should you reuse the one in extensionManagement.ts instead of duplicating?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.

treeExplorerNodeProviderId: string;
treeLabel: string;
icon: string;
}

export const explorerContribtion: IJSONSchema = {
description: localize('vscode.extension.contributes.explorer', "Contributes custom tree explorer viewlet to the sidebar"),
type: 'object',
properties: {
treeExplorerNodeProviderId: {
description: localize('vscode.extension.contributes.explorer.treeExplorerNodeProviderId', 'Unique id used to identify provider registered through vscode.workspace.registerTreeExplorerNodeProvider'),
type: 'string'
},
treeLabel: {
description: localize('vscode.extension.contributes.explorer.treeLabel', 'Human readable string used to render the custom tree viewlet'),
type: 'string'
},
icon: {
description: localize('vscode.extension.contributes.explorer.icon', 'Path to the viewlet icon on the activity bar'),
type: 'string'
}
}
};
}

ExtensionsRegistry.registerExtensionPoint<schema.IExplorer>('explorer', schema.explorerContribtion).setHandler(extensions => {
let baseOrder = 200; // Stock viewlet order goes up to 100
let descriptors = [];

for (let extension of extensions) {
const { treeExplorerNodeProviderId, treeLabel, icon } = extension.value;

const getIconRule = (iconPath) => { return `background-image: url('${iconPath}')`; };
if (icon) {
const iconClass = `.monaco-workbench > .activitybar .monaco-action-bar .action-label.${treeExplorerNodeProviderId}`;
Copy link
Member

Choose a reason for hiding this comment

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

You need styles for .vs-dark and .hc-black base themes as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, activity bar doesn't change with themes for now (I'd like it to though)

const iconPath = join(extension.description.extensionFolderPath, icon);
createCSSRule(iconClass, getIconRule(iconPath));
// Coerce the icon into a style similar to stock icons
createCSSRule(iconClass, '-webkit-filter: grayscale(1) invert(1)');
Copy link
Member

Choose a reason for hiding this comment

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

This one could be pulled into CSS as a general rule

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Member

Choose a reason for hiding this comment

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

I mean pull it into the CSS file, not done in typescript since it's static.

}

descriptors.push(new ViewletDescriptor(
'vs/workbench/parts/explorers/browser/treeExplorerViewlet',
'TreeExplorerViewlet',
VIEWLET_ID_ROOT + treeExplorerNodeProviderId,
treeLabel,
treeExplorerNodeProviderId,
baseOrder++,
true
));
}
Registry.as<ViewletRegistry>(ViewletExtensions.Viewlets).registerExternalViewlets(descriptors);
});
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,12 @@ export interface ITheme {
label: string;
}

export interface ITreeExplorer {
treeExplorerNodeProviderId: string;
treeLabel: string;
icon: string;
}

export interface IExtensionContributions {
commands?: ICommand[];
configuration?: IConfiguration;
Expand All @@ -85,6 +91,7 @@ export interface IExtensionContributions {
menus?: { [context: string]: IMenu[] };
snippets?: ISnippet[];
themes?: ITheme[];
explorer?: ITreeExplorer;
Copy link
Member

Choose a reason for hiding this comment

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

Only one tree per extension?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Decided to start with this, because I don't know which path we'll take if we were to allow more complex explorer contribution in the future.

  1. Allow multiple sections (tree or non-tree) like debug

    image

  2. Allow multiple explorers per extenseion

Better to keep it minimal for first iteration I think.

Copy link
Member

Choose a reason for hiding this comment

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

[needs thinking] The problem is that this means no extension can ever add more than one tree-viewlet

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even if we change this part later, it would be an easy update from extension side. I found use cases for multiple viewlets less unlikely than a single multi-section viewlet.

}

export interface IExtensionManifest {
Expand Down
75 changes: 75 additions & 0 deletions src/vs/vscode.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1348,6 +1348,72 @@ declare module 'vscode' {
provideTextDocumentContent(uri: Uri, token: CancellationToken): string | Thenable<string>;
}

/**
* A node provider for the tree explorer contributed by extension.
Copy link
Member

Choose a reason for hiding this comment

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

Drop "by extension" as it's implied being in this file. How about this:

A node provider for a tree explorer contribution.

*
* Providers are registered through (#workspace.registerTreeExplorerNodeProvider) with a
* `providerId` that corresponds to the `treeExplorerNodeProviderId` in the extension's
* `contributes.explorer` section.
*
* The contributed tree explorer will ask the corresponding provider to provide the root
* node and resolve children for each node. In addition, the provider could **optionally**
* provide the following information for each node:
* - label: A human-readable label used for rendering the node.
* - hasChildren: Whether the node has children and is expandable.
* - clickCommand: A command to execute when the node is clicked.
*/
export interface TreeExplorerNodeProvider<T> {
Copy link
Member

Choose a reason for hiding this comment

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

Some high level feedback on the API

  • Not a big friend of the lonely <T> since that provides little guidance what to actually provide. I'd recommend to look at quick pick for inspiration and have a TreeItem being like interface TreeItem { label: string; command: Command; }
  • The node provider should have a way to signal to the main side that a node/substree has changes (similar to TextDocumentContentProvider#onDidChange) so that UI updates
  • Tree elements should support icons - in one or the other way... maybe even reuse our existing icon story?
  • We should consider adding something like paging to the resolveChildren-call. I see this API being used for large sets or sets that are slow to resolve.
  • The API lacks a way to reveal, select, focus a node in a tree. Unsure if the TreeItem should be able to do that or if another API is needed (think of something likewindow.activeTreeWidget)

Copy link
Member

Choose a reason for hiding this comment

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

General comment on these; keep in mind that one of the goals was to make the API as slim and solid as possible, opting to iterate on feedback/user need on extras. Icons, updates, reveal, select, focus, etc. were all considered but put on hold initially as we would prefer @octref to ship something solid and useful that can be iterated on over not shipping at all.

This strategy worked great imo for the terminal API which was slim almost to the point that it was not usable for a month, however the following month filled all the major gaps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback!


Not a big friend of the lonely

TreeItem was what I was initially using. However after talking to Kai and Daniel we feel the current approach is cleaner. The main advantages are:

  • We wanted to allow TreeItem to be any shape. Just taking a few properties off the TreeItem for rendering feels bad.
  • Separating concerns of storing data(T, extension will do that) and rendering (all logic in Provider).
  • From extension author's POV, extending TreeItem and then upcasting the TreeItem to their own class in resolveChildren and the registered commands callback feels weird.
  • The extension looks pretty good with TS2's tagged union: https://github.com/octref/explorer-api-example/blob/master/dep/src/extension.ts

The node provider should have a way to signal to the main side that a node/substree has changes

Agreed. I also wanted this. It's not there as Kai was initially averse to it, but he now seems to support it. We have a refresh button on top of the viewlet.


Tree elements should support icons - in one or the other way

Decided to leave that for next iteration. Wanted to get some feedback from users before deciding how to do it.


We should consider adding something like paging to the resolveChildren-call. I see this API being used for large sets or sets that are slow to resolve.

We are showing the progress bar if the resolveChildren is taking long, similar to what the search viewlet is doing. I feel we can delegate paging to extension authors. Resolve first page -> notify VSCode of change in the subtree when they resolve next page.

Better to wait for some feedback before adding it, I think.


The API lacks a way to reveal, select, focus a node in a tree.

Yep. But while I was writing my 3 examples I never found the need for it. And I can't think of a use case for DB Explorer, which we are designing this for.

So better wait for next iteration when we understand the use cases more.


Will sync with Kai/Daniel this afternoon and talk through your concerns.

Copy link
Member

Choose a reason for hiding this comment

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

You don't need to up-cast anything when your define it as <T extends TreeItem>. It makes an easy to understand API and most importantly it aligns with other parts of the API. Keeping things consistent (look at quick pick) is key to make an easily understandable API. Your sample proofs that the label provider indirection is artificial as the actual data is stored in the node and not computed by a provider.

Copy link
Member

Choose a reason for hiding this comment

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

We actually had it earlier as you're requesting and @kieferrm convinced us to go in this direction.

Your sample proofs that the label provider indirection is artificial as the actual data is stored in the node and not computed by a provider

There are a few benefits to doing it this way:

  • Instead of passing in some TreeItem data structure, the functions work on the actual backing object itself. This removes the need to go and construct a bunch of node objects but instead derive the label from the object you're working on. This extends to the click command, has children, etc.

  • In the previous paradigm the TreeItem typically required some data attribute to disambiguous duplicate labels that appear at multiple places within the tree. Consider a class explorer that have multiple nested objects that may have the same labels but has different click, children etc,

    class foo
      int a
      class foo
        int b
    

    When it comes to provide the second foo's nodes, you need some additional context to handle this. This came up in the examples @octref wrote, so it would be typical to add a TreeItem.data property anyway, this just cuts the middle man.

  • It becomes very easy to switch all labels to use a different function by simply changing the function and triggering an update for example.

There's some more context on this in https://github.com/octref/vscode/issues/4 and https://github.com/octref/vscode/issues/5.

I was also originally opposed to this way but it's definitely grown on me and I think it's quite a nice way to work on a tree structure. I agree consistency very important - just some background/insights into the current situation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of QuickPickItem, I was more aligning the API to registerTextDocumentContentProvider and TextDocumentContentProvider.

It all boils down to where we want to put the responsibility of providing label, hasChildren and command. It feels cleaner to centralize it in provider, instead of letting each item set some specific attributes on itself.

Imagine a more complex DepTree. Where you show moduleName + version + installed, like [email protected], and modules will be installed upon click.

In the current approach, all logic can be in getLabel

getLabel (node: DepNode) {
  const installMark = node.installed ? '✓' : '✘'
  return `${node.moduleName} ${node.version} ${installMark}`
} 

Whereas if we put logic in TreeItem, it would need to construct a label at constructor, and then later in the click command callback modify it. The logics of getting labels ended up in multiple places, and label etc becomes magic attributes that users can't use. What if their node has an icon property and in next iteration we add icon to TreeItem?

Copy link
Member

Choose a reason for hiding this comment

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

Having worked on Eclipse before I am very well aware of how the LabelProvider and ContentProvider patterns work. The question I am asking and not getting feedback on is how does it align with QuickPick, how would a tree'ish quick pick data structure look like, and how does it generally fit into the API?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jrieken

how does it align with QuickPick

I admit it is very different from QuickPick API. I think it's more aligned to other Providers like CodeLensProvider, or DocumentSymbolProvider.

how would a tree'ish quick pick data structure look like

That's what I like about the current design. It doesn't care what your TreeItem data-structure look like. As long as your provider can getLabel for your TreeItem, you can work with our API.

how does it generally fit into the API?

It doesn't resemble any existing API. Previously we did have a version like this:

export interface TreeExplorerNodeProvider {
    provideRootNode(): Thenable<TreeExplorerNode>;
    resolveChildren(node: TreeExplorerNode): Thenable<TreeExplorerNode[]>;
}

export interface TreeExplorerNode {
    label: string;
    shouldInitiallyExpand: boolean;
    onClickCommand: string;
}

but we switched to the TreeExplorerNode-less design for reasons @Tyriar mentioned.


/**
* Provide the root node. This function will be called when the tree explorer is activated
* for the first time.
* The root node is hidden and its direct children will be displayed on the first level of
Copy link
Member

Choose a reason for hiding this comment

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

Attach this to the previous line.

* the tree explorer.
*
* @return The root node.
*/
provideRootNode(): T | Thenable<T>;
Copy link
Member

Choose a reason for hiding this comment

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

@jrieken currently this returns the invisible root node, what do you think about that versus a provideRootNodes(): T[] | Thenable<T[]> (or provideTopLevelNodes) to hide the invisible root node?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about it, the thing is the invisible root node actually holds the label for here:

vscode and open editors

image

I feel later we are gonna consider letting extension contribute multi section viewlets, and I want to leave it here so they can specify label for each section later.


/**
* Resolve the children of `node`.
*
* @param node The node from which the provider resolves children.
* @return Children of `node`.
*/
resolveChildren(node: T): T[] | Thenable<T[]>;

/**
* Provide a human-readable string that will be used for rendering the node.
*
* Default to use `node.toString()` if not provided.
Copy link
Member

Choose a reason for hiding this comment

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

Defaults to node.toString().

I'd attach this sentence to the first line.

*
* @param node The node from which the provider computes label.
* @return A human-readable label.
*/
getLabel?(node: T): string;

/**
* Determine if `node` has children and is expandable.
*
* Default to return `true` if not provided.
Copy link
Member

Choose a reason for hiding this comment

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

Defaults to true.

I'd attach this sentence to the first line.

*
* @param node The node to determine if it has children and is expandable.
* @return A boolean that determines if `node` has children and is expandable.
*/
getHasChildren?(node: T): boolean;

/**
* Get the command to execute when `node` is clicked.
*
* Commands can be registered through (#commands.registerCommand). `node` will be provided
Copy link
Member

Choose a reason for hiding this comment

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

[registerCommand](#commands.registerCommand)

* as the first argument to the command's callback function.
*
* @param node The node that the command is associated with.
* @return The command to execute when `node` is clicked.
*/
getClickCommand?(node: T): string;
}

/**
* Represents an item that can be selected from
* a list of items.
Expand Down Expand Up @@ -3803,6 +3869,15 @@ declare module 'vscode' {
*/
export function registerTextDocumentContentProvider(scheme: string, provider: TextDocumentContentProvider): Disposable;

/**
* Register a [tree explorer node provider](#TreeExplorerNodeProvider).
Copy link
Member

Choose a reason for hiding this comment

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

tree explorer node provider -> TreeExplorerNodeProvider

*
* @param providerId A unique id that identifies the provider.
* @param provider A [TreeExplorerNodeProvider](#TreeExplorerNodeProvider).
* @return A [disposable](#Disposable) that unregisters this provider when being disposed.
*/
export function registerTreeExplorerNodeProvider(providerId: string, provider: TreeExplorerNodeProvider<any>): Disposable;

/**
* An event that is emitted when a [text document](#TextDocument) is opened.
*/
Expand Down
5 changes: 5 additions & 0 deletions src/vs/workbench/api/node/extHost.api.impl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import { ExtHostDocuments } from 'vs/workbench/api/node/extHostDocuments';
import { ExtHostDocumentSaveParticipant } from 'vs/workbench/api/node/extHostDocumentSaveParticipant';
import { ExtHostConfiguration } from 'vs/workbench/api/node/extHostConfiguration';
import { ExtHostDiagnostics } from 'vs/workbench/api/node/extHostDiagnostics';
import { ExtHostTreeExplorers } from 'vs/workbench/api/node/extHostTreeExplorers';
import { ExtHostWorkspace } from 'vs/workbench/api/node/extHostWorkspace';
import { ExtHostQuickOpen } from 'vs/workbench/api/node/extHostQuickOpen';
import { ExtHostHeapService } from 'vs/workbench/api/node/extHostHeapService';
Expand Down Expand Up @@ -65,6 +66,7 @@ export function createApiFactory(threadService: IThreadService, extensionService
const extHostDocumentSaveParticipant = col.define(ExtHostContext.ExtHostDocumentSaveParticipant).set<ExtHostDocumentSaveParticipant>(new ExtHostDocumentSaveParticipant(extHostDocuments, threadService.get(MainContext.MainThreadWorkspace)));
const extHostEditors = col.define(ExtHostContext.ExtHostEditors).set<ExtHostEditors>(new ExtHostEditors(threadService, extHostDocuments));
const extHostCommands = col.define(ExtHostContext.ExtHostCommands).set<ExtHostCommands>(new ExtHostCommands(threadService, extHostEditors, extHostHeapService));
const extHostExplorers = col.define(ExtHostContext.ExtHostExplorers).set<ExtHostTreeExplorers>(new ExtHostTreeExplorers(threadService, extHostCommands));
const extHostConfiguration = col.define(ExtHostContext.ExtHostConfiguration).set<ExtHostConfiguration>(new ExtHostConfiguration(threadService.get(MainContext.MainThreadConfiguration)));
const extHostDiagnostics = col.define(ExtHostContext.ExtHostDiagnostics).set<ExtHostDiagnostics>(new ExtHostDiagnostics(threadService));
const languageFeatures = col.define(ExtHostContext.ExtHostLanguageFeatures).set<ExtHostLanguageFeatures>(new ExtHostLanguageFeatures(threadService, extHostDocuments, extHostCommands, extHostHeapService, extHostDiagnostics));
Expand Down Expand Up @@ -332,6 +334,9 @@ export function createApiFactory(threadService: IThreadService, extensionService
onWillSaveTextDocument: (listener, thisArgs?, disposables?) => {
return extHostDocumentSaveParticipant.onWillSaveTextDocumentEvent(listener, thisArgs, disposables);
},
registerTreeExplorerNodeProvider(providerId: string, provider: vscode.TreeExplorerNodeProvider<any>) {
Copy link
Member

Choose a reason for hiding this comment

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

[needs change] This function must be marked as proposed - look at the sampleFunction

return extHostExplorers.registerTreeExplorerNodeProvider(providerId, provider);
},
onDidChangeConfiguration: (listener: () => any, thisArgs?: any, disposables?: extHostTypes.Disposable[]) => {
return extHostConfiguration.onDidChangeConfiguration(listener, thisArgs, disposables);
},
Expand Down
2 changes: 2 additions & 0 deletions src/vs/workbench/api/node/extHost.contribution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import { MainThreadDiagnostics } from './mainThreadDiagnostics';
import { MainThreadDocuments } from './mainThreadDocuments';
import { MainThreadEditors } from './mainThreadEditors';
import { MainThreadErrors } from './mainThreadErrors';
import { MainThreadTreeExplorers } from './mainThreadTreeExplorers';
import { MainThreadLanguageFeatures } from './mainThreadLanguageFeatures';
import { MainThreadLanguages } from './mainThreadLanguages';
import { MainThreadMessageService } from './mainThreadMessageService';
Expand Down Expand Up @@ -70,6 +71,7 @@ export class ExtHostContribution implements IWorkbenchContribution {
col.define(MainContext.MainThreadDocuments).set(create(MainThreadDocuments));
col.define(MainContext.MainThreadEditors).set(create(MainThreadEditors));
col.define(MainContext.MainThreadErrors).set(create(MainThreadErrors));
col.define(MainContext.MainThreadExplorers).set(create(MainThreadTreeExplorers));
col.define(MainContext.MainThreadLanguageFeatures).set(create(MainThreadLanguageFeatures));
col.define(MainContext.MainThreadLanguages).set(create(MainThreadLanguages));
col.define(MainContext.MainThreadMessageService).set(create(MainThreadMessageService));
Expand Down
15 changes: 15 additions & 0 deletions src/vs/workbench/api/node/extHost.protocol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ import { SaveReason } from 'vs/workbench/services/textfile/common/textfiles';
import { IWorkspaceSymbol } from 'vs/workbench/parts/search/common/search';
import { IApplyEditsOptions, TextEditorRevealType, ITextEditorConfigurationUpdate, IResolvedTextEditorConfiguration, ISelectionChangeEvent } from './mainThreadEditorsTracker';

import { InternalTreeExplorerNode } from 'vs/workbench/parts/explorers/common/treeExplorerViewModel';

export interface InstanceSetter<T> {
set<R extends T>(instance: T): R;
}
Expand Down Expand Up @@ -115,6 +117,11 @@ export abstract class MainThreadEditorsShape {
$tryApplyEdits(id: string, modelVersionId: number, edits: editorCommon.ISingleEditOperation[], opts: IApplyEditsOptions): TPromise<boolean> { throw ni(); }
}

export abstract class MainThreadTreeExplorersShape {
$registerTreeExplorerNodeProvider(providerId: string): void { throw ni(); }
$showMessage(severity: Severity, message: string): void { throw ni(); }
Copy link
Member

Choose a reason for hiding this comment

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

[needs change] Why is this here? It duplicates functionality we already have in other places (ExtHostMessageService). Also, failure to resolve a tree item shouldn't automatically result in an error message. I propose to let the error bubble (as rejected promise) and handle the error in the tree, like showing an error node.

}

export abstract class MainThreadErrorsShape {
onUnexpectedExtHostError(err: any): void { throw ni(); }
}
Expand Down Expand Up @@ -257,6 +264,12 @@ export abstract class ExtHostEditorsShape {
$acceptTextEditorRemove(id: string): void { throw ni(); }
}

export abstract class ExtHostTreeExplorersShape {
Copy link
Member

Choose a reason for hiding this comment

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

Since InternalTreeExplorerNode is a class I won't send it over the wire. JSON serialisation will drop the prototype, functions etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

InternalTreeExplorerNode doesn't have any method. It's the way we communicate between ExtHost/MainThread. Node will be provided and stored at ExtHost side. For each node we use getLabel etc to get info from the node, and construct a slim internal node to send to MainThread.

Copy link
Member

Choose a reason for hiding this comment

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

I understand that but the practice is to be explicit about API and implementation. The protocol is data-driven and therefore we are explicit about only using primitive types and plain (data) interfaces that map directly to JSON. This is more a convention thing than a problem

Copy link
Contributor Author

@octref octref Nov 2, 2016

Choose a reason for hiding this comment

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

Oops sorry. Actually I designed with this in mind and should've used InternalTreeExplorerNodeContent, which is an interface.

$provideRootNode(providerId: string): TPromise<InternalTreeExplorerNode> { throw ni(); };
$resolveChildren(providerId: string, node: InternalTreeExplorerNode): TPromise<InternalTreeExplorerNode[]> { throw ni(); }
Copy link
Member

Choose a reason for hiding this comment

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

As mentioned above, instead of MainThreadTreeExplorersShape.$showMessage return a rejected promise from this call and handle the promise (rejected/resolved) in the main side.

$executeCommand(providerId: string, node: InternalTreeExplorerNode): TPromise<void> { throw ni(); }
Copy link
Member

@jrieken jrieken Nov 1, 2016

Choose a reason for hiding this comment

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

[needs change] Related to MainThreadTreeExplorersShape.$showMessage - Remove this, because it duplicates functionality and also supports extension host side commands only. The on main side, implement this by calling ICommandService#executeCommand.

To preserve the semantics wrt to the node bing the command argument, make sure to use modes.ICommand, the CommandConverter, and the node as argument

}

export abstract class ExtHostExtensionServiceShape {
$localShowMessage(severity: Severity, msg: string): void { throw ni(); }
$activateExtension(extensionDescription: IExtensionDescription): TPromise<void> { throw ni(); }
Expand Down Expand Up @@ -331,6 +344,7 @@ export const MainContext = {
MainThreadDocuments: createMainId<MainThreadDocumentsShape>('MainThreadDocuments', MainThreadDocumentsShape),
MainThreadEditors: createMainId<MainThreadEditorsShape>('MainThreadEditors', MainThreadEditorsShape),
MainThreadErrors: createMainId<MainThreadErrorsShape>('MainThreadErrors', MainThreadErrorsShape),
MainThreadExplorers: createMainId<MainThreadTreeExplorersShape>('MainThreadExplorers', MainThreadTreeExplorersShape),
MainThreadLanguageFeatures: createMainId<MainThreadLanguageFeaturesShape>('MainThreadLanguageFeatures', MainThreadLanguageFeaturesShape),
MainThreadLanguages: createMainId<MainThreadLanguagesShape>('MainThreadLanguages', MainThreadLanguagesShape),
MainThreadMessageService: createMainId<MainThreadMessageServiceShape>('MainThreadMessageService', MainThreadMessageServiceShape),
Expand All @@ -351,6 +365,7 @@ export const ExtHostContext = {
ExtHostDocuments: createExtId<ExtHostDocumentsShape>('ExtHostDocuments', ExtHostDocumentsShape),
ExtHostDocumentSaveParticipant: createExtId<ExtHostDocumentSaveParticipantShape>('ExtHostDocumentSaveParticipant', ExtHostDocumentSaveParticipantShape),
ExtHostEditors: createExtId<ExtHostEditorsShape>('ExtHostEditors', ExtHostEditorsShape),
ExtHostExplorers: createExtId<ExtHostTreeExplorersShape>('ExtHostExplorers', ExtHostTreeExplorersShape),
ExtHostFileSystemEventService: createExtId<ExtHostFileSystemEventServiceShape>('ExtHostFileSystemEventService', ExtHostFileSystemEventServiceShape),
ExtHostHeapService: createExtId<ExtHostHeapServiceShape>('ExtHostHeapMonitor', ExtHostHeapServiceShape),
ExtHostLanguageFeatures: createExtId<ExtHostLanguageFeaturesShape>('ExtHostLanguageFeatures', ExtHostLanguageFeaturesShape),
Expand Down
101 changes: 101 additions & 0 deletions src/vs/workbench/api/node/extHostTreeExplorers.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
/*---------------------------------------------------------------------------------------------
* Copyright (c) Microsoft Corporation. All rights reserved.
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/
'use strict';

import { TreeExplorerNodeProvider } from 'vscode';
import { TPromise } from 'vs/base/common/winjs.base';
import { Disposable } from 'vs/workbench/api/node/extHostTypes';
import { IThreadService } from 'vs/workbench/services/thread/common/threadService';
import { MainContext, ExtHostTreeExplorersShape, MainThreadTreeExplorersShape } from './extHost.protocol';
import { InternalTreeExplorerNode } from 'vs/workbench/parts/explorers/common/treeExplorerViewModel';
import { ExtHostCommands } from 'vs/workbench/api/node/extHostCommands';
import { asWinJsPromise } from 'vs/base/common/async';
import { Severity } from 'vs/platform/message/common/message';

export class ExtHostTreeExplorers extends ExtHostTreeExplorersShape {
private _proxy: MainThreadTreeExplorersShape;

private _treeExplorerNodeProviders: { [providerId: string]: TreeExplorerNodeProvider<any> };
private _externalNodeMaps: { [providerId: string]: { [id: number]: any } };

constructor(
threadService: IThreadService,
private commands: ExtHostCommands
) {
super();

this._proxy = threadService.get(MainContext.MainThreadExplorers);

this._treeExplorerNodeProviders = Object.create(null);
this._externalNodeMaps = Object.create(null);
}

registerTreeExplorerNodeProvider(providerId: string, provider: TreeExplorerNodeProvider<any>): Disposable {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this return vscode.Disposable from import * as vscode from 'vscode';?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw extHostDocuments using this:

import { Range, Position, Disposable } from 'vs/workbench/api/node/extHostTypes';

So I just followed the pattern.

Copy link
Member

Choose a reason for hiding this comment

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

Both types are equal - extHostTypes.Disposable is the implementation of vscode.Disposable

this._proxy.$registerTreeExplorerNodeProvider(providerId);
this._treeExplorerNodeProviders[providerId] = provider;

return new Disposable(() => {
delete this._treeExplorerNodeProviders[providerId];
Copy link
Member

Choose a reason for hiding this comment

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

Don't you need to clean up _externalNodeMaps too?

});
}

$provideRootNode(providerId: string): TPromise<InternalTreeExplorerNode> {
const provider = this.getProvider(providerId);

return asWinJsPromise(() => provider.provideRootNode()).then(externalRootNode => {
const treeNodeMap = Object.create(null);
this._externalNodeMaps[providerId] = treeNodeMap;

const internalRootNode = new InternalTreeExplorerNode(externalRootNode, provider);
this._externalNodeMaps[providerId][internalRootNode.id] = externalRootNode;
return internalRootNode;
}, err => {
this.showErrorMessage(`TreeExplorerNodeProvider '${providerId}' failed to provide root node.`);
Copy link
Member

Choose a reason for hiding this comment

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

[needs change] Replace with return TPromise.wrapError('TreeEx...')

});
}

$resolveChildren(providerId: string, mainThreadNode: InternalTreeExplorerNode): TPromise<InternalTreeExplorerNode[]> {
const provider = this.getProvider(providerId);

const externalNodeMap = this._externalNodeMaps[providerId];
const externalNode = externalNodeMap[mainThreadNode.id];

return asWinJsPromise(() => provider.resolveChildren(externalNode)).then(children => {
return children.map(externalChild => {
const internalChild = new InternalTreeExplorerNode(externalChild, provider);
externalNodeMap[internalChild.id] = externalChild;
return internalChild;
});
}, err => {
this.showErrorMessage(`TreeExplorerNodeProvider '${providerId}' failed to resolve children.`);
Copy link
Member

Choose a reason for hiding this comment

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

[needs change] Like above return Promise.wrapError(...)

});
}

$executeCommand(providerId: string, mainThreadNode: InternalTreeExplorerNode): TPromise<void> {
if (mainThreadNode.clickCommand) {
const externalNode = this._externalNodeMaps[providerId][mainThreadNode.id];
return asWinJsPromise(() => this.commands.executeCommand(mainThreadNode.clickCommand, externalNode)).then(() => {
return null;
}, err => {
this.showErrorMessage(`Failed to execute command '${mainThreadNode.clickCommand}' provided by TreeExplorerNodeProvider '${providerId}'.`);
});
}

return TPromise.as(null);
}

getProvider(providerId: string): TreeExplorerNodeProvider<any> {
const provider = this._treeExplorerNodeProviders[providerId];
if (!provider) {
this.showErrorMessage(`No TreeExplorerNodeProvider with id '${providerId}' registered.`);
}

return provider;
}

private showErrorMessage(message: string): void {
this._proxy.$showMessage(Severity.Error, message);
Copy link
Member

Choose a reason for hiding this comment

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

You could probably make ExtHostMessageService a dependency and call into that from here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thought about that, mainly worried about this recent change:
https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/api/node/extHost.api.impl.ts#L61-L83

Don't know what @jrieken meant by // Addressable instances, but it seems to me extHostExplorer should belong to the top part, as it's similar to extHostDocuments. And none of the top services depend on // Other instances.

}
}
Loading