Re: g_boolean_handled_accumulator patch

[Owen Taylor <otaylor redhat com>]

On Fri, 2003-09-12 at 11:27, Tim Janik wrote:
> On Thu, 11 Sep 2003, Owen Taylor wrote:
> 
> > Here's a patch to add g_boolean_handled_accumulator().
> >
> > (http://bugzilla.gnome.org/show_bug.cgi?id=80487)
> >
> > It adds a testaccumulator.c to testgobject, which includes
> > a test for this, and the accumulator testing from
> > testgobject.c (I didn't remove it from their, however.)
> 
> we're getting a bit too many test files here i think.
> can you put this test into a new directory gobject/tests/
> and have them executed upon make check please?
> (especially since your tests tend to be real checks with
> no output).

I suggested that a few mails ago. I'd sort of prefer tests/gobject
to gobject/tests, because otherwise having tests/ at the toplevel
is confusing.

> if you want to take that opportunity to move the other tests
> there as well, feel free. i'd just like to keep testgobject.c
> as an executable in gobject/ to hack/test random stuff while
> implementing/changing things in gobject/.
> 
> > I added a separate testmarshalers.[ch] to support the
> > test case, since I didn't want to add "random" marshalers
> > to gmarshal.h.
> 
> that should be in tests/ as well then (unless you mean to explicitely
> provide new marhsallers that we'll have to keep offering API-wise through
> future versions).

Not making them part of the API is why I didn't add them
to gmarshal.h or to an installed header file.

> > Index: gobject/gsignal.c
> > ===================================================================
> > RCS file: /cvs/gnome/glib/gobject/gsignal.c,v
> > retrieving revision 1.52
> > diff -u -p -u -r1.52 gsignal.c
> > --- gobject/gsignal.c	19 Aug 2003 02:15:40 -0000	1.52
> > +++ gobject/gsignal.c	11 Sep 2003 19:23:41 -0000
> > @@ -2586,6 +2586,22 @@ type_debug_name (GType type)
> >      return "<invalid>";
> >  }
> >
> > +gboolean
> > +g_boolean_handled_accumulator (GSignalInvocationHint *ihint,
> > +			       GValue                *return_accu,
> > +			       const GValue          *handler_return,
> > +			       gpointer               dummy)
> > +{
> > +  gboolean continue_emission;
> > +  gboolean signal_handled;
> > +
> > +  signal_handled = g_value_get_boolean (handler_return);
> > +  g_value_set_boolean (return_accu, signal_handled);
> > +  continue_emission = !signal_handled;
> > +
> > +  return continue_emission;
> > +}
> > +
> >  /* --- compile standard marshallers --- */
> >  #include	"gobject.h"
> >  #include	"genums.h"
> 
> two things here:
> - do we really need this accumulator in glib? afaik, this kind of accumulator
>   is used only for events, i.e. widgets, so having gtk provide this seems
>   sufficient to me.

We've found it very useful in different contexts in GTK+; we
don't just use it for events any more - basically
any time a signal should be handled by only a single entity it's
a useful pattern. E.g. GtkIMContext::delete-surrounding.

I don't think the usefulness has anything to do with GTK+ being a 
GUI library. 

> - what's the rationale to put the accumulator into the g_boolean_ namespace?
>   alternatives are:
>   - g_accumulator_ (better if we'll get more accumulators in the future)
>   - g_signal_ (only usefull if the accumulator isn't moved to gtk)

It's not in the 'g_boolean' namespace, it's just in the g_ namespace.

What the name means is:
 
 an accumulator for signals where a boolean return means "handled"

g_accumulator_boolean_handled() is OK, though it reads less well.
(It's GtkButtonRadio rather than GtkRadioButton)

g_signal_boolean_handled_accumulator() reads better, but is getting
long. 

What do you prefer?

> > Index: docs/reference/gobject/tmpl/signals.sgml
> > ===================================================================
> > RCS file: /cvs/gnome/glib/docs/reference/gobject/tmpl/signals.sgml,v
> > retrieving revision 1.28
> > diff -u -p -u -r1.28 signals.sgml
> > --- docs/reference/gobject/tmpl/signals.sgml	18 Apr 2003 00:18:06 -0000	1.28
> > +++ docs/reference/gobject/tmpl/signals.sgml	11 Sep 2003 19:23:41 -0000
> > @@ -870,3 +870,21 @@ Returns the invocation hint of the inner
> >  @Returns:
> >
> >
> > +<!-- ##### FUNCTION g_boolean_handled_accumulator ##### -->
> > +<para>
> > +A predefined #GSignalAccumulator for signals that return a
> > +boolean values. The behavior that this accumulator gives is
> > +that a return of %TRUE stops emission of subsequent callbacks,
> 
> signals are emitted, callbacks are called or invoked, so
> s/emission/invocation/

OK, I'll make it "stops the signal emission. No further callbacks
will be invoked."

> > +while a return of %FALSE allows emission to coninue. The idea
> 
> s/emission/the emission/

Will fix.

> > +here is that a %TRUE return indicates that the callback
> > +<emphasis>handled</emphasis> the signal, and no further
> > +handling is needed.
> > +</para>
> > +
> > + ihint: standard #GSignalAccumulator parameter
> > + return_accu: standard #GSignalAccumulator parameter
> > + handler_return: standard #GSignalAccumulator parameter
> > + dummy: standard #GSignalAccumulator parameter
> > + Returns: standard #GSignalAccumulator result
> 
> is the word "standard" really necessary (usefull) here?

I think it's more meaningful with it there. 

Regards,
						Owen