[Spice-devel] [PATCH spice-server 1a/3] red-channel: Use directly a GArray to pass capabilities

[Christophe Fergeau]

On Wed, Mar 01, 2017 at 05:39:09AM -0500, Frediano Ziglio wrote:
> > 
> > On Tue, Feb 28, 2017 at 12:14:25PM -0500, Frediano Ziglio wrote:
> > > > 
> > > > On Tue, Feb 28, 2017 at 09:43:33AM +0000, Frediano Ziglio wrote:
> > > > > Capabilities where almost always passed using 2 arguments,
> > > > > a number of elements and an array but then before using
> > > > > these were converted to a GArray.
> > > > > Converting to GArray much earlier allows to easily pass
> > > > > the capabilities around.
> > > > 
> > > > Hey, nice improvement!
> > > > 
> > > > > 
> > > > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > > > ---
> > > > > I don't know if is worth defining an abstract type
> > > > > instead of using a simple GArray.
> > > > > Maybe a "typedef GArray RedCapabilities" could help future
> > > > > replacement of the GArray?
> > > > 
> > > > There's already a RedChannelCapabilities in red-channel.h, I'd reuse
> > > > this one.
> > > > 
> > > 
> > > Mumble... contains both common_caps and normal ones but can
> > > be done, we always pass them together.
> > > I was thinking about just joining caps+num.
> > 
> > Yeah, I know, but during review I came across that preexisting type.
> > Since that type is already there, imo it's even cleaner to pass a single
> > set of caps rather than always passing the 2 arrays.
> > 
> > > 
> > > > > On the other side this patch moves most of the array boxing
> > > > > into a single function.
> > > > 
> > > > I'd create the GArray one level higher, in reds_channel_do_link() rather
> > > > than red_channel_connect, let reds.c do the grunt work, and have
> > > > RedChannel deal with higher level types.
> > > > 
> > > > Christophe
> > > > 
> > > 
> > > If you want to reuse RedChannelCapabilities we could just box this
> > > type and avoid the GArray change.
> > > Or use GArray in a follow up (or not!).
> > > Other work for reds.c? Is not enough the cacophonous stuff in here?
> > 
> > I think I'd change RedChannelCapabilities to store 2 GArrays and pass
> > that around. As RedChannel properties, it's probably easier to keep 2
> > different GArrays (?)
> > 
> > Christophe
> > 
> 
> I missed tha main channel so now the patch make stuff even better,
> https://cgit.freedesktop.org/~fziglio/spice-server/commit/?h=common_graphics_channel_client&id=9ae24c5bf2528e6debf3465ec8760983007ea108
> 
> About possible improves I'm quite of the opinion that would be
> better to have a follow up.

Yep, probably, in your patch you were wondering about using a new type
VS GArray, I merely pointed out the preexisting type which could be
reused. I'm indeed not fully sure about how I'd want the end result to
look like, nor what the git history leading there would be.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170301/7e7a2786/attachment.sig>