[Spice-devel] [RFC spice-gtk v3 1/1] Gstreamer: Use GstVideoOverlay if possible

[Frediano Ziglio]

> 
> On Wed, May 16, 2018 at 3:54 PM, Frediano Ziglio <fziglio at redhat.com> wrote:
> >>
> >> On Wed, May 16, 2018 at 10:46:35AM +0200, Christophe Fergeau wrote:
> >> > On Wed, May 16, 2018 at 10:43:20AM +0200, Christophe Fergeau wrote:
> >> > > On Wed, May 16, 2018 at 10:23:01AM +0300, Snir Sheriber wrote:
> >> > > Yes, indeed, this is similar to what you were doing in earlier
> >> > > iterations.
> >> > >
> >> > > > I personally really like the current flow of the request for handle
> >> > > > using the signal and getting it as a response, avoiding of setting
> >> > > > and getting an handle from different components.
> >> > >
> >> > > Using signals as some generic way of calling a getter on some unknown
> >> > > class is rather unusual, and feels like something you should not
> >> > > really
> >> > > be using signals for.
> >> >
> >> > Just realized, this change is most likely an ABI break:
> >>
> >> (I promise I'll stop replying to myself :d). Not sure we are supporting
> >> inheritance from SpiceDisplayChannel, nor that it makes sense, so maybe
> >> not a big problem.
> >>
> >> Christophe
> >>
> >
> > Then class should be declare final is possible and details removed from
> > public interface.
> > Looking also at 941f4adf1322a5d2548af676059003df17613b47 seems to confirm
> > that this class was never meant to be inherited.
> 
> That's a commit from 0.1 era ;)
> 

Yes, you are right.
I though I looked in the past and found another ABI breakage more recent.

> We may have made some mistakes, but in theory, we bumped
> major/minor/etc correctly enough whenever adding/modifying/removing
> interface (breaking ABI).
> 
> During next break (some day), we may use the opportunity to make make
> most channel classes final. This is not a priority though. In the
> meantime, we don't need to add class signal handlers, and simply use
> the private struct for data.
> 

>From g_signal_new documentation:

class_offset
The offset of the function pointer in the class structure for this type. Used to invoke a class method generically.
Pass 0 to not associate a class method slot with this signal.

I suggested as a follow up to make this signal "private" not making the documentation
public. I think we don't need to use this signal as a method so I think we can remove
the field in SpiceDisplayChannelClass and pass 0 to avoid ABI break, kind of:

diff --git a/src/channel-display.c b/src/channel-display.c
index 3f2fb3dc..1c0021e8 100644
--- a/src/channel-display.c
+++ b/src/channel-display.c
@@ -470,9 +470,7 @@ static void spice_display_channel_class_init(SpiceDisplayChannelClass *klass)
     signals[SPICE_DISPLAY_STREAMING_MODE] =
         g_signal_new("streaming-mode",
                      G_OBJECT_CLASS_TYPE(gobject_class),
-                     0,
-                     G_STRUCT_OFFSET(SpiceDisplayChannelClass,
-                                     streaming_mode),
+                     0, 0,
                      NULL, NULL,
                      g_cclosure_user_marshal_POINTER__BOOLEAN,
                      G_TYPE_POINTER,
diff --git a/src/channel-display.h b/src/channel-display.h
index 9c51aa2a..31293094 100644
--- a/src/channel-display.h
+++ b/src/channel-display.h
@@ -141,8 +141,6 @@ struct _SpiceDisplayChannelClass {
                                gint x, gint y, gint w, gint h);
     void (*display_mark)(SpiceChannel *channel,
                          gboolean mark);
-    void (*streaming_mode)(SpiceChannel *channel,
-                           gboolean streaming_mode);

     /*< private >*/
 };


Frediano