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

plugins: view/title menus in all dock panel views #12763

Merged
merged 1 commit into from
Aug 21, 2023

Conversation

cdamus
Copy link
Contributor

@cdamus cdamus commented Jul 26, 2023

What it does

In VS Code, contributions of menus to the "view/title" contribution point are supposed to be included in all views in the dock panels, so long as the "when" condition (if any) is satisfied. So the contribution handler is updated to register the menu delegate for all non-editor views.

Fixes #12705

How to test

Per the directions in the description of #12705:

  1. Check out the menu_bug_4 branch of the OP's sample repository and build the myext extension.
  2. Link the extension into your plugins/ directory and launch Theia.
  3. See that the "Hello World" menu action is contributed with a pencil icon in all of the views in the dock panels as shown in the following graphic:

CleanShot 2023-08-07 at 12 29 58

where

  • at ① you can see, for example, that it appears in the Explorer view
  • at ② you can see that views such as the Problems view that are in the bottom panel also show the contribution
  • at ③ likewise views in the secondary panel
  • but at ④ you can see that editor widgets do not get the contribution

Subsequently, verify that by use of the "when" condition on the contribution this action can be restricted to selected views. For example:

  1. Add a when condition restricting the menu contribution to the "Hello World" view, only, as shown in the screen grab below
  2. Rebuild the extension and launch Theia again to verify that the action now only shows in the toolbar of the Hello World view.
CleanShot 2023-07-26 at 16 09 36@2x

Review checklist

Reminder for reviewers

@cdamus cdamus force-pushed the issue/12705 branch 2 times, most recently from 5bd2eec to ba614e0 Compare August 1, 2023 18:14
@vince-fugnitto vince-fugnitto added vscode issues related to VSCode compatibility menus issues related to the menu labels Aug 3, 2023
In VS Code, contributions of menus to the
"view/title" contribution point are supposed to
be included in all views that are not editors,
so long as the "when" condition (if any)
is satisfied. So the contribution handler is
updated to register the menu delegate in
all non-editors.

Fixes eclipse-theia#12705

Signed-off-by: Christian W. Damus <[email protected]>
@cdamus
Copy link
Contributor Author

cdamus commented Aug 7, 2023

Commit fc12c9c amends the fix to include all views that are not editors, including views in the bottom panel and the secondary panel. The PR description is updated accordingly.

@cdamus cdamus changed the title plugins: view/title menus in all side panel views plugins: view/title menus in all dock panel views Aug 7, 2023
@martin-fleck-at martin-fleck-at self-requested a review August 8, 2023 07:40
Copy link
Contributor

@martin-fleck-at martin-fleck-at left a comment

Choose a reason for hiding this comment

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

The change looks good to me and works well, thank you Christian!

From what I can see in the VS Code documentation on menu contributions, they only state that:

Contribute a menu item for a command to the editor or Explorer.

and as view they count anything that is contributed to the following view containers:

  • explorer: Explorer view container in the Activity Bar
  • scm: Source Control Management (SCM) view container in the Activity Bar
  • debug: Run and Debug view container in the Activity Bar
  • test: Test view container in the Activity Bar
  • Custom view containers contributed by Extensions.

In the Theia documentation we mainly differentiate between editor and view, so any widget not being an editor is a view. Therefore, I agree that view title menu contributions in Theia should appear on any non-editor widget and not be restricted to the ones that VS Code offers.

@colin-grant-work What do you think? You did a lot of refactoring in this area "recently" with this PR: #11290

@cdamus
Copy link
Contributor Author

cdamus commented Aug 8, 2023

In the Theia documentation we mainly differentiate between editor and view, so any widget not being an editor is a view. Therefore, I agree that view title menu contributions in Theia should appear on any non-editor widget and not be restricted to the ones that VS Code offers.

Thanks, Martin. That was the basis of my thinking in the revised version of this PR.

The only reservation that I have, because the general expectation would be that extensions are designed for VS Code and not for Theia, is that perhaps it will be surprising to see them appearing also on the left-side dock container, where in VS Code they do not appear. If there's a reliable way to identify that container, to exclude the contribution from it, I'd be happy to employ it.

@martin-fleck-at
Copy link
Contributor

martin-fleck-at commented Aug 8, 2023

@cdamus The application shell has a method called getAreaFor that returns the area in which a widget resides, do you think that could be helpful?

I understand that we want VS Code compatibility but I assume our where conditions also support more options. In any case, I think this is maybe a discussion that should also involve other people. Whatever the decision, it should be documented somehow.

@JonasHelming
Copy link
Contributor

@msujew @vince-fugnitto @tsmaeder opinions?

@vince-fugnitto
Copy link
Member

