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

[Michael Meeks <michael ximian com>]

Hi Bill,

On Mon, 2002-01-21 at 15:14, Bill Haneman wrote:
> > +       * 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.

	I'm just worried that we will create problems for ourself issuing grabs
we don't need. man XGrabKey says:

>	The active grab is terminated automatically when the
>       logical state of the keyboard has the specified key released
>	(independent of the logical state of the modifier keys).

	Which I took to mean that we need to re-grab only when we get a key
release event; but I've commented the check for release out.

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

	It's really not clear to me what the XAllowEvents is doing; it appears
to me that if we release a key, then X automatically de-registers the
grab, creating a race condition whereby we need to re-register the grab
as soon as possible - as suggested by the section of man page above. I
assume this is why we need a different to do this.

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

	I couldn't find the hooks for a state listener; and I grepped atk, and
gail when I removed the method.

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

	With my changes to libgnome it is my intention that you be able to
dynamically enable and disable accessibility support.

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

	Grief - the duplication is not bad; I drastically cut that down with my
last commit there - the warning and presence of unused code is tedious
but hey, it's back.

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

	Done.

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

	Great.

	Just committed,

		Michael.

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