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

[Bill Haneman <bill haneman sun com>]

Hi...

Michael said:
> 	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.

We kept this to avoid breaking API compat (breaking bincompat was a
lesser concern).
 

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

That would be a nice future addition.  But one for GNOME-2.4, since API
freeze for 2.2 is today :-)

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

Done, thanks.
 
> 	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.

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

Hmm, thought our "sp"s were all stack allocated.  Will check.

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

Will fix as time permits, thanks.
 
> > +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.

Also something to look at when time permits.
 
> 	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 {
...
> 	This should be in spi-private.h - 

Yes.

This, and adding any remaining accessors we will need in 2.2, I will do
today, since API freeze is upon us.

regards and thanks,

Bill