GNOME Bugzilla – Bug 528147
Break up large scripts into packages
Last modified: 2008-06-03 20:20:06 UTC
Some scripts are large, and should be fragmented across several files.
Created attachment 109278 [details] [review] Make package from staroffice script
Created attachment 109279 [details] [review] Make package from gecko script I would putt this in the scripts subdirectory, but I know that is an unpopular sentiment, so I will keep it in src/orca.
As discussed in a meeting, we are going to move forward with this. Also toolkit scripts will be in orca.scripts.toolkits, and app scripts in orca.scripts.apps. The progress for this could be tracked in my git tree: http://github.com/eeejay/orca/tree The refactor is happening in a branch called "script_stuff".
Here is a diffstat of all the changes together: src/orca/J2SE-access-bridge.py | 518 src/orca/Makefile.am | 2 src/orca/focus_tracking_presenter.py | 45 src/orca/scripts/Evolution.py | 1776 - src/orca/scripts/Makefile.am | 27 src/orca/scripts/Mozilla.py | 36 src/orca/scripts/StarOffice.py | 2799 -- src/orca/scripts/Thunderbird.py | 235 src/orca/scripts/acroread.py | 696 src/orca/scripts/apps/Makefile.am | 32 src/orca/scripts/apps/Mozilla.py | 36 src/orca/scripts/apps/Thunderbird/Makefile.am | 9 src/orca/scripts/apps/Thunderbird/__init__.py | 29 src/orca/scripts/apps/Thunderbird/script.py | 214 src/orca/scripts/apps/Thunderbird/speech_generator.py | 54 src/orca/scripts/apps/acroread.py | 696 src/orca/scripts/apps/evolution/Makefile.am | 10 src/orca/scripts/apps/evolution/__init__.py | 22 src/orca/scripts/apps/evolution/script.py | 1544 + src/orca/scripts/apps/evolution/speech_generator.py | 69 src/orca/scripts/apps/evolution/where_am_i.py | 226 src/orca/scripts/apps/gcalctool/Makefile.am | 10 src/orca/scripts/apps/gcalctool/__init__.py | 22 src/orca/scripts/apps/gcalctool/script.py | 144 src/orca/scripts/apps/gcalctool/speech_generator.py | 58 src/orca/scripts/apps/gcalctool/where_am_i.py | 49 src/orca/scripts/apps/gdmlogin.py | 72 src/orca/scripts/apps/gedit/Makefile.am | 9 src/orca/scripts/apps/gedit/__init__.py | 28 src/orca/scripts/apps/gedit/script.py | 579 src/orca/scripts/apps/gedit/speech_generator.py | 67 src/orca/scripts/apps/gnome-keyring-ask.py | 62 src/orca/scripts/apps/gnome-mud.py | 179 src/orca/scripts/apps/gnome-panel.py | 107 src/orca/scripts/apps/gnome-search-tool.py | 188 src/orca/scripts/apps/gnome-system-monitor.py | 98 src/orca/scripts/apps/gnome-terminal.py | 299 src/orca/scripts/apps/gnome-window-properties/Makefile.am | 9 src/orca/scripts/apps/gnome-window-properties/__init__.py | 28 src/orca/scripts/apps/gnome-window-properties/script.py | 52 src/orca/scripts/apps/gnome-window-properties/speech_generator.py | 59 src/orca/scripts/apps/gnome_segv2.py | 62 src/orca/scripts/apps/liferea.py | 145 src/orca/scripts/apps/metacity.py | 187 src/orca/scripts/apps/nautilus.py | 185 src/orca/scripts/apps/notification-daemon.py | 66 src/orca/scripts/apps/pidgin/Makefile.am | 11 src/orca/scripts/apps/pidgin/__init__.py | 23 src/orca/scripts/apps/pidgin/constants.py | 57 src/orca/scripts/apps/pidgin/script.py | 949 src/orca/scripts/apps/pidgin/speech_generator.py | 118 src/orca/scripts/apps/pidgin/where_am_i.py | 163 src/orca/scripts/apps/planner/Makefile.am | 10 src/orca/scripts/apps/planner/__init__.py | 28 src/orca/scripts/apps/planner/braille_generator.py | 87 src/orca/scripts/apps/planner/script.py | 57 src/orca/scripts/apps/planner/speech_generator.py | 77 src/orca/scripts/apps/rhythmbox/Makefile.am | 10 src/orca/scripts/apps/rhythmbox/__init__.py | 28 src/orca/scripts/apps/rhythmbox/braille_generator.py | 56 src/orca/scripts/apps/rhythmbox/script.py | 52 src/orca/scripts/apps/rhythmbox/speech_generator.py | 55 src/orca/scripts/apps/soffice/Makefile.am | 13 src/orca/scripts/apps/soffice/__init__.py | 2 src/orca/scripts/apps/soffice/braille_generator.py | 233 src/orca/scripts/apps/soffice/constants.py | 3 src/orca/scripts/apps/soffice/script.py | 2059 + src/orca/scripts/apps/soffice/speech_generator.py | 348 src/orca/scripts/apps/soffice/where_am_i.py | 252 src/orca/scripts/gaim.py | 1164 - src/orca/scripts/gcalctool.py | 192 src/orca/scripts/gdmlogin.py | 72 src/orca/scripts/gedit.py | 617 src/orca/scripts/gnome-keyring-ask.py | 62 src/orca/scripts/gnome-mud.py | 179 src/orca/scripts/gnome-panel.py | 107 src/orca/scripts/gnome-search-tool.py | 188 src/orca/scripts/gnome-system-monitor.py | 98 src/orca/scripts/gnome-terminal.py | 299 src/orca/scripts/gnome-window-properties.py | 81 src/orca/scripts/gnome_segv2.py | 62 src/orca/scripts/liferea.py | 145 src/orca/scripts/metacity.py | 187 src/orca/scripts/nautilus.py | 185 src/orca/scripts/notification-daemon.py | 66 src/orca/scripts/planner.py | 161 src/orca/scripts/rhythmbox.py | 107 src/orca/scripts/toolkits/GAIL.py | 31 src/orca/scripts/toolkits/Gecko/Makefile.am | 13 src/orca/scripts/toolkits/Gecko/__init__.py | 4 src/orca/scripts/toolkits/Gecko/bookmarks.py | 294 src/orca/scripts/toolkits/Gecko/braille_generator.py | 524 src/orca/scripts/toolkits/Gecko/constants.py | 102 src/orca/scripts/toolkits/Gecko/script.py | 8855 ++++++++ src/orca/scripts/toolkits/Gecko/speech_generator.py | 796 src/orca/scripts/toolkits/Gecko/where_am_i.py | 221 src/orca/scripts/toolkits/J2SE-access-bridge.py | 518 src/orca/scripts/toolkits/Makefile.am | 12 src/orca/scripts/toolkits/VCL.py | 31 src/orca/settings.py | 15 test/harness/orca-customizations.py.in | 4 104 files changed, 21849 insertions(+), 20710 deletions(-)
So the patch was so insanely complicated with a million renames, that I decided to put it up as a separate SVN branch. It's called "script_refactor" we could erase the branch after we are through reviewing.
Created attachment 110248 [details] results from the FF regression test run No regressions. Woohoo! All failures are known (i.e. marked) "BUG?"s with these exceptions: moz_menu.py, uiuc_grid.py, uiuc_tree.py, and xul_role_list_item.py. If you take a look at the sheer volume of the tests, this is awesome. Plus these are not regressions, but things that come and go seemingly with the phases of the moon. And any time I update the tests to reflect one condition, the other condition immediately appears. I guess I need to mark those as BUGS? * The first, one failure: If I set the cursor to 0, the test gives me 1; set it to 1 and it gives me 0. * The next two are an extra BRAILLE LINE and VISIBLE that sometimes we get and sometimes we don't. * The last one is the speaking of 'Default font:' which we sometimes speak and sometimes don't. Flip a coin. Anyhoo.... Way to get Eitan! :-)
Some pylint issues to resolve (I'll look into these): ./run_pylint.sh `find src -name \*.py` > pylint.out grep "Your code has been rated" *.pylint | grep -v "at 10.00" J2SE-access-bridge.pylint:Your code has been rated at 5.05/10 braille_generator.pylint:Your code has been rated at 9.18/10 default.pylint:Your code has been rated at 9.99/10 (previous run: 9.99/10) speech_generator.pylint:Your code has been rated at 9.44/10 where_am_i.pylint:Your code has been rated at 8.23/10
(In reply to comment #7) > Some pylint issues to resolve (I'll look into these): With the exception of the J2SE script, which was just missing "orca." prefixes in the package imports, the bulk of the problems were with the style of doing "import *" on the constants stuff. There's also an odd empty soffice/utils.py module we probably should get rid of. For the wildcard imports, I know I said it was OK to do this. But in looking back on all the years of scratching my head in Java code, wondering where the heck a symbol came from, I remember now why wildcard imports make me so tense - they save time up front, but can cost more time in the overall maintenance of the code. I have a patch coming that fixes all the pylint errors (I think), with the only remaining offenders being the wildcard imports in: Gecko/script.py pidgin/script.py pidgin/speech_generator.py pidgin/where_am_i.py I feel somewhat strongly about this and I'm willing to unwild things. But...that can happen after the commit of this refactor (i.e., no need to hold up the commit due to this stylistic preference). Patch for the other stuff coming...
Created attachment 110276 [details] [review] Patch to address pylint issues with the exception of the wildcard imports This patch addresses several pylint issues, with the exception of the wildcard imports. You might notice some wildcards were removed, but that was because no constants were being used in the associated files. I still need to run the regression tests, but if they work, the only remaining work is to get rid of that empty soffice/utils.py file and the reference to it in soffice/Makefile.am.
(In reply to comment #8) > I feel somewhat strongly about this and I'm willing to unwild things. > But...that can happen after the commit of this refactor (i.e., no need to hold > up the commit due to this stylistic preference). How do you think it is best to deal with this? import constants And then prefix the constants with "constants."? I think that could be fine in script.py. But __init__.py needs to have them directly imported in to the namespace, so I am thinking either import them all explicitly, or put in a pylint exception in a comment in __init__.py alone. I don't like the idea of explicit imports in __init__.py, because we will always need to remember to tweak that file too.
(In reply to comment #10) > (In reply to comment #8) > > I feel somewhat strongly about this and I'm willing to unwild things. > > But...that can happen after the commit of this refactor (i.e., no need to hold > > up the commit due to this stylistic preference). > > How do you think it is best to deal with this? > > import constants > > And then prefix the constants with "constants."? This is what I would prefer for us script maintainers. > But __init__.py needs to have them directly imported in to the namespace, so I > am thinking either import them all explicitly, or put in a pylint exception in > a comment in __init__.py alone. This is fine. > I don't like the idea of explicit imports in __init__.py, because we will > always need to remember to tweak that file too. Agreed. Thanks!
(In reply to comment #9) > I still need to run the regression tests, but if they work, the only remaining > work is to get rid of that empty soffice/utils.py file and the reference to it > in soffice/Makefile.am. The gtk-demo regression tests work well with this branch and the given patch applied. Thanks!
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.
I think the refactoring caused the following problem with one of the oocalc regression tests: trunk/test/keystrokes/oocalc/bug_363804.py Traceback (most recent call last):
+ Trace 197080
self.applyButtonClicked(widget)
self.writeUserPreferences()
ftp.loadAppSettings(self.appScript)
__import__(name, globals(), locals(), [''])
orca.scripts.StarOffice.speakCellCoordinates = False
Looks like the code in app_prefs.py needs to be adjusted for the new script file layout.
Created attachment 110590 [details] [review] soffice fix. The problem is in the setAppPreferences() method for any script that needs to write out some app specific preferences. I think this currently is only soffice and pidgin. Here's the fix for the soffice script. pidgin one coming up next. Eitan, I'll let you test them and check them in. Thanks.
Created attachment 110591 [details] [review] pidgin fix Hopefully here's a fix for pidgin (untested).
Created attachment 110597 [details] [review] soffice + pidgin fixes. Thanks to Joanie for working out the magic incantation to make this work (I'd run out of dead chickens).
Created attachment 110598 [details] [review] Gecko fix Tested, pylinted, committed.
Excellent. Sorry for being MIA. Could this bug be closed now?
Created attachment 110653 [details] [review] Another fix for the soffice script. Now that the application script is divided into multiple files, it looks like you need to adjust setAppPreferences() in the script.py file, to write out the preferences for each file that is going to use it. In other words: ... prefs.writelines( \ "orca.scripts.apps.soffice.script.speakCellCoordinates = %s\n" % \ speakCellCoordinates) prefs.writelines( \ "orca.scripts.apps.soffice.speech_generator.speakCellCoordinates = %s\n" % \ speakCellCoordinates) Ugh! Eitan, is this really the way we need to do it now? If so, then we'll need to double check the pidgin and Gecko preferences to see if they are used in the braille_generator.py, speech_generator.py and where_am_i.py files. Patch not committed yet, but it does fox oocalc regression test bug_363804.py
Another question is how do you adjust the code so that it correctly pylint's? I changed it to: prefs.writelines("orca.scripts.apps.soffice.script." \ "speakCellCoordinates = %s\n" % speakCellCoordinates) prefs.writelines("orca.scripts.apps.soffice.speech_generator." \ "speakCellCoordinates = %s\n" % speakCellCoordinates) and it runs nicely, but pylint says: ************* Module orca.scripts.apps.soffice.script C0301:311: Line too long (84/80) C0301:316: Line too long (86/80) which is very strange as 316 is the start of the next method.
Created attachment 110657 [details] [review] Version of the recent soffice/script.py change that pylints. I've gone ahead and committed this change as it fixes up the bug_363804.py oocalc regression test.
Joannie and I were just chatting about this on IRC. The current situation w.r.t. app-specific preferences needs to be adjusted a bit. Do you remember that old CS adage? "Constants aren't. Variables won't". Well that's what we've got here. There are values that are currently in the constants.py that can be adjusted via the Orca application specific preferences dialog to new values. Maybe the solution here is to call constants.py, variables.py or have YA file called settings.py under each of the script directories (as needed) for the applications that have these adjustable settings. That's the first problem. The second problem is having to (currently) set two lines in the app preferences file for settings if they are used in two different files (in script.py and speech_generator.py for soffice for example). Perhaps the solution here is to do something like we do in orca.py (about line 875): if _userSettings: try: reload(_userSettings) reloaded = True except ImportError: debug.printException(debug.LEVEL_FINEST) except: debug.printException(debug.LEVEL_SEVERE) else: try: _userSettings = __import__("user-settings") except ImportError: debug.printException(debug.LEVEL_FINEST) except: debug.printException(debug.LEVEL_SEVERE) Eitan, I think we'll throw the ball back to you to come up with a great solution. Thanks!
Created attachment 110659 [details] [review] app settings in soffice Rich, This is the simple way of doing what you did above. I didn't actually test this, but it should work :P The idea is that modules are global, so you really only need to assign the constants once.
> This is the simple way of doing what you did above. I didn't actually test > this, but it should work :P And I should be wealthy. :P :P Eitan, please try the following and see what happens: 1. Apply your patch. 2. Get into OOo and bring up the app-specific prefs dialog. 3. Uncheck the lone checkbox on the soffice page. 4. Press the OK button. Then quit Orca. 5. Relaunch Orca and repeat step 2. What is the state of the checkbox? It should be unchecked (and is without your patch). For me, with your patch, it's checked again. :-( What am I missing? Thanks!
Yup. As Joanie says, the above doesn't work. An alternate here would be to leave the user settable settings in the various script.py files. Then there would be a single preferences line to write out in the setAppPreferences() method and the other classes in the same application script directory would have to be adjusted to reference the preferences with something like: orca.scripts.apps.soffice.script.speakCellCoordinates
Created attachment 110978 [details] [review] Proposed patch Yeah, you guys were right. There are issues with importing symbols directly into a namespace, it makes a copy as opposed to referencing the original module symbol. This patch changes 'constants' to 'script_settings'. Let me know what you think that name convention. It's also not directly imported into __init__.py (with import *). instead you need to do orca.scripts.apps.soffice.script_settings.speakBlahBlah = True
Will I'd rather you verify this one.
Eitan - can you also make the same constants.py->script-settings.py change across the board? wwalker@server:~/orca/trunk/src/orca/scripts$ find . -name constants.py ./apps/soffice/constants.py ./apps/pidgin/constants.py ./toolkits/Gecko/constants.py
Rich said he'd take the constants.py -> script-settings.py rename. :-)
Created attachment 111673 [details] [review] Patch to move user settable variables to script_settings.py Because this creates three new files and deletes three files (and SVN isn't very good at generatings diffs that can easily be applied to test this), I've just gone ahead and checked the patch into SVN trunk and moved the bug to "[pending]". Note that you might want to do a fresh checkout after this. You also might initially have some tracebacks if you have existing files under ~/.orca/app-settings.py for soffice.py, pidgin.py and Firefox.py.
Closing!