Re: [g-a-devel]next at-spi patch ...

[Bill Haneman <bill haneman sun com>]

Michael Meeks wrote:
> 
> Hi Bill,
> 
>         This adds the hooks for libgnome to be able to init / shutdown
> accessibility on demand, and continues to look over the keystroke code,
> fixing bugs and making the tests more demanding.
> 
>         May I commit ?
> 
>         Regards,
> 
>                 Michael.
> 
> Index: ChangeLog
> ===================================================================
> RCS file: /cvs/gnome/at-spi/ChangeLog,v
> retrieving revision 1.148
> diff -u -p -u -r1.148 ChangeLog
> --- ChangeLog   2002/01/17 12:17:37     1.148
> +++ ChangeLog   2002/01/18 11:54:33
> @@ -1,3 +1,41 @@
> +2002-01-18  Michael Meeks  <michael ximian com>
> +
> +       * test/test-simple.c
> +       (key_listener_cb): consume the key.
> +       (test_keylisteners): update.
> +       (main): wait for any pending unrefs on events.

OK.

> +       * registryd/deviceeventcontroller.c
> +       (spi_controller_update_key_grabs): only re-issue the
> +       grab on a key release.

Not sure this is safe.  There are conditions (particularly with AccessX,
or with auto-repeat) where you get multiple keypresses in a row, without
corresponding releases.  We should play it safe here and do it on press
and release, at least until we can do more rigorous testing.  The
patched code passes our existing tests but I think this change could
cause breakage in certain cases.

> +       (spi_device_event_controller_forward_key_event):
> +       refresh the keygrabs before we notify the listeners,
> +       to reduce the X ungrab / re-grab race.

Mmm, I am a little unsure about this.  We call XAllowEvents right
after... if we don't consume the events (i.e. we call with the
ReplayKeyboard enum) do the released events not then get caught again? 
At the moment we are consuming the events always I believe, but if we
don't then this could cause a loop.

> +       (spi_controller_register_with_devices): remove
> +       XSelectInput - we do that with the gdk_window_ call.
> +       (_spi_controller_device_error_handler): return a value.
> +       s/GDK_DISPLAY/spi_get_display/

Thanks :-)

> +
> +2002-01-17  Michael Meeks  <michael ximian com>
> +
> +       * registryd/deviceeventcontroller.c
> +       (_deregister_keygrab): don't blow out the later
> +       assertion.
> +
> +       * test/test-simple.c (test_keylisteners): do a
> +       more intelligent validation.
> +
> +2002-01-14  Michael Meeks  <michael ximian com>
> +
> +       * atk-bridge/bridge.c
> +       (gnome_accessibility_module_init),
> +       (gnome_accessibility_module_shutdown): impl.
> +       (gtk_module_init): protect vs. double inits.
> +       (add_signal_listener): impl.

OK

> +       (spi_atk_bridge_state_event_listener): kill

Don't kill this function, or its declaration at the beginning of
bridge.c.

This needs to stay - however you remind me that I need to do some work
on it.
I see that we're not calling it at the moment, but that's an oversight I
should
correct today.

> +       (deregister_application): split out of
> +       (spi_atk_bridge_exit_func): here.
> +
OK;

Much of the refactoring here seems unnecessary - for instance there is
really no need 
to deregister the atk event listeners on exit since the whole bridge
code is about to
be unloaded, and since the objects are all in-process they will not
persist.

However if one ever wanted to dynamically 'unload' accessibility support
from a running
GNOME application this would be the correct way to do it.  So I'm not
rejecting this part
of the patch, I just don't see that it was in any way urgent.


So:

Please commit, with the following small revisions:

(1) reinstate the state_listener code in bridge.c, I will make use of
it.  If the resulting duplication is offensive I can refactor a bit.

(2) re-issue the keygrabs on key press and key release, at least until
we can rule out conflicts with key repeat.


The possible problem with ReplayKeyboard can wait for now, if it turns
out to be an issue we will need to do some other fancy footwork with the
X server instead, since I agree that your change reduces a possible
race.

Thanks

- Bill

-Bill