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

[Tim Janik <timj gtk org>]

hey owen.

sorry it took so long for me to get back on the issue,
but i had a bunch of exams and other stuff on my list.

On 10 Jul 2003, Owen Taylor wrote:

> Hi Tim,
>
> Here's another iteration of the properties on interfaces patch
> incorporating 2 changes that we discussed on IRC some while
> ago (I've attached the discussion to
> http://bugzilla.gnome.org/show_bug.cgi?id=105894)
>
>  - Rename G_PARAM_OVERRIDE to G_PARAM_REDIRECT and
>    g_object_property_find_overridden() to
>    g_object_property_get_redirect_target()
>
>    GParamSpecOverride keeps its name, as discussed
>
>  - g_object_interface_install_property() now takes a
>    pointer to an interface (from base_init) rather than
>    a GType
>
>    g_object_interface_install_property() an
>    g_object_interface_list_properties keep the GType as
>    their parameter because they aren't called from base_init()
>    and it would be really annoying to have to find a type
>    implementing an interface in order to list the properties
>    of the interface.


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

> +		 g_type_name (g_type),
> +		 pspec->name);
> +      return;
> +    }
> +
> +  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_type);
> +}
> +
>  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.

>  }
>
> +/**
> + * g_object_interface_install_property:
> + * @g_iface: an interface structure passed to the base_init
> + *   function for an interface.
> + * @pspec: the #GParamSpec for the new property
> + *
> + * Add a property to an interface; this is only useful
> + * for interfaces that are added to GObject-derived
> + * types. Adding a property to an interface has two primary
> + * purposes. First, all object types with that interface will
> + * be required by GObject to implement a property with the
> + * same name and type. Second, the %G_PARAM_REDIRECT flag can be
> + * used on the implementation properties to indicate that
> + * the nick and blurb should be looked up from the interface
> + * property when missing from the instance property.
> + * (See g_object_property_get_redirect_target ()) #GParamSpecOverride
> + * provides an easy way to create an implementation property.
> + *
> + * This function is meant to be called from the interface's
> + * base_init function the first time that base_init function
> + * is called.
> + *
> + * <informalexample><programlisting>
> + * static void
> + * my_iface_base_init (gpointer g_iface)
> + * {
> + *   static gboolean initialized = FALSE;
> + *   if (!initialized)
> + *     {
> + *       g_object_interface_install_property (g_iface,
> + *                                             g_param_spec_boolean ("my_prop",
> + *                                                                   "My Property",
> + *                                                                   "Some boolean property",
> + *                                                                   FALSE,
> + *                                                                   G_PARAM_READWRITE));
> + *        initialized = TRUE;
> + *     }
> + *  }
> + * </informalexample></programlisting>
> + *
> + * Note that in the rare case that you are using dynamically loaded
> + * interfaces, then you have to keep a count of the difference between
> + * the number of times the base_init and base_finalize are called
> + * and install properties when that count goes from 0 to 1.
> + **/
> +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.

> +  g_return_if_fail (G_IS_PARAM_SPEC (pspec));
> +  g_return_if_fail (PARAM_SPEC_PARAM_ID (pspec) == 0);	/* paranoid */
> +
> +  install_property_internal (iface_class->g_type, 0, pspec);
> +}
> +
>  GParamSpec*
>  g_object_class_find_property (GObjectClass *class,
>  			      const gchar  *property_name)
> @@ -312,6 +380,108 @@ g_object_class_find_property (GObjectCla
>  				   TRUE);
>  }
>
> +/**
> + * 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.

> +  return g_param_spec_pool_lookup (pspec_pool,
> +				   property_name,
> +				   iface_type,
> +				   FALSE);
> +}
> +
> +/**
> + * 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()).

[snipped a bunch of lookup logic]
> +}
> +
>  GParamSpec** /* free result */
>  g_object_class_list_properties (GObjectClass *class,
>  				guint        *n_properties_p)
> @@ -330,6 +500,38 @@ g_object_class_list_properties (GObjectC
>    return pspecs;
>  }
>
> +/**
> + * g_object_interface_list_properties:
> + * @iface_type: an interface type
> + * @n_properties_p: location to store number of properties
> + *                  returned.
> + *
> + * Lists the properties of a given interface.
> + *
> + * Return value: a pointer to an array of pointers to
> + *               #GParamSpec structures. The structures
> + *               are owned by GLib, but the array should
> + *               be freed with g_free() when you are
> + *               done with it.
> + **/
> +GParamSpec** /* free result */
> +g_object_interface_list_properties (GType         iface_type,
> +				    guint        *n_properties_p)
> +{
> +  GParamSpec **pspecs;
> +  guint n;
> +
> +  g_return_val_if_fail (G_TYPE_IS_INTERFACE (iface_type), NULL);

dito comment about GType arg from g_object_interface_find_property().

> +
> +  pspecs = g_param_spec_pool_list (pspec_pool,
> +				   iface_type,
> +				   &n);
> +  if (n_properties_p)
> +    *n_properties_p = n;
> +
> +  return pspecs;
> +}
> +
>  static void
>  g_object_init (GObject *object)
>  {
> @@ -591,6 +793,99 @@ g_object_new (GType	   object_type,
>    return object;
>  }
>
> +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.

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.

> Index: gobject.h
> ===================================================================
> RCS file: /cvs/gnome/glib/gobject/gobject.h,v
> retrieving revision 1.26
> diff -u -p -r1.26 gobject.h
> --- gobject.h	21 Mar 2002 00:34:04 -0000	1.26
> +++ gobject.h	10 Jul 2003 21:39:27 -0000
> @@ -116,6 +116,16 @@ GParamSpec* g_object_class_find_property
>  					       const gchar    *property_name);
>  GParamSpec**g_object_class_list_properties    (GObjectClass   *oclass,
>  					       guint	      *n_properties);
> +
> +void        g_object_interface_install_property (gpointer     g_iface,
> +						 GParamSpec  *pspec);
> +GParamSpec* g_object_interface_find_property    (GType        iface_type,
> +						 const gchar *property_name);
> +GParamSpec**g_object_interface_list_properties  (GType        iface_type,
> +						 guint       *n_properties_p);
> +
> +GParamSpec* g_object_property_get_redirect_target (GParamSpec *pspec);
> +
>  gpointer    g_object_new                      (GType           object_type,
>  					       const gchar    *first_property_name,
>  					       ...);
> Index: gparam.c
> ===================================================================
> RCS file: /cvs/gnome/glib/gobject/gparam.c,v
> retrieving revision 1.27
> diff -u -p -r1.27 gparam.c
> --- gparam.c	7 Feb 2003 22:04:24 -0000	1.27
> +++ gparam.c	10 Jul 2003 21:39:27 -0000
> @@ -21,6 +21,7 @@
>   * MT safe
>   */
>
> +#include        "gobject.h"
>  #include	"gparam.h"
>
>
> @@ -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.

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

