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

Tkinter.Menu.delete doesn't delete command of entry #42536

Closed
svenil mannequin opened this issue Oct 30, 2005 · 14 comments
Closed

Tkinter.Menu.delete doesn't delete command of entry #42536

svenil mannequin opened this issue Oct 30, 2005 · 14 comments

Comments

@svenil
Copy link
Mannequin

svenil mannequin commented Oct 30, 2005

BPO 1342811
Nosy @loewis, @benjaminp
Files
  • test_menuleak.py: Tests and a possible fix.
  • tkinter_menuleak.patch: patch against r65367
  • tkinter_menu-error.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 = None
    closed_at = <Date 2008-08-22.08:29:54.004>
    created_at = <Date 2005-10-30.21:49:23.000>
    labels = ['expert-tkinter']
    title = "Tkinter.Menu.delete doesn't delete command of entry"
    updated_at = <Date 2008-08-22.08:29:53.995>
    user = 'https://bugs.python.org/svenil'

    bugs.python.org fields:

    activity = <Date 2008-08-22.08:29:53.995>
    actor = 'schuppenies'
    assignee = 'schuppenies'
    closed = True
    closed_date = <Date 2008-08-22.08:29:54.004>
    closer = 'schuppenies'
    components = ['Tkinter']
    creation = <Date 2005-10-30.21:49:23.000>
    creator = 'svenil'
    dependencies = []
    files = ['1827', '11025', '11146']
    hgrepos = []
    issue_num = 1342811
    keywords = ['needs review']
    message_count = 14.0
    messages = ['26765', '70550', '70831', '70971', '71243', '71348', '71371', '71372', '71446', '71447', '71604', '71610', '71674', '71729']
    nosy_count = 5.0
    nosy_names = ['loewis', 'svenil', 'benjamin.peterson', 'gpolo', 'schuppenies']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = None
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue1342811'
    versions = ['Python 2.3']

    @svenil
    Copy link
    Mannequin Author

    svenil mannequin commented Oct 30, 2005

    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

    @svenil svenil mannequin assigned loewis Oct 30, 2005
    @svenil svenil mannequin added the topic-tkinter label Oct 30, 2005
    @svenil svenil mannequin assigned loewis Oct 30, 2005
    @svenil svenil mannequin added the topic-tkinter label Oct 30, 2005
    @schuppenies
    Copy link
    Mannequin

    schuppenies mannequin commented Aug 1, 2008

    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.

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Aug 7, 2008

    The patch is fine, please apply (also to the 2.5 and 3.0 branches).
    Don't forget a Misc/NEWS entry.

    @loewis loewis mannequin assigned schuppenies and unassigned loewis Aug 7, 2008
    @schuppenies
    Copy link
    Mannequin

    schuppenies mannequin commented Aug 10, 2008

    Fixed in r65622. Backported to the release25-maint and merged into the
    py3k branch.

    @schuppenies schuppenies mannequin closed this as completed Aug 10, 2008
    @schuppenies schuppenies mannequin closed this as completed Aug 10, 2008
    @gpolo
    Copy link
    Mannequin

    gpolo mannequin commented Aug 16, 2008

    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)

    @gpolo gpolo mannequin reopened this Aug 16, 2008
    @gpolo gpolo mannequin reopened this Aug 16, 2008
    @schuppenies
    Copy link
    Mannequin

    schuppenies mannequin commented Aug 18, 2008

    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?

    @gpolo
    Copy link
    Mannequin

    gpolo mannequin commented Aug 18, 2008

    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.

    @gpolo
    Copy link
    Mannequin

    gpolo mannequin commented Aug 18, 2008

    change this:

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

    to:

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

    @schuppenies
    Copy link
    Mannequin

    schuppenies mannequin commented Aug 19, 2008

    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.

    @gpolo
    Copy link
    Mannequin

    gpolo mannequin commented Aug 19, 2008

    If this needs approval of someone else, and such thing doesn't happen in
    time then the earlier patch should be reverted.

    @gpolo
    Copy link
    Mannequin

    gpolo mannequin commented Aug 21, 2008

    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.

    @schuppenies
    Copy link
    Mannequin

    schuppenies mannequin commented Aug 21, 2008

    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.

    @benjaminp
    Copy link
    Contributor

    I think the new patch looks fine and should be applied.

    @schuppenies
    Copy link
    Mannequin

    schuppenies mannequin commented Aug 22, 2008

    Fixed in r65971. Backported to the release25-maint and merged into the
    py3k branch.

    @schuppenies schuppenies mannequin closed this as completed Aug 22, 2008
    @schuppenies schuppenies mannequin closed this as completed Aug 22, 2008
    @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
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant