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

[Christophe Fergeau]

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"

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.

Christophe