Vscode: Review usage/overrides of get.*Actions on ViewPanes
With the introduction of View Menu Actions its possible for ViewPanes to accidentally override them. We should verify if this is happening and consider if we still want View Menu Actions with DnD for views already introduced.
/cc @eamodio
sbatten
All 9 comments
Looking a little more at this, there may be other uses?
sbatten
on 5 Mar 2020
@sbatten @sandy081 I wanted to follow up on a related topic, not sure if it is the same as this issue: today, Composite has methods getActions and getSecondaryActions (even getContextMenuActions?). We probably have other view related pieces with this kind of API.
That API should be deprecated imho and removed in favour of registering commands to Menu locations, as we e.g. do with all editor related actions in the editor tool bar. There should be no reason that any piece of UI is required to return actions upfront from these methods and every location should be capable of receiving actions with proper menu contributions and commands.
Example for the proper way of registering actions:
bpasero
on 5 Mar 2020
@sbatten Not sure if I understand this debt item. Can you please elaborate more?
sandy081
on 6 Mar 2020
@bpasero Yes, I completely agree.
Thanks to @jrieken who provided support for contributing menu actions to any view. I discussed with him then and discussed about the future plan of moving away from get*Actions to MenuRegistry.
We can use this item to migrate.
sandy081
on 6 Mar 2020
I also quickly looked into this then and figured out that there are some actions which need ActionItem infrastructure. For eg:
FilterActionin problems panel- `Dropdowns in Terminal and Output panel etc.,
sandy081
on 6 Mar 2020
FWIW, When adding commands to the Timeline view -- I first added them through the MenuRegistry and context keys, but I ultimately removed all of that and moved to get*Actions because it was so much easier, especially dealing with internal view state. Also the Timeline has a custom list of "filters" which are the labels of the sources registered, so hooking that into getSecondaryActions was quite trivial, where as to provide that with the MenuRegistry would require a lot more indirection and complexity.
eamodio
on 6 Mar 2020
@eamodio Can you please provide an example of the complexity and indirection you mentioned. At the end you need to have a logical condition to show an action in get*Actions or through MenuRegistry. Is not it possible to compute the logical state in your view and set that as context?
sandy081
on 7 Mar 2020
Ultimately it turned out not to be a big deal -- I converted Timeline back to using the MenuRegistry via registerAction2
eamodio
on 7 Mar 2020
If I understand the desired action from @bpasero and @sandy081, this is the list of usages that should be migrated. Perhaps we can have view owners migrate during upcoming debt week.
sbatten
on 2 Jun 2020
Related issues
chrisdias
路
3Comments
omidgolparvar
路
3Comments
lukehoban
路
3Comments
shanalikhan
路
3Comments
curtw
路
3Comments
Most helpful comment
Ultimately it turned out not to be a big deal -- I converted Timeline back to using the
MenuRegistryviaregisterAction2