Re: gobject docs

[Owen Taylor <otaylor redhat com>]

On Mon, 2003-10-13 at 20:41, Matthias Clasen wrote:
> Hi all,
> 
> I've finally taken the time to fill in the api docs on closures. Since
> most of the information has been obtained by staring at the source, I'd
> be grateful for a little review. 

Probably more than you wanted, but here's what I came up with reading
through it :-) My suggested additions/replacements below were written
rather quickly, so I definitely wouldn't use them without giving them
a careful readthrough.

Regards,
							Owen

===
The intro needs to go into more detail about how C marshallers are
used in GObject and also link to glib-genmarshal.

I don't think there is sufficient information here to really explain
when/why you would call g_closure_set_marshal() yourself when writing
a C library. I think a full example of a callback done with a closure
would be a good idea, especially to show things like checking 
if (G_CLOSURE_NEEDS_MARSHAL (closure)) {} before setting your own
marshaller.

It wouldn't hurt to have a bit more motivation for GClosure in the 
introduction; a few things off the top of my head:

 - Closures allow the callee to get the types of the callback
   parameters, which means that language bindings don't have to 
   right individual glue for each callback type.
 - The reference counting of GClosure makes it easy to handle 
   reentrancy right; if a callback is removed during while it
   is being invoked, the closure and it's parameters won't
   be freed until the invocation finishes.
 - g_closure_invalidate() and invalidation notifiers allow 
   callbacks to be automatically removed when the objects they
   point to go away.

===
Creates a new closure which invokes @callback_func with @user_data as
last 
parameter. 
===                                                                 ^
the

===
@destroy_data: destroy notify to be called when @user_data is destroyed
===

I tend to say "is no longer used". @user_data isn't destroyed,
the closure is destroyed.

===
Creates a new closure which invokes @callback_func with @user_data as
first 
parameter.       
====                                                                ^
the

====
<!-- ##### FUNCTION g_cclosure_new_object ##### -->
<para>
A variant of g_cclosure_new() which uses @object as @user_data
and calls g_object_watch_closure() on @object and the 
created closure. This function is mainly useful when implementing new
types 
of closures.
===

I don't think the last is true; while g_signal_connect_object()
may be *more* used, this function is useful, e.g., for
g_source_set_closure(). Same comment for g_cclosure_new_swap().
Also should describe *why* calling g_object_watch_closure()
useful. Something along the lines of "This function is 
useful when you have a callback closely associated with a #GObject,
and want the callback to no longer run after the
object is is freed."

<!-- ##### FUNCTION g_closure_sink ##### -->
<para>
Takes over the initial ownership of a closure.
When closures are newly created, they get an initial reference count
==
of 1, eventhough no caller has yet invoked g_closure_ref() on the
@closure.
==
          ^
I think it would be good to describe the mechanics in more detail.
Something like:

"Each closure is initially created in a
<firstterm>floating</firstterm> state, which means that the initial
reference count is not owned by any caller. g_object_sink() checks
to see if the object is still floating, and if so, unsets the
floating state and decreases the reference count. If the object
is not floating, g_object_sink() does nothing. The reason for
the existance of the floating state is to prevent cumbersome
code sequences like:

 closure = g_cclosure_new (cb_func, cb_data);
 g_source_set_closure (source, closure);
 g_closure_unref (closure); /* XXX GObject doesn't really need this */

Because g_source_set_closure() (and similar functions) take ownership
of the initial reference count, if it is unknowned, we instead can
write:

 g_source_set_closure (source,
                       g_cclosure_new (cb_func,cb_dat));

===
Code entities that store closures for notification purposes are supposed
to call this function, for example like this:
===

Would read better as:

"Generally, this function is used together with g_object_ref(). An
example of storing a closure for later notification looks like:"

===
Decrements the reference count of a closure after it was
previously incremented by the same caller. The closure
will most likely be destroyed and freed after this function
returns.
===

"After this function returns"? I think it would be better to
say something like "If no other callers are using the closure,
then the closure will be destroyed and freed". Which
leaves the implication that if other callers are using the
closure, it will be destroyed later.

==
Invokes the closure.
===

Needs to say what "invoke" means. The meaning here is technical
jargon that few readers, whether English speaking or not would
understand.

===
<!-- ##### FUNCTION g_closure_invalidate ##### -->
<para>
Sets a flag on the closure to indicate that it's calling environment has
become invalid, and thus causes any future invocations of
g_closure_invoke() 
on this @closure to be ignored.
Also, invalidation notifiers installed on the closure will be called
at this point, and since invalidation notifiers may unreference
the closure, @closure should be considered an invalidated pointer
after this function, unless g_closure_ref() was called beforehand.
</para>
===

I'd modify that a last sentence a bit and say something like
".... at this point. Note that unless you are holding a reference to
the closure yourself, the invalidation notifiers may unref the
closure and cause it to be destroyed, so if you need to access
the closure after calling g_closure_invalidate(), make sure
that you've previously called g_closure_ref()"

I'm not sure why that's better, but it seems clearer to me :-)

I'd also add a note that if it hasn't been called previously,
g_closure_invalidate() will be called when the reference count
on a closure drops to zero.

===
Registers a finalization notifier which will be called when the
reference
count of @closure goes down to 0. Finalization notifiers are invoked
after
invalidation notifiers, in an unspecified order.
===

The last sentence is a bit misleading; because finalization
notifiers aren't necessarily invoked at the same time as invalidate
notifiers. Maybe

"Multiple finalization notifiers on a single closure are invoked
 in unspecified order. If a single call to g_closure_unref()
 results in the closure being both invalidated and finalized,
 then the invalidate notifiers will be run before the
 finalize notifiers"

===
Removes a finalization notifier. Notifiers may only be removed before or
during their invocation.
===

That second might be clearer as "Notifiers are automatically removed
after they are run."

===
Sets the marshaller of @closure. A marshaller set with
g_closure_set_marshal() 
should interpret its @marshal_data argument as a callback function to be
invoked instead of @closure->callback, provided it is not %NULL. GObject
provides a number of predefined marshallers  for use with #GCClosure<!--
-->s, 
see the g_cclosure_marshal_*() functions.
====

I think the stuff about @marshal_data here is confusing; after all,
most people writing custom marshallers aren't writing them to run
C callbacks. I think you should  say something along the lines of
"the @marshal_data provides a way for a meta-marshaller to provide
additional information to the marshaller. (See
g_closure_set_meta_marshal().) For GObject's C marshallers, what it
provides is a callback function to use instead of closure->callback".

===
Adds a pair of notifiers which get invoked before and after the closure 
callback, respectively. See g_object_watch_closure() for a use of
marshal 
guards.
====

Maybe add something like: "This is typically used to protect the
extra arguments for the duration of the callback" 

===
Sets the meta marshaller of @closure. A meta marshaller 
should use its @marshal_data argument together with @closure->data 
to determine the callback function to use, and pass it as @marshal_data
to @closure->marshal.
===

I think you need to distinguish here:

 A) GClosure in general
 B) GClosure being used for C callbacks

"A meta marshaller wraps the closure->marshal and modifies the way
it is called in some fashion. The most common use of this facility
is for C callbacks. The same marshallers (the marshallers generated
by glib-genmarshal) are used everywhere, but the way that we
get the callback function differs. In most cases we want to
use closure->callback, but in other cases we want to use use some
different technique to retrieve the callbakc function. For example,
class closures for signals (see g_signal_type_cclosure_new()) retrieve
the callback function from a fixed offset in the class structure. The
meta-marshaller retrieves the right callback and passes it to the
marshaller as the @marshal_data argument".