Author ned.deily
Recipients Lita.Cho, Ramchandra Apte, ezio.melotti, jesstess, kbk, ned.deily, orsenthil, rhettinger, ronaldoussoren, terry.reedy
Date 2014-07-26.07:49:26
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <1406360967.24.0.306792643676.issue17172@psf.upfronthosting.co.za>
In-reply-to
Content
Here are some review comments on turtle_demo_v2.patch.

First, the subprocess call to start turtledemo may work ok in your build directory but it will not work in general.  Using the standard idiom for invoking a new process running the current instance of python, the call should look something like:

    cmd = [sys.executable, '-c', 'from turtledemo.__main__ import main; main()']
    p = subprocess.Popen(cmd)

Also, note that the imp module is deprecated in 3.4 in favor of importlib:

    https://docs.python.org/dev/library/imp.html#imp.find_module

I'm not sure there is much point in having this test, though.  The only thing it would catch is if some third-party distributor decided to move turtledemo into a separate package or not ship it altogether.  It would not work for 2.7, even if Demo/turtle was shipped, since, in 2.7, turtledemo is not structured as an importable package.

(And, to answer Terry's earlier question: no, the Demo directory is also not shipped with the python.org OS X installers for 2.7.  I expect that the standard practice among Unix distributors would also be not to ship it by default; for one thing, they have to figure out where to install it since we don't provide a standard location to do so.  I see that Debian does package up Demo into an optional "python2.7-examples" Debian package.  So there seems to be no point in applying this change to 2.7 without also backporting the turtledemo repackaging done in 3.x and that would be a larger undertaking needing discussion and approval.)

(Ah, but looking at current Debian and Ubuntu, for Python 3.4 I see that they have packaged turtledemo as part of their optional 'libpython3.4-testsuite' source package.  Plus, they have long packaged IDLE separately ('idle-python3.4').  That means end users will need to ensure both packages are installed to be able to use turtledemo with IDLE.  So I guess that says that there *is* a point to the import test.  Ugh.  I'm not sure what other popular distributions do.)

Then there is a specific and serious usability problem with this feature on OS X.  The subprocess call starts a new process with a second Python interpreter to run a second Tcl/Tk instance to run turtledemo next to IDLE.  The turtledemo appears *but* the keyboard and mouse focus remains on IDLE which also means that the IDLE menu remains active (recall that there is only one menu bar on OS X and it shows only the menus from the currently "focused" GUI application).  Especially with the proposed menu changes in Issue22065, it would be very confusing to the novice user to see the turtledemo window appear, possibly covering any IDLE windows, but with the IDLE menu still active and keyboard/mouse focus still on IDLE.  It turns out to be a bit tricky to reliably "activate" tbe turtledemo application programmatically from Python code without resorting to some hacks.  Here is one hack, making use of a bit of AppleScript, that seems to work.  It would need to be tested in non-English OS X environments to make sure it works there also.  

diff Lib/turtledemo/__main__.py
--- a/Lib/turtledemo/__main__.py        Fri Jul 25 15:01:18 2014 -0700
+++ b/Lib/turtledemo/__main__.py        Fri Jul 25 22:43:58 2014 -0700
@@ -69,6 +69,7 @@
 """
 import sys
 import os
+import subprocess

 from tkinter import *
 from idlelib.Percolator import Percolator
@@ -111,6 +112,20 @@
         self.root = root = turtle._root = Tk()
         root.title('Python turtle-graphics examples')
         root.wm_protocol("WM_DELETE_WINDOW", self._destroy)
+        if sys.platform == 'darwin':
+            # Make sure we are the currently activated OS X application
+            # so that our menu bar appears.
+            p = subprocess.Popen(
+                    [
+                        'osascript',
+                        '-e', 'tell application "System Events"',
+                        '-e', 'set frontmost of the first process whose '
+                              'unix id is {} to true'.format(os.getpid()),
+                        '-e', 'end tell',
+                    ],
+                    stderr=subprocess.DEVNULL,
+                    stdout=subprocess.DEVNULL,
+            )

         root.grid_rowconfigure(0, weight=1)
         root.grid_columnconfigure(0, weight=1)

There are deprecated OS X Carbon interfaces to do the equivalent, as was used in the third-party package appscript. I'm noseying Ronald to see if he has any better suggestions for a non-deprecated way to do this (via Cocoa perhaps) and that could possibly be made available more generally: IDLE itself could benefit from it and would be a better solution than what was used in Issue11571.

I have not tried the patch on Windows or X11 Tk to see if there are similar usability issues.  If so, it's possible the "wm attribtues . -topmost" dance, as in Issue11571, might help.

And last, Lita, please don't forget to run "make patchcheck" before uploading a patch; there are several whitespace problems with the patch that makes applying and reviewing it more difficult.
History
Date User Action Args
2014-07-26 07:49:27ned.deilysetrecipients: + ned.deily, rhettinger, terry.reedy, kbk, ronaldoussoren, orsenthil, ezio.melotti, jesstess, Ramchandra Apte, Lita.Cho
2014-07-26 07:49:27ned.deilysetmessageid: <1406360967.24.0.306792643676.issue17172@psf.upfronthosting.co.za>
2014-07-26 07:49:27ned.deilylinkissue17172 messages
2014-07-26 07:49:26ned.deilycreate