-
Notifications
You must be signed in to change notification settings - Fork 275
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
Setting a default error callback, two ideas #266
Comments
A constructor argument would be my preference. Would this potentially replace the try/catch statements for sending and connecting to ports? |
What is the benefit of an error handler over try/catch?
With the callback solution you just drop the type safety for exceptions. On the other hand in the background threads you cannot throw exceptions and callbacks must be realtime ready. So you must not interact with the user in such a callback. Otherwise everything might break. Error reporting starts to get interesting after a port is opened, because RtMidi cannot pass information it received before to the application. So checking if everything is ok can be done when the port is opened. And before you open a port there is plenty of time to install a callback. Global callbacks have the additional drawback that you must think about a way to find out which object is affected when the callback is called. Note: you don't have access to the internal state of the API. |
@keinstein interestingly i was trying to implement your own idea here from #228 ;) So to be honest I am not sure, maybe exceptions are fine in fact, in the case of #228? There is the idea in RtAudio at least of moving away from the use of exceptions... I completely open to the possibility that there is no need for this change.. but I guess with the existence of the |
Moving away from exceptions would be moving away from C++. OK. RtMidi never used the full power of C++ and its type checking algorithms (e. g. it uses void pointers to pass user data instead of using inheritence and virtual functions, same with the backend data). In the internet you can find statements that complain that RtMidi follows C philosophy. Modern exception implementations are fast as long as there are no exceptions thrown. So from that point of view throwing them in functions that are called from user code can simplify the code a lot. On the other hand in backend callbacks (e.g. MIDI receive callbacks) the calling function usually doesn't know how to deal with exceptions. Thus they are dangerous in these callbacks as uncaught exceptions may kill the software. Here we need error callbacks for communication with the user code. My concerns against putting the error callback into the constructor is that it bloats the constructor interface if we are adding more and more parameters to it. And it gets hard for the user code to handle them, as they have a strict linear order in which they can be omitted. If you have 10 optional parameters, the first optional parameter is nearly mandatory as it must be provided always when one optional parameter shall be changed. If we had a proper way to pass arbitrary parameters to the backends, then a realtime error callback parameter could be one of them. I personally miss the options to control the JACK server using RtMidi. In automated tests we would like to start it automatically, while in user applications this probably should be configurable by the end user. The more I think about it, the more I think it's right to make a destinction between error callbacks for the realtime part and using exceptions for the non-realtime part. |
Bumping this :) I wouldn't opt against exceptions because there is a separate C++ API out there, and one could just manually set the error handler in the struct before the "constructor" was called. To me, exceptions seem like the most elegant and consistent solution to this problem, because minimal changes to the code are needed. |
Looking at #228, the idea is that it is problematic not to be able to provide an error handler before the API was initialized, making it impossible to catch and display warnings in some other context.
Two ideas to handle this:
RtMidi::setDefaultErrorHandler
which can be called early in the program and will be used if no RtMidi object has a specific handler set. This is implemented here: radarsat1@c5bff5eTo be honest I am not sure which is best, they both seem to have some advantages. For (2), the default value for the parameter is
nullptr
, so it is not required to give anything here and RtMidi will work just as before. Similar for (1), ifsetDefaultErrorHandler
is not called. So neither changes the API.I think it comes down to preference, so asking for opinions. Constructor arguments or static function?
The text was updated successfully, but these errors were encountered: