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

fix: menu item enablement issues #1750

Merged
merged 1 commit into from
Dec 15, 2022
Merged

fix: menu item enablement issues #1750

merged 1 commit into from
Dec 15, 2022

Conversation

kittaakos
Copy link
Contributor

@kittaakos kittaakos commented Dec 8, 2022

Motivation

Fixes the Sketch > Upload/Upload Using Programmer menu items enablement. This PR is related to #1735 and covers #1533.


IDE2 switched from always-enabled menu items to dynamic ones with this change and here is the crucial part.

Without rebuilding the entire menu, there is no way to support a dynamic menu in an electron app. So in Theia, the menu is calculated at startup, and all menu items are always enabled even though the underlying commands could be disabled. The menu remains the same for the app session—no need to rebuild anything, but the command enablement can change at runtime. If a menu is enabled and the underlying command is disabled, nothing happens when the user clicks on the menu item. This is the vanilla Theia behavior.

IDE2 heavily relies on dynamic menus. IDE2 has to recalculate the menu on board attach/detach, selection change, library/platform installation uninstallation, etc., events. A dynamic menu with the correct menu items enablement state is something IDE2 could provide without much additional effort if it were done correctly. See #1533.

This PR cleans up the placeholder menu items, makes the menu items rely on the command enablement state, and "manually" updates the menus when needed.

Change description

The following logic was changed:

  • Edit > Increase Font Size/Decrease Font Size (no behavior change is expected),
  • Tools > Upload SSL Root Certificates (disabled when the dialog is opened),
  • Tools > WiFi101 / WiFiNINA Firmware Updater (disabled when the dialog is opened),
  • Sketch > Verify/Compile (disabled when verifying),
  • Sketch > Export Compiled Binary (disabled when verifying),
  • Sketch > Upload (disabled when uploading),
  • Sketch > Configure and Upload (disabled when uploading), and
  • Sketch > Upload Using Programmer (disabled when uploading)

Other expected behavior changes (my PR is a proposal, I am happy to adjust if required)

  • When Sketch > Verify/Compile is disabled, Sketch > Export Compiled Binary is also disabled and vice versa.
  • When Sketch > Upload is disabled, Sketch > Upload Using Programmer is also disabled and vice versa. The same is true for Sketch > Configure and Upload.
  • When an upload is running, for the (automatic) verify part, the Sketch > Verify/Compile and Sketch > Export Compiled Binary are disabled.
  • When the upload is running, but the (automatic) compilation part has already passed, the Sketch > Verify/Compile and Sketch > Export Compiled Binary menu items are enabled, but the Sketch > Upload, Sketch > Upload Using Programmer, and Sketch > Configure and Upload menu items are still disabled until the end of the upload command.

Finally, this PR eliminates the following warning on macOS, maybe on other OSs too. Can be reproduced when starting the IDE2 from a terminal, and the window receives a blur and then focus event. (Lose focus from IDE2 app, set focus to IDE2 app):

DEBUG Skipping menu item with missing command: "workbench.action.files.newUntitledFile".

This PR has no intention changing anything regarding the command/menu enablement in the toolbar.

Other information

Closes #1533
Closes #1722

Reviewer checklist

  • PR addresses a single concern.
  • The PR has no duplicates (please search among the Pull Requests before creating one)
  • PR title and description are properly filled.
  • Docs have been added / updated (for bug fixes / features)

@kittaakos kittaakos added type: enhancement Proposed improvement topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project labels Dec 8, 2022
Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

I noticed this PR causes a change in the behavior of the buttons. I think the new behavior is reasonable, but it was not mentioned in the PR description so I thought it was worth mentioning in case it was unintended/unwanted.

I seem to remember we discussed the idea of having this behavior some time ago and it was rejected, but I might be misremembering and I don't have a strong preference for either behavior.

Behavior of IDE from main

When the user triggers an "Upload" operation in any of the builds from the main branch, the "Upload" button, and only that button, is given the active coloration during the entire course of the operation (which includes both a compile and an upload phase):

main

Behavior of IDE from PR

When the user triggers an "Upload" operation using the build from this PR, in addition to the "Upload" button having the active coloration during the entire course of the operation, the "Compile" button is given the active coloration during the compile phase of the "Upload" operation:

pr

@kittaakos
Copy link
Contributor Author

Great that you have mentioned it. I did not plan to change it. We can restore what we have on the main, but we can continue with this "new feature" if it adds value.

I seem to remember we discussed the idea of having this behavior some time ago and it was rejected, but I might be misremembering and I don't have a strong preference for either behavior.

Yes. The thread is starting here: #1260 (review)

Let me know how to proceed. Thank you!

Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

@kittaakos I've been giving it some more thought and I think the behavior of the button coloration in the builds from main is better. I think it is good to give the user some feedback about the progress thought the phases of the "Upload" operation to the user (as is already done to some extent via the notifications), but I don't think button coloration is the right way to do that. If I click a button, I don't expect another button to show an activation state.

Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
@kittaakos
Copy link
Contributor Author

I have restored the toolbar behavior. Now, it should be the same as on the main. The menu item disablement is still expected to work as described in #1750 (comment). Please review. Thank you!

Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

I verified that:

Thanks Akos!

@kittaakos kittaakos merged commit 908ec4c into main Dec 15, 2022
@kittaakos kittaakos deleted the #1722 branch December 15, 2022 08:21
@kittaakos kittaakos removed their assignment Dec 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself type: enhancement Proposed improvement type: imperfection Perceived defect in any part of project
Projects
None yet
2 participants