GNOME Bugzilla – Bug 665870
About reducing accessible-name, accessible-description change notifications
Last modified: 2012-03-07 12:36:35 UTC
In relation with bug 660218. At this moment atk_object_set_name and atk_object_set_description emits a property notification change each time you call those methods. This have some problems: a) At initial time, if you have a lot of objects, you will have a lot of notifications, flooding any AT. b) On the current implementation, although both methods are virtual, the notification is called on the method itself. That means that you couldn't create an atkobject subclass trying to be smart on the notification. About a). After quick a little some options could be: 1.) Do not notify if the current name/description is NULL, assuming that this is the initial set_name. 2.) Try to be smart using the state of the object. For example, notify only if it is VISIBLE, FOCUSED etc. Right now Joanmarie is checking when the accessible-name notification is required. About b). As AtkObject itself provides a atk_object_set_name default implementation, in my opinion the notification should be moved there. Anyway, although this would allow any developer to create a smart-atkobject, that would also means that this subclass would require to manage the name/description by itself, as calling the parent implementation to finally set the name will means that notification. Finally, before doing this change we need to check how this affects the bridge. AFAIK, it uses a cache, so not sure if removing notification will means an outdated cache. If this is the case, probably the same ideas I mentioned on 1.) or 2.) could be used on the bridge.
(In reply to comment #0) > 1.) Do not notify if the current name/description is NULL, assuming that this > is the initial set_name. FWIW, this is already done with the roles. If current role is UNKNOWN, no notification is sent. FWIW2, there is a nasty workaround. atkobject->name is public. So one option could be avoid to call atk_object_set_name, and just access it. But: * Concerns about not having notifications, bridge and outdated cache are still valid. * This doesn't seems a really Object-Oriented approach. And we plan to remove that access in the future (bug 647488) FWIW3. As I mention on bug 647488 comment 4, g_object_notify is redefined on atkobject in order to emit this as a property-change signal. In that case, we could go for option 2.) for any property, trying to be smart for any notification. But not sure if there is a general rule for any atkobject property. Anyway, this reduces my concerns about having the notification on the call itself, and not on the virtual implementation. If a custom atk object wants to be smart with the notification, it only needs to redefine again g_object_notify
Right now, for objects without ATK_STATE_TRANSIENT, libatspi caches the name and relies on a PropertyChange signal to update its cache. After an object is created, atk-bridge sends an AddAccessible signal with the initial name. This is done in an idle handler run after receiving a child-added signal. Roles are handled the same way, so my guess is that the bridge would not need to be modified if atk was modified not to send the notification if the initial name was NULL. I think this would address the original issue with Thunderbird causing a flood of object:property-change:accessible-name events. If we are going to send object:property-change:accessible-name events only when an object is visible and/or focused, then I think we need to clearly document when they are required and/or add an API into ATK so that the toolkit can set the level of caching done by libatspi. It would be possible to have libatspi only assume that a cached name is valid if the object had STATE_VISIBLE and STATE_FOCUSED at the time the cache was updated and never lost it, although of course this would make the caching rules more complicated and create the potential for bugs to lead to seemingly odd behavior (ie, if an object was focused, became unfocused, did not send a state-changed notification because of a bug, and then had its name changed, then libatspi would still report the old name because atk didn't send a notification because the object was unfocused and libatspi still trusted its old cached value because the state-changed:focused was not sent). One option would be to add an UpdateAccessible signal that would update the name, description, states, etc. at the same time, whenever there was a change, but this might also slow things down if computing the state set is an expensive operation for the toolkit. Also, if we move the notification into atk_object_real_set_name, then we should keep in mind that this could introduce regressions with atk implementors that override set_name, since previously they were not required to call the base function or send a notification, and now they are. Of course, the requirements for third-party gtk+ widgets wanting to implement atk have changed with gtk+ 3.2 anyhow, so maybe that doesn't matter so much. In any case, it would mean that someone should review the various atk implementations.
(In reply to comment #2) Thanks for the really detailed answer. > Right now, for objects without ATK_STATE_TRANSIENT, libatspi caches the name > and relies on a PropertyChange signal to update its cache. After an object is > created, atk-bridge sends an AddAccessible signal with the initial name. This > is done in an idle handler run after receiving a child-added signal. Roles are > handled the same way, so my guess is that the bridge would not need to be > modified if atk was modified not to send the notification if the initial name > was NULL. I think this would address the original issue with Thunderbird > causing a flood of object:property-change:accessible-name events. Ok, taking into account this paragraph, and the following one, I will implement this solution, for both accessible-name and accessible-description. In summary: * It is easier to implement on the ATK side * Makes the atk-bridge life easier So easier for both parts. > Also, if we move the notification into atk_object_real_set_name, then we should > keep in mind that this could introduce regressions with atk implementors that > override set_name, since previously they were not required to call the base > function or send a notification, and now they are. Of course, the requirements > for third-party gtk+ widgets wanting to implement atk have changed with gtk+ > 3.2 anyhow, so maybe that doesn't matter so much. In any case, it would mean > that someone should review the various atk implementations. In the end I will not move the notification. As I said on comment 1, g_object_notify is already redefined. So it is not required to move the notification into atk_object_real_set_name to implement a "smart-notification" object. Any atk implementor could redefine again g_object_notify. And after all, this is working and nobody is complaining, so I will not modify something that it is already working.
Created attachment 203521 [details] [review] Fixes the bug This patch just checks if the current name/description is NULL. This can be problematic in this use case: * AtkObject creation * NULL->"something" : no notification => fine * "something"->"something other" : notification => fine * "something other"->NULL : notification => fine * NULL -> something : no notification => not ideal One way to avoid that should be a flag on the object. But the lack of a private structure (bug 647488) makes this complex.
(In reply to comment #4) > Created an attachment (id=203521) [details] [review] > Fixes the bug Thank you, thank you, thank you! :) Oh, and btw, thank you. :) > This patch just checks if the current name/description is NULL. > > This can be problematic in this use case: > * AtkObject creation > * NULL->"something" : no notification => fine > * "something"->"something other" : notification => fine > * "something other"->NULL : notification => fine > * NULL -> something : no notification => not ideal For names, an event flood is even less ideal. So, from where I'm sitting, who cares? ;) Just make the problem go away so users' systems stop grinding to a halt. Then worry about making it ideal. ;) For names. For descriptions.... Hmmm.... I dunno. And I say that for the following reasons: 1. Orca doesn't listen to property-change events on the accessible description. So we are of course not being flooded by those. ;) What I do not know yet is, would we be flooded by them if we were? Easy enough to test that, and I'll do so shortly. But maybe we wouldn't because most apps are not setting the accessible description whereas they are indeed setting the accessible name. 2. At the moment, I cannot imagine a use case in which an application or toolkit would keep an accessible object around, change its name to NULL, and then change its name to something non-NULL, and an AT would care. I can, however, imagine a hypothetical use case in which this might occur for a description: Imagine a widget which sometimes displays red "error" text in a nearby label (or other texty object) when the user input has been deemed bogus. I can see an app creator who cares about accessibility taking the time to set the description to the error when the red text appears, and to NULL when the red text goes away. If new bogus input is then provided within that widget.... So unlike the name, in the case of a description, we might see the subsequent NULL -> something you point out above. And we might care. (Aside 1: Personally I think that situation would instead call for using a described_by/description_for AtkRelation pair. But setting a property like the description is, I believe, programmatically easier and more "obvious." And many people are all sorts of lazy, and our documentation could definitely be improved. Aside 2: I've never actually seen this because Orca is not listening for description changes. But maybe we should. <shrugs>) Anyhoo.... Given this: > One way to avoid that should be a flag on the object. But the lack of a private > structure (bug 647488) makes this complex. Pending the answer to item 1 above, I wonder if it would make sense to use this solution for names now and leave descriptions alone. Then deal with bug 647488 and make it "ideal" for both. <shrugs again>
To follow up: (In reply to comment #5) > 1. Orca doesn't listen to property-change events on the accessible description. > So we are of course not being flooded by those. ;) What I do not know yet is, > would we be flooded by them if we were? Easy enough to test that, and I'll do > so shortly. But maybe we wouldn't because most apps are not setting the > accessible description whereas they are indeed setting the accessible name. Now I know. :) Using Accerciser and launching Thunderbird with the current folder having 6 messages: * name-change events upon launch: 139 * description-change events upon launch: 16 Using Accerciser and launching Thunderbird with the current folder having 218 messages: * name-change events upon launch: 991 * description-change events upon launch: 16 So, again, eliminating the name-change-upon-creation events will be very nice to have, especially for users with 2000+ messages in their inbox. ;) If you feel the solution is "not ideal" and do not feel any need to implement it for both name and description, perhaps just for name is sufficient for now?? Thanks again very much for doing this!
Atk doesn't allow the name or description to be (re)set to NULL. There's a g_return_if_fail that checks for that.
Created attachment 203563 [details] [review] docs: Explains that NULL is not valid for name/description
(In reply to comment #7) > Atk doesn't allow the name or description to be (re)set to NULL. There's a > g_return_if_fail that checks for that. Good catch thanks. As this is not explained on the docs, I added a new patch (comment 8). In this aspect NULL for name/description is similar to ATK_ROLE_UNKNOWN on the case of the roles.
So, it solves the event flood and I'm not seeing any regressions thus far from it. Were it me I'd commit it to unstable, roll a release, and hope downstreams pick it up quickly so the bleeding-edge Orca users can beat on it. Thanks again!
(In reply to comment #10) > So, it solves the event flood and I'm not seeing any regressions thus far from > it. Were it me I'd commit it to unstable, roll a release, and hope downstreams > pick it up quickly so the bleeding-edge Orca users can beat on it. Done committed. Included on ATK 2.3.3 release. > Thanks again! Thanks for your testing.
*** Bug 660218 has been marked as a duplicate of this bug. ***
It just occurred to me (since I think that Trevor filed a similar bug for atk_object_set_parent) that, in theory, it is possible for a class to override both get_name and set_name so that some other mechanism is now used for recording the name, and the object's "name" field is not used at all, in which case atk_object_set_name would continue to see that the object's "name" is NULL and thus not send notifications. I can't think of a reason for a class to do this, though, so perhaps it would be best to just document somewhere that, if a class does this, then it needs to handle sending notifications itself. An alternative would be to call atk_object_get_name(), rather than check the "name" field, to fetch the old value, although this requires that the class's get_name function return the old value at the time set_name is called.
(In reply to comment #13) > alternative would be to call atk_object_get_name(), rather than check the > "name" field, to fetch the old value, although this requires that the class's > get_name function return the old value at the time set_name is called. I also think that this is the better solution.
Created attachment 208459 [details] [review] Using atk_object_get_name instead of atkobj->name Patch just in case someone want to review it. Not committed as I didn't test it.
I will be try the patch after I installed latest Precise live CD my machine. Enough to download latest atk source, apply the patch and do autogen.sh, make and make install command? When I looked friday for example Thunderbird launching, I need wayting 40 seconds before possible working any message folder in Thunderbird. I experienced this problem under Ubuntu Precise, Orca 3.3.90 version. Thunderbird version is 11.0~B3. My inbox have 2339 messages, and have some other folders with I filtered my messages. Attila
Created attachment 208474 [details] A debug file with showing what happening after I applyed the attached patch I generated a debug file with Orca after I applied the attached patch. Hopefuly showing this file why launch very slow Thunderbird if Orca is running. Of course, this problem happening only if have large number of messages for example the inbox folder. I not updated yet with at-spi2-core and at-spi2-atk packages, only pulled from git the atk source. Attila
(In reply to comment #16) > I will be try the patch after I installed latest Precise live CD my machine. > Enough to download latest atk source, apply the patch and do autogen.sh, make > and make install command? > When I looked friday for example Thunderbird launching, I need wayting 40 > seconds before possible working any message folder in Thunderbird. I > experienced this problem under Ubuntu Precise, Orca 3.3.90 version. > Thunderbird version is 11.0~B3. > My inbox have 2339 messages, and have some other folders with I filtered my > messages. Please, take into account that this bug should be already solved (take into account that the bug is solved) and that this change is already released. The patch that I included on comment 15 is just a little improvement of the original patch (comment 4) to avoid some possible problems in the future, as mentioned by Mike on comment 13. I mentioned that I needed to test it to check that I didn't cause any regression with that patch.
Sorry, I missunderstood the patch purpose. Alejandro, I need opening a new report with Thunderbird related and attaching Orca debug.out file? Future possible resolving this performance related problem? 40 second wayting all Thunderbird launching is very disturb the awerage work if have large number messages of any mailbox folder. Attila Attila
(In reply to comment #19) > Sorry, I missunderstood the patch purpose. > Alejandro, I need opening a new report with Thunderbird related and attaching > Orca debug.out file? Future possible resolving this performance related > problem? > 40 second wayting all Thunderbird launching is very disturb the awerage work if > have large number messages of any mailbox folder. Yes I think so. As the accessible-name/description notification is now more filtered, I think that you found a new issue. So I think that it would be better to create a new bug with that debug.out and check what it is happening. Thanks
Alejandro, I think Trevor Saunders and I found why happening I described issue. If you have a little time, look the bug 670802 report. Attila
(In reply to comment #15) > Created an attachment (id=208459) [details] [review] > Using atk_object_get_name instead of atkobj->name > > Patch just in case someone want to review it. > > Not committed as I didn't test it. Although I committed it, in the end I needed to revert the change: https://bugzilla.mozilla.org/show_bug.cgi?id=733712 Anyway, in the future it would be good to implement something similar to what Alexander Surkov proposed, but probably after 3.4