On Thu, 2016-05-26 at 06:02 -0400, Frediano Ziglio wrote:
> >
> > On Fri, 2016-05-13 at 10:16 +0100, Frediano Ziglio wrote:
> > > The existence of this class can be hidden to user of CursorChannel class
> > >
> > > Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> > > ---
> > > server/cursor-channel.c | 15 +++++++++++----
> > > server/cursor-channel.h | 7 ++-----
> > > server/red-worker.c | 4 ++--
> > > 3 files changed, 15 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> > > index 66f0181..688bab4 100644
> > > --- a/server/cursor-channel.c
> > > +++ b/server/cursor-channel.c
> > > @@ -31,6 +31,8 @@
> > > #define CURSOR_CACHE_HASH_MASK (CURSOR_CACHE_HASH_SIZE - 1)
> > > #define CURSOR_CACHE_HASH_KEY(id) ((id) & CURSOR_CACHE_HASH_MASK)
> > >
> > > +typedef struct CursorChannelClient CursorChannelClient;
> > > +
> > > enum {
> > > RED_PIPE_ITEM_TYPE_CURSOR = RED_PIPE_ITEM_TYPE_COMMON_LAST,
> > > RED_PIPE_ITEM_TYPE_CURSOR_INIT,
> > > @@ -439,9 +441,9 @@ CursorChannel* cursor_channel_new(RedWorker *worker)
> > > return cursor_channel;
> > > }
> > >
> > > -void cursor_channel_client_migrate(CursorChannelClient* client)
> > > +void cursor_channel_client_migrate(RedChannelClient *rcc)
> > > {
> > > - RedChannelClient *rcc;
> > > + CursorChannelClient* client = (CursorChannelClient*)rcc;
> > >
> > > spice_return_if_fail(client);
> > > rcc = RED_CHANNEL_CLIENT(client);
> > > @@ -548,7 +550,7 @@ void cursor_channel_reset(CursorChannel *cursor)
> > > }
> > > }
> > >
> > > -void cursor_channel_init(CursorChannel *cursor, CursorChannelClient
> > > *client)
> > > +static void cursor_channel_init_client(CursorChannel *cursor,
> > > CursorChannelClient *client)
> > > {
> > > spice_return_if_fail(cursor);
> > >
> > > @@ -565,6 +567,11 @@ void cursor_channel_init(CursorChannel *cursor,
> > > CursorChannelClient *client)
> > > red_channel_pipes_add_type(RED_CHANNEL(cursor),
> > > RED_PIPE_ITEM_TYPE_CURSOR_INIT);
> > > }
> > >
> > > +void cursor_channel_init(CursorChannel *cursor)
> > > +{
> > > + cursor_channel_init_client(cursor, NULL);
> > > +}
> > > +
> > > void cursor_channel_set_mouse_mode(CursorChannel *cursor, uint32_t mode)
> > > {
> > > spice_return_if_fail(cursor);
> > > @@ -592,5 +599,5 @@ void cursor_channel_connect(CursorChannel *cursor,
> > > RedClient *client, RedsStream
> > > red_channel_client_ack_zero_messages_window(rcc);
> > > red_channel_client_push_set_ack(rcc);
> > >
> > > - cursor_channel_init(cursor, ccc);
> > > + cursor_channel_init_client(cursor, ccc);
> > > }
> > > diff --git a/server/cursor-channel.h b/server/cursor-channel.h
> > > index 6bba822..6c89bc3 100644
> > > --- a/server/cursor-channel.h
> > > +++ b/server/cursor-channel.h
> > > @@ -24,14 +24,11 @@
> > > #include "red-parse-qxl.h"
> > >
> > > typedef struct CursorChannel CursorChannel;
> > > -typedef struct CursorChannelClient CursorChannelClient;
> > > -
> > > -#define CURSOR_CHANNEL_CLIENT(Client) ((CursorChannelClient*)(Client))
> > >
> > > CursorChannel* cursor_channel_new (RedWorker *worker);
> > > void cursor_channel_disconnect (CursorChannel
> > > *cursor_channel);
> > > void cursor_channel_reset (CursorChannel *cursor);
> > > -void cursor_channel_init (CursorChannel *cursor,
> > > CursorChannelClient* client);
> > > +void cursor_channel_init (CursorChannel *cursor);
> > > void cursor_channel_process_cmd (CursorChannel *cursor,
> > > RedCursorCmd *cursor_cmd);
> > > void cursor_channel_set_mouse_mode(CursorChannel *cursor,
> > > uint32_t mode);
> > > void cursor_channel_connect (CursorChannel *cursor,
> > > RedClient *client,
> > > @@ -40,6 +37,6 @@ void cursor_channel_connect
> > > (CursorChannel *cursor, RedClien
> > > uint32_t *common_caps,
> > > int
> > > num_common_caps,
> > > uint32_t *caps, int
> > > num_caps);
> > >
> > > -void cursor_channel_client_migrate(CursorChannelClient*
> > > client);
> > > +void cursor_channel_client_migrate(RedChannelClient
> > > *client);
> >
> > Seems like this may be a potential future RedChannelClient vfunc? Not really
> > relevant to this patch though.
> >
>
> It's already a callback (so a vfunc).
Well, sort of. There is a client_cbs.migrate function callback owned by
RedChannel, but this function is never assigned to that function pointer:
$ git grep cursor_channel_client_migrate
server/cursor-channel.c:void cursor_channel_client_migrate(RedChannelClient
*rcc)
server/cursor-channel.h:void
cursor_channel_client_migrate(RedChannelClient *client);
server/red-worker.c: cursor_channel_client_migrate(rcc);
And therefore it does not really act like a vfunc since it is called directly
rather than through the function pointer:
[red-worker.c]
static void handle_dev_cursor_migrate(void *opaque, void *payload)
{
RedWorkerMessageCursorMigrate *msg = payload;
RedChannelClient *rcc = msg->rcc;
spice_info("migrate cursor client");
cursor_channel_client_migrate(rcc);
}
I think that the function should probably be a vfunc of RedChannelClient rather
than a function pointer owned by RedChannel. In other words, the
RedChannelClient struct should have a 'migrate' function pointer. Then
cursor_channel_client_migrate() could be static wouldn't need to be exposed in
the header at all.
>
> Actually it's quite relevant for this patch.
> This patch is trying to quite strongly demonstrate that users of a channel
> does not need to know even the EXISTENCE of a client class.
> This was not well defined in the source and is getting to be hidden by GObject
> patches (not by GObject needs, just was easier to retain this knowledge).
> The simple needs to include a "cursor-channel-client.h" in "cursor-channel.h"
> is now explicitly a mistake. This is valid in mostly RedChannel hierarchies
> (I'm trying to define it even for DisplayChannel but code is quite entangled,
> but I'm getting some patches).
> The derived RedChannel and relative derived RedChannelClient are quite bound
> together and some stuff (like the pipe items between them) define the
> relationship between them (and don't need to be known by users); this is
> the reason I suggested to move Item declarations (like
> RED_PIPE_ITEM_TYPE_* in a11b785f191d2381932f8c1bb6281539f2afe483) to
> client header instead of main one I think I suggested the move for another
> class).
Yes, I agree with all of this.
>
> About the vfunc... I think it's better to explain this in reply of 4/4 which
> is trying to document this.
>
> > >
> > > #endif /* CURSOR_CHANNEL_H_ */
> > > diff --git a/server/red-worker.c b/server/red-worker.c
> > > index f6d626b..ce65444 100644
> > > --- a/server/red-worker.c
> > > +++ b/server/red-worker.c
> > > @@ -687,7 +687,7 @@ static void dev_create_primary_surface(RedWorker
> > > *worker,
> > > uint32_t surface_id,
> > > red_channel_push(&worker->display_channel->common.base);
> > > }
> > >
> > > - cursor_channel_init(worker->cursor_channel, NULL);
> > > + cursor_channel_init(worker->cursor_channel);
> > > }
> > >
> > > static void handle_dev_create_primary_surface(void *opaque, void
> > > *payload)
> > > @@ -1000,7 +1000,7 @@ static void handle_dev_cursor_migrate(void *opaque,
> > > void
> > > *payload)
> > > RedChannelClient *rcc = msg->rcc;
> > >
> > > spice_info("migrate cursor client");
> > > - cursor_channel_client_migrate(CURSOR_CHANNEL_CLIENT(rcc));
> > > + cursor_channel_client_migrate(rcc);
> > > }
> > >
> > > static void handle_dev_set_compression(void *opaque, void *payload)
> >
> > Acked-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> >
> >
> >
>
> Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel