classification
Title: Tkinter.Menu.delete doesn't delete command of entry
Type: Stage:
Components: Tkinter Versions: Python 2.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: schuppenies Nosy List: benjamin.peterson, gpolo, loewis, schuppenies, svenil
Priority: normal Keywords: needs review

Created on 2005-10-30 21:49 by svenil, last changed 2008-08-22 08:29 by schuppenies. This issue is now closed.

Files
File name Uploaded Description Edit
test_menuleak.py svenil, 2005-10-30 21:49 Tests and a possible fix.
tkinter_menuleak.patch schuppenies, 2008-08-01 12:19 patch against r65367
tkinter_menu-error.patch schuppenies, 2008-08-18 16:29
Messages (14)
msg26765 - (view) Author: Sverker Nilsson (svenil) Date: 2005-10-30 21:49
Tkinter.Menu.delete does not delete the commands
defined for the entries it deletes. Those objects
will be retained until the menu itself is deleted.

For example, after code like this:

    button = Menubutton(root, text='Window')
    menu = Menu(button)
    button['menu'] = menu
    def command():
	print 'command button pressed'
    menu.add_command(command=command)
    menu.delete(END)
    del command


the command function will still be referenced and
kept in memory - until the menu object itself is
destroyed.

This may not always be a serious problem, but in
my case the menu was a 'Window' menu and the
command was a method on a window top level widget,
so retaining a pointer to it after deleting the
menu entry kept a reference to that entire window,
with any associated data.

I have figured out a possible fix that is in the
attached file test_menuleak.py that contains some
test functions.

I also changed the comment - for as far as I can
see, the second optional index is actually
INCLUDED in the range of entries deleted.

Version info

Python 2.3.3 (#2, Mar 11 2004, 19:45:43) 
[GCC 2.95.2 20000220 (Debian GNU/Linux)] on linux2

I think it applies to all versions: I tested with
the latest 2.4.2 as well.


Sverker Nilsson
msg70550 - (view) Author: Robert Schuppenies (schuppenies) * (Python committer) Date: 2008-08-01 12:19
The problem does still exist (Python 2.6b2).

I attached a patch for Tkinter.py which addresses this problem. It is
the same as in the first proposed fix, but adds an additional check
whether the menu item has a 'command' property.
I also removed the "INDEX2 (is included)" comment, as it is not the
desired behavior (see http://www.tcl.tk/man/tcl8.5/TkCmd/text.htm#M98).
I cannot confirm the behavior, but it should be a separate issue
nevertheless.
msg70831 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2008-08-07 14:36
The patch is fine, please apply (also to the 2.5 and 3.0 branches).
Don't forget a Misc/NEWS entry.
msg70971 - (view) Author: Robert Schuppenies (schuppenies) * (Python committer) Date: 2008-08-10 11:32
Fixed in r65622. Backported to the release25-maint and merged into the
py3k branch.
msg71243 - (view) Author: Guilherme Polo (gpolo) * (Python committer) Date: 2008-08-16 22:04
Uhm, this patch can cause trouble if not adapted a bit. The index method
may return None for either index1 or index2, which will cause a
TypeError in that for loop.

If code is needed to confirm this, try the following:
menu = Tkinter.Menu(tearoff=False)
menu.delete(0)
msg71348 - (view) Author: Robert Schuppenies (schuppenies) * (Python committer) Date: 2008-08-18 16:29
You are right. How about the attached patch, do you see any problems
here? Tkinter seems to ignore any delete calls when either of the
indices is None, so the deletion of commands may be ignored as well. But
I couldn't find a description making this API behavior official.

And does anybody know about a test suite for the Tkinter library where
issues like these are tested?
msg71371 - (view) Author: Guilherme Polo (gpolo) * (Python committer) Date: 2008-08-18 20:08
You could return if in that new if statement.

As you noted, the None argument is ignored there, that is because
_tkinter checks for a None parameter, and if it happens to be a None, it
then stops processing new arguments, so this is not directly related to
the delete command of the Menu widget.

Regarding a test suite.. it would be very nice to get one for Tkinter.
Some weeks ago I did some googling but found no attempts for my surprise.
msg71372 - (view) Author: Guilherme Polo (gpolo) * (Python committer) Date: 2008-08-18 20:09
change this:

"You could return if in that new if statement."

to:

"You could return in that new if statement.", please.
msg71446 - (view) Author: Robert Schuppenies (schuppenies) * (Python committer) Date: 2008-08-19 16:58
I was thinking about returning in that new if statement, too, but
decided not too. The reason is that I didn't want to anticipate _tkinter
implementations, which may change (although not likely, still possible).

Also, with the third beta tomorrow, I am not sure if somebody will find
the time to approve the patch in time.
msg71447 - (view) Author: Guilherme Polo (gpolo) * (Python committer) Date: 2008-08-19 17:05
If this needs approval of someone else, and such thing doesn't happen in
time then the earlier patch should be reverted.
msg71604 - (view) Author: Guilherme Polo (gpolo) * (Python committer) Date: 2008-08-21 03:33
Well, beta3 was released and the problem remained there.

Robert, I believe MvL assigned this to you for a reason.. I'm a bit
stressed but what I'm trying to say is that you could have decided about
this issue yourself and you decided to keep the broken patch there.
msg71610 - (view) Author: Robert Schuppenies (schuppenies) * (Python committer) Date: 2008-08-21 07:29
I am sry that you see it that way, I do not. I was given commit access
solely for gsoc purposes and committing changes before a late release
without review from a committer IMHO violates strict policies. I tried
to get somebody to review the patch twice on the #python-dev channel,
but was ignored. Maybe I should have made more fuss about it. Also,
since it is still a beta, it is not the end of the world. I don't like
it either but take the blame now.
msg71674 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-08-21 20:05
I think the new patch looks fine and should be applied.
msg71729 - (view) Author: Robert Schuppenies (schuppenies) * (Python committer) Date: 2008-08-22 08:29
Fixed in r65971. Backported to the release25-maint and merged into the
py3k branch.
History
Date User Action Args
2008-08-22 08:29:54schuppeniessetstatus: open -> closed
resolution: fixed
messages: + msg71729
2008-08-21 20:05:04benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg71674
2008-08-21 19:05:40gpolosetkeywords: + needs review, - patch
2008-08-21 07:29:56schuppeniessetmessages: + msg71610
2008-08-21 03:33:00gpolosetmessages: + msg71604
2008-08-19 17:05:43gpolosetmessages: + msg71447
2008-08-19 16:58:56schuppeniessetmessages: + msg71446
2008-08-18 20:09:16gpolosetmessages: + msg71372
2008-08-18 20:08:11gpolosetmessages: + msg71371
2008-08-18 16:29:59schuppeniessetfiles: + tkinter_menu-error.patch
messages: + msg71348
2008-08-16 22:04:04gpolosetstatus: closed -> open
nosy: + gpolo
resolution: fixed -> (no value)
messages: + msg71243
2008-08-10 11:32:06schuppeniessetstatus: open -> closed
resolution: accepted -> fixed
messages: + msg70971
2008-08-07 14:36:08loewissetassignee: loewis -> schuppenies
resolution: accepted
messages: + msg70831
2008-08-01 12:19:33schuppeniessetfiles: + tkinter_menuleak.patch
nosy: + schuppenies
messages: + msg70550
keywords: + patch
2005-10-30 21:49:23svenilcreate