Re: [API] GObject watched closures

[Tim Janik <timj gtk org>]

On Mon, 4 Feb 2002, Owen Taylor wrote:

> 
> Tim Janik <timj gtk org> writes:
> 
> > i just noticed a function missing from gobject.[hc]:
> > 
> > GSList* g_object_list_watched_closures (GObject *object);
> > 
> > for code portions which deal with closures that use objects
> > as data, aside from signal connections, this is required
> > to allow disconnections, i.e., provided we have a notification
> > source:
> > 
> > void foo_notify_connect (GClosure *closure);
> > void foo_notify_disconnect (GClosure *closure);
> > 
> > then, the (func, data) API will look like:
> > 
> > void foo_object_connect (GObject *object,
> >                          gpointer func,
> >                          gpointer data,
> >                          gpointer destroy_func)
> 
> When do we provide this API? Looks rather unusual to me... I'd expect
> to have func/data and maybe GObject/func, but not GObject/func/data.

yeah, either you have:
  closure = g_cclosure_new (func, data, destroy_func);
  g_object_watch_closure (object, closure);
or:
  closure = g_cclosure_new (func, object, destroy_func);
  g_object_watch_closure (object, closure);
in the latter case, data is simply == object.

> I'm going to assume for the rest of the mail the 'data' argument
> is extraneous.

good enough.

> > {
> >   GClosure *closure = g_cclosure_new (func, object, destroy_func);
> >   
> >   g_object_watch_closure (object, closure); /* binds closure life time to object */
> >   foo_notify_connect (closure);
> > }
> > 
> > void foo_object_disconnect (GObject *object,
> >                             gpointer func,
> >                             gpointer data)
> > {
> >   GSList *node, *slist = g_object_list_watched_closures (object);
> >   
> >   for (node = slist; node; node = node->next)
> >     {
> >       GClosure *closure = node->data;
> >       
> >       if (closure->data == data && closure->func == func)
> >         {
> >           g_closure_invalidate (closure);
> >           foo_notify_disconnect (closure);
> >           g_slist_free (slist);
> >           return;
> >         }
> >     }
> >   g_slist_free (slist);
> >   
> >   g_warning ("Couldn't find %p(%p)", func, data);
> > }
> 
> Is this implementation correct? if you have
> 
>  foo_object_connect (object, func);
>  bar_object_connect (object, func);
>  foo_object_disconnect (object, func);
>  bar_object_disconnect (object, func);
> 
> You shouldn't be able to connect with foo, then disconnect with bar,
> even with the same object/func pair.

yep, in practice you'll want to also look at invalidation notifiers or
(meta-) marshaller (the signal system does this for instance).

> > an argument could be made, that foo_object_connect() should maintain
> > a list of constructed closures on object as object data, so
> > foo_object_disconnect() could find the required closure from that
> > list. however, this list needs to be cleaned up once the object or
> > closure dies, which is essentially duplicating the logic (and data)
> > already implemented behind g_object_watch_closure().
> 
> I'd really expect foo_object_disconnect() to simply look through the
> list of closures that foo_notify_connect() keeps. The only reason I
> see not to do this is efficiency.

unfortunately, this is usually not possible, when foo_notify_connect()/
foo_notify_disconnect() are provided by third party code they usually
won't provide you with the closure list they keep, (like for instance with
the bonobo ui component, the signal system or g_source_set_closure()).

so, aside from performance concerns, an object_disconnect() implementation
will need to peek at the closures watched by the object, because that's
the only list it could be able to get a handle off (modulo some problems
i'm getting to further down).

> Even with the object_list_watched_closures() API, your code examples don't look
> exactly short or easy to get right. Having to look at the invalidate notifier
> makes it even worse.
> 
> IMO, if there is a missing API here, it is something more highlevel; maybe
> something simple like:
> 
>  GClosure *g_object_find_watched (GObject *object, GCallback func, GClosureNotify invalidate_notifier);
> 
> Maybe something a bit more opaque. 

the problem with that is, depending on the implementation of
foo_object_connect/foo_object_disconnect, what will have to be
looked at might be a combination of the invalidate/finalize
notifiers and/or masrshallers (e.g. the signal system internally
looks at the marshaller to tell GCClsoure from GClosure).
also, looking for (func,data) pairs need to reveal a list of matching
closures (which is why we have g_signal_handlers_disconnect_matched(),
i.e. "handlers" in plural).

> For now, I don't think it would be awful to say that if people want to
> avoid the search through all notifiers, then they should keep their
> own list of closures for the object in object data. The above code
> isn't pretty enough to make me happy about breaking the API freeze for
> it :-)

keeping your own list of closures on the object would be the clean-room
aproach, if you can maintain the same semantics for that list as the
internal watch closure list has. but that essentially involves:

1) maintaining a GSList of closure pointers via object data from
   connect/disconnect
2) installing a weak-ref notifier to destroy the closure list upon
   ->dispose()   

which is a lot of complicated code on the user side, and comparatively
uneccessary since gobject.c does that internally already.

but maybe this goes more into the direction of high level API
that you expected (wcl==watch_closure_list):

void   g_object_wcl_add  (GObject  *object,
                          GQuark    wcl_quark,
                          GClosure *closure);
GList* g_object_wcl_list (GObject  *object,
                          GQuark    wcl_quark);

this will enable users to write:

void
my_object_connect (GObject  *object,
                   GCallback func,
                   gpointer  data,
                   GDestroy  destroy_func)
{
  GClosure *closure = g_cclosure_new (func, data, destroy_func);
  
  /* this calls g_object_watch_closure() internally */
  g_object_wcl_add (object, my_wcl_quark, closure);
  
  foo_notify_connect (closure);
}

void
my_object_disconnect (GObject  *object,
                      GCallback func,
                      gpointer  data)
{
  GList *node, *list = g_object_wcl_list (object, my_wcl_quark);
  
  for (node = list; node; node = node->next)
    {
      GClosure *closure = node->data;
      GCClosure *cclosure = node->data;
      
      if (closure->data == data && cclosure->callback == func)
        g_closure_invalidate (closure);
    }
  g_list_free (list);
}

with that, users don't have to deal with closure list maintenance,
and gobject.c just needs one extra GQuark per watched closure that
is in a wcl list.

and to adress micheals point, with this, the assumption of the
closure being a C closure can validly be made, without additional
checks like comparing the marshaller or invalidation notifiers,
because the caller of g_object_wcl_list() knows what type of
closure he added (i.e. could also be some PerlGlueClosure etc).

> 
> Regards,
>                                                   Owen

---
ciaoTJ