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

[Christophe Fergeau]

On Fri, Sep 08, 2017 at 05:10:06AM -0400, Frediano Ziglio wrote:
> > 
> > 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.

And in spite of these flaws, my whole desktop environment is built on
top of this, and I would say my working environment's very stable.
GSignal far from being perfect, but it's working well enough.

> 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.

This is good, but then _we_ have to take into account the overall
consistency of the project, and where we want to head with it.
Each of us cannot individually come up with their own preferred 'signal'
mechanism and use that in their patches.

Christophe