GNOME Bugzilla – Bug 491837
[will] Implement Gecko.py 'go to' functions with Collections
Last modified: 2008-07-22 19:33:54 UTC
Many of the 'go to' functions in Gecko.py might be able to take advantage of the performance increase available from using the Collections interface. Implement one such method, possibly a navigate by role method, that would best illustrate the performance gain.
Created attachment 98407 [details] [review] first version of findNextRole using Collection This is the first version of findNextRole() using Collection. The call to findNextRole() branches based on the availability of Collection. The iterative version is called if Collection is not available. Unfortunately, this version is not functional with the current state of Collection. Ariel and I have been in correspondence and are working out two issues. 1) Collection does not descend into objects that implement the Collection interface. This original design was considered to make Collections more efficient. However, this is proving to be a major obstacle for findNext|Prev routines. The 'feature' will either be taken out or a boolean parameter will be provided. 2) Collection does not traverse out of subtrees. This is a major issue because Collection will never find a given object outside a subtree if the starting obj lies within the subtree. I consider this a bug and Ariel is working on a solution. This patch is placed here for both safe keeping and as a place to begin discussion about integration of Collection into Orca for findNext|Prev routines. Once the kinks are out of Collection, the goal should be a global reorganization of the find routines using a set of MatchRules and a single findByMatchRule routine.
Cool work! It's great to see progress being made on this. > Unfortunately, this version is not functional with the current state of > Collection. Ariel and I have been in correspondence and are working out two > issues. Do you have bug numbers we can add to the dependency list for this bug so we can track the progress and patches?
The main Collections bug report can be seen here http://bugzilla.gnome.org/show_bug.cgi?id=326516 . More specifically, this Collections bug is what we are waiting on http://bugzilla.gnome.org/show_bug.cgi?id=496232 . This latest bug will be added to the "depends on" list.
Created attachment 104401 [details] [review] second version of Implement Gecko.py 'go to' functions with Collections This version wraps Gecko.findNextRole() and Gecko.findPrevRole() to use the Collection interface if it is available. Backwards compatibility is maintained if Collection is not available. I am now seeing about a 4-5 times performance increase for all go to role type user commands. Note: This patch must in conjunction with this atspi patch http://bugzilla.gnome.org/attachment.cgi?id=104394 .
Created attachment 104574 [details] [review] third version of Implement Gecko.py 'go to' functions with Collections This version addresses the situation where setCaretContext() places us deeper in the hierarchy than our previously found object. Thanks for testing the previous version Halim!
(In reply to comment #5) > Created an attachment (id=104574) [edit] > third version of Implement Gecko.py 'go to' functions with Collections > > This version addresses the situation where setCaretContext() places us deeper > in the hierarchy than our previously found object. Thanks for testing the > previous version Halim! > From reviewing the patch, this should work even if the Collections patch from bug 496232 is not present, right? If so, I think this looks fine and it pylint's to a 10.00. I say "commit" if you are comfortable that it doesn't introduce any regressions in the presence or absence of the patch in from bug 496232. Thanks!
> > From reviewing the patch, this should work even if the Collections patch from > bug 496232 is not present, right? No, bug 496232 is required because it introduces two new Collection methods which are used in this patch.
more accurately showing dependency tree
(In reply to comment #7) > > > > From reviewing the patch, this should work even if the Collections patch from > > bug 496232 is not present, right? > No, bug 496232 is required because it introduces two new Collection methods > which are used in this patch. D'Oh. OK. I just took a look at bug 496232. The Collections API is very confusing to me. It may be due to the total lack of documentation in the IDL, but it is just not intuitively obvious to me how the API is supposed to work. :-( In any case, I commented on the bug and I agree we should hold off on committing this patch until the API is resolved.
Created attachment 105648 [details] [review] fourth version of Implement Gecko.py 'go to' functions with Collections Will, this is an intermediate fix for all the Gecko 'go to' methods. I just want to make sure that I am heading in the right direction. This version creates two new methods in Gecko.py: findNextByMatchRule(self, col, matchrule, wrap, currentObj=None) findPrevByMatchRule(self, col, matchrule, wrap, currentObj=None) where col=Collection Interface, matchrule, wrap and currentObj=start of search object. These two methods were then used in findNextRole() and findPreviousRole() as an example of how these two methods can be used. Additional notes: 1) The iterative versions seem to be a little flaky at times and findPreviousRole() certainly fails when the last object on the page matches the search criteria. In this case, it simply doesn't find the object and continues the search upwards. 2) A Collection IDL change made it necessary to fix the page summary functionality. The fix is included in this patch.
Keep in mind that any testing will require the latest version of at-spi and the latest patch from http://bugzilla.gnome.org/show_bug.cgi?id=496232
(In reply to comment #10) > Will, this is an intermediate fix for all the Gecko 'go to' methods. I just > want to make sure that I am heading in the right direction. Looks good - I'm assuming the collections support is enabled here by default just for testing. I added some simple "print" lines in my local copy to make sure it was using the Collections stuff and it was. :-) The only question I have is with respect to the following pattern: try: try to do something with collections except NotImplementedError: do iterative form Is it possible that the "try to do something with collections" part might issue an exception other than NotImplementedError? If so, would it be safer in general to just do a blanket "except"? That would tend to make sure we always fallback to the iterative form regardless of what is broken. The disadvantage, of course, is that if there is something else wrong in the Orca code, we tend to hide it. In any case, this certainly does seem to make the 'go to' functions work much faster.
Created attachment 105768 [details] [review] fifth version of Implement Gecko.py 'go to' functions with Collections This version upgrades the following 'go to' methods. Note that only the 'next' method is listed, but 'prev' is also included: findNextRole: this method is used extensively in various 'go to' methods including heading, headingAtLevel, list, listitem, and table. goNextBlockquote: goNextVisitedLink: goNextUnvisitedLink: goNextLandmark: All other 'go to' methods have logic within the method that make it impractical for Collection. The try/except strategy outlined above was employed and each method is shielded from Collection with settings.useCollection settings.useCollection is currently True for testing purposes.
Will had asked me to try this patch. I'm afraid it's no longer trunk compatible. A quick glance seems to indicate that it's just the need to update it because of the wrapping fix made to link structural navigation for bug 518502.
Created attachment 106176 [details] [review] sixth version of Implement Gecko.py 'go to' functions with Collections New patch against branches/gnome2-22. Joanie, I did notice a behavior difference on find previous (un)visisted link. The Collection version does not announce that it has found the link if the startobj == foundobj. It will announce 'no more (un)visited links'. In other words, if there is one (un)visited link on the page, the iterative version will keep announcing that it has found that link.
Created attachment 106195 [details] [review] seventh version of Implement Gecko.py 'go to' functions with Collections This version wraps Collection calls in try/except clauses a little better for findNextRole() and findPreviousRole(). Joanie, the backwards compatibility regression tests (old at-spi and useCollection=True) may be a pain to interpret because Will and I thought it would be best to let the user know there were problems via debug.printException(). Let me know if I can help.
Woo hoo!!!!!!!!! Scott, you did it! :-) Cycle 2 passes. I guess for the purpose of documentation on this bug, I should clarify what on earth I'm talking about (from #orca discussion). To me it seems that there are three possible scenarios we want to examine given the nature of the change: 1. useCollection is False 2. useCollection is True, but the user doesn't have the latest at-spi which is needed for Scott's code to do its thang (in which case our old methods should kick in) 3. useCollection is True and the user does have the latest at-spi. Test cycle 1 passed; test cycle 2 failed rather badly. Hence Scott's latest patch, which I just finished regression testing, and which succeeded marvelously. Given that Scott and Mike have been functionally testing this already, my bet is on cycle 3 work beautifully as well -- perhaps minus the difference Scott alluded to in comment #15. But if it's just a little thing here and there, my vote would be to check this puppy in for the performance benefits and then let's open new bugs for any little nits. I'll start cycle three now just for the sake of thoroughness.
I wrote and submitted this comment (or one to this effect) last night. Apparently it didn't take.... ---------------------- Okay, cycle 3 was fine with one exception: Going to the previous list with Shift+L in a page with nested lists. The test is html_struct_nav_lists.py Other structural navigation (at least in terms of the regression tests) worked as expected. So.... I'll leave it to someone else to decide what to do. I'm still of the opinion that since the functionality seems to work for the most part and is more performant, that we check it in and open new bugs to address the little things that aren't quite right.
I fully agree with Joanie here this is a great speed improvement.
Committed to trunk. Marked as FIXED. Nested List bug as described in comment #18 is posted here http://bugzilla.gnome.org/show_bug.cgi?id=519587
Correction: This fix was committed to branches/gnome2-22, not trunk.
Also, committed to trunk.