Interface properties (Re: 2.4 schedule)

[Owen Taylor <otaylor redhat com>]

On Thu, 2003-10-02 at 01:54, Tim Janik wrote:
> On Thu, 25 Sep 2003, Owen Taylor wrote:
> 
> > 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.
> 
> ah, but then all combinations of object/interface list/find need to
> do the lookup as an override pspec shouldn't be user visible.

- Object list does it internally to the pspec pool
- I've now added it for object find
- Interface list/find don't need it, since installing a redirected
  property on an interface makes no sense.

> > > 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.
> 
> yeah, and if we use it for anything else than figuring the property ID
> and owner type, we're doing something wrong, so it's best to have its
> methods check/catch that.

I don't follow that. Why make things harder on ourself?

> > What about construct() and GObject::notify? What GParamSpecs get
> > passed there?
> 
> those are user visible, so they shouldn't be GParamSpecOverride either.

gtk_settings_notify looks at pspec->property_id. If that is at all
typical, then you certainly want to pass in the GParamSpecOverride.

(Admittedly, ->notify default handlers, and when they do exist,
usually what you are doing is catching changes on a parent property
that isn't overridden)

When would somebody do something other than looking at 
pspec->name or pspec->property_id?

> > 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.
> 
> it would be faster yes, but it's quite possible the user actually
> messes around with the GObjectConstructParam array, i.e. it's
> pspecs or values (that's why it's exposed to him). so it's part
> of the user visible API and needs to be redirected already.

The reason they get passed in as GParamSpec rather than as property
names is presumably to keep the default g_object_constructor() from
having to look them up again from names. Passing in the possibly
redirected paramspecs defeats that.

Every single constructor I've ever seen simply passes the 
GObjectConstructParam verbatim when chaining up. Do you have an
idea what else one would do with that array?

[...]

> > >   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?
> 
> basically, to make adding the interface with the implementing
> property work after class_init. we just added the necessary gtype
> logic to check that case.

Presumably, the reason for adding an interface after class_init is
to augment the set of interfaces beyond what the object was originally
designed to have.

But this functionality doesn't make sense for interfaces with properties
because any properties must already be handled in that function's
set_property method.

It's just barely conceivable that an object would have a generic
set_property method that could handle properties that weren't 
known about ahead of time (perhaps some sort of object implementation
in an interpreted language)

But even then, there's an obvious place to override the property that
works without unattached GParamSpecOverrides ... inside the
interface_init function.

> > > 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.
> 
> that's just an implementation detail though.

An implementation detail yes. A decidely non-minor one though.
The early versions of my patch needed delayed resolution of 
redirects (to deal with class_init being called before 
base_iface_init). 

That was implemented. Things got a lot simpler.

What you are proposing is going to add a lot of that complexity 
back, or perhaps more, for a feature that *nobody* is ever going
to use.

At this point, I really don't want to fool around further with
things that are "just implementation details", I just want to get 
this *implemented*.

> > 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)
> 
> i listed two paths we could persue, and expressed bias towards
> the second, which was getting rid of a public set_redirect_target
> and implementing automated pool-internal set_redirect_target.
>
> i now think we should definitely implement the second option,
> eventhough it has a conceptually messy implementation detail ;)

Why do you want it inside the paramspec pool? What advantage do we gain
by that other than having to allocate auxiliary memory blocks? If we
want to be able to redirect arbitrary paramspecs, the current use of
qdata seems right to me. If we don't want to redirect arbitrary
paramspecs, then we might as well store the information directly in
GParamSpecOverride. I don't see why we want to redirect arbitrary
paramspecs.

What I'm going to do is produce a version of my patch that:
 
 - Changes g_object_interface_find/list_properties to take

 - Has the docs moved out of the source files

 - Removes the g_param_spec_set_redirect_target() in favor
   of storing the data in GParamSpecOverride

 - Leaves GParamSpecOverride fully functional in terms of
   get_name/get_nick/validate/etc.

 - Leaves the GParamSpecOverrides visible in constructor(),
   notify, since I don't think it's worth the expense of fully
   hiding them, and since I think that keeping the right
   paramspec ID's is important.

This is what I believe should go into GObject. If you disagree
after seeing the patch, we can then discuss ways we can get
something more to your taste in expeditiously.

Regards,
						Owen