GNOME Bugzilla – Bug 551626
Storing guessed labels would increase performance and decrease repeated speech.
Last modified: 2008-09-10 18:48:40 UTC
While working on the fix for bug 546815, I was reminded of something that's been bugging me for a while: Given a line which begins with a radio button or check box and is followed by text which is *functioning* as the label, but is not *technically* a label, arrowing Up/Down will result in repeated speech because: * The form field is focused * Proper labels are ignored by getUtterancesFromContents(); plain ol' text on the other hand is not. * The speechgenerator calls guessTheLabel() for the focused form field, and guessTheLabel() guesses the text on the right. * getUtterancesFromContents() moves on to the next object, which just so happens to be the text we just guessed. :-) Here's Ye's test case: http://bugzilla.gnome.org/attachment.cgi?id=116314&action=view We need a way to determine that a given string is serving as a label for a particular form field. We can accomplish that by storing the guessed label for comparison. Also, if we store the guessed label -- assuming we are careful to not keep the guesses around too long -- we'll increase performance in cases where the user is Tabbing/Shift+Tabbing/Arrowing around in a form. (The initial guess will still be expensive, and we need to improve performance on that front.)
Created attachment 118415 [details] [review] revision 1 This patch is pylinted, regression tested, and seems to get the job done on both fronts. I performance tested for good measure (Orca+Tabbed through the Bugzilla Advanced Search page from top to bottom two full times each test). Sure enough, with the patch the average time per call was cut in half. (I'll look more at the performance of the initial calls this week -- both through bug 546815 and bug 517759 (maybe I can beat that patch into compliance). Will, I'd appreciate a review/sanity check on this one when you have the opportunity. My question/concern is: Is there some event which I missed that might indicate that our stored labels are no longer valid? In the spirit of high benefit, low risk this patch considers nearly everything as a provocation to destroy the stored guesses :-) Mike please test.
(In reply to comment #1) > Will, I'd appreciate a review/sanity check on this one when you have the > opportunity. My question/concern is: Is there some event which I missed that > might indicate that our stored labels are no longer valid? In the spirit of > high benefit, low risk this patch considers nearly everything as a provocation > to destroy the stored guesses :-) I think this looks good, though there is definitely a lot of cache management going on. I'm not sure how that can be avoided, and this fix definitely looks like a great improvement. So, I say commit. :-) Nice job.
(In reply to comment #2) > (In reply to comment #1) > there is definitely a lot of cache management going on. Yeah, I know. :-( > I'm not sure how that can be avoided, Well.... This is just off the top of my head (i.e. I've not thought it out, merely allowed myself to get distracted by your question for a few moments. ;-) ) But.... Maybe we could add a method to script.py called cleanUpCrap() or doYourThang() or something along those lines but less snarky. This method would be called by _processObjectEvent() for all (or nearly all) events. By default, it would just return. But scripts which have a particular task which needs doing for a significant number of events could override this method. Then we'd only be cleaning up in one place. Thoughts?
(In reply to comment #3) > Maybe we could add a method to script.py called cleanUpCrap() or doYourThang() > or something along those lines but less snarky. This method would be called by > _processObjectEvent() for all (or nearly all) events. By default, it would just > return. But scripts which have a particular task which needs doing for a > significant number of events could override this method. Then we'd only be > cleaning up in one place. Thoughts? Pre-process and post-process hooks might definitely be interesting to consider post 2.24. :-)
Nice! this does seem snappier. Note that this patch fails to apply to the tests but did apply to the actual source just fine.
Thanks Mike! I did just notice that as a matter of fact. :-) The last checkin (stuckage) modified the test in question. Patch committed with an updated regression test. I'm feeling pretty confident about the safeness/stability of this one, even more so given Will's review. So closing as FIXED. Should I be wrong, we can always reopen it. :-)