Re: Another iteration of the properties-on-interfaces patch

[Tim Janik <timj gtk org>]

On 22 Jul 2003, Owen Taylor wrote:

> > > +void
> > > +g_object_interface_install_property (gpointer      g_iface,
> > > +				     GParamSpec   *pspec)
> > > +{
> > > +  GTypeInterface *iface_class = g_iface;
> > > +
> > > +  g_return_if_fail (G_TYPE_IS_INTERFACE (iface_class->g_type));
> > > +  g_return_if_fail (G_TYPE_IS_OBJECT (iface_class->g_instance_type));
> >
> > ugh. there's G_TYPE_CHECK_CLASS_TYPE (g_iface, G_TYPE_OBJECT) for that.
>
> If casting a GTypeInterface to a GTypeClass is acceptable, then
> the *first* check maybe can be written as
>
>  G_TYPE_CHECK_CLASS_TYPE (g_iface, G_TYPE_IS_INTERFACE)
>
> But the second one can't be changed that way - we aren't checking
> that the interface is a G_TYPE_OBJECT, we are checking that it's
> instance_type is G_TYPE_OBJECT; there aren't any macros for that
> that I know of.

that is the same kind of check. g_type_is_a (iface_type, object_type);
for an iface_type that has object_type or one of its parents as prerequisite
does succeed.
in general, you shouldn't dereference any of the GType instance/class or iface
structure fields. if you have to, that means there's a macro missing from gtype.h.
and yes, interfaces may be cast into classes, they share the same first member.

> > > +/**
> > > + * g_object_interface_find_property:
> > > + * @iface_type: an interface type
> > > + * @property_name: name of a property to lookup.
> > > + *
> > > + * Find the #GParamSpec with the given name for an
> > > + * interface.
> > > + *
> > > + * Return value: the #GParamSpec for the property of the
> > > + *  interface with the name @property_name, or %NULL
> > > + *  if no such property exists.
> > > + **/
> > > +GParamSpec*
> > > +g_object_interface_find_property (GType         iface_type,
> > > +				  const gchar  *property_name)
> > > +{
> > > +  g_return_val_if_fail (G_TYPE_IS_INTERFACE (iface_type), NULL);
> > > +  g_return_val_if_fail (property_name != NULL, NULL);
> > > +
> >
> > i'm not very fond of taking just a GType here. an interface
> > intiializer will have to be called anyway for this function to
> > suceed, so taking just a GType and not an interface class, only
> > suggests you could use it on pure interface types which is simply
> > not true.
>
> On one hand, yes, if you require an interface class you do
> make explicit the trap that the base_init function has to
> already been called. On the other hand, it makes the job
> of someone trying to introspect an interface even more difficult.
>
> With the current proposal, making interface introspection
> in gtk-doc is the same as for signals ... you just have to order
> things right.
>
> With your proposed interface, you'd have to hunt through all the
> classes to find one that implemented the interface.
>
> It's pretty clear to me what the *right* fix is:
>
>   Make interfaces "classed" by having the class_init() function,
>   if supplied, be used to create a prototype class that
>   is copied for instances of those interfaces, rather than
>   using g_new0().

hm, there'd be an instance type of 0 in that GTypeInterface structure
(for the duration of that interface's class_init() it's user visible).

> Advantages:
>
>  - Makes interfaces more like classes, so less confusing
>  - Fixes the ugly base_init() hack for signals and properties
>  - It might even be occasionally useful to provide default
>    implementations.

base_init() currently allowes for default implementaitons, so that
is a solved problem.

> I think that can be done compatibly,. *However* it's also a major
> change, and doubtless has all sorts of unexpected side effects
> throughout gtype.c. Not something I wanted to try and tackle.

it doesn't look all that intriguing to implement actually. i'll see
whether i can do that this weekend (will leave for a day trip this
morning, so i can't get around to that earlier).

> Without making that change, I think taking an interface class
> here is really too ugly.

hm. do you mean to imply, that the user should be able to get
his hand on this "default" interface class, by means other than
storing away a pointer during class init?
note, that it can't be gotten via g_type_interface_peek() because
that requires an instance type, and it can't be gotten via
g_type_class_ref() either since it's not a real class.

so, i don't see this change make the gtk-doc issue any
easier actually.

> I suppose if we simply *planned* to make the change but didn't
> actually to it for GObject-2.4, it might be OK to accept the
> temporary ugliness.

> > > +static gboolean
> > > +g_object_class_check_iface_properties (GObjectClass *class)
> > > +{
> >
> > [loop over all interfaces]
> >   [loop over all properties]
> >     [pool lookup for each property]
[...]

> I think having this check is important - otherwise, it's really easy
> to forget one property from an interface when implementing it, and
> not discover until months later when someone hits a problem at run
> time.
>
> What I said in:
>
> http://mail.gnome.org/archives/gtk-devel-list/2003-March/msg00237.html
>
> was:
>
> } * It's not clear to me when the checks for an object class
> }   implementing all necessary properties should happen. The
> }   patch currently does it in g_object_newv(), but:
> }
> }    a) that's pretty clearly too expensive (with some thread
> }       locking trouble, you could manage to cache the info)
> }    b) you can add interfaces after an object already exists.
> }
> }   The right point seems to be right after interface_init is
> }   called for the class/interface pair, but there is no
> }   way for gobject.c to hook into that.

i'm not sure we shouldn't go for simply adding a hook to gtype.[hc] for
a callback to be called after all interfaces got initialized for this class.

having this check special cased in g_object_new() is way ugly and requires
further special casing to ensure iface properties are checked for every
parent class but not over-and-over for the same interfaces.

