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

[Owen Taylor <otaylor redhat com>]

On Mon, 2003-07-21 at 21:51, Tim Janik wrote:

> > Index: gobject.c
> > ===================================================================
> > RCS file: /cvs/gnome/glib/gobject/gobject.c,v
> > retrieving revision 1.58
> > diff -u -p -r1.58 gobject.c
> > --- gobject.c	30 May 2003 18:44:57 -0000	1.58
> > +++ gobject.c	10 Jul 2003 21:39:27 -0000
> > @@ -258,6 +258,25 @@ g_object_do_class_init (GObjectClass *cl
> >  		  1, G_TYPE_PARAM);
> >  }
> >
> > +static void
> > +install_property_internal (GType       g_type,
> > +			   guint       property_id,
> > +			   GParamSpec *pspec)
> > +{
> > +  if (g_param_spec_pool_lookup (pspec_pool, pspec->name, g_type, FALSE))
> > +    {
> > +      g_warning (G_STRLOC ": type `%s' already has a property named `%s'",
> 
> the G_STRLOC breaks with this move.

OK, changed to "when installing property: ..." in my copy.

> >  void
> >  g_object_class_install_property (GObjectClass *class,
> >  				 guint	       property_id,
> > @@ -276,18 +295,8 @@ g_object_class_install_property (GObject
> >    if (pspec->flags & (G_PARAM_CONSTRUCT | G_PARAM_CONSTRUCT_ONLY))
> >      g_return_if_fail (pspec->flags & G_PARAM_WRITABLE);
> >
> > -  if (g_param_spec_pool_lookup (pspec_pool, pspec->name, G_OBJECT_CLASS_TYPE (class), FALSE))
> > -    {
> > -      g_warning (G_STRLOC ": class `%s' already contains a property named `%s'",
> > -		 G_OBJECT_CLASS_NAME (class),
> > -		 pspec->name);
> > -      return;
> > -    }
> > +  install_property_internal (G_OBJECT_CLASS_TYPE (class), property_id, pspec);
> >
> > -  g_param_spec_ref (pspec);
> > -  g_param_spec_sink (pspec);
> > -  PARAM_SPEC_SET_PARAM_ID (pspec, property_id);
> > -  g_param_spec_pool_insert (pspec_pool, pspec, G_OBJECT_CLASS_TYPE (class));
> >    if (pspec->flags & (G_PARAM_CONSTRUCT | G_PARAM_CONSTRUCT_ONLY))
> >      class->construct_properties = g_slist_prepend (class->construct_properties, pspec);
> >
> > @@ -299,6 +308,65 @@ g_object_class_install_property (GObject
> >      class->construct_properties = g_slist_remove (class->construct_properties, pspec);
> 
> in this function, we should check whether the property is a REDIRECT property,
> lookup the redirect target from the pool (error out if there is none) and if so,
> call g_param_spec_set_redirect_target(pspec,target). i'm going into more details
> how to get this to work below.

This gives more strict ordering about the ordering of installing
properties and adding interfaces than we have currently, though 
I don't personally see it as a problem. 

(Noting the issue that you describe below about base_init being
called too late.)

[...]

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

> > +/**
> > + * 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().

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.

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.

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

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.

> > + * g_object_property_get_redirect_target:
> > + * @pspec: a #GParamSpec
> > + *
> > + * Given a property, finds the property that provides the
> > + * defaults for this property. (The common example of this
> > + * is using a #GParamSpecOverride in a class implementation
> > + * to provide the implementation for a property in one
> > + * of the class's interfaces.) @pspec must have the
> > + * %G_PARAM_REDIRECT flag set for this function to do
> > + * anything interesting. If  %G_PARAM_REDIRECT is set,
> > + * then first the parent types of the property's owner
> > + * type are checked for a property with the same name, then the
> > + * interfaces that the owner type implements are checked.
> > + * If the flag is not set, %NULL is returned immediately.
> > + *
> > + * Return value: a #GParamSpec, or %NULL if this paramspec
> > + *   does not inherit defaults from another paramspec.
> > + **/
> > +GParamSpec*
> > +g_object_property_get_redirect_target (GParamSpec *pspec)
> > +{
> 
> pspec redirection should not be GObject specific.
> so this should be g_param_spec_get_redirect_target()
> and be implemented merely by reading out pspec->qdata
> (which carries the result of g_param_spec_set_redirect_target()).

If we add the ordering constraints described above, then, yes,
it could be done this way.

[...]

> > +static gboolean
> > +g_object_class_check_iface_properties (GObjectClass *class)
> > +{
>
> [loop over all interfaces]
>   [loop over all properties]
>     [pool lookup for each property]
> 
> > +
> > +  return result;
> > +}
> > +
> >  gpointer
> >  g_object_newv (GType       object_type,
> >  	       guint       n_parameters,
> > @@ -609,6 +904,10 @@ g_object_newv (GType       object_type,
> >    g_return_val_if_fail (G_TYPE_IS_OBJECT (object_type), NULL);
> >
> >    class = g_type_class_ref (object_type);
> > +
> > +  if (!g_object_class_check_iface_properties (class))
> > +    return NULL;
> > +
> >    for (slist = class->construct_properties; slist; slist = slist->next)
> >      {
> >        clist = g_list_prepend (clist, slist->data);
> 
> this looks extremely expensive, especially doing this check for every
> object creation. i guess we could get away with
> GObjectClass { [...] gboolean iface_properties_checked; } which we set
> if this check has been performed once, but i wonder whether we want to
> take this performance hit at all.

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.

> and as a side note, doing ref (foo); if (bar) return; simply hurts my
> eyes.., there should at least be a comment next to the return; about
> the check having thrown a critical or warning if the branch is taken.

Added a comment to my copy.

> > @@ -246,7 +247,20 @@ g_param_spec_get_nick (GParamSpec *pspec
> >  {
> >    g_return_val_if_fail (G_IS_PARAM_SPEC (pspec), NULL);
> >
> > -  return pspec->_nick ? pspec->_nick : pspec->name;
> > +  if (pspec->_nick)
> > +    return pspec->_nick;
> > +  else if (pspec->flags & G_PARAM_REDIRECT &&
> 
> i think REDIRECT should take precedence over ->_nick.

That would prevent overriding some aspects of the 
paramspec while leaving others the same. I simply
don't see a point in making REDIRECT win over 
explicitely setting the nick/blurb. If you don't
want to change the nick/blurb, you simply don't set
them, right?

> > +	   pspec->owner_type != G_TYPE_NONE &&
> 
> there really shouldn't be a need to evaluate owner_type at all
> in the whole patch. the redirect mechanism has to work on pspecs
> without owner_type being set anyway.

If we *change* the way we do the "redirect target", then
yes, we could get rid of the owner_type check. As currently
implemented, it make sense.

> > Index: gparamspecs.c
> > ===================================================================
> > RCS file: /cvs/gnome/glib/gobject/gparamspecs.c,v
> > retrieving revision 1.25
> > diff -u -p -r1.25 gparamspecs.c
> > --- gparamspecs.c	7 Feb 2003 22:04:24 -0000	1.25
> > +++ gparamspecs.c	10 Jul 2003 21:39:27 -0000
> > @@ -25,6 +25,7 @@
> >
> >  #include	"gparamspecs.h"
> >
> > +#include        "gobject.h"	/* For g_object_property_find_overridden */
> 
> as a matter of principle:
> making the paramspec implementation dependent on GObject is a no-no.
> the gtype docs say that gobject is merely a sample implementation
> of an object type, so that third-party object types may be implemented
> on top of gtype, with equally good support for pspecs and signals.
> making one of these components dependant on gobject or special case it
> violates encapsulation principles.
>
> practice:
> in effect, the include would be uneccessary, since gparamspecs.h already
> includes gobject.h. that's only because the constructor g_param_spec_object()
> is implemented in this file for reasons of simplifying its implementation.
> conceptually, you'd be right to make an argument for it to be moved into
> gobject.[hc] and the gobject.h include be removed from gparamspecs.h.

The point here is that GParamSpecOverride is GObject specific; 
if we rework things so that you can have
g_param_spec_get_redirect_target() then we can remove the dependency;
otherwise if you create MyObject, you'll have to 
Create MyParamSpecOverride.

And yes, the include is unnecessary because of the include in 
gparamspecs, so I've removed it from my copy,

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

The only reason to use the REDIRECT flag without GParamSpecOverride
is to change nick/blurb/min/max/etc.

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

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?

> coming back to the g_object_class_install_property() respectively
> g_param_spec_set_redirect_target() issue mentioned earlier.
> in install_property, we currently can't lookup the interface
> property because it might not already be installed at class_init()
> time, since interface initializers are called after their
> implementing class' initializer. it's somewhat ridiculous to
> build an introspective type sytsem, that can't introspect itself
> because the information required isn't made available early enough.
> so to allow this, the idea is basically to change class/interface
> initialization order to become:
> - create class (copied over from parent)
> - base initialize class
> - base initialize interfaces
> - initialize class
> - initialize interfaces
> 
> interface properties are installed in interface base initializers,
> class properties are installed in class initializers. so with the
> above ordering, the redirect targets can be auto-set on the class
> properties upon installation.

Are you going to disallow adding interfaces during class_init()?
If you can add them before and add them after, not allowing them
to be added during seems a bit strange.

Hmm, looking at the code, this looks currently broken - don't
interfaces added during class_init() get intialized twice?

I guess we could base_init interfaces immediately when they
are added in class_init() and then finish the init at the end
of class init. (Flagging types as "in class init" will be needed
eventually to fix  http://bugzilla.gnome.org/show_bug.cgi?id=64764
anyways.)

[...]

> >  - I'd really like to get this in CVS soon, even if the
> >    implementation isn't perfect, so that we can start
> >    using it, especially for the new file chooser.
> 
> i can see that, and i'll have to take the blame for blocking
> on the issue for so long, but i honestly think there was little
> chance for me to arrange things differently, timewise.
> 
> i hope in the above, i made clear that there're still major
> issues with the iface property implementation and API that
> need to be sorted out before things get into CVS.
> (semester ended for me last week, and i got only two exams
> left, so i'll be around for working on the pspec issue now)

Well, I'm not not in a position to criticize slow review
of patches, really :-). Still, I can't say I would have
minded if a few of these comments had been brought up a
bit earlier.

If we could get something landed in the next week or
so, that would be utterly cool. The problem, from my
perspective, is that I can't really get other people working
on GtkFileChooser without resolving this.

I suppose it serves me right for using not-yet-landed
GObject features in the design; on the other hand, it
worked out really nicely, so I can't feel to bad about
it.

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

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

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

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

Regards,
					Owen