GNOME Bugzilla – Bug 649577
atk_add_global_event_listener should only accept ATK events
Last modified: 2011-09-05 14:43:55 UTC
Current documentation of this method is the following: "guint atk_add_global_event_listener (GSignalEmissionHook listener, const gchar *event_type); Adds the specified function to the list of functions to be called when an event of type event_type occurs. listener : the listener to notify event_type : the type of event for which notification is requested Returns : added event listener id, or 0 on failure." The event_type definition is vague. This allow to ask to register to any possible event, and in the practice it is used to register to Atk events and to window events. Also the format it is not clear. For example, on at-spi2-atk code we can find: atk_add_global_event_listener (child_added_listener, "Gtk:AtkObject:children-changed"); BTW, on gail this "Gtk:" header is discarded, and only matters that there are three elements This is really tied to Gtk implementation, and means that any other toolkit (Clutter, Gecko, Unity, etc) needs to check the implementation to know what to do. So, as this is a atk method to register to a event, I think that it makes sense to only accept to register to ATK events. This doesn't require to modify the API. This only requires to decide a proper event_type format, clarify the documentation and update the implementation of this method (also this requires to solve the window related events). As a event_type proposal, I guess that this one is good: "atk_class::event_name" ie "AtkObject::focus_event"
> This doesn't require to modify the API. This only requires to decide a proper > event_type format, clarify the documentation and update the implementation of > this method (also this requires to solve the window related events). Update the implementation means update current ATK implementor (GAIL, Cally, Gecko, etc), not the current ATK implementation that it is basically a dummy implementation.
ATK 2011 Hackfest conclusion: * AtkWindow would be added so we could implement this "be more strict" * But we need to be careful about how to do that, as the atk-bridge is right now using this way to register to events. * Probably that means that we should make gail-cally-atk-bridge release at the same time, or that we would require to add a "deprecation time", supporting both ways to register to the events for some time.
BTW, there is an additional advantage if we are more restrictive with it, and we limit to ATK events. As Mike said on bug 611507 comment 1, that would allow to implement atk_add_global_event_listener on ATK itself, instead of on each ATK implementor.
Related chat log from #a11y: <Company> totally wrong is the fact that gail overrides vfuncs in parent classes <Company> that makes every programmer cringe <joanie> is that a bug you saw or one we need to file <Company> it's how AtkUtil is implemented <Company> gail (and cally, too) goes and says atk_util_class->add_event_listener = gail_add_event_listener; <Company> so it overrides a function in an object provided by a different library <Company> this mostly works as long as everybody knows it's happening <Company> unfortunately developers change <Company> and nobody would ever assume somebody does something this bad <Company> and then suddenly it's done from two places <Company> and then those two places are run by the same program <Company> and then things crash <Company> similar thing with global event listeners <Company> global event listeners get emitted whenever _any_ object emits this event <joanie> Company: is that this one? https://bugzilla.gnome.org/show_bug.cgi?id=649577 <Company> close <Company> better: "should not exist" <Company> actually i have stronger opinion <Company> "must not exist" <Company> anyway, on to the problem: <Company> every time a window is maximized, we emit the window:maximize event on the AtkObject that was maximized <Company> so the event handler runs and does things <Company> if the project includes GTK code, it's run for the windows provided by GTK <Company> if the project includes Clutter code, it's run for the windows provided by Clutter <Company> and if the project includes libatspi's shiny new ATK interface, it's run for the windows provided by libatspi <Company> so if we're lucky we'll end up advertising objects over dbus that were advertised to us over dbus which only exist because they were advertised to us via dbus <Company> and if we're unlucky, things will crash, because who'd have thought those objects in the events would ever be anything BUT objects provided by gail? <Company> and that is one reason why one doesn't use global event listeners <joanie> so I believe you. Honest. :-) But then.... Any guesses as to why we are? <joanie> i.e. smoking crack? or hacking to deal with a real problem? <joanie> and if the latter, I think we should find out what the problem is. <Company> it's very convenient <Company> because it allows you to not have to care about new objects <Company> and hooking them up <Company> you just wait until they show up in the event listener and then you handle them <mgorse> we have an atk_add_global_event_listener, which is usually a wrapper around g_signal_add_emission_hook <Company> right <Company> emission hooks are what i was talking about <Company> the fact that atk uses a separate function for wrapping it and it's using an evil madness for that is just a minor annoyance <joanie> In a nutshell, what would be the impact of removing atk_add_global_event_listener? (i.e. who would have to do what, where?) <Company> the bridge would need to keep track of wich objects it is actually monitoring <joanie> okay, so ATs and toolkits are then off the hook. mgorse what would be involved in causing the bridge to do what Company is proposing? <Company> well <Company> it also means that gail and cally need to properly emit the right events when new objects show up <Company> so that atk-bridge knows it should monitor them <Company> and the bridge needs to g_signal_connect() to the signals of those objects where it uses emission hooks today <Company> on the plus side <Company> you know which objects you are monitoring <Company> so you don't emit events for objects you don't care about <Company> which allows for a lot of nice optimizations of dbus traffic <mgorse> and, also, keep track of which signals an AT wants to listen to, and keep track of each id assigned to the signal handler for every combination of object and signal handler, so that it can disconnect handlers for a given signal when an AT unregisters <Company> mgorse: you can use g_signal_handler_disconnect_matched() for a start <Company> mgorse: but yes, a lot of bookkeeping... <mgorse> Company: Looking. Thanks <Company> http://developer.gnome.org/gobject/unstable/gobject-Signals.html#g-signal-handlers-disconnect-matched <Company> mgorse: also, did you ever look into GDbus? <mgorse> Company: Well, it is much slower than libdbus from the testing I've done, and no one has been prioritizing trying to optimize it (including me, since I hadn't seen a major reason for needing to port to it) <Company> yeah <Company> the whole droute magic etc seems to be done by gdbus, too <Company> so you could maybe get rid of most of the code in there and use gdbus instead <Company> and then we could go blame davidz and desrt <mgorse> AT-SPI is kind of stretching what dbus was originally designed for, I think <Company> yeah <Company> so it's a good test case for people claiming they support dbus ;) <Company> looks like i just volunteered to rewrite atk-bridge using gdbus <Company> just to see how fast it goes
(In reply to comment #4) > Related chat log from #a11y: > > <Company> totally wrong is the fact that gail overrides vfuncs in parent > classes > <Company> that makes every programmer cringe > <joanie> is that a bug you saw or one we need to file > <Company> it's how AtkUtil is implemented > <Company> gail (and cally, too) goes and says > atk_util_class->add_event_listener = gail_add_event_listener; > <Company> so it overrides a function in an object provided by a different > library > <Company> this mostly works as long as everybody knows it's happening Yes, this was the reason that you needed to be really careful loading hail (gail:hail:atk-bridge), and cally (cally:atk-bridge). In the same way, current status makes a mixed-toolkit environment a hack. You can see what firefox is doing, as they basically redefine some of AtkUtil. And a clutter/gtk mixed environment could be also a mess. But as Mark Doffman already said, AtkUtil right now is broken: https://mail.gnome.org/archives/gnome-accessibility-list/2009-August/msg00027.html > <Company> anyway, on to the problem: > <Company> every time a window is maximized, we emit the window:maximize event > on the AtkObject that was maximized > <Company> so the event handler runs and does things > <Company> if the project includes GTK code, it's run for the windows provided > by GTK > <Company> if the project includes Clutter code, it's run for the windows > provided by Clutter > <Company> and if the project includes libatspi's shiny new ATK interface, it's > run for the windows provided by libatspi > <Company> so if we're lucky we'll end up advertising objects over dbus that > were advertised to us over dbus which only exist because they were advertised > to us via dbus > <Company> and if we're unlucky, things will crash, because who'd have thought > those objects in the events would ever be anything BUT objects provided by > gail? I don't understand this specific crash case. > <Company> it's very convenient > <Company> because it allows you to not have to care about new objects > <Company> and hooking them up > <Company> you just wait until they show up in the event listener and then you > handle them Yes I also think that were added by convenience. > <joanie> okay, so ATs and toolkits are then off the hook. mgorse what would be > involved in causing the bridge to do what Company is proposing? > <Company> well > <Company> it also means that gail and cally need to properly emit the right > events when new objects show up Right now this is more or less done with AtkObject:children-changed. In fact there are some cache problems if a object is added and the signal is not emitted. But this is done with the current global event listener, so I guess that you are proposing a new API here. So, you are proposing a new API here, and probably somewhat global, as it is about report a new object, no matters where in the hierarchy is placed. Any quick-draft for that new API? > <Company> so that atk-bridge knows it should monitor them > <Company> and the bridge needs to g_signal_connect() to the signals of those > objects where it uses emission hooks today > <Company> on the plus side > <Company> you know which objects you are monitoring > <Company> so you don't emit events for objects you don't care about Well, sometimes the problem is decide the objects you don't care about. Ie. With respect of orca. Simplifying a lot, orca main event to listen is the focus event. We already said that current atk/at-spi pair should provide a way to know the current focused object, so it should also provide a global event "focus-change". But, what about things like: * orca reacts to window:activate events. Although probably it can be bypassed as this is also related with a focus:change * orca reacts to new notifications (see bug 648633): and in this case the object doesn't get the focus, it is just being shown. * <other orca reactions to global events> How would orca would manage that? In the same way that atk-brige is tracking all the objects, should orca also track all the objects? Anyway probably I missing something (see above about API proposal). > <Company> which allows for a lot of nice optimizations of dbus traffic > <mgorse> and, also, keep track of which signals an AT wants to listen to, and > keep track of each id assigned to the signal handler for every combination of > object and signal handler, so that it can disconnect handlers for a given > signal when an AT unregisters > <Company> mgorse: you can use g_signal_handler_disconnect_matched() for a start > <Company> mgorse: but yes, a lot of bookkeeping... > <mgorse> Company: Looking. Thanks So Mike, your opinion about global listeners ? <no comments on gdbus-libdbus stuff, seems off-topic here> Finally, and going again with this short-medium-long term things. Benjamin proposal has a lot of advantages, and probably this is how things should behave. But still requires some refinement and answer some questions, so that means that it is a medium-long term solution. And in the same way, current ATK would improve with this change (and that AtkWindow and so on) in the short-term. And, IMHO, this is not a big change (in terms of code) on ATK, and would allow to remove some code from ATK implementors. So not now, as we still need some debate, but eventually I think that it would be better to remove that "should be eliminated completely" from the bug summary, and, IMHO, track the removal of the global event listeners on a different bug.
[Mass-reassigning open atk bug reports for better trackability as requested in https://bugzilla.gnome.org/show_bug.cgi?id=653179 . PLEASE NOTE: If you have watched the previous assignee of this bug report as a workaround for actually getting notified of changes in atk bugs, you yourself will now have to add atk-maint@gnome.bugs to your watchlist at the bottom of https://bugzilla.gnome.org/userprefs.cgi?tab=email to keep watching atk bug reports in GNOME Bugzilla. Sorry for the noise: Feel free to filter for this comment in order to mass-delete the triggered bugmail.]
(In reply to comment #5) > Finally, and going again with this short-medium-long term things. In the name of this short-medium term proposal, and taking into account that bug 638924 was solved, I have just updated atk_add_global_event_listener to indicate that this method purpose is register to ATK events. So I will close this bug. Probably next step is implement this on ATK itself, as current implementation is really similar on any ATK implementor. (In reply to comment #5) > > <joanie> okay, so ATs and toolkits are then off the hook. mgorse what would be > > involved in causing the bridge to do what Company is proposing? > > <Company> well > > <Company> it also means that gail and cally need to properly emit the right > > events when new objects show up > > Right now this is more or less done with AtkObject:children-changed. In fact > there are some cache problems if a object is added and the signal is not > emitted. But this is done with the current global event listener, so I guess > that you are proposing a new API here. > > So, you are proposing a new API here, and probably somewhat global, as it is > about report a new object, no matters where in the hierarchy is placed. > > Any quick-draft for that new API? Just to say that explicitly: IMHO that belongs to the already in-process API disruptive ATK3. Company, feel free to create a new "global event is a bad idea" bug and propose an alternative way to allow AT to listen to the server (apps) events.
Also editing again the bug title. As I said on previous comment, "(or possibly should be eliminated completely)" should be managed in a different bug.