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

[Frediano Ziglio]

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

unfortunately we are not implementing desktop application.

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

That's good but this lead different possibilities:
1- use the current plain C way;
2- use GLib signals directly;
3- use another GLib signal implementation;
4- wrap GLib signals;
5- wrap another signal implementation.

Just I'm against option 2.

Frediano