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

testing/profiles/context menu contribution #14028

Conversation

rschnekenbu
Copy link
Contributor

What it does

This Pull Request adds the support of the newly introduced menu contribution point 'testing/profiles/context'. This contribution point, despite being named 'context', is a contribution to the toolbar entries in the equivalent of the Test Explorer View... It was designed to get additional actions in the drop down menu of the tool bar items that runs Test, debug them or run them with Coverage.

sample-vscode

I created a small sample extension for testing:

This kind of toolbar button with drop-down in Theia does not exist yet as far as I could see, so I implemented as:

  • a simple entry in the main toolbar for the Test Explorer view with a simple mapping (see Commit 23ea30b). Unfortunately, this has several drawbacks, as for example polluting the main toolbar, where it would be nice to keep all contributed actions in submenus).
  • I then introduced a new mapping in the vscode to Theia mappings, to move the action to the "..." more actions at the end of the toolbar. This solves the issue of cluttering the main toolbal, but this shows a new problem: There are new context keys that are added when building the menus, depending on the test action group: run, coverage or debug (see testingExplorerView in vscode). In VS Code, the context matcher may have a different overlay. With the 2nd commit, the entries are now placed in the "..." at the end, when the testing.profile.context.group value is not checked in the command.
  • So I introduced a new contextKeyOverlay entry on the ActionMenuNode, so the various values of the context key could be evaluated. This is the role of the third commit. However, I feel like I introduce a lot of code only to support a menu entry from VS code extensions. With this one, the test sample displays nicely the 4 available actions, but this seems like a workaround rather than a proper framework. Maybe the support of the drop down toolbar button may be a better contribution here.

That is the reason why I introduced this PR with 3 different commits, representing the 3 different steps to implement from a lightweight version to a more complex one.

Fixes #14013

Contributed on behalf of STMicroelectronics

How to test

Install the provided extension sample, and go to the Test Explorer View.
Additional menu entries will come, depending on the commit selected:

  • first commit: the simple entry will be shown in the main toolbar of the Test Explorer view
  • 2nd commit: the simple entry will be shown in the "..." menu at the end of the main toolbar of the Test Explorer View
  • 3rd commit: All entries will be displayed in the "..." menu at the end of the main toolbar of the Test Explorer View

Follow-ups

none yet.

Review checklist

Reminder for reviewers

fixes eclipse-theia#14013

contributed on behalf of STMicroelectronics
Signed-off-by: Remi Schnekenburger <rschnekenburger@eclipsesource.com>
@rschnekenbu rschnekenbu requested a review from tsmaeder August 8, 2024 15:51
@tsmaeder
Copy link
Contributor

tsmaeder commented Aug 9, 2024

@rschnekenbu why is this a draft PR?

@tsmaeder
Copy link
Contributor

tsmaeder commented Aug 9, 2024

Here's a couple of remarks:

  1. When I just check out the branch, I only ever get the "basic" action. If we don't have separate contexts, I would expect all actions to allways be visible.
  2. I don't think the feature makes much sense if we can't distinguish the context. Imagine an extension contributing four actions named "run" that do different things depending on context.
  3. We already have some "button with drop down" items, for example in the editor toolbar actions of the "gitlens" extension. So there is support for this UI element somewhere in our code, we just need to find it. Then the context of the menu action would be the drop-down and we wouldn't have to introduce the whole "context overlay" thing.

On a more general level, our approach seems to be to have a single algorithm that can compute the tab bar for all widgets. I don't think this is a good approach: I suspect it would lead to much simpler code if widgets would control their own toolbar contents with the help of the contribution registry instead of the contribution registry controlling the widget toolbar. But that's just a general note.

@tsmaeder
Copy link
Contributor

One of the problems with implementing this feature is that we use the menuBarPath as a key in ToolbarRegistry: the feature as implemented in VS Code shows the same action in multiple toolbar drop-downs. At the very least, we would have to relax this unique key constraint to implement the same.

- support multiple items with the same menu path, but different context
- allow for items with both commands and menus
- fixed menu rendering with native drop-downs
- simplified typing around toolbar items

Signed-off-by: Thomas Mäder <t.s.maeder@gmail.com>
@tsmaeder tsmaeder marked this pull request as ready for review August 21, 2024 08:15
@tsmaeder tsmaeder removed their request for review August 21, 2024 08:16
@tsmaeder
Copy link
Contributor

One of the problems with implementing this feature is that we use the menuBarPath as a key in ToolbarRegistry:

Actually, that's not a problem: the "menuDelegate" just links a menu menu to a toolbar. The link is done via a "when" function. We can't link multiple conditions for showing the toolbar, but we can show the same menu as a submenu or a toolbar item.

@msujew msujew self-requested a review August 21, 2024 13:18
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Looks generally pretty good. I have just one minor suggestion, see below 👍

Comment on lines +531 to +532
font-size: 8px;
vertical-align: bottom;
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: This doesn't look correct to me. The chevron should be vertically centered:

VS Code:
image

Current:

image

Copy link
Contributor

Choose a reason for hiding this comment

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

I was not trying to imitate VS Code visually, but to keep the "chevron at the bottom" look we had before while implementing the functionality of having both a command and a drop-down. Do you think the VS Code version looks better?

Copy link
Member

Choose a reason for hiding this comment

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

I think looks better - also I was confused by the small size of the chevron - makes it a bit difficult to click on it, since the whole button is just a few pixel in widget.

@tsmaeder tsmaeder requested review from msujew and tsmaeder August 26, 2024 15:14
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

The changes look good to me. If the chevron CSS issue annoys me too much, I'll submit a PR for that myself :)

@tsmaeder tsmaeder merged commit 30fc45c into eclipse-theia:master Aug 27, 2024
11 checks passed
@jfaltermeier jfaltermeier added this to the 1.53.0 milestone Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[vscode] Support testing/profiles/context menu contribution point
4 participants