[Spice-devel] [PATCH spice-server v2 7/8] Convert RedChannel heirarchy to GObject
Jonathon Jongsma
jjongsma at redhat.com
Tue Oct 11 21:03:30 UTC 2016
On Tue, 2016-10-11 at 09:55 -0400, Frediano Ziglio wrote:
> >
> > > > diff --git a/server/cursor-channel.h b/server/cursor-channel.h
> > > > index a3ddaa3..8b3bc17 100644
> > > > --- a/server/cursor-channel.h
> > > > +++ b/server/cursor-channel.h
> > > > @@ -19,6 +19,17 @@
> > > > # define CURSOR_CHANNEL_H_
> > > >
> > > > #include "common-graphics-channel.h"
> > > > +#include "red-parse-qxl.h"
> > > > +
> > > > +G_BEGIN_DECLS
> > > > +
> > > > +#define TYPE_CURSOR_CHANNEL cursor_channel_get_type()
> > > > +
> > > > +#define CURSOR_CHANNEL(obj) (G_TYPE_CHECK_INSTANCE_CAST((obj),
> > > > TYPE_CURSOR_CHANNEL, CursorChannel))
> > > > +#define CURSOR_CHANNEL_CLASS(klass)
> > > > (G_TYPE_CHECK_CLASS_CAST((klass),
> > > > TYPE_CURSOR_CHANNEL, CursorChannelClass))
> > > > +#define IS_CURSOR_CHANNEL(obj)
> > > > (G_TYPE_CHECK_INSTANCE_TYPE((obj),
> > > > TYPE_CURSOR_CHANNEL))
> > > > +#define IS_CURSOR_CHANNEL_CLASS(klass)
> > > > (G_TYPE_CHECK_CLASS_TYPE((klass),
> > > > TYPE_CURSOR_CHANNEL))
> > > > +#define CURSOR_CHANNEL_GET_CLASS(obj)
> > > > (G_TYPE_INSTANCE_GET_CLASS((obj),
> > > > TYPE_CURSOR_CHANNEL, CursorChannelClass))
> > > >
> > > > /**
> > > > * This type it's a RedChannel class which implement cursor
> > > > (mouse)
> > > > @@ -26,6 +37,22 @@
> > > > * A pointer to CursorChannel can be converted to a
> > > > RedChannel.
> > > > */
> > > > typedef struct CursorChannel CursorChannel;
> > > > +typedef struct CursorChannelClass CursorChannelClass;
> > > > +typedef struct CursorChannelPrivate CursorChannelPrivate;
> > > > +
> > > > +struct CursorChannel
> > > > +{
> > > > + CommonGraphicsChannel parent;
> > > > +
> > > > + CursorChannelPrivate *priv;
> > > > +};
> > > > +
> > > > +struct CursorChannelClass
> > > > +{
> > > > + CommonGraphicsChannelClass parent_class;
> > > > +};
> > > > +
> > > > +GType cursor_channel_get_type(void) G_GNUC_CONST;
> > > >
> > >
> > > Why we need to expose this here ? On the C file is not enough ?
> >
> > This is used above in all of the 'standard' GObject macros (e.g.
> > CURSOR_CHANNEL(), IS_CURSOR_CHANNEL(), etc). It's standard practice
> > to
> > expose it in the header. In this case, it's possible that we could
> > move
> > it down to the .c file, depending on whether any other files use
> > these
> > macros, but I don't see much advantage to doing so. I'd prefer to
> > leave
> > the standard GObject boilerplate here.
> >
>
> Some months ago my sister decided it was time to change the car.
> As she has some child she decided for a bigger car. She was also
> thinking
> about having additional space to allow to go to holidays. After
> viewing
> different car we discussed if it was worth buying a bigger car
> considering
> it was more expensive to buy and to maintain (insurance and fuel).
> At the end she decided to buy a car enough for the child and family
> but that was not worth buying some more big one just for the holiday
> opting for a car roof bag.
> The 'standard' solution for the car is buying a bigger car but not
> all standard solutions are the best.
> The main reason why the standard is that way is that it supports more
> cases and so it's easier to put in a guide or tutorial or a book...
> just that we are not writing a book or following a tutorial or guide.
>
> Exposing CursorChannel and CursorChannelClass in the header is a way
> to tell that this is the object structure and can be used for
> instance
> for inheritance. Some languages introduced the "final" keyword to
> avoid
> inheritance... C just do not support objects so not exposing the
> structure it's a way to use.
> Also is there is no need to expose the structure why to do it?
> Another reason in favor of not exposing it is that you could avoid to
> pay the penalty (memory, code and cpu) of the "priv" field.
>
> About all that macros boilerplate I don't understand why they
> don't came to a better solution, something like
>
> G_TYPE_CAST(pointer, RedChannel)
>
> instead of having to define all these macro for every object...
These are for convenience. You could use G_TYPE_CHECK_INSTANCE_CAST()
directly if you want, but it's nicer to use a convenience macro like
CURSOR_CHANNEL().
> Require that if there is a GObject typename "RedChannel" you
> have a RedChannel_get_type() function does not seem so hard
> or cause so many problems (but probably I miss something).
> Also instead of
>
> if (IS_CURSOR_CHANNEL(obj))
>
> why not using
>
> if (CURSOR_CHANNEL(obj))
>
> and avoid to define 2 macros?
It's mostly a matter of convention. Most people simply expect these
macros to exist with a standard naming convention for any GObject type.
It's true that not all of these macros are always used for every type.
For instance, In this particular case we don't use them except within
the cursor-channel.c implementation. So we could certainly move them
down to the .c file (or remove them entirely) as you suggested. But if
I came across a GObject implementation that didn't define the standard
cast macros in the header, it would strike me as odd.
> As macro only users of these headers can use them and we
> are the only users.
> So can't we define our macros so we don't have all that
> boilerplate?
We could define our own macros, I suppose. But why? Hundreds of GObject
applications and libraries do it this way. I don't think there's much
to be gained by bucking tradition here and implementing our own custom
cast macros just to avoid a symbol in the header.
> > > >
> > > > diff --git a/server/inputs-channel.c b/server/inputs-channel.c
> > > > index 83c1360..c351dad 100644
> > > > --- a/server/inputs-channel.c
> > > > +++ b/server/inputs-channel.c
> > > > @@ -57,6 +57,83 @@
> > > > #define RECEIVE_BUF_SIZE \
> > > > (4096 + (REDS_AGENT_WINDOW_SIZE +
> > > > REDS_NUM_INTERNAL_AGENT_MESSAGES) *
> > > > SPICE_AGENT_MAX_DATA_SIZE)
> > > >
> > > > +G_DEFINE_TYPE(InputsChannel, inputs_channel, RED_TYPE_CHANNEL)
> > > > +
> > > > +#define CHANNEL_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE((o),
> > > > TYPE_INPUTS_CHANNEL, InputsChannelPrivate))
> > > > +
> > > > +struct InputsChannelPrivate
> > > > +{
> > > > + uint8_t recv_buf[RECEIVE_BUF_SIZE];
> > > > + VDAgentMouseState mouse_state;
> > > > + int src_during_migrate;
> > > > + SpiceTimer *key_modifiers_timer;
> > > > +
> > > > + SpiceKbdInstance *keyboard;
> > > > + SpiceMouseInstance *mouse;
> > > > + SpiceTabletInstance *tablet;
> > > > +};
> > > > +
> > > > +
> > > > +static void
> > > > +inputs_channel_get_property(GObject *object,
> > > > + guint property_id,
> > > > + GValue *value,
> > > > + GParamSpec *pspec)
> > > > +{
> > > > + switch (property_id)
> > > > + {
> > > > + default:
> > > > + G_OBJECT_WARN_INVALID_PROPERTY_ID(object,
> > > > property_id,
> > > > pspec);
> > > > + }
> > > > +}
> > > > +
> > > > +static void
> > > > +inputs_channel_set_property(GObject *object,
> > > > + guint property_id,
> > > > + const GValue *value,
> > > > + GParamSpec *pspec)
> > > > +{
> > > > + switch (property_id)
> > > > + {
> > > > + default:
> > > > + G_OBJECT_WARN_INVALID_PROPERTY_ID(object,
> > > > property_id,
> > > > pspec);
> > > > + }
> > > > +}
> > > > +
> > >
> > > I noted there are these empty function here and in different
> > > file.
> > > Don't GObject provide some default functions like these?
> >
> > In general, GObject is a pain and has a lot of boilerplate. So I
> > often
> > use a GObject "generator" application that creates a bunch of
> > function
> > skeletons. In this case I simply forgot to delete the ones I didn't
> > need.
> >
>
> I personally don't like that much generators if they generate too
> much code as you'll have to maintain lot of code.
> In this case for instance if the next version of GObject decide to
> change the way to implement properties you'll have to update all
> that code.
> Just to make clear I don't consider programs like bison/flex/gperf
> as generators as you don't expect to maintain their output but
> you just use them in the chain.
In case it wasn't clear, I was trying to suggest that I will remove
this extra code. I simply missed it.
Jonathon
More information about the Spice-devel
mailing list