> +	   G_TYPE_IS_OBJECT (pspec->owner_type))
> +    {
> +      GParamSpec *redirect_target;
> +
> +      redirect_target = g_object_property_get_redirect_target (pspec);
> +      if (redirect_target && redirect_target->_nick)
> +	return redirect_target->_nick;
> +    }
> +
> +  return pspec->name;
>  }
>
>  G_CONST_RETURN gchar*
> @@ -254,7 +268,20 @@ g_param_spec_get_blurb (GParamSpec *pspe
>  {
>    g_return_val_if_fail (G_IS_PARAM_SPEC (pspec), NULL);
>

dito comment about _nick.

> +  return NULL;
>  }
>
>  static void
> Index: gparam.h
> ===================================================================
> RCS file: /cvs/gnome/glib/gobject/gparam.h,v
> retrieving revision 1.22
> diff -u -p -r1.22 gparam.h
> --- gparam.h	3 Mar 2002 03:14:43 -0000	1.22
> +++ gparam.h	10 Jul 2003 21:39:27 -0000
> @@ -53,7 +53,8 @@ typedef enum
>    G_PARAM_CONSTRUCT	      = 1 << 2,
>    G_PARAM_CONSTRUCT_ONLY      = 1 << 3,
>    G_PARAM_LAX_VALIDATION      = 1 << 4,
> -  G_PARAM_PRIVATE	      = 1 << 5
> +  G_PARAM_PRIVATE	      = 1 << 5,
> +  G_PARAM_REDIRECT            = 1 << 6
>  } GParamFlags;
>  #define	G_PARAM_READWRITE	(G_PARAM_READABLE | G_PARAM_WRITABLE)
>  #define	G_PARAM_MASK		(0x000000ff)
> @@ -73,7 +74,7 @@ struct _GParamSpec
>    gchar         *name;
>    GParamFlags    flags;
>    GType		 value_type;
> -  GType		 owner_type;	/* class using this property */
> +  GType		 owner_type;	/* class or interface using this property */
>
>    /*< private >*/
>    gchar         *_nick;
> 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.

>  #include	"gvaluecollector.h"
>  #include	"gvaluearray.h"
>  #include	<string.h>
> @@ -966,6 +967,67 @@ param_object_values_cmp (GParamSpec   *p
>    return p1 < p2 ? -1 : p1 > p2;
>  }
[GParamSpecOverride implementation.]


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.



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.



> Other notes:
>
>  - I didn't try to change the way GParamSpecPool works
>    to optimize g_object_property_find_overridden();
>    that can be done later.

i don't think this is necessary anymore with the above
class initializer reordering (and that means, soerens
patch to speedup g_param_spec_pool_lookup() might
actually get applied).
in any case, changing the lookup mechanism from the way
your patch works wasn't meant as an optimization, but because
the whole redirect thing doesn't belong into gobject.[hc] at all
(and is pretty tedious to handle there as your patch shows).

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

re finishing up the interface property issue, have you had
thoughts about how to implement child properties for container
interfaces yet?

>
> Regards,
> 					Owen

---
ciaoTJ