Hi
On Thu, May 17, 2018 at 1:50 PM, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
>>
>> On Wed, May 16, 2018 at 3:54 PM, Frediano Ziglio <fziglio@xxxxxxxxxx> 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:
>
I'd rather follow Christophe plan to call
channel_display_set_window_handle() early on. This API should be
public.
> 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
--
Marc-André Lureau
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel