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

Add support for "enablement" property in plugin command contributions #12483

Merged
merged 2 commits into from
May 16, 2023

Conversation

tsmaeder
Copy link
Contributor

@tsmaeder tsmaeder commented May 4, 2023

What it does

Reads the enablement property for commands contributed in a plugin package.json and updates toolbar items when necessary.

Fixes #12426

Contributed on behalf of STMicroelectronics

How to test

  1. Install the attached plugin
  2. Open the view "Test View Drag And Drop"
  3. There are two toolbar actions: the left one prints the selction to the console and the right one changes the enablement of the other action
  4. Click on the second action
  5. Observe: the toolbar item becomes grayed-out/normal
  6. Observe: you cannot execute the action if the item appears disabled and vice-versa.
  7. Click on the "more" menu in the view and make the enablement in the menu is correct
  8. Open the context menu on an item in the view and make sure the enablement is correct
  9. Repeat the above tests in electron-native and electron-custom mode
  10. Repeat the above tests in browser mode
  11. Check main-menu behavior: in electron-custom and browser mode, enablement/disablement should work, in electron-native mode, main menu items should always be enabled (see Support dynamic menu items in electron #446). The "Selection" menu is a good candidate to check the behavior.

Review checklist

Reminder for reviewers

@tsmaeder tsmaeder marked this pull request as draft May 4, 2023 10:31
Fixes eclipse-theia#12426

Contributed on behalf of STMicroelectronics

Signed-off-by: Thomas Mäder <t.s.maeder@gmail.com>
@tsmaeder tsmaeder force-pushed the 12426_command_enablement branch from 6a52fb9 to ab75432 Compare May 5, 2023 06:32
@tsmaeder tsmaeder marked this pull request as ready for review May 5, 2023 06:38
@tsmaeder
Copy link
Contributor Author

tsmaeder commented May 5, 2023

Here's the test plugin:
custom-view-samples-0.0.1.zip

And here's the source:

tree-view-sample.zip

Copy link
Contributor

@planger planger left a comment

Choose a reason for hiding this comment

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

Thank you Thomas! Works as advertised in browser, electron native and electron custom mode.
Code looks reasonable to me. I just added a minor nitpick inline.

@@ -162,10 +171,14 @@ export class TabBarToolbar extends ReactWidget {
</div>;
}

protected isEnabled(item: AnyToolbarItem): boolean {
return (!!item.command && this.commandIsEnabled(item.command) && this.evaluateWhenClause(item.when)) || (!!item.menuPath && !item.command);
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: maybe it helps readability if we extract two variables with helpful names for each of the two parts of the conditions.

@JonasHelming JonasHelming mentioned this pull request May 9, 2023
1 task
@tsmaeder tsmaeder mentioned this pull request May 11, 2023
11 tasks
Signed-off-by: Thomas Mäder <t.s.maeder@gmail.com>
@tsmaeder tsmaeder requested a review from planger May 16, 2023 07:56
@tsmaeder
Copy link
Contributor Author

@planger I changed the isEnabled() logic. Have a quick look?

Copy link
Contributor

@planger planger left a comment

Choose a reason for hiding this comment

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

Thank you, much better imho!

@tsmaeder tsmaeder merged commit 3e0319f into eclipse-theia:master May 16, 2023
@vince-fugnitto
Copy link
Member

@tsmaeder @planger we may have to revert this change, it breaks context-menus in electron.
For example: #12548.

@tsmaeder
Copy link
Contributor Author

@vince-fugnitto since action enablement works just fine for other context menus (file explorer or the general context menu on the editor), this might just be a bug in the action enablement that is revealed since we now actually honor the enabled state.

tsmaeder added a commit to tsmaeder/theia that referenced this pull request May 23, 2023
…eclipse-theia#12483)

Fixes eclipse-theia#12426

Contributed on behalf of STMicroelectronics

Signed-off-by: Thomas Mäder <t.s.maeder@gmail.com>
@vince-fugnitto vince-fugnitto added this to the 1.38.0 milestone May 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement enablement Property in Command Contributions
3 participants