> > i gave the GParamSpecOverride idea we discussed some more thoughts,
> > and it occoured to me, that it contradicts one of the most important
> > purposes that brought param specs into existance. that is, all information
> > related to a property (be that on an object, an interface or a procedure)
> > are meant to be accessible in one place (from one structure).
> > this allowes APIs like:
> > GtkWidget*  build_property_gui  (GParamSpec *pspec, GValue *value);
> > void        pspec_send_remote   (GParamSpec *pspec, Wire*);
> > GParamSpec* pspec_receive_remote (Wire*);
> > etc.
> > with GParamSpecOverride, that'd not anymore be the case, thus breaking
> > all kind of APIs like the above already created.
> >
> > REDIRECT pspecs are basically ok for the above, since once you
> > retrieved a REDIRECT pspec (say from object_class_list_properties), you
> > can fetch the resulting pspec through one indirection:
> > if (pspec->flags & G_PARAM_REDIRECT)
> >   rt_pspec = g_param_spec_get_redirect_target (pspec);
> > this can be accomplished by:
> > - us changing g_object_list_properties() to do this on the fly (to some extend
> >   an API change since currently you get the pspecs you installed from this
> >   function)
> > - offering g_object_list_effective_properties() which does this on the fly
> > - putting the burden on the user after using g_object_list_properties()
> > (the above applies to g_object_find_property() as well)
> >
> > this is not the case for GParamSpecOverride pspecs though, since the
> > resulting parameter would be something like the rt_pspec + anything
> > that the original pspec wants to override, like name, nick, blurb or
> > the default. there's no single pspec anymore specifying actual parameter
> > behaviour.
>
> What you seem to be objecting to is not GParamSpecOverride, but
> the notion that you can override anything but the implementation;
> if we just allowed implementation overriding, then something
> like GParamSpecOverride becomes even more central.

> Weren't you the person who suggested using the override facility
> to change default values, however?

i think so. but i hadn't fully realised at that point, that we either:
- force people into a major API change, where they need to special case
  REDIRECT and Override pspecs (and may ned to change their own APIs
  based on pspecs), or
- imeplement clone() functionality for every pspec type. (could also be
  copy-constructors besides the normal constructors in gparamspecs.h)

the second solution is still one we can persue for future releases, and
if we find a person brave enough to actually attempt writing all those
copy-constructors. the first option however introduces API changes that
i'd like to avoid at this point if at all possible.

> Anyways, I really don't see a problem with requiring people writing
> property introspection code to know about GParamSpecOverride
> or the REDIRECT flag - if we added specific new type of GParamSpec,
> they'd have to know about that as well, right?

not necessarily, you could use your own set of pspecs and be fine
with them, however you'd still have to change your code for some of
the uses of REDIRECT/Override we tossed around here.


anyway, relating to yesterdays irc discussion, the least intrusive,
though also least flexible way is to add just:

G_PARAM_REDIRECT /* flag */

GParamSpec* g_param_spec_get_redirect_target (GParamSpec *pspec);
void        g_param_spec_set_redirect_target (GParamSpec *pspec,
                                              GParamSpec *redirect_target);

GParamSpecOverride* g_param_spec_override (const gchar *name);
/* this is basically your implementation without the extra accessors */

void        g_object_class_override_property (GObjectClass   *oclass,
                                              guint           property_id,
                                              const gchar    *prop_name);
/* here, prop_name is the name of a property already existing
 * for this object. (from an interface or parent type)
 */

g_object_class_override_property() creates a GParamSpecOverride internally,
sets the REDIRECT flag on it as discussed earlier, and sets either an interface
or a parent type property as redirect-target on it.
every other API bit that exposes pspecs to the user (g_object_class_find_property,
g_object_class_list_properties, set_property, get_property) needs to exchange the
override pspec for the redirect-target beforehand.
that is 0 API change on existing property code, but it also doesn't allow us
to change flags/default/nick/blurb/ranges/etc. in an easy manner.
the only thing we actually achive with this is changing the class
providing the set/get_property implementations and the property_id being
used, which amounts to changing the implementation and nothing else.
i do think the simplicity and maintained compatibility of this aproach is
worth the downsides outlined.
and, to give proper credit, after you mentioned it to me on irc, i
now see that this comes close to what you had in mind in:

  http://mail.gnome.org/archives/gtk-devel-list/2003-February/msg00107.html

i just didn't get the idea the way i outlined above from your email.


> > re finishing up the interface property issue, have you had
> > thoughts about how to implement child properties for container
> > interfaces yet?
>
> No, I haven't thought about it at all. IMO, it can wait until
> someone actually wants it; if you go with your scheme where
> the redirect target is stored on the pspec, then it seems
> that most things that GObject does could also be done in
> GtkContainer.

right, that's one of the points in keeping the pspec
implementation from using g_object_* functions.

> So, moving forward here; are you planning to work on this,
> or should I make another iteration?

i guess you can adapt your patch to the API i gave above.
however, for it to work, gtype.c changes are required (at
least the base_init() ordering patch).

>  - I think I can handle the base_init ordering change,
>    though it looks a bit hairy.

as i said, i think i can get to that this weekend, but i wouldn't
mind if you try to reduce my amount of work to a patch review plus
a couple tweaks... ;)

>  - The rest of storing redirect properties on the pspec
>    should be pretty straightforard.
>
>  - I'd like to here your opinions future directions for
>    class_init for interfaces before changing the prototype
>    for list_properties()

i'm all for it, so i plan to add the iface class_init() support
to gtype.c in the same go.

> Regards,
> 					Owen


---
ciaoTJ