@msujew @vince-fugnitto @tsmaeder opinions?

I believe keeping it as consistent as possible with VS Code would be a good base implementation, where the contribution would only apply for non-editor widgets. The behavior at the moment is a little inconsistent as in VS Code I believe they treat anything in the main-panel as "editors" while we display the contribution (ex: welcome page, markdown preview).

It might also not be super critical since it's likely bad practice for a plugin to pollute the application with toolbar contributions everywhere (it would at least annoy most users anyways).

@cdamus
Copy link
Contributor Author

cdamus commented Aug 8, 2023

Thanks, Vince.

I believe keeping it as consistent as possible with VS Code would be a good base implementation, where the contribution would only apply for non-editor widgets. The behavior at the moment is a little inconsistent as in VS Code I believe they treat anything in the main-panel as "editors" while we display the contribution (ex: welcome page, markdown preview).

That is a good point and seems not difficult to accommodate, given that we can determine that a widget is in this area.

It might also not be super critical since it's likely bad practice for a plugin to pollute the application with toolbar contributions everywhere (it would at least annoy most users anyways).

I can imagine that there are actions legitimately applicable to all views, especially things relating to workspace layout and accessibility concerns.

@cdamus
Copy link
Contributor Author

cdamus commented Aug 8, 2023

Commit bf4bd5c makes this further refinement to the contribution logic, to exclude also non-editors that are in the 'main' area of the workbench layout:
CleanShot 2023-08-08 at 09 18 11
I think now that this is most consistent with both VS Code and Theia expectations, especially:

  • as at ① the Theia definition of "view" is a bit richer than VS Code's, which IMHO appropriately seats the contribution here as whatever is applicable generally to views is applicable to this container and
  • anything operating ② in the "main" area in an editor-like role is now excluded

@vince-fugnitto
Copy link
Member

@cdamus I'm not sure it's as simple as completely disregarding the main area, the framework supports moving views and editors to all areas of the workbench. At the moment the problems-view by default displays the contribution, but moving it to the main-area will remove it:

image

image

@martin-fleck-at
Copy link
Contributor

From my point of view, it is very hard to estimate what users would expect. However, I'd argue that the original "non-editor" implementation was a bit more predictable than the "non-main area" implementation. I understand that pollution is a topic that may concern users but isn't that also true within VS Code? If a plugin author does not properly use the when condition, they will also pollute views other than what they may want to target. It is just that within Theia the term "view" is a bit broader.

@cdamus
Copy link
Contributor Author

cdamus commented Aug 8, 2023

Indeed, internal consistency in Theia should be more important than elusive (and illusory) consistency with VS Code. So I've reverted back to commit fc12c9c which just contributes "view/title" menus to everything that doesn't appear to be an editor. Which, as it happens, is the exact dual of the condition for contributions to the "editor/title" point.

If we stipulate that every contributed widget is either an editor or a view, then it makes sense that the union of the "editor/title" and "view/title" contribution points should cover every widget and any further refinements must consider the impact on both contribution points.

@martin-fleck-at
Copy link
Contributor

@vince-fugnitto Would you be okay to define a view as everything that is not an editor, independent from its current location? Or how can we best continue with this PR?

@vince-fugnitto
Copy link
Member

@vince-fugnitto Would you be okay to define a view as everything that is not an editor, independent from its current location? Or how can we best continue with this PR?

@martin-fleck-at I believe that both approaches would present differences in behavior compared to vscode since we fundamentally have a different behavior in our workbench compared to them without the their restrictions:

  • non-editors: in vscode they treat non-editors as anything not in the main-panel, while we may have some views (ex: getting-started) with the menu contribution when it otherwise wouldn't
  • based on area: would represent problems when moving views around which is not permissible in vscode

In the end I think non-editors should be fine, we just need to be aware that in the framework we would give the menu to views which otherwise do not have it in vscode. I also believe that it is the responsibility of a plugin to properly set the when clause for their menu contributions to avoid polluting other views so we can say the issue at the moment about going the non-editor approach is less critical.

@martin-fleck-at
Copy link
Contributor

@vince-fugnitto Thank you! I agree that the non-editor approach is a bit more intuitive and consistent. I'll wait another day in case somebody else has a different opinion, otherwise I'll merge the PR.

@martin-fleck-at martin-fleck-at merged commit 20e3771 into eclipse-theia:master Aug 21, 2023
12 checks passed
@cdamus cdamus deleted the issue/12705 branch August 28, 2023 12:05
@vince-fugnitto vince-fugnitto added this to the 1.41.0 milestone Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
menus issues related to the menu vscode issues related to VSCode compatibility
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

VS Code menu contribution to "view/title" doesn't appear in "built-in" views
4 participants