Re: [g-a-devel]at-spi patch (bugfixes and extended events)

[Michael Meeks <michael ximian com>]

Hi Bill,

	I think we're getting a lot closer; there are however still a number of
problems:

On Thu, 2002-11-21 at 15:42, Bill Haneman wrote:
> > 	This really disturbs me; and follows the existing (bad) convention of
> > exposing a struct which is essentially:
> > 
> > typedef struct {
> > 	Object magic_1_I_guess_we_need_an_object;
> > 	long   magic_2_we_might_need_a_long;
> > 	double magic_3_someone_might_need_a_double;
> > 	int    magic_4_an_int_for_good_measure;
> > 	string magic_5_a_string;
> > 	string magic_6_another_string_I_happened_to_need;
> > } Event;
> 
> Well, we are not creating such a struct

	I'm referring to the existing AccessibleEvent structure which has:

typedef struct {
  const char  *type;
  Accessible  *source;
  long         detail1;
  long         detail2;
} AccessibleEvent;

	Where 'detail1/2' have functionality that is almost entirely opaque to
the user. Eg. 'detail1' could be a char index, child index, etc. and
differs depending on the context in an unusual fashion.

> However I agree that there are
> drawbacks to trying for a "generic" API here, the more specific methods
> would be better.  The attached patch uses them, i.e.
> 
> AccessibleTextChangedEvent_getChangeString (AccessibleEvent *e),

	That's great :-) I'd really like to add (in future) methods to wrap the
'detail1' type elements such as:

long AccessibleChildChangedEvent_getChildIndex (AccessibleEvent *e);

	etc. so it's far clearer what is going on in some of the more
interesting code:

      char *new_text = AccessibleText_getTextAtOffset (text, (long)
 		event->detail1, SPI_TEXT_BOUNDARY_WORD_START, &start,
 		&end);
 
> In the current model we pass memory that is valid for the emission,
> however the Accessible being pointed to (for cases where that's what is
> obtained) may not be in a pingable state by the time the event is
> received.

	Ok: but please re-arrange the registry code to:

 a = Accessibility_Accessible_getChildAtIndex (BONOBO_OBJREF (desktop), 
						index, &ev);
  /* FIXME
  spi_init_any_object (&e.any_data, a);
  */
  spi_init_any_nil (&e.any_data);
-  Accessibility_Accessible_unref (a, &ev);
  Accessibility_Registry_notifyEvent (BONOBO_OBJREF (registry),
				      &e, &ev);
+ bonobo_object_release_unef (a, &ev);
  Accessibility_Desktop_unref (e.source, &ev);

	Also, that last Desktop_unref looks bogus; we never reffed the desktop
in this method, so why do we unref it ? looks like it's a good thing
that object is immortal. Code seems to have been cut and pasted several
times, it'd be worth fixing them all.

> +      if (any) e.any_data = *any;
> +      else spi_init_any_nil (&e.any_data);

	nice.

> +  if (signal_query->signal_id == atk_signal_text_changed)
> +    {
> +      sp = atk_text_get_text (ATK_TEXT (gobject),
> +			      detail1,
> +			      detail1+detail2);
> +      spi_init_any_string (&any, &sp);
...
> +  if (detail)
> +    spi_atk_emit_eventv (gobject, detail1, detail2, &any,
> +			 "object:%s:%s", name, detail);
> +  else
> +    spi_atk_emit_eventv (gobject, detail1, detail2, &any,
> +			 "object:%s", name);


	we're missing a 'g_free (sp)' here I think.

> +static char *
> +cspi_internal_event_get_object (const InternalEvent *e)
> +{
> +  CORBA_any *any;
> +  g_return_val_if_fail (e, NULL);
> +  g_return_val_if_fail (e->data, NULL);
> +  any = (CORBA_any *) e->data;
> +  if (any->_type == TC_CORBA_Object) 
> +    return cspi_object_add (* (CORBA_Object *) any->_value);

	This looks bogus to me; we should be using the

	cspi_object_borrow, (and vitally) cspi_object_return 'loan' concept
here to minimise round-trip referencing. cspi_object_add does a
reference ownership transfer which we don't want here.

> +char *
> +AccessibleTextChangedEvent_getChangeString (const AccessibleEvent *e)
> +{
> +  InternalEvent *foo = (InternalEvent *) e;
> +  /* TODO: check the event type? expensive... */
> +  return cspi_internal_event_get_text (e);

	We perhaps do want to check the event type, it's not really that
expensive, but if so we could trivially GQuark the incoming event type,
and cache it on the InternalEvent.

> -  s = AccessibleEvent_getContextString (event);
> +  s = AccessibleTextChangedEvent_getChangeString (event);

	Nice.

	There is one particularly burning issue that you don't seem to have
addressed though; and that's:

spi-listener.h [ public header ]:

/* 
 * For internal use by CSPI implementation only
 */
typedef struct {
  AccessibleEvent event;
  void           *data; 
} InternalEvent;

	This should be in spi-private.h - since as it says it's for internal
use only, and we _really_ don't want to expose it. Also, when it's in
spi-private.h we can strongly type the 'data' member, which we can call
any_data to be more useful - and then we can start binning the various
nasty bits of code like this:

+  CORBA_any *any;
...
> +  any = (CORBA_any *) e->data;

	And just use the member, type-safely, directly.

	HTH,

		Michael.

-- 
 mmeeks gnu org  <><, Pseudo Engineer, itinerant idiot