-
Notifications
You must be signed in to change notification settings - Fork 9
Avoid broken jack_on_info_shutdown callback and more checks #16
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
Avoid broken jack_on_info_shutdown callback and more checks #16
Conversation
apf/jackclient.h
Outdated
| { | ||
| (void)code; // avoid "unused parameter" warning | ||
| throw jack_error("JACK shutdown! Reason: " + std::string(reason)); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you keeping the old overloaded jack_shutdown_callback()?
Shouldn't this be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this will be called. I implemented a function overload for no arguments which should be the one getting registered. However, to avoid confusion I renamed according to shutdown_info (with arguments) and shutdown (without arguments). The whole chain of events is quite convoluted but now seems to be jack_on_shutdown -> _jack_shutdown_callback -> (cast)->jack_shutdown_callback -> jack_shutdown_callback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this will be called.
Yes, that's exactly my point.
It's never called, we don't need it, we should remove it. Right?
There is no need to define a jack_info_shutdown_callback() if we don't intend to ever use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was hoping that the windows specific issue will be resolved soon, as it seems this is related to the Jack library.
But I understand it's unnecessary for now. Would you prefer to remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, if we don't use it, we should remove it.
If the Windows issue will be solved at some point, that would be great, then we can simply change it back.
|
FYI: I fixed the CI error in #17. |
apf/jackclient.h
Outdated
| { | ||
| (void)code; // avoid "unused parameter" warning | ||
| throw jack_error("JACK shutdown! Reason: " + std::string(reason)); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this will be called.
Yes, that's exactly my point.
It's never called, we don't need it, we should remove it. Right?
There is no need to define a jack_info_shutdown_callback() if we don't intend to ever use it.
|
@chris-hld I would like to merge this, but what about my last two comments? |
|
Thanks @chris-hld! |
Related to jackaudio/jack2#919