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

[Owen Taylor <otaylor redhat com>]

On Wed, 2003-07-23 at 21:10, Tim Janik wrote:
> 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.

No, that's not what I was checking. I think requiring a prerequisite
is too strong; all I'm checking there is that (since currently
ever interface vtable corresponds to some class) is that that class
derives from G_TYPE_OBJECT.

If we go with the class_init scheme then that check doesn't make
sense any more and can just be omitted.

[...]

> > 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).

I think that's fine. And I wouldn't restrict the visibility to 
class_init(). If we really want take a vtable as the argument
to g_object_interface_list_properties(), then that implies
to me that
 
 g_type_class_ref (MY_IFACE_TYPE)

and so forth should work and return this prototype vtable.

[...]

> > 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).

It didn't look like the final patch would have a lot of lines, 
but figuring out what lines those were seemed challenging to
me. Certainly something I'd rather leave to you to do :-)

> > 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?

Yes, that's logical conclusion to me. One major goal of this
fix is that given a interface type MY_IFACE_TYPE, I can 
introspect it's properties and signals. The only generic way
to do that now is to create a dummy instance type and add
the interface to it.

> 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.

What practical problems come from making g_type_class_ref() 
work for interface types? g_type_class_ref() has
no *other* meaning for interface types currently, right?

[...]

> > } * 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 don't see anything fundementally wrong with something like:

 g_type_set_interface_postinit_function (instance_type, func)

that is called after the interface_init() function for each interface
added to a class deriving from @instance_type. Rather specialized,
but not useful here.

[...]

> > 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)

Note that a problem with any clone-style implementation is that
a property editor which is trying to group properties can't figure
what class implemented a particular property. (I suppose you
could have a CLONED flag or something)

And of course, as we discussed earlier, I don't see how you implement
clone() with a reasonable API, at least without having a 
GParamSpecClonable *interface*.

 g_type_class_install_property (class,
    g_param_spec_int_copy (g_type_class_find_property (class,
                                                       "myprop")));

Isn't a very appealing.
                                
> 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.

If you have complete control over what paramspecs people are using, 
then you can be sure that nobody will use will use GParamSpecOverride,
right?

If you don't have this complete control, then they might use
GParamSpecOverride, but they might also use MyParamSpecBlah, which
you would also have to add code to handle.

> 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.

 - I still think there is a bit of a problem that you can't change the
   default for a property (without duplicating the nick/blurb/etc,
   which is horrible codiing style)

 - I don't see why requiring people to add support for redirected 
   properties in order to support them is a problem.

 - But that being said, the above is nice and simple, and it probably
   is compatible with a later more complex solution that allows for
   changing property attributes when overriding. 

And at this point, about any solution that allows me to get
GtkFileChooser into GTK+ is going to get my support :-)

[...]

> > 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.

I'm going to be pretty busy with patch review for the next few days, so
I don't think I'm going to get to this. If you haven't started
working on it by Monday, I'll see what I can figure out then.

Regards,
					Owen