[Spice-devel] [RFC PATCH spice-server v3 10/20] stream-channel: Allows to register callback to get new stream request

[Frediano Ziglio]

> 
> On Fri, Aug 25, 2017 at 12:22:41PM -0400, Frediano Ziglio wrote:
> > > 
> > > On Fri, 2017-08-25 at 05:39 -0400, Frediano Ziglio wrote:
> > > > > 
> > > > > I had a similar comment in a different patch during the last
> > > > > version of
> > > > > the series, but I personally would prefer a signal to handle this
> > > > > situation. For example, StreamChannel could have a "supported-
> > > > > codecs"
> > > > > property, Then when a new client connected, or a client
> > > > > disconnected,
> > > > > it would update this property. The StreamDevice would connect to
> > > > > the
> > > > > "notify::supported-codecs" signal, and then it would start and stop
> > > > > the
> > > > > stream as necessary. I think that encapsulates the logic of
> > > > > starting
> > > > > and stopping the stream a little bit better within the device class
> > > > > as
> > > > > well.
> > > > > 
> > > > > After writing the above paragraph, I did a little more
> > > > > investigation
> > > > > and I noticed that DisplayChannel already has a similar design --
> > > > > it
> > > > > has a "video-codecs" property.
> > > > > 
> > > > > Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>
> > > > > 
> > > > 
> > > > As long as I can have type safety, static binding and
> > > > I don't have to write too much I'm not against properties
> > > > and signal.
> > > > 
> > > > As far as I know this is not possible with GObject,
> > > > at least not in the current C implementation.
> > > > 
> > > > Frediano
> > > 
> > > Yes, there is a tradeoff between type-safety and convenience when
> > > you're using C. But we already have plenty of signals and properties in
> > > other parts of the code. Are you suggesting that we should not have any
> > > of these in the code because they're not fully type-safe? And if not:
> > > why should we treat this class differently and refuse to use them here
> > > when we already use them (for good reasons) elsewhere?
> > > 
> > 
> > Callbacks are managed in spice-sever in lot of different way
> > - normal functions + data (like this);
> > - passing list of functions;
> > - overriding methods in GObjects;
> > - signals (very few!).
> > 
> > We limit the usage of property to the constructor case with few
> > exceptions and signals are limited to "sin" and "video-codecs".
> 
> This limited usage is mainly because we did not get to it yet :)
> So far a lot of the focus was on splitting the code in smaller/more
> manageable classes, for that we chose to use GObject, and to use
> GObject, we had to change some of the code to use properties.
> The rest of the code did not change, as the focus really was on
> introducing a meaningful split in objects.
> But every time I see a foo_on_frobnicate() method, my reaction is always
> "I really should look if it makes sense to replace this with signals"
> 

Honestly the GLib implementation of signals is the worst I ever seen.
If I look at stuff like this (not a language I use)
https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/delegates/using-delegates
I can say the advantage. If I look at GLib implementation honestly is
really not comparable.
I can understand the gain of using proper signal implementation as
allows to dynamically bind actions to events, just GLib is not
"proper" at all. No thread safe, no type safe.
About dynamic binding I would say that this can be worked around
using some macros and mainly avoiding string literals, kind of

   red_channel_connect_SIGNAL_NAME(channel, WHATEVER);

> Yes they could have more type safety, yes they could be faster. I don't
> expect we'll have tons of signals in the code, and we know we have to be
> careful when we see one, so I don't expect a lot of issues with them.
> 
> We really should be using signals instead of our homegrown solution when
> appropriate. Not a perfect tool, but the one which is used in our
> codebase.
> 

Currently we use C function pointers. Yes we pass function pointer +
argument pointer but I would say it's not far from base C.

For every library I use, or better every part of library I use
I consider the integration of it in the project. I can use
part of a library to do some stuff and use another library
that best suite me for other part even if the same function is
provided on the first one. Integration have it's costs to consider.
If a library can provide the feature I need but the integration
cost is not worth the effort I search another solution, write
my own or I wrap the feature in a more considerate API if needed.

OT: this reminds me Christophe flight recorder proposal.

> Christophe
> 

Frediano