-
Notifications
You must be signed in to change notification settings - Fork 250
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
Solve the sequencer client unregistering problem #610
Conversation
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 think I like this approach better than #609.
To solve the problem with custom callbacks not receiving the FLUID_SEQ_UNREGISTERING
event anymore, how about introducing another internal event FLUID_SEQ_UNREGISTER
(or similar). That new event is used in all places where we currently use FLUID_SEQ_UNREGISTERING
. But before we actually unregister the client in fluid_sequencer_send_now
, we send a FLUID_SEQ_UNREGISTERING
event to the callback to inform it of the upcoming unregister.
Also a breaking change, as custom clients can no longer be unregistered by sending a FLUID_SEQ_UNREGISTERING
event. But as the explicit unregister is no longer necessary for those clients anyway it feels like a more consistent API.
And IMHO, unregistering clients via an event feels fine as an internal implementation detail, but slightly weird as part of a public API.
// client found, remove it from the list to avoid recursive call when calling callback | ||
seq->clients = fluid_list_remove_link(seq->clients, tmp); | ||
|
||
// call the callback (if any), to free underlying memory (e.g. seqbind structure) |
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 think this comment would need to be updated (remove reference to seqbind structure)?
Ok, thanks. I also prefer this solution.
Misunderstanding (I guess the new if-clause in Clients will still receive the unregistering event (this is done in |
Yes, and I didn't notice that
Yes, that sounds much better :-) And I totally agree that this is a bug that needs to be fixed, not just a changed feature/behaviour. |
This is the second solution to the problem of #609.
The difference is that responsibility for calling
fluid_sequencer_unregister_client()
in case ofFLUID_SEQ_UNREGISTERING
events has been moved tofluid_sequencer_send_now()
. In other words, aFLUID_SEQ_UNREGISTERING
event now really unregisters the client, no matter how the client's callback function looks like. I'm afraid that technically this is a breaking change. This is also the reason why the unit testtest_seq_event_queue_sort
currently fails. (I'll fix the test if we decide to move forward with this approach.)Opinions?