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

[Michael Meeks <michael ximian com>]

Hi Bill,

On Mon, 2002-11-18 at 19:25, Bill Haneman wrote:
> I attach a patch for at-spi to add support for extended events

	Overall the patch looks good; but (as you might expect ;-) I have a
number of comments / questions:

> +typedef union {
> +  gchar       *string;
> +  AtkObject   *object;
> +  gpointer    *foo;
> +} AtkBridgeEventContextData;
> +
> +typedef struct {
> +  AtkBridgeEventContextType _type;
> +  AtkBridgeEventContextData _data;
> +} AtkBridgeEventContext;

	It's extremely unclear to me why this is necessary; this looks like
you're creating your own 'Any' type structure internally so the flow of
event emission looks like:

	create_my_own_event_struct;
	poke_in_it's_innards
	convert_my_own_event_struct_to_an_any
	send_any

	Why is this intermediate necessary ? surely; it's far more pleasant to
simply construct the correct Any with one of the nice helper functions
you have written, and pass that in instead of this expensive /
unnecessary intermediate:

	CORBA_any any;
	spi_any_set_object (&any, foo_object);

	send_event ...

	Rather than:

	FooType baa;
	atk_init_baa (&baa);
	baa.type = THIS_SHOULD_BE_IN_A_HELPER;
	baa.union.somevalue = ditto_magic_value;

	CORBA_any any;
	make_any_from_foo_type (&any, baa);

	send_it ...

	:-) it just seems cumbersome, slow and unnecessary.

	Also - why was the 'Atk' namespace used for this; surely it should all
be inside the 'spi' namespace; I started poking in 'atk' to see if this
had spread down there, and it hasn't.

> +  if (ctx) 
> +    spi_atk_bridge_event_context_free (ctx);

	Can we do that inside the 'free' method; but then if we bin the
intermediate we loose ~all of this code.

> +/**
> + * AccessibleEvent_getContextString:
...
	This has to have a const char * return; the lifecycle of that string is
owned by the event.

> +/**
> + * AccessibleEvent_getContextObject:
> 

	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;

	etc. except they're not called such descriptive names: 'detail1',
'detail2' ;-)

	Please don't continue this tradition; what we require is a self
documenting API; where just by reading the header it is instantly
apparent what something is. Eg. I would name these methods:

AccessibleEventChildrenChanged_getChildObject, and
AccessibleEventTextChanged_getNewText - or whatever it does.

	Thus people can instantly see what they do; of course; it'd be nice to
have:

AccessibleEventMouse_getButton

	instead of having to know that 'detail2 & FOO_MASK' is the button [ or
whatever ;-].

	This approach also makes user code, far, far, far easier to read /
maintain / debug / upgrade. So instead of 'event->detail1' we have
something relatively legible.

	I'd really like to see more verbose accessors there.

	This has to be one of the most incomprehensible bits of the patch to
me:

> --- registryd/registry.c	17 Nov 2002 13:53:57 -0000	1.52
> +++ registryd/registry.c	18 Nov 2002 19:23:52 -0000
> @@ -89,12 +89,17 @@
...
> +  a = Accessibility_Accessible_getChildAtIndex (BONOBO_OBJREF (desktop), 
> +						index, &ev);
> +  spi_init_any_object (&e.any_data, a);
> +  Accessibility_Accessible_unref (a, &ev);
>    Accessibility_Registry_notifyEvent (BONOBO_OBJREF (registry),
>  				      &e, &ev);

	Why do we do a round-trip from the registry -> child to fetch the
accessible at the index that is given ? this seems to almost entirely
nullify any benefit you had; inasmuch that the event can be internally
inconsistant; (consider the possible races) - and worse, it involves 2
extra roundtrips to the client, to get information that the client
authoritatively knew when it emitted the event; what's up with that ?
Perhaps I'm missing something.

	Oh - and there's a leak / memory corruption issue there; you have to
garentee the lifetime of 'a' over the event emission, otherwise you pass
off an object handle that may well be pointing at invalid memory - it
seems you do that by leaking the 'a', there needs to be a
bonobo_object_release_unref after the notify event (with this model) -
but I think this code badly needs re-thinking. [ problem happens
wherever this code was cut & pasted ].

> +  Accessibility_Accessible a;
...
> +  spi_init_any_object (&e.any_data, a);

>    Accessibility_Registry_notifyEvent (BONOBO_OBJREF (registry),
>  				      &e, &ev);

	This is (incidentally) great in terms of not having the intermediate
AtkFooCustomAny structure; it'd be great if this is how things happened
on the client side too.

> @@ -624,11 +638,11 @@
>      {
>        ctx.ev = ev;
>        ctx.e_out = *e;
> +      CORBA_any__copy (&ctx.e_out.any_data, &e->any_data);
>        ctx.source = e->source;

	I'm not clear where that any is freed; what's up there ?

--- cspi/spi-listener.h	17 Nov 2002 13:53:54 -0000	1.19
+++ cspi/spi-listener.h	18 Nov 2002 19:23:47 -0000
@@ -39,6 +39,14 @@
   long         detail1;
   long         detail2;
 } AccessibleEvent;
+  
+/* 
+ * For internal use by CSPI implementation only
+ */
+typedef struct {
+  AccessibleEvent event;
+  void           *data; 
+} InternalEvent;
 
	It's not clear to me why we want the non-namespaced, internal event
structure in the public CSPI header; and added to the api so clients can
see it.

	This should be in some internal private header.

	HTH,

		Michael.

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