Support dynamically contributed commands for contributed trees context menus

[DanTup]

Currently we have to know what items should appear on a TreeItem's context menu at the time that we render the tree. This means if we want to display a fully expanded tree at startup but add context menu items, we have to fetch them all immediately.

Would it be possible to have an async method that asks us for the items at the time of selection so we can lazily-fetch these items (I understand this will delay rendering of the context menu until the promise resolves).

[vscodebot]

(Experimental duplicate detection)
Thanks for submitting this issue. Please also check if it is already covered by an existing one, like:

• Proposal: Add mnemonics to context menu items (#1792)

[sandy081]

@DanTup Not sure if I understand your scenario. Can you please explain a bit more

This means if we want to display a fully expanded tree at startup but add context menu items, we have to fetch them all immediately.

[DanTup]

@sandy081

I'm rendering a tree where the items that should be on the context menu vary for each node and are (relatively) expensive to compute. Currently the API requires that I set the contextValue of a tree-node at creation time, and if I want the tree to be expanded immediately, that means I have to compute the context menus for every node all in one go.

I would like to be able to lazily provide the context menu/contextValue for a tree node at the point that the user clicks/right-clicks on it.

Hope that makes sense!

[sandy081]

@DanTup I see. If it is expensive to compute the context menu, then delaying it until context menu appears will make context menu to be async (not instinctive). This might not be expected by the user when user does right click?

[DanTup]

@sandy081 Computing a single one is hopefully not very slow (hopefully < 10ms), but computing them for every node of the tree in a single-threaded server would be. I understand the concerns with making the context menu async, but we hope it'd be fast enough to not affect the experience here.

[sandy081]

@DanTup

but computing them for every node of the tree in a single-threaded server would be

But you will only compute when the node is shown (when getTreeItem is called) right, not for all nodes in the tree. Also it is synchronous only for sibling nodes.

[DanTup]

But you will only compute when the node is shown (when getTreeItem is called) right, not for all nodes in the tree.

Yeah, but if we want the tree to default to expanded, that's basically everything in a very short space of time?

Also it is synchronous only for sibling nodes.

Yeah, it's more about the resource usage and total time for all the round trips we're trying to minimise. It seems wasteful for us to compute the context menus for 100 nodes if the user only ever invokes the context menu on one (or none).

[DanTup]

@sandy081 Another possibility - what if we could provide an update the enabled/disabled status of the context menu asynchronously? That way the menu can render immediately, but items may become greyed later. It's up to the provider to ensure it's fast and handle the case where the item was still clicked but wasn't intended to be visible?

[sandy081]

@DanTup Good suggestions.

@jrieken I think this is for asking prerequisite clause for menu items.

[DanTup]

I think this is for asking prerequisite clause for menu items.

I'm not sure what exactly you have in mind, but as long as we can provide info at the time the menu is being shown (and not at the time of constructing TreeItem) then it should work for us.

[jrieken]

I think this is for asking prerequisite clause for menu items.

There are ideas to have a precondition-clause on commands - that would manage the enablement-state, not visibility which is managed per menu.

can provide info at the time the menu is being shown

We don't plan for dynamic menus, the tree might want to accept vscode.Command objects somewhere.

[DanTup]

I'm not sure I understand. I already have the ability to control the visibility using when (though what I did was a bit of a hack - using a regex so I can treat it like a bitmask to enable multiple different items per node). If preconditions still need declaring at the time of creating the TreeItem that won't help with this request. This request is specifically about allowing me to decide which items are valid (I don't care if it's disabled of hidden) at the time of showing the menu, so that I can avoid doing the computations for which actions are valid for each note all at the time of building the tree (since it starts expanded, all nodes are created up-front).

[jrieken]

This request is specifically about allowing me to decide which items are valid (I don't care if it's disabled of hidden) at the time of showing the menu, so that I can avoid doing the

Like keybindings, menu-definitions are static and cannot be computed dynamically. We do that to eventually support user-custimizable menus (like user customisable keybindings) and we have no plans for dynamic menus.

The only thing you might be able to get is something special with the tree, e.g. a property that you need to provide with the TreeItem - that's up to @sandy081

[DanTup]

Like keybindings, menu-definitions are static and cannot be computed dynamically. We do that to eventually support user-custimizable menus (like user customisable keybindings) and we have no plans for dynamic menus.

That's fine, the commands are already predefined in package.json. This is specifically about choosing which of those items are available for a given node, which I'd like to do when the menu is being shown rather than when the TreeItem is constructed.

The only thing you might be able to get is something special with the tree, e.g. a property that you need to provide with the TreeItem - that's up to @sandy081

@sandy081 Let me know your thoughts on this, as it may influence dev we do elsewhere. The simplest way would be for me to provide an async method on the tree provider that given a TreeItem is able to affect the visibility of the menu items (for ex. a resolveContextValue or something).

[sandy081]

The only thing you might be able to get is something special with the tree, e.g. a property that you need to provide with the TreeItem - that's up to @sandy081

@jrieken What is the thinking behind exposing a property in TreeItem for dynamic custom menus? We already expose a property contextValue on TreeItem object. This is set when the item is shown in the view. But, I think what @DanTup requires is to contribute to context menu when the menu is requested to open. Ideally he requires following

• Extension author contributes to context menu, when the user asks for context menu.

@DanTup Correct me if I am wrong.

[jrieken]

requires is to contribute to context menu when the menu is requested to open

@sandy081 Blocking/delaying the right click with extension code won't happen therefore I propose middle ground. Instead of going through contextValues and package.json-contributions allow to have vscode.Command-objects. That's similar to code actions which don't require context keys and static commands

[DanTup]

@sandy081 Your understanding is correct.

@jrieken I'm already able to provide the set of commands dynamically using a regex in the when clause. It's not beautiful but it works so I don't think you need to build anything new to support that.

Blocking/delaying the right click with extension code won't happen

How about letting the menu appear immediately, but an async call is able to disable (grey out) items? I'm sure we can easily respond in < 50ms so the user won't notice them update from black to grey and I'm happy to be responsible for handling the command graceful if the user is able to click one because I didn't respond fast enough.

[sandy081]

Instead of going through contextValues and package.json-contributions allow to have vscode.Command-objects

This is like contributing commands when TreeItem is created. If I understand correctly what @DanTup asking is to contribute commands at the time of opening context menu which I think will not be supported Blocking/delaying the right click with extension code won't happen.

[DanTup]

Btw, it seems like the explorer menu may already be doing this - eg. Paste is only available when you have a file in the clipboard. I can't figure out from the source how this works though - is it something we can piggy-back?

[sandy081]

@DanTup This is done by using precondition which again uses context values.

[DanTup]

Ah, gotcha :(

[sandy081]

@DanTup Please provide your suggestion of having onDidSelectionChange event to have dynamic menus. Would be interested to know how this solves.

Thanks

[DanTup]

@sandy081 My comment on the other issue was actually unrelated to dynamic context menus; but rather what's in that case. In this comment there's a screenshot suggesting we could contribute buttons to above the tree. If that's the case, then having onDidSelectionChange would allow these buttons to be enabled/disabled based on the selection.

That seems like a reasonable way for me to achieve what I wanted here, but is not possible (that is, provide some actions that are available/not available depending on the selected node).

[sandy081]

Are you saying you can update the context (which a menu command depends on) by listening to selection change event?

[DanTup]

@sandy081 I might be missing something, but I was thinking that if we had an onSelectionChange for the tree, then we can set a context with executeCommand("setContext", "xxx", true); (and we could do this asynchronously, which has been the problem so far). Assuming the buttons on the toolbar shown on the screenshot I linked above are a) possible and b) update when contexts change, this would allow us to have buttons above a tree which enable/disable based on the selected tree node (without having to know all of the contexts for every node at the time it was rendered)?

[sandy081]

Yes title bar buttons are

• available already
• can be made visible/invisible by changing the context.

[DanTup]

@sandy081 That's great - going back to this comment could we have an onSelectionChange event then? This seems like a reasonable use case? :-)

[sandy081]

@DanTup Yes here it is - #50662

Don't know what is the status of this item, may I know if current issue is still valid after implementing #50662? And if so what more is needed?

[DanTup]

@sandy081 I think if #50662 gets implemented then I have another way to do what I need without requiring async-dynamic context menus (we'll just use the title bar buttons instead of context menu; which is actually what we originally wanted, but I didn't realise it existed :-))

[DanTup]

Oh, one other question (slightly off-topic for this issue, but hey)... those title bar buttons - can we have them visible-but-disabled? So they don't jump around as the user clicks between nodes (just become disabled/enabled)?

[sandy081]

@DanTup You can only show/hide them but not disable. I think there is a request for this in general.

Closing this.