GNOME Bugzilla – Bug 540187
Wrapped structural navigation toggle
Last modified: 2009-03-10 00:05:20 UTC
It would be great to have a structure navigation wrapping toggle. So when we reach the end of the document and no further objects are found, we simply announce "not found".
Please give me a little time to submit my own possible patch :)
Jon, that would be awesome! You rock! Assigning this bug to Jon. :-)
Created attachment 113476 [details] [review] patch v0.1 Thanks Joannie, :) Since you are the Gecko expert, and this is my first patch, would you mind having a look over it? This is what I have attempted to do: 1. structural_navigation.py: not pass the wrap variable around in function calls, but the function that requires it will use the settings.wrappedStructureNavigation variable. 2. settings.py: making the wrappedStructureNavigation available, and setting default value to true. 3. orca-setup.glade: added wrapped structure navigation checkbox to the general tab, dont know if this is the best place to put it. Was thinking to put it in the specific page for ff, but since we are hoping to add structural navigation more widely, so decided to leave it here for the moment. 4. orca_gui_prefs.py: Added a handler for the checkbox when it changes, and an entry to load the value into the checkbox when the ui is created. my problem: structural_navigation.py functions dont seem to be called? Or at least they are not printing out the debug messages, nor are they having any noticeable effect when the wrap function is true/false. I checked that scripts/toolkit/Gecko/structural_navigation.py werent overriding any of the functions that i modified, but no further light was brought. I also have the orca.debug.debugLevel = orca.debug.LEVEL_SEVERE in user-settings.py Thank you in advance for your help
Nice work Jon! I just tried it with visited links and it worked for me: No wrapping. It looks like you only put the debugging statements in for the predicate-based find methods; by default the collections-based methods -- _find{Prev,Next}ByMatchRule -- are used. So try adding debugging statements there as well. What structural navigation object are you attempting to navigate among where your change fails to prevent wrapping? Minor nits: 1. Structural navigation; not structure navigation. <smile> 2. It looks like you've inserted tab characters rather than spaces. Some editors will convert tabs to spaces; others won't. We use spaces. You can quickly find out where you've done that by running run_pylint.sh from your top level directory after doing a make install. The output files list all the horrible pythonic crimes you have commented. <grin> We pylint religiously. Suggestions/Things to think about: 1. Right now, the find methods are just being called by goObject(). goObject() handles jumping from item to item. But will goObject() always be the only thing to use the find methods? Maybe those find methods will be needed in the future to get a count of objects for the purpose of presenting the user with a summary of objects on the page. Or maybe we'll want to implement some support for dialog-based selection of objects. Or maybe we'll want to do something else we haven't even thought of yet. In all of those cases, we would not want to wrap, but the user's preference might be to wrap. So.... I would put wrap back as an argument in all of the places you removed it, and would instead set wrap at the top of goObject() to be your new setting. Make sense? 2. I think it will be confusing to have some structural navigation settings on the Firefox page and another on the General page. In addition, we already have one setting that doesn't exist in the Preferences dialog at all (largeObjectTextLength). What I would suggest is that we leave it out of the GUI for now and let users set it in their orca-customizations.py. Ultimately, we will need to consider where all settings structural navigation should live, but today doesn't need to be that day as far as I'm concerned. <smile> If you really, really think it needs to be set via the Preferences dialog, I'd add it to the Firefox page for now. Hope this helps. And thanks again for doing this!!
(In reply to comment #4) > I just tried it with visited links and it worked for me: No wrapping. > It looks like you only put the debugging statements in for the predicate-based > find methods; by default the collections-based methods -- > _find{Prev,Next}ByMatchRule -- are used. So try adding debugging statements > there as well. aah my bad, must have been half asleep :) > What structural navigation object are you attempting to navigate among where > your change fails to prevent wrapping? it didnt seem to work for headings and lists, but will have a go again. > Minor nits: > 1. Structural navigation; not structure navigation. <smile> :) Sorry > 2. It looks like you've inserted tab characters rather than spaces. Some > editors will convert tabs to spaces; others won't. We use spaces. You can > quickly find out where you've done that by running run_pylint.sh from your top > level directory after doing a make install. The output files list all the > horrible pythonic crimes you have commented. <grin> We pylint religiously. :) Thank you, i should have thought of that one before, will reconfigure editor. > Suggestions/Things to think about: > 1. Right now, the find methods are just being called by goObject(). goObject() > handles jumping from item to item. But will goObject() always be the only thing > to use the find methods? Maybe those find methods will be needed in the future > to get a count of objects for the purpose of presenting the user with a summary > of objects on the page. Or maybe we'll want to implement some support for > dialog-based selection of objects. Or maybe we'll want to do something else we > haven't even thought of yet. In all of those cases, we would not want to wrap, > but the user's preference might be to wrap. So.... I would put wrap back as an > argument in all of the places you removed it, and would instead set wrap at the > top of goObject() to be your new setting. Make sense? Excellent, thank you > 2. I think it will be confusing to have some structural navigation settings on > the Firefox page and another on the General page. In addition, we already have > one setting that doesn't exist in the Preferences dialog at all > (largeObjectTextLength). What I would suggest is that we leave it out of the > GUI for now and let users set it in their orca-customizations.py. Ultimately, > we will need to consider where all settings structural navigation should live, > but today doesn't need to be that day as far as I'm concerned. <smile> If you > really, really think it needs to be set via the Preferences dialog, I'd add it > to the Firefox page for now. Ok, thats all good, Maybe if we get a lot of these rare but "nice to be able to configure" variables, we could have an advanced tab, where these misc variables are set. I'll remove it alltogether from the ui for now. > Hope this helps. Thank you, it certainly does
Created attachment 113771 [details] [review] minimal diff to place wrapping choice into settings.py Applying this patch, i still dont see any change in behaviour, nor is the print line being output.
Created attachment 113773 [details] sample output from run_pylint.sh run_pylint.sh produced two files, attached above is the settings.pylint file, is this the expected output?
(In reply to comment #7) > Created an attachment (id=113773) [edit] > sample output from run_pylint.sh > > run_pylint.sh produced two files, attached above is the settings.pylint file, > is this the expected output? Hey Jon: First of all -- hooray to you for writing Orca code! AWESOME! Thanks also for taking the care to run pylint to check your code. The error you see from pylint indicates that you are using an older version of pylint that does not have support for the warning in question. What you need to do is update your pylint, logilab-common, and logilab-astng to the latest packages, which you can obtain from http://www.logilab.org/857. Thanks again for jumping in here! The more people that can work on Orca, the better! Will
Thanks Will, updated successfully and pylint running fine now. Developing using python is intresting, due to its spacing, .. especially on a 40 cell braille display :) I can hardly say that ive written any serious code here, but im hoping to be more useful as i learn more. Please be patient with me in the meantime. :) Joannie, sorry to trouble you again, the print statement in goObject function doesnt seem to ever excicute. ive run make; sudo make install ran orca from terminal, to see output, loaded up firefox, and jumped between headings, lists and unvisited links, but nothing on the terminal. im missing Something obvious i assume. Thank you
> I can hardly say that ive written any serious code here, but im hoping to be > more useful as i learn more. Please be patient with me in the meantime. :) Jon, no worries. I for one am totally jazzed that you're doing this. Also, I just tried your minimal patch. I launched Orca from gnome-terminal, launched Firefox, and pressed h for heading: jd@blockhead:~/orca$ orca goObject, wrapped=True So it's definitely getting called. Question is: Why aren't you seeing it? In the spirit of paranoia, do you know for certain that you're using the new structural navigation code? One way to check would be to look at your keybindings. Paragraph is now P/Shift+P and a bunch of new object have been added as unbound bindings. Are they showing up?
Created attachment 113811 [details] find results Thats intresting, p/shift+p doesnt do anything, nor do they appear in the keybindings tab, and there doesnt seem to be anything that strikes me as new. Mgorse on the irc channel, is also running trunk, and p/shift+p doesnt seem do do anything for him either. Is there any way that we could be using old Gecko.py code somehow? I did sudo apt-get remove gnome-orca then: find / | grep Gecko output attached. Thank you for any enlightenment.
Created attachment 114022 [details] [review] Hopefully final patch please test.
Essentially, yeah. <grin> Silly nits that I'm embarrassed to admit bother me: "Weather" is the stuff we talk about when we have nothing else to say. "Whether" is what you want. :-) Given the number of settings, we try to group them. In settings.py I'd put wrappedStructuralNavigation with the other structural navigation settings (e.g. the table navigation settings and large object size). But from a code and functionality point of view, it seems to work quite nicely, it pylints, and, while I have not tried the regression tests on it yet, there's no reason the tests would fail because of this. So if you make my anal retentive changes (oh my gawd, I've become Willie Walker. Yikes!), I would say we're good to go. Nice job!!! And thanks!!!
Created attachment 114277 [details] [review] final
think of my embarrassment when I noticed I made the weather/whether mistake .. yet another time :) Does "its my third language" excuse count anymore? :) I guess not, 8 years of English and I can't differentiate between the two. Hope the above attachment is accepted :)
The above attachment is awesome! Seeing that you've tested it, and that I've tested it, and that the change is sound, I recommend we check it in. Mike, any objections? Jon, are you authorized to check in code via svn? If not, I would be honored to check this in for you, but I'll need to know the name to use for the ChangeLog entry. AFAIK we just know you as Jon Orca User ;-) Thanks!
No, I currently dont have permission to svn commit. It might be safest for the time being :) until the day that i create clean patches. changelog name: Mesar Hameed Do I need to do anything here with this bugzilla status? or only close it as fixed once the patch is in trunk. Thank you
Thanks! As soon as we hear from Mike, I'll do the commit. As you may have noticed, we have our own team customs on bugzilla. :-) Once the patch is committed to trunk, we put the word "[pending]" at the beginning of the summary to give the patch a little time in the hands of the bleeding edge users and Mike. After nothing bad has resulted from the commit, Mike will then remove "[pending]" and replace it with "[verified]". That's the green light to close the bug as FIXED (also please remove the "[verified]" when you do so). Our system might initially strike some as being a little bit detailed, but it really makes it easy to keep on top of where bugs are in the process, who is expected to do what with them, whose input is needed (in the case of the [team] and [team-member-name]) prefixes, etc.
It seems like a lot of hard work has yielded a good patch here. Lets get it checked in.
Thanks Mike. Patch committed to trunk. Moving this bug to pending. Way to go Jon! :-)