classification
Title: turtle.py - bug in Screen.__init__()
Type: behavior Stage:
Components: Versions: Python 2.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: benjamin.peterson Nosy List: benjamin.peterson, georg.brandl, gregorlingl, loewis
Priority: normal Keywords: patch

Created on 2008-09-24 13:42 by gregorlingl, last changed 2008-09-29 22:20 by loewis. 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2008-09-29 22:20
Committed (with the two proposed modifications) as r66686 and r66687.
History
Date User Action Args
2008-09-29 22:20:12loewissetstatus: open -> closed
resolution: accepted -> fixed
messages: + msg74051
2008-09-29 21:22:15loewissetmessages: + msg74048
2008-09-29 06:35:29gregorlinglsetmessages: + msg74011
2008-09-29 06:07:30gregorlinglsetmessages: + msg74010
2008-09-28 22:07:45gregorlinglsetmessages: + msg73994
2008-09-28 17:54:00loewissetfiles: - singleton.diff
2008-09-28 17:53:52loewissetfiles: + singleton.diff
2008-09-28 13:39:37loewissetfiles: + singleton.diff
messages: + msg73972
2008-09-28 09:09:04loewissetmessages: + msg73961
2008-09-25 23:34:47gregorlinglsetmessages: + msg73823
2008-09-24 19:07:31loewissetmessages: + msg73750
2008-09-24 18:59:11loewissetmessages: + msg73749
2008-09-24 16:24:29georg.brandlsetassignee: benjamin.peterson
resolution: accepted
messages: + msg73725
nosy: + georg.brandl
2008-09-24 13:45:50gregorlinglsetfiles: + turtleDemo.diff
messages: + msg73713
2008-09-24 13:42:57gregorlinglcreate