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 for #42 #44

Closed

Conversation

NatureIsFrequency
Copy link
Contributor

Fix for #42

Guard against null _plugin within PluginHost::setParentWindow
Follow error convention in source code

Tested arch: arm64
Confirm no longer crash

Guard against null _plugin within PluginHost::setParentWindow
Follow error convention in source code

Tested arch: arm64
Confirm no longer crash
@abique
Copy link
Contributor

abique commented Feb 7, 2024

As a matter of style, you could assert that the plugin isn't null, and if it is return without logging.

@NatureIsFrequency
Copy link
Contributor Author

For sure, however you'd most like code style I'll happily follow :)

Running clap-host outside of a debug environment e.g. direct from terminal does mean logging is seen.

In previous companies we tend to lean towards asserts with logging, to allow the debugger to break for those running and also logging for QA to include in reports.

I'll push another commit with assert later today, cheers Alexandre!

And cheers for allowing contribution to the project!

@NatureIsFrequency
Copy link
Contributor Author

Had a play with the following:

assert(_plugin);
if(!_plugin)
    return;

However, when the assert is hit it will trigger a SIGABRT.
So the program will abort without letting the user continue debugging.
And if the user is running the program without a debugger it will still crash.

Ideally we have a cross platform msvc DebugBreak() equivalent which I believe C++26 will introduce:
https://en.cppreference.com/w/cpp/utility/breakpoint

For now I'd suggest we keep the original commit with no assert to avoid crashes.
But it's your project so no problem if you'd prefer the assert, what do you think?

Cheers :)

@Trinitou
Copy link
Contributor

Trinitou commented Feb 9, 2024

I think we should rather look for the cause of the problem. I mean inside of PluginHost we are in full control of what is called when. There are two calls of PluginHost::setParentWindow:

  • For the one in Engine::loadPlugin it is sure that the plugin has been loaded successfully
  • But the call inside MainWindow::recreatePluginWindow looks a bit problematic. Without trying it out: To me it looks like there is a menu entry "Recreate Plugin Window" which is present (and enabled!) even if there is no plugin loaded. So either the menu entry should be automatically disabled if no plugin is loaded or the check should happen inside MainWindow::recreatePluginWindow

... same actually goes for the check in #40 and the menu entry "Load Native Plugin Preset"!

@NatureIsFrequency
Copy link
Contributor Author

Agreed yep, ideally the menu entry is disabled so both the user knows this is not a possible action, and the code is not executed

Great shout

@NatureIsFrequency
Copy link
Contributor Author

Whipped up an underlying fix in pull request #45
Cheers again for the suggestions both.

I'll close this pull request in favour of #45

@NatureIsFrequency NatureIsFrequency deleted the Fix_Issue_#42 branch February 10, 2024 00:11
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