Issue3956
This issue tracker has been migrated to GitHub,
and is currently read-only.
For more information,
see the GitHub FAQs in the Python's Developer Guide.
Created on 2008-09-24 13:42 by gregorlingl, last changed 2022-04-11 14:56 by admin. This issue is now closed.
Files | ||||
---|---|---|---|---|
File name | Uploaded | Description | Edit | |
turtle.diff | gregorlingl, 2008-09-24 13:42 | |||
turtleDemo.diff | gregorlingl, 2008-09-24 13:45 | patch for turtleDemo.py | ||
singleton.diff | loewis, 2008-09-28 17:53 |
Messages (13) | |||
---|---|---|---|
msg73712 - (view) | Author: Gregor Lingl (gregorlingl) | Date: 2008-09-24 13:42 | |
There is a bug in Screen.__init__() (The Screen class uses the Borg idiom to simulate a Singleton). This bug is demonstrated best interactively from IDLE (using the -n switch) like this: >>> from turtle import Screen, Turtle >>> t = Turtle() >>> t.fd(100) # idea: let's have a yellow background, so we need # *the* screen >>> s = Screen() # the drawing vanishes >>> s.turtles() [] This is undesired behaviour. Instead the Screen() call should leave the drawings an the turtles untouched and return the already existing Screen. So the call of turtles() would result in something like: >>> s.turtles() [<turtle.Turtle object at 0x01490330>] This is accomplished by the patch described in the attached file turtle.diff Of course sequences of commands like those shown in the interactive session above do not occur in well designed scripts, but they may well occur during sessions of students in interactive classroom use. Two more important notes: (1) This patch is already done in turtle.py for Python 3.0. So in 2.6 it would ensure that Turtles and the Screen show identical behaviour in both versions. (2) This patch makes necessary one other patch in turtleDemo.py - in the Demo directory - which is shown in the attached turtleDemo.diff turtleDemo.py is not a normal turtle application but a GUI - utility designed to run a series of 'normal' turtle - apps conforming to some simple rules (These apps in most cases use a Screen()). So when switching from one to another demo script within turtleDemo one wants to reinitialize the Canvas - what is just the contrary of what one wants normally as explained above. This is accomplished by this patch of turtleDemo.py. (This patch is also already done for Python 3.0) |
|||
msg73713 - (view) | Author: Gregor Lingl (gregorlingl) | Date: 2008-09-24 13:45 | |
Do the patch of turtleDemo.py if and only if the patch of turtle.py is accepted. |
|||
msg73725 - (view) | Author: Georg Brandl (georg.brandl) * | Date: 2008-09-24 16:24 | |
Looks ok, like the 3.0 code. Benjamin, please review and commit. |
|||
msg73749 - (view) | Author: Martin v. Löwis (loewis) * | Date: 2008-09-24 18:59 | |
I'm opposed to this patch. As a design principle, any class should always call its base class' __init__, unless that is known to be empty. With this patch, the base __init__ call becomes conditional, which hints at a design bug of the entire class hierarchy. I don't quite understand *what* the problem is. If Screen is intended to be a singleton, then the right fix would be to make Screen a function, which maintains a singleton reference, and Screen be renamed to _Screen. The fact that the same design bug already exists in 3.0 doesn't mean it should be backported to 2.6. |
|||
msg73750 - (view) | Author: Martin v. Löwis (loewis) * | Date: 2008-09-24 19:07 | |
As a follow-up, I also don't understand the two if blocks in Screen.__init__: if there is meant to be a single Screen instance anyway, why have _root, _canvas, and _title as class variables, whereas everything else is in (shared) instance variables? How could _root be initialized, and _canvas not (or vice versa)? And why is Turtle._screen being set to the last Screen instance being created (even under this patch), whereas all other initialization is keyed to the creation of the first Screen instance? This code could surely use some comments. |
|||
msg73823 - (view) | Author: Gregor Lingl (gregorlingl) | Date: 2008-09-25 23:34 | |
First of all I'd like to point you at a posting which I posted to Python-dev on August 18th, where I addressed this issue in full detail. http://mail.python.org/pipermail/python-dev/2008-August/081846.html I hoped to provoke a clarifying discussion but I have to complain, that I didn't succeed with this probably because many people were on vacation at this time. The only substantial reply I got was by Vern Ceder, a turtle graphics veteran who contributed a lot to the old turtle module. He wrote: Gregor, I don't feel authoritative on the correctness/appropriateness of the implementation, but I do agree completely that behavior b, or what you have in the 3.0 version, is vastly preferable. Cheers, Vern The design decision to use a singleton is of interest only for interactive use. (A well designed Script uses at most one Screen() call at the beginning. So that is no issue.) I'll demonstrate here in a short interactive session (which you can reproduce using IDLE with the -n switch as usual), why the solution Martin proposes doesn't meet the requirements I tried to accomplish with my code. This session 'simulates' a student curiously playing around and experimenting with *the* Screen(): >>> from turtle import Screen, Turtle >>> class YellowScreen(Screen): def __init__(self): Screen.__init__(self) self.bgcolor("yellow") >>> s = YellowScreen() >>> t = Turtle() >>> for i in range(10): t.fd(50) t.lt(36) >>> s1 = Screen() >>> s1.bgcolor("pink") >>> s = YellowScreen() >>> s1.bgcolor() 'yellow' >>> s1.turtles() [<turtle.Turtle object at 0x01490450>] >>> class TextScreen(Screen): def __init__(self): Screen.__init__(self) self. writer = Turtle(visible=False) self.writer.pu() def writeAt(self, x, y, txt): self.writer.goto(x,y) self.writer.write(txt) >>> s = TextScreen() >>> s.writeAt(0, -200, "Read me ...") >>> s.turtles() [<turtle.Turtle object at 0x01490450>, <turtle.Turtle object at 0x014902B0>] >>> s.writer <turtle.Turtle object at 0x014902B0> >>> class RedScreen(Screen): def __init__(self): Screen .__init__(self) self.bgcolor(1, 0.5, 0.5) >>> u = RedScreen() >>> u.writeAt(0,200, "Read me too....") # should fail! Traceback (most recent call last): File "<pyshell#21>", line 1, in <module> u.writeAt(0,200, "Read me too....") AttributeError: 'RedScreen' object has no attribute 'writeAt' >>> u = TextScreen() >>> u.writeAt(0,200, "Read me too....") >>> u.turtles() # at this point for the first time occurs an # unfavorable consequence of what Martin called a # 'design bug', namely a turtle in the turtles() list # which is not used any more. [<turtle.Turtle object at 0x01490450>, <turtle.Turtle object at 0x014902B0>, <turtle.Turtle object at 0x014C2D10>] >>> u.writer <turtle.Turtle object at 0x014C2D10> >>> s.writer <turtle.Turtle object at 0x014C2D10> # We want a fresh screen ... >>> s.clear() >>> s.turtles() [] So there occurres this and possibly some more somewhat weird elements, presumably a result of using the Singleton pattern which is especially controversial in combination with inheritance. Nevertheless I decisively propose to accept this behaviour in order to be able do things like the ones demonstrated above. Morover I also consider it to be a benefit to use spezialized and/or reusable screens derived from the Screen() class in scripts, what would not be possible with a Screen()-function returning *the* single _Screen() object. (All this meant in the context of teaching and learning OOP). Now for the additional questions of Martin: Yes, the second if statement is superfluous. It looks like to be a remain from Pen.__init__ from the old turtle module, where - in contrast - it was not superfluous. My fault. It could be removed, but it doesn't do harm. (The overwhelming part of the module is *re*written but it still contains (in this case regrettably) quite some traces of the old module.) And also the assignement of self to Turtle._screen in the last line could be put into the if-statement-body. But somehow I seem to have felt that a conditionally empty body of __init__() looks dangerous - or at least not nice. More seriously taken, the Borg idiom ensures the creation of new instances (with different id), sharing state and behaviour and I somehow felt it to be better to have the last created instance in Turtle._screen (whereas in fact this is of no importance). My preliminary conclusion: as I'm not a professional programmer but a teacher I can't judge the importance of observing the design principle mentioned by Martin. (I know the principle, I normally observe it and consider it's non observation in the Screen class to be well founded) But if you think, that there is no way around and it has to be observed strictly, one had to leave everything as is and find a better solution (with the desired behaviour) for 2.6.1. Otherwise, having done a lot of work with the Screen class and having not observed any unexpected behaviour (let alone crashes) when using it I'd pragmatically still much prefer the solution which is implemented in 3.0 Even then the search for a better solution may be indicated. Anyway I'd like to express my hope that this controversial implementation of the last element in the class hierarchy for a very special purpose is *not* a hint at a design bug of the entire class hierarchy. Regards, Gregor |
|||
msg73961 - (view) | Author: Martin v. Löwis (loewis) * | Date: 2008-09-28 09:09 | |
> I'll demonstrate here in a short interactive session (which you can > reproduce using IDLE with the -n switch as usual), why the solution > Martin proposes doesn't meet the requirements I tried to accomplish with > my code. This session 'simulates' a student curiously playing around and > experimenting with *the* Screen(): > >>>> from turtle import Screen, Turtle >>>> class YellowScreen(Screen): > def __init__(self): > Screen.__init__(self) > self.bgcolor("yellow") Hmm. Why is it important to be able to inherit from Screen? I don't think inheritance from a singleton is a meaningful concept, and I believe that students are misguided when they are told that this is the way to produce yellow screen. Instead, they should write def YellowScreen(): s = Screen() s.bgcolor("yellow") return s This much better captures the notion of a singleton object: you can not *create* them, but you have to *modify* them. >>>> s1 = Screen() >>>> s1.bgcolor("pink") This is really confusing. It does *not* create a new screen, but modifies the existing one. How can you tell your students the meaning of object creation in the light of this strange behavior? So I urge you to reconsider that inheritance from Screen should be deliberately dropped. It is a flawed, undesirable concept. > Nevertheless I decisively > propose to accept this behaviour in order to be able do things like the > ones demonstrated above. You can do the things you want to just as fine with functions. > Morover I also consider it to be a benefit to > use spezialized and/or reusable screens derived from the Screen() class > in scripts, what would not be possible with a Screen()-function > returning *the* single _Screen() object. (All this meant in the context > of teaching and learning OOP). Not at all. It would be very easy with a Screen function. > But if you think, that there is no way around and it has to be > observed strictly, one had to leave everything as is and find a > better solution (with the desired behaviour) for 2.6.1. I propose that Screen is change to a function for 2.6. This is the more restrictive solution (not allowing inheritance). If inheritance would be allowed now, it could be taken back for backwards compatibility. However, if it is changed to a function now, it can be changed back to a class later. > Anyway I'd like to express my hope that this controversial > implementation of the last element in the class hierarchy for a very > special purpose is *not* a hint at a design bug of the entire class > hierarchy. Perhaps not of the entire hierarchy, but certainly of the Screen class and its potential subclasses. |
|||
msg73972 - (view) | Author: Martin v. Löwis (loewis) * | Date: 2008-09-28 13:39 | |
Here is a patch that takes the alternative route, of making Screen a singleton function, and renaming the class to _Screen. |
|||
msg73994 - (view) | Author: Gregor Lingl (gregorlingl) | Date: 2008-09-28 22:07 | |
I agree to Martin's patch for 2.6, because it seems to provide a more clean solution. And, as he states, there will be time and opportunity to discuss it more thoroughly later to possibly find another approach. As I remarked before I know that singleton+inheritance is a controversial subject. However the gang of four state in their book that one should use the singleton design pattern if one needs exactly one instance af a class and this should be extensible by subclassing ... (And there are reasons why I'd like to have Screen() subclassable.) Anyway, I think there are two more things to do: 1) Add a docstring to the Screen() function 2) Insert (at least) one sentence in the docs, 9.th paragraph of the Introduction. Something like: ... so there can exist only one instance of Screen at a time. *Notice that consequently Screen cannot be sublassed.* It should be used when turtle is used as a standalone tool for doing graphics. ... Perhaps one should also note, that the singleton object is returned by a function. I've applied the proposed patch (file11645) to a working copy and I've tested it in interactive mode as well as with turtleDemo and all the supplied demoscripts and found it to work well. Last, but not least: thanks Martin for your contribution. Regards, Gregor |
|||
msg74010 - (view) | Author: Gregor Lingl (gregorlingl) | Date: 2008-09-29 06:07 | |
Agreeing to this patch, I'd just like to mention, that nevertheless this change is not very pleasing for me. Not feeling that singleton with inheritance is a 'meaningless' but rather a 'very special' concept,the meaning of which could be explained, I've used this concept in the last chapter of my book "Python for Kids" designing sort of a GameScreen class derived from Screen(). Hmmm. So sthis seems to remain sort of a unique selling point of xturtle for now. And in the 4th edition I can certainly adapt this chapter. When 2.6 is out, what do you think is the right point for a further discussion of this? (I think certainly not the bugtracker and particularly not this issue. Regards, Gregor |
|||
msg74011 - (view) | Author: Gregor Lingl (gregorlingl) | Date: 2008-09-29 06:35 | |
... I meant: the right place for a further discussion Sorry, Gregor |
|||
msg74048 - (view) | Author: Martin v. Löwis (loewis) * | Date: 2008-09-29 21:22 | |
> When 2.6 is out, what do you think is the right point for a further > discussion of this? (I think certainly not the bugtracker and > particularly not this issue. Depends on what you want to achieve with the discussion. If you want a public opinion, post to comp.lang.python. Most likely, the public doesn't care - at least not so much to spend actually time trying to understand the issue. |
|||
msg74051 - (view) | Author: Martin v. Löwis (loewis) * | Date: 2008-09-29 22:20 | |
Committed (with the two proposed modifications) as r66686 and r66687. |
History | |||
---|---|---|---|
Date | User | Action | Args |
2022-04-11 14:56:39 | admin | set | github: 48206 |
2008-09-29 22:20:12 | loewis | set | status: open -> closed resolution: accepted -> fixed messages: + msg74051 |
2008-09-29 21:22:15 | loewis | set | messages: + msg74048 |
2008-09-29 06:35:29 | gregorlingl | set | messages: + msg74011 |
2008-09-29 06:07:30 | gregorlingl | set | messages: + msg74010 |
2008-09-28 22:07:45 | gregorlingl | set | messages: + msg73994 |
2008-09-28 17:54:00 | loewis | set | files: - singleton.diff |
2008-09-28 17:53:52 | loewis | set | files: + singleton.diff |
2008-09-28 13:39:37 | loewis | set | files:
+ singleton.diff messages: + msg73972 |
2008-09-28 09:09:04 | loewis | set | messages: + msg73961 |
2008-09-25 23:34:47 | gregorlingl | set | messages: + msg73823 |
2008-09-24 19:07:31 | loewis | set | messages: + msg73750 |
2008-09-24 18:59:11 | loewis | set | messages: + msg73749 |
2008-09-24 16:24:29 | georg.brandl | set | assignee: benjamin.peterson resolution: accepted messages: + msg73725 nosy: + georg.brandl |
2008-09-24 13:45:50 | gregorlingl | set | files:
+ turtleDemo.diff messages: + msg73713 |
2008-09-24 13:42:57 | gregorlingl | create |