New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add turtledemo to IDLE menu #61374
Comments
The turtledemo is an on-ramp for younger programmers and we should make it easy to launch. |
Will attach patch. Coincidentally I'm am a younger programmer. |
Should be "... I'm a younger..." |
Attached is a patch. I hope the File menu is the right place for this. I had to move the code in Lib/turtledemo.py after "if __name__ ==..." into main(). |
Should this be added to Lib/idlelib/NEWS.txt ? |
I left a comment in rietveld.
I think that's the best place where to put it.
Why?
Yes. |
Because turtledemo doesn't have a main() function. I moved the code under 'if __name__ == "__main__"' into a main() function. |
Ezio, would you like to take it from here? |
I tested the patch and it looks correct upon inspection. It looks like it applies cleanly and a straight forward solution. I made a slight change so that when the Demo exits, it has a better message. I also added the change to the NEWS.txt file. I also ran the IDLE tests, and everything passed. I didn't see new tests added, but I am not sure if that is needed, since we are just adding a menu binding. I've attached my patch with the changes listed above. It should be ready for review. |
My thoughts on this. Overall, +.something (and increasing as I look at it more ;-). I am opposed to adding this to the relatively long file menu. It has nothing to do with manipulating or editing files. If it is to go in, please put it on the help menu, at least for now, as the intention is to somehow help beginners. If we ever add a demo section or submenu to the Help menu or Demo to the top-level level menu, we can move this. Any patch should be applied to both 3.4 and 3.5. Turtledemo apparently does not exist in 2.7. Idle NEWS items first go into the Idle section of the regular NEWS file. They are supposed to be copied into the idlelib/NEWS with each release. That has sometimes happened but not always. (I should open separate issue to fix both the idlelib files and the procedure.) In any case, NEWS entries should not be added to a patch until it is about to be committed. Otherwise, merge conflicts are possible or even likely. With the main code in turtledemo.__main__ wrapped in a function, we could pass an 'idle' argument so that the demo could modify its behavior when run from Idle. I mention a possibility below. The patch runs turtledemo within the Idle process. It might be better to run it in a separate subprocess:
When Idle is run windowless with pythonw on Windows, attempts to print to the non-existent console crash the process. There have been several bug issues about this. The current patch should not be pushed with turtledemo as is unless tested with installed 3.4.1 by hand-patching idlelib. Or, all console prints should be eliminated. However, a future addition to turtledemo could create a problem. That is why I think it better that only idlelib code run within the idle process. I am bothered a bit that there is little relationship between Idle and turtledemo. Both using tkinter seems not too relevant. That both tend to be used by less experienced Python users seems a bit thin. The relationship would be stronger if there were other demos (which I would like) or if there were option in the demo to load the code for a particular example into an Idle editor window. (Each demo will run independently in an Idle user process - text output, including tracebacks resulting from ungracefully closing an example before it is done, appears in the Shell window.) |
I did not know that NEWS items should not be edited unless it is about to be committed. The previous comments suggested to put it in. Thank you for the feedback. I can move the Turtle Demp into the Help Menu rather than the File menu. I agree that it seems out of place. I can also make it such that it runs the demo as an external process rather than within IDLE. That way, it is completely separate by IDLE and we don't create a dependency. I think if we were to load in future demos, they could be run as other processes rather than within IDLE, so that future additions don't end up crashing IDLE. |
I am currently in the process of editing this patch such that the Turtle Demo launches from the Help Menu and spawns a separate process. However, I am deciding whether if the separate process should be asynchronous or not. Currently, I have it working with the subprocess module, but the IDLE window is hanging. I can use the multiprocess module or Popen to make it asynchronous. However, I am not sure where the clean-up should happen once the turtle process has been terminated. |
Check to make sure it is actually hung. The event-loop can make it look hung but it is actually just waiting for an event. An IDLE restart suffices to kill it sometimes. |
Okay, maybe "hanging" is not the right word. The IDLE window becomes busy since it spawned off the Turtle demo subprocess, and it is waiting for the subprocess to finish. After I close the Turtle window, it returns back to normal. I was wondering if the Turtle Demo should be a separate asynchronous process so that users could use the IDLE window as well as the Turtle demo. |
I currently have a patch where the Turtle Demo now shows up in the Help menu rather than in File menu. I also have it such that Turtle is now launched as a separate process rather than within the IDLE process. Currently, the commend is calling ./python.exe so it uses my build of Python rather than my system's python. But I can change that once people agree this is the right way to go. |
I think a patch should reuse the run module function that Idle already has. No need to re-invent something. I will say more tomorrow after sleeping. |
A possible inplementation of 'easy to launch' would be to load turtledemo.py into an editor window and generate a '<<run-module>>' event. This, of course, does more than is needed and has the danger of a user accidentally overwriting the original module code. An alternative would be a stripped down version of ScriptBinding.ScriptBinding._run_module_event.
An advantage of an extension is that it can be turned off if someone, such as a college course instructor, does not like it present. This change could be made before considering point 1. I would like to have one extension that could handle multiple external modules listed in the config entry. After reading config-bindings.def and some of the extension files, especially RstripExtension.py, I believe this is possible. However, I would expect to add this after there is a patch just for turtledemo.
|
I withdraw my suggestion that turtledemo get anything added just for moving code to Idle. What is does need is the ability to widen the code pane so one can more easily read or cut (by normal means). See bpo-21597. One of my concerns about putting turtledemo on the Idle menu is that people will see it as part of Idle. Fine it it works well ;-). Not so fine if it does not. The solution is to fix any important current problems. bpo-18132 (buttons disappear on small screens) has a patch that fixes the problem but creates another. I have not yet looked at the other open turtledemo issues. |
Okay! That makes sense. Any bugs that Turtle has, people will assume IDLE has them too if they launch it from IDLE. I will take on bpo-21597, and work on that instead. Thanks! |
Hi Terry, can we close this issue? Thanks! |
In 2.7, turtledemo is in <pythondir>/Demo/turtle when those directories are present. They are not currently installed by the 2.7 Windows installer, but this could be requested (of Steve Dower) on another issue. A 2.7 patch would be slightly tricker as it would have to check for the existence of turtledemo. Options:
Is turtledemo present on *nix/mac often enough to make a 2.7 addition worthwhile even without Windows? |
I personally think it would be better to check to see if the turtledemo exists during startup, and if so, add the menu entry. Otherwise, don't add it when loading up IDLE. |
Here is a new patch where it checks to see if turtledemo exists first before loading it onto the bindings. |
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:
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 bpo-22065, 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 bpo-11571. 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 bpo-11571, 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. |
I wasn't aware of make patchcheck. I will run this script when submitting patches in the future. Thanks, Ned! |
I've updated this patch to include the changes Ned mentioned. I am waiting to hear from Ronald if he has a better solution about dealing with the focus problem with the keyboard and mouse. |
turtle_demo_v3.patch looks good to me. I have satisfied myself that the AppleScript works OK with a non-English system. And, while it is a bit of a kludge for OS X and assuming the rest of the change works OK on Windows and Linux, I would recommend to Terry that we proceed with this approach for 3.5, 3.4 if someone feels strongly about it, and skip 2.7. If a better way to do it turns up, we can add that later. Time to move on to other things. |
I don't have a better solution for this, although I'd slightly prefer to add a hack to the turtledemo main function to force itself to the foreground on startup. That way the hack also works when the user starts the script in a Terminal window. |
Thanks for the input Ronald! How would I go about forcing the turtledemo to be in the foreground? Do I just need to call 'fg' on the subprocess? I've been Googling and couldn't find anything obvious. |
Ronald:
That's exactly what the proposed patch does. The AppleScript "activate" causes turtledemo, however started, to move to the foreground. |
I agree with skipping 2.7. The current patch is based on importing turtledemo, while on 2.7, turtleDemo.py is in Demo/turtle/ and cannot be imported. It could run with execfile, but the viewer will only run if /turtle is the current directory. On Windows, this would still be useless as the 2.7 installer does not install Demo or anything therein. When turtleDemo is installed, the location is system dependent. An extended version of the new checker extension, bpo-21880, would allow users to configure 'run turtledemo' to work on a system that has it. The patch works fine on Windows, including not blocking other actions with Idle. The only change I might make is The main remaining demo viewer fix is font re-sizing, bpo-21933, which I expect we should finish within a week. 'Run turtledemo' should go on the Run menu, along with other 'run <external program>' menu items. The Run menu is easily enabled in the Shell window by adding '("run", "_Run"),' to menu_specs before options (line 842). However, putting the entry where it would belong, after Run Module, means that the addition has to be delayed until after the ScriptBinding extension is loaded. That would be done most easily as part of loading the checker extension bpo-21880. The open_turtle_demo function would be moved and perhaps generalized. The change to make turtledemo run properly in a subprocess on Mac appears to be a separate issue from adding it to Idle's menu. I presume that if turtledemo.__main__ is loaded into Idle, and run with F5, as I have been doing, there is the same problem (on a Mac). I would guess that the same would be true of any tkinter app run in a subprocess from Idle. How about non-gui apps that are not mac-aware? If any of these are true, the fix would be generally useful outside of turtledemo and would seem to belong somewhere in the tkinter package. (Is the same true for some of maxosxSupport). If the activate code is moved to macosxSupport.activate(), does the following work in EditorWindow.open_turtle_demo? |
The activate code has to be called in the subprocess after Tk() has been called because only then is the subprocess guaranteed to have been promoted to an OS X GUI process. So that suggestion will not work. The idea of adding something to tkinter was discussed in msg192523 of bpo-11571, though not this specific solution. If you want to pursue more generalized issues of GUI application behaviors on OS X, that should be the subject of another issue. I suggest that this issue remain focused on the original topic. I also would be concerned about adding the Run menu to the Shell window (or system menu bar when the Shell window is active) merely to accommodate this feature. Having turtledemo in the Help menu seems fine to me. |
New changeset 04dd26ca02f4 by Terry Jan Reedy in branch '3.4': |
I would have made the turtledemo fix a separate issue. Given the combined patch, I will just make separate NEWS entries (which I am working on now). I expect there to be other new entries for Shell/Run, and will move when that is true. |
New changeset e344539cda11 by Terry Jan Reedy in branch '3.4': |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: