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

Enable/Disable Menu items based on plugin loaded #45

Merged
merged 2 commits into from
Feb 12, 2024

Conversation

NatureIsFrequency
Copy link
Contributor

@NatureIsFrequency NatureIsFrequency commented Feb 10, 2024

Per @Trinitou suggestions, a plugin specific menu item should only be enabled (available to execute by user) when the plugin is loaded, and disabled when the plugin is not loaded

Fixes

This fixes the underlying cause of the crashes reported in #40 and #42
Fix for #40
Fix for #42

Details

Upon plugin load change we update the menu items
Reverts the now obsolete change in previous merged pull request #43
Tab indentation set to 3 to be consistent with existing code
Note: Follows the QT memory ownership model, and the QAction pointers lifetime is handled by their parents

Testing

Arm64 Debug

  1. Launch with no plugin loaded, see menu items are greyed out (disabled),
    user cannot select these items (as intended) therefore code is not executed and app doesn't crash
  2. Launch with plugin loaded, see menu items are available and executed when user clicks
    confirm still no crashes

Test 1: Plugin not loaded

MenuItemPluginNotLoaded1 MenuItemPluginNotLoaded2

Test 2: Plugin loaded

MenuItemPluginLoaded1 MenuItemPluginLoaded2

Per @Trinitou suggestions, a plugin specific menu item should only be enabled (available to execute by user)

This fixes the underlying cause of the crashes reported in free-audio#40 and free-audio#42

Therefore this also reverts the now obsolete change in previous merged pull request free-audio#43

Upon load change we update the menu items

Tab indentation set to 3 to be consistent with existing code

Note: Follows the QT memory ownership model, and the QAction pointers lifetime is handled by their parents

Testing:
Launch with no plugin loaded, see menu items are greyed out (disabled), user cannot select these items (as intended) therefore code is not executed and app doesn't crash

Launch with plugin loaded, see menu items are available and executed when user clicks, and confirm still no crashes

Images taken for testing
@NatureIsFrequency NatureIsFrequency mentioned this pull request Feb 10, 2024
Copy link
Contributor

@Trinitou Trinitou left a comment

Choose a reason for hiding this comment

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

I woke up at night and the problem popped up in my head. And then I saw that you already solved it 😲 👍 😄

host/plugin-host.cc Outdated Show resolved Hide resolved
host/plugin-host.cc Outdated Show resolved Hide resolved
host/main-window.cc Outdated Show resolved Hide resolved
host/main-window.hh Outdated Show resolved Hide resolved
Per @Trinitou feedback:

1. pluginLoadedChanged is now a signal of PluginHost

2. Connect MainWindow:: updatePluginMenuItems to this signal

3. Rename updateMenuItems to updatePluginMenuItems (so it specifies it's these are plugin related menu items)

4. Remove i_prefix in function parameter to follow code style

5. Use default argument false for updatePluginMenuItems
Copy link
Contributor

@Trinitou Trinitou left a comment

Choose a reason for hiding this comment

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

after the adjustments, the changes feel a lot more "native" now 👍

not completely sure whether the assert is really necessary.

Another idea that came into my mind: updatePluginMenuItems even could be a local lambda which captures the menu actions so those even don't have to be members.

All in all, I'd say it's done and maybe let's hear what @abique thinks about it before doing any further adjustments. IMO we could already take it like it is.

@abique
Copy link
Contributor

abique commented Feb 12, 2024

I haven't tested it, but if you both tested it and it works well for you, then I trust both of you 👍

@abique abique merged commit ff98954 into free-audio:main Feb 12, 2024
1 check passed
@NatureIsFrequency
Copy link
Contributor Author

Cheers @abique, tested my end on Mac Arm64 (no windows to hand a the mo).

Big thanks for the merge, and if I get some time later in the week I'll take a look at the plugin load menu item too, have a good week 👍

@NatureIsFrequency NatureIsFrequency deleted the PluginMenuItems branch February 12, 2024 22:06
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.

3 participants