python / cpython

The Python programming language
https://www.python.org
Other
63.34k stars 30.32k forks source link

missing update() in _Screen.setup() of Lib/turtle.py #48367

Closed d7aed3d3-a50f-4921-82bc-7f31dc3f0a67 closed 14 years ago

d7aed3d3-a50f-4921-82bc-7f31dc3f0a67 commented 16 years ago
BPO 4117
Nosy @loewis, @birkenfeld, @abalkin
Files
  • _Screen.setup.diff: bugfix
  • setup_bug_demo.py: demonstrates the init - update bug
  • setup_patch.diff
  • setup_bug_demo.py
  • 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: ```python assignee = None closed_at = created_at = labels = ['type-bug', 'library'] title = 'missing update() in _Screen.setup() of Lib/turtle.py' updated_at = user = 'https://bugs.python.org/gregorlingl' ``` bugs.python.org fields: ```python activity = actor = 'belopolsky' assignee = 'none' closed = True closed_date = closer = 'belopolsky' components = ['Library (Lib)'] creation = creator = 'gregorlingl' dependencies = [] files = ['11777', '12079', '12080', '12081'] hgrepos = [] issue_num = 4117 keywords = ['patch'] message_count = 14.0 messages = ['74710', '74721', '76022', '76029', '76046', '76071', '76077', '76081', '76087', '76131', '76132', '76135', '119150', '119335'] nosy_count = 5.0 nosy_names = ['loewis', 'georg.brandl', 'belopolsky', 'gregorlingl', 'gpolo'] pr_nums = [] priority = 'normal' resolution = 'out of date' stage = 'resolved' status = 'closed' superseder = None type = 'behavior' url = 'https://bugs.python.org/issue4117' versions = ['Python 2.6', 'Python 3.0'] ```

    d7aed3d3-a50f-4921-82bc-7f31dc3f0a67 commented 16 years ago

    In Lib/turtle.py

    _Screen.setup() needs a final update() call in order to ensure, that immediately following calls of window_width() or window_height() return correct values.

    Consequently the setup() method call in _Screen.__init() has to take place only after the call of TurtleScreen.__init()

    d7aed3d3-a50f-4921-82bc-7f31dc3f0a67 commented 16 years ago

    This patch applies to Python 2.6 as well Gregor

    d7aed3d3-a50f-4921-82bc-7f31dc3f0a67 commented 15 years ago

    I'd also like to see this patch accepted and done for Python 2.6.1 and (at the same time) python 3.0 before the last rc is released. So perhaps you could you dedicate a few minutes to this one also.

    Thanks for your efforts, Gregor

    1fecb825-97ee-46b6-913c-bb07e2a288a6 commented 15 years ago

    Why are you using update instead of update_idletasks ? I'm not talking exactly about this line being added, but this ends up calling TurtleScreenBase._update which calls self.cv.update(), cv being a canvas. update_idletasks should be used exactly for performing the kind of tasks expected here, display update and window layout calculations, while calling update may also process other events or pending errors.

    d7aed3d3-a50f-4921-82bc-7f31dc3f0a67 commented 15 years ago

    I cannot call the Canvas method _update_idletasks() from within _Screen.setup() becaus this would contradict to the architecture of the module which isolates all direct references to Tkinter to TurtleScreenBase. (The idea behind this is to make the module easily portable, by porting only this class).

    So if one followed your proposition one had to change TurtleScreenBase which would result in a different (additional) patch.

    For now I'd like to stick with the one proposed here - I worked a lot with it, it works and I didn't experience unwanted side effects.

    My question: I suppose to change the call self.cv.update() to self.cv.update_idletasks() in TurtleScreenBase._update wouldn't work properly so one had to add a new method to TurtleScreenBase - something like TurtleScreenBase._update_idletasks. Originally I had the intention to keep this interface as small as possible and use only things that are really needed. Did you experience any problems or undesired behaviour because of using unly cv.update? If so, please report it. Perhaps you could also provide an example.

    Regards, Gregor

    61337411-43fc-4a9c-b8d5-4060aede66d0 commented 15 years ago

    I cannot call the Canvas method _update_idletasks() from within _Screen.setup() becaus this would contradict to the architecture of the module which isolates all direct references to Tkinter to TurtleScreenBase. (The idea behind this is to make the module easily portable, by porting only this class).

    I find that desire misguided; this is (IMO) a case of false abstraction. Is there any kind of proof that this design actually works, i.e. can be ported to a different GUI library (like, say, PythonWin? or AWT, when run in Jython?)

    Unless such proof can be provided (and then integrated into the code), I recommend to give up that objective, and start merging the base class code into the subclasses where reasonable.

    Did you experience any problems or undesired behaviour because of using unly cv.update?

    I agree with with gpolo: it's a general Tk programming principle to defer updates whenever possible. This allows the event handler to return more quickly, making the system more responsive. Each individual update call will only contribute a small amount of time to the response time. It's many of these which eventually make the entire system sluggish.

    d7aed3d3-a50f-4921-82bc-7f31dc3f0a67 commented 15 years ago

    > I find that desire misguided; this is (IMO) a case of false abstraction. > Is there any kind of proof that this design actually > works, i.e. can be ported to a different GUI library (like, say, > PythonWin? or AWT, when run in Jython?)

    Yes there is. I have a (nearly complete) port to Pygame. (The only exception is the ondrag method, which I will incorporate if I have time for it.) It runs all of the turtle graphics example scripts (except colormixer which uses ondrag) without problems.

    > Unless such proof can be provided (and then integrated into the code), > I recommend to give up that objective, and start merging the base class > code into the subclasses where reasonable.

    Sorry, but I definitely shall not follow your recommendation. I have presented the architecture of the turtle module at Europython 2006 in a talk which was visited also by Guido v. Rossum and later in Leipzig at a workshop where you yourself, Martin, was present. On both of these occasions I showed working prototypes of this port (along with another one to VPython) and nobody had any objections nor were there any objections by other useres who have used it up to now against this design. You can find this also in the "Tagungsband" to the Leipzig Python workshop together with some screenshots.

    I'm very confident that this is a good design and I know (form the experience mentioned above) that it works. So instead I'll proceed with porting it to Jython and I for my part would consider it as an advantage to have the same turtle module in both of these flavours of Python.

    Two more remarks on this discussion, a specific one and a general one.

    1. The bug I submitted a patch for is in the method setup() and in the __init__ method of _Screen, both of which usually will be called only once in a program.So in *this case it cannot cause any performance problems. The bug has annoying consequences and I found a simple remedy, which I consider appropriate for a bug-fix-release like 2.6.1. I don't see any reason why to keep a known bug like this one in the code. Acceptance of the patch will certainly not affect any more fundamental amendments to follow (possibly). Moreover up to now I didn't hear of a single complaint about the performance of the turtle module. I think this is (i) because for beginning programmers this is no issue and (ii) there are means (to call tracer() and also to call update() directly) to control performance to a considerable extent. Shortly, my opinion is that there are *good reasons why the implementation is done like this, seen from the educator's point of view.

    2. Is it really fruitful to discuss general design issues along with (comparatively) small problems like this one - in the sense of alternative ways to fix that problem? Implementations of redesigns like the one you recommended wouldn't be accepted anyway for bug fix releases or releases in rc phase. I'm open to fundamental criticism also, but I think that should be led in Python-Dev or possibly comp.lang.python. And working out an amended concept would take some time and also it's implementation and it's testing. Moreover, to find co-workers to work on this would be an advantage.

    Regards, Gregor

    61337411-43fc-4a9c-b8d5-4060aede66d0 commented 15 years ago
    1. Is it really fruitful to discuss general design issues along with (comparatively) small problems like this one - in the sense of alternative ways to fix that problem?

    Most definitely. The module went into Python without any review whatsoever. Nobody (but you) has ever looked at the code in detail. Only now that I look at it I wonder whether the design of the code is appropriate.

    In this specific case, gpolo suggested a reasonable change to the proposed patch. You opposed this change, pointing out that this change contradicts the design behind it. As I think the change gpolo requested is desirable, it *must* be the design that is wrong (as it restricts us from doing what I think should be done).

    So: if you bring up "the design" as a reason for doing things the way they are done, expect the design to be challenged.

    You might argue that with due process, review should have taken place before the code was integrated. You might be right, but then the new turtle module wouldn't have been part of Python 2.6.

    Wrt. the specific design issue: I believe that attempts to provide cross-platform GUI in a simple fashion are doomed to fail. Java AWT is an extraordinary example of it, but many more libraries exist that essentially prove that cross-platform GUI is a bad idea; one may argue that Tk itself is also proof of that (although it is fairly sophisticated, and not at all simple). The fact that I have not objected to it earlier is simply because I must have ignored any claims about cross-platform GUIs.

    d7aed3d3-a50f-4921-82bc-7f31dc3f0a67 commented 15 years ago

    Most definitely. The module went into Python without any review whatsoever. Nobody (but you) has ever looked at the code in detail.

    That's not True! Brad Miller, for example, who also had submitted patches to the pythontracker, coauthor of "Python Programming in Context", has used a predecessor of turtle.py as a main tool (swiss army knife, as he says) in his book. He has contributed a few patches (via private communication, before the module went into the Python trunk), one of them directly concerning the update method. He had also suggested some of the features, which I have added towards the end of the development.

    You might argue that with due process, review should have taken place before the code was integrated. You might be right, but then the new turtle module wouldn't have been part of Python 2.6.

    Rigth, more or less. At least I had expected, that someone reads the doc-strings of the approx. 15 classes in the module. The one of TurtleScreenBase reads like this:

    """Provide the basic graphics functionality. Interface between Tkinter and turtle.py.

    To port turtle.py to some different graphics toolkit a corresponding TurtleScreenBase class has to be implemented. """

    Gregor

    d7aed3d3-a50f-4921-82bc-7f31dc3f0a67 commented 15 years ago

    The attached file setup_bug_demo.py shows the bug of this issue. It's taken out of a working game application and radically abridged to concentrate on this issue.

    Now the patch to fix the bug has to be changed a bit (because of the changes of Martin's Singleton fix). A diff file follows immediately (setup_patch.diff).

    I tried out (in a simple straight forward way and in contradiction with my ideas mentioned in a previous posting)

    (1) to replace the update() call by _Screen._canvas.update_idletasks() and (2) to do a similar replacement in TurtleScreenBase._update()

    Neither of these work. The second change even affects seriously the working of the modules demo().

    I'd like to mention that in the former turtle module upto 2.5 (remember: the compatibility to it was a requirement for the new one) there are also several Canvas.update() calls but no Canvas.update_idletasks() call.

    I never noticed negative consequences of this and as far as I know, neither concerning the old module nor the new one there were ever complaints concerning bad ("sluggish") performance due to this (or any other) fact.

    d7aed3d3-a50f-4921-82bc-7f31dc3f0a67 commented 15 years ago

    Here the patch, updated

    d7aed3d3-a50f-4921-82bc-7f31dc3f0a67 commented 15 years ago

    Sorry, but there is a remain from testing different versions of the turtle module in the demo file's import statement.

    Should read (of course):

    from turtle import Screen, Turtle, mainloop

    A corrected version is attached

    Sorry, again, for the inconvenience Gregor

    abalkin commented 14 years ago

    It looks like this has been fixed in turtle 1.1. See r72318 and bpo-5923.

    abalkin commented 14 years ago

    See also "turtle.py update for 3.1" post on python-dev specifically mentioning this bug as fixed.

    http://mail.python.org/pipermail/python-dev/2009-May/089383.html