Re: [API] GObject watched closures

[Owen Taylor <otaylor redhat com>]

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.

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

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

If you have huge numbers of foo_notify_connect() but expect
only a few watched closures per object, I guess you could get
the list of watched closures, then look at each closure for
the invalidate notifier for foo_notify_connect(). But this is
getting into pretty evil hack land.

> note that the signal system, which also notifies via closure invokation,
> implements its own, very special hackery to work around this problem, by
> letting:
> 
> gulong   g_signal_handler_find                (gpointer           instance,
>                                                GSignalMatchType   mask,
>                                                guint              signal_id,
>                                                GQuark             detail,
>                                                GClosure          *closure,
>                                                gpointer           func,
>                                                gpointer           data);
> 
> peek at the ->callback and ->data members of the closures it keeps internally,
> if this functions is invoked as
> 
> g_signal_handler_find (object, G_SIGNAL_MATCH_FUNC | G_SIGNAL_MATCH_DATA,
>                        0, 0, NULL,
>                        func, data);
> [g_signal_handler_find() has the same API and is the backend function being
> used behind functions like g_signal_handlers_block*() or
> g_signal_handlers_disconnect*()]
> 
> i don't think the same hackery should be forced on every implementation
> that uses closures as object callbacks. here's an example for code
> notification components ala foo_notify_connect()/foo_notify_disconnect()
> (which of course doesn't implement the same hackery):
> 
> void bonobo_ui_component_add_listener_full   (BonoboUIComponent  *component,
>                                               const char         *id,
>                                               GClosure           *closure);
> void bonobo_ui_component_remove_listener_by_closure (BonoboUIComponent  *component,
>                                                      GClosure           *closure);
> 
> since GClosure is our new canonical mechanism for callback maintenance,
> writing code like foo_notify_connect()/foo_notify_disconnect() is actually
> recommended now, and thus convenience wrappers like foo_object_connect()
> and foo_object_disconnect() are also likely to appear. so in order for
> people to be able to actually implement foo_object_disconnect() variants,
> i think we should add g_object_list_watched_closures() to GObject, despite
> the API freeze, comments are aprechiated.

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. 

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

Regards,
                                                  Owen