Skip to content
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

Closed
rhettinger opened this issue Feb 9, 2013 · 35 comments
Closed

Add turtledemo to IDLE menu #61374

rhettinger opened this issue Feb 9, 2013 · 35 comments
Assignees
Labels
topic-IDLE type-feature A feature request or enhancement

Comments

@rhettinger
Copy link
Contributor

BPO 17172
Nosy @rhettinger, @terryjreedy, @kbkaiser, @ronaldoussoren, @orsenthil, @ned-deily, @ezio-melotti
Dependencies
  • bpo-21597: Allow turtledemo code pane to get wider.
  • bpo-21823: Catch turtle.Terminator exceptions in turtledemo
  • bpo-21824: Make turtledemo 2.7 help show file contents, not file name.
  • Files
  • issue.patch
  • issue.patch
  • turtle_demo.patch
  • turtle_demo_v2.patch
  • turtle_demo_v3.patch
  • 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:

    assignee = 'https://github.com/terryjreedy'
    closed_at = <Date 2014-07-29.02:41:07.634>
    created_at = <Date 2013-02-09.20:32:46.152>
    labels = ['expert-IDLE', 'type-feature']
    title = 'Add turtledemo to IDLE menu'
    updated_at = <Date 2014-07-29.02:41:18.094>
    user = 'https://github.com/rhettinger'

    bugs.python.org fields:

    activity = <Date 2014-07-29.02:41:18.094>
    actor = 'terry.reedy'
    assignee = 'terry.reedy'
    closed = True
    closed_date = <Date 2014-07-29.02:41:07.634>
    closer = 'terry.reedy'
    components = ['IDLE']
    creation = <Date 2013-02-09.20:32:46.152>
    creator = 'rhettinger'
    dependencies = ['21597', '21823', '21824']
    files = ['29048', '35328', '35383', '36081', '36121']
    hgrepos = []
    issue_num = 17172
    keywords = ['patch']
    message_count = 35.0
    messages = ['181757', '181780', '181781', '181954', '181955', '182160', '182208', '192929', '218983', '219026', '219064', '219237', '219243', '219245', '219246', '219252', '219318', '219330', '219538', '220413', '221220', '222815', '223913', '224026', '224091', '224098', '224154', '224161', '224176', '224181', '224206', '224209', '224213', '224214', '224215']
    nosy_count = 11.0
    nosy_names = ['rhettinger', 'terry.reedy', 'kbk', 'ronaldoussoren', 'orsenthil', 'ned.deily', 'ezio.melotti', 'jesstess', 'python-dev', 'Ramchandra Apte', 'Lita.Cho']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue17172'
    versions = ['Python 2.7', 'Python 3.4', 'Python 3.5']

    @rhettinger
    Copy link
    Contributor Author

    The turtledemo is an on-ramp for younger programmers and we should make it easy to launch.

    @rhettinger rhettinger added easy topic-IDLE type-feature A feature request or enhancement labels Feb 9, 2013
    @RamchandraApte
    Copy link
    Mannequin

    RamchandraApte mannequin commented Feb 10, 2013

    Will attach patch. Coincidentally I'm am a younger programmer.

    @RamchandraApte
    Copy link
    Mannequin

    RamchandraApte mannequin commented Feb 10, 2013

    Should be "... I'm a younger..."

    @RamchandraApte
    Copy link
    Mannequin

    RamchandraApte mannequin commented Feb 12, 2013

    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().

    @RamchandraApte
    Copy link
    Mannequin

    RamchandraApte mannequin commented Feb 12, 2013

    Should this be added to Lib/idlelib/NEWS.txt ?

    @ezio-melotti
    Copy link
    Member

    I left a comment in rietveld.

    I hope the File menu is the right place for this.

    I think that's the best place where to put it.

    I had to move the code in Lib/turtledemo.py after "if __name__ ==..." into main().

    Why?

    Should this be added to Lib/idlelib/NEWS.txt ?

    Yes.

    @RamchandraApte
    Copy link
    Mannequin

    RamchandraApte mannequin commented Feb 16, 2013

    Because turtledemo doesn't have a main() function. I moved the code under 'if __name__ == "__main__"' into a main() function.

    @rhettinger
    Copy link
    Contributor Author

    Ezio, would you like to take it from here?

    @LitaCho
    Copy link
    Mannequin

    LitaCho mannequin commented May 23, 2014

    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.

    @terryjreedy
    Copy link
    Member

    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:
    python -c "from turtledemo.__main__ import main; main('idle')"
    One reason, aside from not leaving all the imports around, is that 3 of the demos print to console.

    • When clock is stopped, a traceback is printed; this in mentioned in a tracker issue (and should be fixed).
    • Penrose prints each info about each round; this text could/should be put in a popup text window.
    • Tree prints '1024'; this could be eliminated or put on the status bar under the text window, as another demo does.

    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.)

    @LitaCho
    Copy link
    Mannequin

    LitaCho mannequin commented May 24, 2014

    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.

    @LitaCho
    Copy link
    Mannequin

    LitaCho mannequin commented May 27, 2014

    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.

    @rhettinger
    Copy link
    Contributor Author

    the IDLE window is hanging

    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.

    @LitaCho
    Copy link
    Mannequin

    LitaCho mannequin commented May 27, 2014

    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.

    @LitaCho
    Copy link
    Mannequin

    LitaCho mannequin commented May 28, 2014

    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.

    @terryjreedy
    Copy link
    Member

    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.

    @terryjreedy
    Copy link
    Member

    1. My general interest is in running external programs and in particular, for this issue, python modules intended to be run as main. Some other scripts I would like to be able to easily launch from Idle include List/test.test_idle.py, Tools/Scripts/patchcheck.py, and a possible 'idle_tour.py' (in progress) that would be a live demo/tutorial of some Idle widgets and functions. I would like the list to be user-configurable for other choices.

    2. As with any python script, turtledemo can be run now, without modification, in a separate process, asynchronously, from within idle, by loading the file into an editor window and selecting Run / Run Module or hitting F5. Communication and cleanup issues are already solved. Having output appear in the Shell window obviates the need for a separate console window to receive output, keeps it visible even after the external process quits. One thing that remains impossible is to run two separate modules at the same time.

    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.

    1. This feature should be implemented as an extension rather than being 'baked' into Idle. Run-module itself is an extension (ScriptBinding.py), as are several other functions (see config-extensions.def). A possible name would be 'external_module.py'

    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.

    1. If Run were on the menu of Shell windows, that would be the appropriate place for 'run external module' as opposed 'run editor module'. This would be especially true with multiple entries.

    @terryjreedy
    Copy link
    Member

    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.

    @LitaCho
    Copy link
    Mannequin

    LitaCho mannequin commented Jun 2, 2014

    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!

    @LitaCho
    Copy link
    Mannequin

    LitaCho mannequin commented Jun 13, 2014

    Hi Terry, can we close this issue? Thanks!

    @terryjreedy
    Copy link
    Member

    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:

    • check before installing the menu entry and dont add it if not present.
    • always make menu entry and check when clicked.

    Is turtledemo present on *nix/mac often enough to make a 2.7 addition worthwhile even without Windows?

    @LitaCho
    Copy link
    Mannequin

    LitaCho mannequin commented Jul 12, 2014

    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.

    @LitaCho
    Copy link
    Mannequin

    LitaCho mannequin commented Jul 25, 2014

    Here is a new patch where it checks to see if turtledemo exists first before loading it onto the bindings.

    @ned-deily
    Copy link
    Member

    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 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.

    @ned-deily ned-deily removed the easy label Jul 26, 2014
    @LitaCho
    Copy link
    Mannequin

    LitaCho mannequin commented Jul 26, 2014

    I wasn't aware of make patchcheck. I will run this script when submitting patches in the future. Thanks, Ned!

    @LitaCho
    Copy link
    Mannequin

    LitaCho mannequin commented Jul 27, 2014

    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.

    @ned-deily
    Copy link
    Member

    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.

    @ronaldoussoren
    Copy link
    Contributor

    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.

    @LitaCho
    Copy link
    Mannequin

    LitaCho mannequin commented Jul 28, 2014

    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.

    @ned-deily
    Copy link
    Member

    Ronald:

    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.

    That's exactly what the proposed patch does. The AppleScript "activate" causes turtledemo, however started, to move to the foreground.

    @terryjreedy
    Copy link
    Member

    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
    cmd = [sys.executable, '-m', 'turtledemo']
    which is more readable and does the same as the current version. But see below. (Lita, don't make a new patch for this or anything else in this message.)

    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?
    cmd = [sys.executable, '-c',
    'from idlelib.macosxSupport import activate as A; A()\n'
    'from turtledemo.__main__ import main; main()']
    Does the activate code have to be called after, say, the tkinter import in turtledemo, or is anytime in the process ok? While something like this is not needed to run turtledemo, which we can edit, it would be to run tkinter (or mac-unaware gui apps) that we cannot edit.

    @ned-deily
    Copy link
    Member

    If the activate code is moved to macosxSupport.activate(), does the
    following work in EditorWindow.open_turtle_demo?
    cmd = [sys.executable, '-c',
    'from idlelib.macosxSupport import activate as A; A()\n'
    'from turtledemo.__main__ import main; main()']
    Does the activate code have to be called after, say, the tkinter import in
    turtledemo, or is anytime in the process ok?

    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.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jul 29, 2014

    New changeset 04dd26ca02f4 by Terry Jan Reedy in branch '3.4':
    Issue bpo-17172: Add the ability to run turtledemo from Idle.
    http://hg.python.org/cpython/rev/04dd26ca02f4

    @terryjreedy
    Copy link
    Member

    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.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jul 29, 2014

    New changeset e344539cda11 by Terry Jan Reedy in branch '3.4':
    Issue bpo-17172: add NEWS
    http://hg.python.org/cpython/rev/e344539cda11

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    topic-IDLE type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants