| msg43493 - (view) |
Author: Stephan R.A. Deibel (sdeibel) |
Date: 2003-04-27 04:02 |
| In using webbrowser.py we uncovered and fixed a number of problems and made some improvements in usability and consistency of behavior. Appended below is a summary of the changes made. This list is also found at the top of the uploaded file, which was based on the version of webbrowser.py found in Python 2.3a2. Sorry to submit so many changes at once but they were all made in a period of a few days when we went through this module for use in Wing IDE. I'm also submitting some unit tests for the module. I've tried to review everything carefully, further review is definately needed. Feel free to contact me if you have questions or comments or want me to make changes and resubmit. Hope this is helpful. - Stephan |
|
|
| msg43494 - (view) |
Author: Stephan R.A. Deibel (sdeibel) |
Date: 2003-04-27 04:14 |
| Logged In: YES user_id=12608 Oops, correcting the upload |
|
|
| msg43495 - (view) |
Author: Stephan R.A. Deibel (sdeibel) |
Date: 2003-04-27 04:32 |
| Logged In: YES user_id=12608 Jeez, here is the list of changes in this patch which I meant to append to the original report. Bugs fixed: * Don't apply lower() to command lines or commands that are going to be executed * Don't confuse browser name/id with command line used to launch the browser * Require that browser commands be executable * Identify user-provided strings as a command line by looking for any args and not just for %s * Handle spaces in user-provided command names, either if quoted on the user-provided command line or escaped with back slashes * Use '%s' instead of "%s", which avoids character interpretation on Unix command lines * Escape and quote urls more safely to prevent crafting urls that result in command lines that execute arbitrary commands * Fixed Galeon so it doesn't hang up the app until the browser is quit * Added the Mac OS X support that doesn't make bad environment assumptions on OS 10.1 * Fixed win32 use of Netscape, which before would never work because the _tryorder entry was being pruned out * Leave found items in _browsers and _tryorder on OS X (but prepend the OS X specific support in _tryorder). OS X is Unix so these are useful there. * Now add %s to end of command lines if they don't contain %s already, for compatibility e.g. with KDE's default value for BROWSER environment * Now add '&' to end of Unix command lines that are going to be launched from GenericBrowser to avoid hanging up on user-provided command lines * Added additional quoting of command names, so some of the browser classes can work with a renamed browser with a space in the executable name * Added a number of return values / checks where before there were bad assumptions in the code that might have led to errors Other internal changes made: * Added unit tests * Removed the loop in get() which always returned in the first iteration * _tryorder more tightly coupled with registering in _browsers, and removed potentially problematic last-ditch GenericBrowser registering clause and the prune-_tryorder clauses from the end of the module * _iscommand also returns true if cmd is an existing file name, and it returns the full path or None instead of just True and False * _synthesize registers and returns GenericBrowser if synthesis fails but the user-provided command line looks valid * Some additional uses of True and False instead of 1 and 0 Changes to the public interface: * Added optional update_tryorder arg to register() (default value is same as previous action of this function) * Open/open_new return False if command definately failed and True if it may have succeeded (both previously returned None in all cases) * Get() returns a GenericCommand if 'using' is given and doesn't name or synthesize a browser (previously it did this correctly only when %s was in the given command line) * Values passed via BROWSER environment will be registered and synthesized in the same way as 'using' arg values are treated in get() |
|
|
| msg43496 - (view) |
Author: Martin v. Löwis (loewis) *  |
Date: 2003-04-29 11:51 |
| Logged In: YES user_id=21627 Because this is quite a large change, I recommend to postpone it after 2.3. |
|
|
| msg43497 - (view) |
Author: Stephan R.A. Deibel (sdeibel) |
Date: 2003-04-29 14:46 |
| Logged In: YES user_id=12608 Yes, I intended it to be post-2.3. The Group popup choices doesn't include 2.4, however. |
|
|
| msg43498 - (view) |
Author: Michael Hudson (mwh) |
Date: 2003-04-29 14:50 |
| Logged In: YES user_id=6656 Is now. |
|
|
| msg43499 - (view) |
Author: Stephan R.A. Deibel (sdeibel) |
Date: 2004-11-24 00:08 |
| Logged In: YES user_id=12608 I'm uploading a newer version of the webbrowser.py file, which contains a small fix to avoid failing when there is an empty ("") BROWSER environment variable set. The diff is just this: --- wingwebbrowser.py 20 Feb 2004 23:45:26 -0000 1.3 +++ wingwebbrowser.py 10 Nov 2004 16:51:36 -0000 1.4 @@ -210,6 +210,8 @@ def _splitcommand(cmd): """Extract command name and args from command line""" + if cmd == '': + return '', '' if cmd[0] in ('"', "'"): name = cmd[1:cmd[1:].find(cmd[0])+1] args = cmd[len(name)+2:].strip() @@ -603,7 +605,8 @@ # Treat choices in same way as if passed into get() but do register # and prepend to _tryorder for cmdline in _userchoices: - _synthesize(cmdline, -1, True) + if cmdline != '': + _synthesize(cmdline, -1, True) cmdline = None # to make del work if _userchoices was empty del cmdline del _userchoices |
|
|
| msg43500 - (view) |
Author: Rodrigo Dias Arruda Senra (rodsenra) |
Date: 2005-03-20 04:38 |
| Logged In: YES user_id=9057 According to the Patch Submission Guidelines, we are supposed to submit patch files nor modified versions of modules nor zip files. I advise you to resubmit according to the guidelines documented in http://python.org/patches/ |
|
|
| msg43501 - (view) |
Author: Rodrigo Dias Arruda Senra (rodsenra) |
Date: 2005-03-20 14:04 |
| Logged In: YES user_id=9057 Against the [http://python.org/patches/ Patch Submission Guidelines] this entry has uploaded both the whole file and .tgz, but no patch file. The large numbers of changes make it difficult to review. All the changes were documented to the top of the file, I don't believe they belong there. At least that is not complaint to the first rule in http://www.python.org/patches/style.html There are several OS X specific corrections that I'm unable to reproduce/validate, since I have no access to that platform. The modules was renamed to wingwebbrowser.py and the test case was built using this name. I believe this is inadequate, the original module name should be kept. There are three categories of changes: bug fixes, internal changes and interface changes. IMO it would be better to break this patch in at least three, matching these types of changes. Bug fixes would have a higher priority, less chance to break backward compatibility and more chance to be incorporated earlier. It should be applied before patch 1077979, otherwise that fix would be reversed. In spite of the formatting comments above, there are *valuable* changes in the submitted file. This is a nice candidate to be tackled in a Python Bug Day, since it functionality involves a broad range of platforms and user browsers preferences. Moreover, thourough test cases are hard to be cooked. I recommend keeping it open until somebody (possibly its author) reshape it properly and then applying it. |
|
|
| msg43502 - (view) |
Author: Stephan R.A. Deibel (sdeibel) |
Date: 2005-03-20 23:46 |
| Logged In: YES user_id=12608 Sorry, guilty as charged re: uploading whole files. I did this for lack of time since there is some value here. Diffs are also not that useful given large number of changes. Agree this needs to be split up. Some additional comments: Splitting off the bug fixes for specific OSes may also be a good idea so they can be reviewed separately. Ediff is a good way to review the changes against current sources and make smaller patches. I deleted the initial (bad) upload. Not sure why I didn't do that originally. |
|
|
| msg43503 - (view) |
Author: Oleg Broytman (phd) * |
Date: 2005-03-23 14:18 |
| Logged In: YES user_id=4799 After two days of studying and playing with the patch I recommend to reject it. I can draw one or two smaller things from it - _isexecutable(), for example. But the entire patch is too big, too invasive, and sometimes does more harm than good. The problem number one. The patch tries to emulate a Unix shell - it splits command-line into words, it escapes shell metacharacters. And of course it fails. Nobody can emulate a shell, because there is no THE shell. There are far too many shells, all different. The patch escapes backslash, dollar sign, backtick and semicolon. What about other special characters, '!', '|', or asterisk? The problem number two. It adds ' &' to the every GenericBrowser command line. That's bad for text-mode console browsers. If I define BROWSER="lynx %s" I certainly don't want to run it in the background. And that's incompatible with the patch 754022 that runs browsers in _tryorder to find one that works; running a browser in the background prevents check if the browser has been started. The problem number three. The patch quotes too much. It quotes %s in remote commands: instead of openURL(http://example.com) it runs openURL('http://example.com'). Mozilla does not accept it and report error 509. (I don't know which instance rejects it - current or remote.) |
|
|
| msg43504 - (view) |
Author: Oleg Broytman (phd) * |
Date: 2005-03-30 08:26 |
| Logged In: YES user_id=4799 I'd like to return to discuss some parts that I've recommended to reject. When I've said "do not try to emulate a shell" I didn't mean "never manipulate a command-line", I meant "never write your own shell". Do not parse a command-line string, do not split it into words, do not interpret these words, quote them, combine and pass them to the real OS shell. Instead of manipulating strings manipulate lists (of strings and markers). Instead of quoting special characters avoid using shell altogether - use fork+exec (or spawn). This is much more safe. That said, I'd like to ask you to rewrite _safequote() and patch the entire webbrowser.py based on these ideas. |
|
|
| msg43505 - (view) |
Author: Georg Brandl (georg.brandl) * |
Date: 2005-09-15 08:04 |
| Logged In: YES user_id=1188172 Relevant ideas are now in #754022. |
|
|