Re: [spice-server 01/10] Remove unused 3rd red_channel_register_client_cbs() arg

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 
> > 
> > On Fri, Feb 10, 2017 at 08:27:27AM -0500, Frediano Ziglio wrote:
> > > > 
> > > > On Wed, Feb 08, 2017 at 12:10:52PM -0600, Jonathon Jongsma wrote:
> > > > > On Tue, 2017-02-07 at 11:59 +0100, Christophe Fergeau wrote:
> > > > > > It was probably meant to be used as a "user_data" argument for the
> > > > > > various callbacks, but turns out not to be used.
> > > > > 
> > > > > Hmm, it looks like the last use of this 'data' variable was removed
> > > > > in
> > > > > 96e94c6f. That commit used g_object_set_data() to attach the
> > > > > 'dispatcher' object to the channel object rather than setting priv-
> > > > > >data via the _register_client_cbs() function. I'm not too fond of
> > > > > g_object_set_data() for that use (even though apparently I was the
> > > > > one
> > > > > that did it). I wonder if there's a better approach than either of
> > > > > these approaches... I'll try to review the rest of the patches in
> > > > > this
> > > > > series and come back to this...
> > > > 
> > > > It seems the use of g_object_set_data() for "dispatcher" could easily
> > > > be
> > > > changed to a regular gobject property as it's only set right after
> > > > creating the cursor/display channels.
> > > > 
> > > > Christophe
> > > > 
> > > 
> > > -1 for property
> > 
> > Not so useful without a rationale... "gobject property" is not exclusive
> > with foo_get_dispatcher()/foo_set_dispatcher() methods if that's the
> > reason for your -1.
> > 
> > Christophe
> > 
> 
> Surely better, even better the Decorator pattern!
> 

A more thought on the original patch. The original patch is not adding
any regression, the data/opaque field is not used.

Maybe we could just apply the patch and add some a TODO comment
for a follow up ?

Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]