frontend: Add template for Qt's invokeMethod

[cg2121]

Description

This adds a template for when OBS signals are called, so we don't have to call invokeMethod each time. This only works if the invokeMethod doesn't have calldata_t used with it.

Motivation and Context

Wanted to find a way so we didn't have to write a invokeMethod function for each OBS signal.

This is only used in MediaControls at this time. If this is accepted, I'll PR for other places later.

How Has This Been Tested?

Used media controls to make sure everything works as expected.

Types of changes

• Code cleanup (non-breaking change which makes code smaller or more readable)

Checklist:

• My code has been run through clang-format.
• I have read the contributing document.
• My code is not on the master branch.
• The code has been tested.
• All commit messages are properly formatted and commits squashed where appropriate.
• I have included updates to all appropriate documentation.

[Warchamp7]

As a general aside, I think we should always be using Qt::QueuedConnection explicitly for any invokeMethod calls based on my naive understanding, but @PatTheMav would have to confirm that.

[cg2121]

I thought Qt automatically selects Qt::QueuedConnection, if it is not in the same thread, I could be wrong though.

[PatTheMav]

I thought Qt automatically selects Qt::QueuedConnection, if it is not in the same thread, I could be wrong though.

Correct, Qt::AutoConnection is used, which internally uses Qt::QueuedConnection across threads, and Qt::DirectConnection otherwise, which is the expected behaviour (or at least I expect it from Qt).

---

I think I like the approach in general, but I'm not fully on board with the naming and the fact that it uses different template names for each call variant (qtMethod, qtMethodWithThing, qtMethodWithOtherThing) even though that's what polymorphism is for (the compiler should choose the correct template based on the arguments).

It would require a bit more template code to handle correctly, but if we go with templates to solve this, we should go all the way. So approach is correct, specific design of this API requires some tuning.

[cg2121]

I've removed the qtMethodSource and qtMethodItem templates, and combined everything into a single qtMethod template.

[PatTheMav]

While the code itself works (and kudos to building it out like that), I've been pondering what this change means architecturally and logically and whether it makes sense to generalise it this way.

My main problem with the implementation is that it moves the details of "what the callback needs to do" into the static function wrapper, conflating two independent responsibilities (of independent abstraction layers) into the same function:

• The wrapper itself should have one purpose (not multiple): To produce a global function whose function pointer can be passed to a C callback API
• Thus the generated wrapper should only accept the call made by the C API and forward it to the actual event handler that is specialised for the receiver.
• What the receiver intends/needs to do is a concern of the receiver, not of the callback wrapper (principles of "encapsulation" and "separation of concerns").

And as-is the wrapper can only wrap very specific callbacks, so even though the use-case is common across the entire OBS code base, it can only be used with member functions of QObjects (i.e. it cannot be used to wrap a normal instance member function for use with a libobs signal).

---

So IMO the template should be stripped down to a simple "callbackWrapper" that can be used to have the compiler generate static callback wrapper functions for any given class member function.

callbackWrapper<&MediaControls::playMedia> would still need to use a memberFunctionTraits struct to correctly deduct the class type of the callback to cast the opaque pointer to the correct type, but then simply use std::invoke:

std::invoke(callback, object, args...);

and playMedia obviously would need to accept the calldata_t pointer as part of the generalised callback API (for C-based callbacks the compiler should be able to correctly deduct what Args means and you can "peel off" the first void pointer for the cast).

It should always be the class's responsibility to implement what should happen when the signal is emitted and that implementation detail should not "leak" into a generalised template.

[PatTheMav]

As I mentioned in my earlier comment, the callbackWrapper should not make any assumptions about the type of callback it wraps and should just serve as a tool to have the compiler automatically create the necessary global function plumbing to connect C++ instance methods to a C-based callback API.

The detail of what should happen when the C API calls the callback function (and how it does its job, in this case using invokeMethod) is an implementation detail of the class. And indeed I think that the callback methods (and the specific implementation of them) is part of a class's "identity" and should not be generalised away.

(Case in point: If SetPlayingState code needs to run on the main thread as this suggests, then it should enforce this in its implementation and not rely on the "outside" to make it so.)

Otherwise callbackWrapper would be artificially limited to Qt slot methods and one could not use it to also wrap instance methods that can safely be executed on the same thread as the one the signal is emitted from.

[cg2121]

I think I get what you mean now. I just have to figure out a good way to do it.

[cg2121]

Is there any place in the code where we use callbacks without Qt's invokeMethod?