Re: 2.4 schedule

[Owen Taylor <otaylor redhat com>]

On Wed, 2003-09-24 at 19:18, Tim Janik wrote:

> >  A) An answer to my question from September 10:
> >
> >   What do you think, then, of making g_param_spec_set_redirect_target()
> >   not public? What about making g_param_spec_get_redirect_target() just
> >   expect a GParamSpecOverride rather than using qdata?
> 
> looking at your patch, g_object_class_list_properties(),
> g_object_class_find_property() and g_object_interface_list_properties()
> don't use get_redirect target, while g_object_interface_find_property()
> does. is this a left over from moving the redirection lookup into
> the pool?

It's a bug - it should have been in g_object_class_find_property(), but
there's it's necessary.

g_param_spec_pool_lookup() has to return the GParamSpecOverride,
not the redirect target, other otherwise we won't have the 
correct owner type and property ID to actually call the 
implementation.

> provided we can make all lookups private, we probably don't
> need public API for redirections.
> 
> i find the pspec-override implementation a bit suspicious though.
> if we resolve redirections only inside the pool (and return NULL
> in case of no redirect target), methods like:
> param_override_set_default, param_override_validate,
> param_override_values_cmp, and the get_nick, get_blurb variants
> should never be called for a redirect pspec, unless you create
> a g_param_spec_override() purposely, don't add it to a class and
> just use it.

Well, for get_nick/blurb/default() that's mostly true. But for
set_default() / validate() / values_cmp(), etc, is that the case?
As above g_param_spec_pool_lookup() can't do redirection, so
inside GObject we'll mostly be dealing with the GParamSpecOverride.

What about construct() and GObject::notify? What GParamSpecs get
passed there? It's certainly going to be easier and faster if
we pass in the GParamSpecOverride. g_object_constructor() would
have to look up every property again by name.

> that's quite pointless without using at least set_redirect_target(),
> so we should do either of:
> - provide public set/get redirect functions, in which case your
>   param_override_*() methods need to handle a NULL target more gracefully

I certainly didn't imagine anyone calling
g_param_spec_set_redirect_target() to *unset* the paramspec
for an already created GParamSpecOverride. GParamSpecOverride
takes a 'GParamSpec *overriden' property that may not be NULL.

> - disallow GParamSpecOverride uses alltogether (leaving its constructor
>   as an implementation detail of object/iface property systems).

I don't think 'private to object property systems' is a coherent
API policy. Either we:

 - Don't export
 - Export but with #define guards becuase we have reduced stability
   guarantees. 
 - Export

I'm happy to remove the "generally" from the current documentation

 "... and generally will not be directly useful unless you are
  implementing a new base type similar to GObject"

>   here, the param_override_*() methods need to error out more clearly,
>   since their invocation can only happen if something went wrong earlier.
> 
> i'd opt for the second, but for that your implementation needs to be
> tweaked to also allow automated setting of the redirect target *after*
> calling g_param_spec_override ("name", NULL). 

Why can't we just require the param spec to be provided at creation
time?

> that's basically attempting
> a second lookup from within the pool if you encounter an override pspec
> with redirect target of NULL, and in case of success doing
> set_redirect_target() on that pspec.

That's conceptually really messy, since it leaves the GParamSpecOverride
without a value type until it first gets looked up.

I don't think you really answered my question:

 Should we *remove* the public g_param_spec_set_redirect_target()
 and make GParamOverride the only way to create a redirected 
 GParamSpec.

That would allow considerable simplification of the code and some
increase in efficiency. (No need to use qdata for the redirect
target)

> there's a left over ptototype g_object_property_get_redirect_target()
> in your patch.

Fixed.

> >  B) A decision on making g_type_class_ref(iface_type) return the
> >     "default vtable".
> 
> i don't like the idea of g_type_class_ref() covering additional types
> for which G_TYPE_IS_CLASSED()==FALSE. since interfaces can't trivially
> be turned into classed types (in the sense that G_TYPE_IS_CLASSED() would
> return TRUE for them) which would also be an invalid API break btw,
> the accessors here need to be seperated:
> 
> gpointer              g_type_default_interface_ref   (GType            type);
> gpointer              g_type_default_interface_peek  (GType            type);
> void                  g_type_default_interface_unref (gpointer         g_iface);

Not really convinced of the reasoning, but sure, I'll do it that way.

> >  C) A decision on whether the 'postcheck' function we were discussing
> >     should be called:
> >
> >     - immediately after the iface_init function gets called for any
> >       interface on any type deriving from the specified type.
> >
> >     - After class_init and every iface_init subsequent to that.
> 
> recently, you said you'd rather stick to solving only the problem at hand,
> which is an extra hook after iface_init, and on second thought i agree with
> you here. "the function" here needs to be a hook list though, similar to
> g_type_add_class_cache_func(), we get:

I assume by "hook list" you don't actually mean GHookList with all
the reentrancy protection, etc. (g_type_add_class_cache_func() looks
basically simple.)

> typedef gboolean (*GTypeInterfaceCheckFunc)  (gpointer         func_data,
>                                               GtypeClass      *g_class,
>                                               GTypeInterface  *g_iface);
> void             g_type_add_interface_check  (gpointer                cache_data,
>                                               GTypeInterfaceCheckFunc cache_func);
> void             g_type_remove_interface_check (gpointer                cache_data,
>                                                 GTypeInterfaceCheckFunc cache_func);
> /* simplest variant, the hooks could also be added _per_ (fundamental)
>  * class type, but i don't expect much use of this, so this is most
>  * likely good enough
>  */

Hmm, a per-fundamental type variant would be more convenient, but
but for use one or two places it's not a big concern.

I don't see much use for removing the functions that are added, but I
guess consistency with g_type_add_class_cache_func() is good.

> and the hooks are to be called whenever type_iface_vtable_iface_init_Wm()
> is called with entry->init_state!=INITIALIZED. to properly guard the code
> here (from gtype.c):
> 
>       entry->init_state = INITIALIZED;
> 
>       type_iface_vtable_iface_init_Wm (lookup_type_node_I (entry->iface_type), node);
> [...]
>  if (class_state >= IFACE_INIT)
>     {
>       type_iface_vtable_iface_init_Wm (iface, node);
>       new_state = INITIALIZED;
>     }
> 
>   if (class_state != UNINITIALIZED && class_state != INITIALIZED)
>     {
>       /* The interface was added while we were initializing the class
>        */
>       IFaceEntry *entry = type_lookup_iface_entry_L (node, iface);
>       g_assert (entry);
> 
>       entry->init_state = new_state;
> 
> 
> the entry->init_state = INITIALIZED; assignment needs to be moved
> *inside* type_lookup_iface_entry_L(), and needs to occour before
> iholder->info->interface_init() is being called.

Sounds reasonable.

I'll try to have a new patch by the end of the week. If you could
clarify your opinion on removing g_param_spec_set_redirect_target(),
that would be useful.

Regards,
						Owen