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 widget.unbind(sequence, funcid) unbind all bindings #75666

Open
j4321 mannequin opened this issue Sep 15, 2017 · 21 comments
Open

Tkinter widget.unbind(sequence, funcid) unbind all bindings #75666

j4321 mannequin opened this issue Sep 15, 2017 · 21 comments
Assignees
Labels
topic-tkinter type-feature A feature request or enhancement

Comments

@j4321
Copy link
Mannequin

j4321 mannequin commented Sep 15, 2017

BPO 31485
Nosy @terryjreedy, @serhiy-storchaka, @j4321, @GiovaLomba
PRs
  • gh-75666: Fix Misc.unbind behaviour when funcid is given #17954
  • 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 = None
    created_at = <Date 2017-09-15.13:57:14.322>
    labels = ['type-feature', 'expert-tkinter', '3.10']
    title = 'Tkinter widget.unbind(sequence, funcid) unbind all bindings'
    updated_at = <Date 2020-05-26.07:51:27.964>
    user = 'https://github.com/j4321'

    bugs.python.org fields:

    activity = <Date 2020-05-26.07:51:27.964>
    actor = 'terry.reedy'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Tkinter']
    creation = <Date 2017-09-15.13:57:14.322>
    creator = 'j-4321-i'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 31485
    keywords = ['patch']
    message_count = 10.0
    messages = ['302256', '302258', '302262', '302293', '302299', '302487', '358227', '359369', '360142', '360145']
    nosy_count = 4.0
    nosy_names = ['terry.reedy', 'serhiy.storchaka', 'j-4321-i', 'glombardo']
    pr_nums = ['17954']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue31485'
    versions = ['Python 3.10']

    Linked PRs

    @j4321
    Copy link
    Mannequin Author

    j4321 mannequin commented Sep 15, 2017

    I am using python 3.6.2 with tk 8.6.7 in Linux and when I call widget.unbind(<Sequence>, funcid), it unbinds all bindings for <Sequence>, while I would expect it to unbind only funcid. Here is an example reproducing the problem:

    import tkinter as tk
    
    root = tk.Tk()
    
    def cmd1(event):
        print('1')
    
    def cmd2(event):
        print('2')
    
    def unbind1():
        l.unbind('<Motion>', b1)
    
    def unbind2():
        l.unbind('<Motion>', b2)
    
    l = tk.Label(root, text='Hover')
    
    b1 = l.bind('<Motion>', cmd1)
    b2 = l.bind('<Motion>', cmd2, True)
    
    l.pack()
    tk.Button(root, text='Unbind 1', command=unbind1).pack()
    tk.Button(root, text='Unbind 2', command=unbind2).pack()
    
    root.mainloop()

    In this example, clicking on one of the buttons unbinds both bindings instead of only one.

    @j4321 j4321 mannequin added type-bug An unexpected behavior, bug, or error topic-tkinter labels Sep 15, 2017
    @j4321
    Copy link
    Mannequin Author

    j4321 mannequin commented Sep 15, 2017

    I have found a workaround to unbind a single binding (inspired by https://mail.python.org/pipermail/tkinter-discuss/2012-May/003151.html):

    def unbind(widget, seq, funcid):
        bindings = {x.split()[1][3:]: x for x in widget.bind(seq).splitlines() if x.strip()}
        try:
            del bindings[funcid]
        except KeyError:
            raise tk.TclError('Binding "%s" not defined.' % funcid)
        widget.bind(seq, '\n'.join(list(bindings.values())))

    @serhiy-storchaka
    Copy link
    Member

    Tk do not provide a way of unbinding a specific command. It supports binding a script with replacing an existing binding, appending a script to an existing binding, and destroying an existing binding. Unbinding a specific command can be implemented in Python with a code similar to your example, but more complex due to taking to account different corner cases.

    Do you want to write a patch and open a pull request Juliette?

    @terryjreedy
    Copy link
    Member

    The unbind docstring, "Unbind for this widget for event SEQUENCE the
    function identified with FUNCID" implies to me the behavior Juliette expected. The NMT reference unpacks this to two sentences. Reading the code
    self.tk.call('bind', self._w, sequence, '')
    if funcid:
    self.deletecommand(funcid)

    the docstring should be something like "Unbind for this widget the event SEQUENCE. If funcid is given, delete the command also" This part of the bind docstring, "Bind will return an identifier to allow deletion of the bound function with unbind without memory leak." implies that the unbind behavior is deliberate.

    Serhiy, are you proposing to add a new method that does what some people thought unbind would do?

    @terryjreedy terryjreedy added 3.7 (EOL) end of life type-feature A feature request or enhancement and removed type-bug An unexpected behavior, bug, or error labels Sep 15, 2017
    @serhiy-storchaka
    Copy link
    Member

    I don't think a new method is needed. We can make unbind() destroying all bindings if FUNCID is not given (note that registered commands are leaked in this case), and only the specified command if it is specified. We can add a boolean parameter (similar to the one in bind()) for restoring the old behavior if this is needed.

    @j4321
    Copy link
    Mannequin Author

    j4321 mannequin commented Sep 18, 2017

    I don't mind writing a patch and opening a pull request, however I don't know which corner cases I need to take into account.

    @GiovaLomba
    Copy link
    Mannequin

    GiovaLomba mannequin commented Dec 10, 2019

    @GiovaLomba
    Copy link
    Mannequin

    GiovaLomba mannequin commented Jan 5, 2020

    I propose the below fix:

        def unbind(self, sequence, funcid=None):
            """Unbind for this widget for event SEQUENCE  the
            function identified with FUNCID."""
            bound = ''
            if funcid:
                self.deletecommand(funcid)
                funcs = self.tk.call('bind', self._w, sequence, None).split('\n')
                bound = '\n'.join([f for f in funcs if not f.startswith('if {{"[{0}'.format(funcid))])
            self.tk.call('bind', self._w, sequence, bound)
    

    @terryjreedy
    Copy link
    Member

    Serhiy, if I understand your last message (msg302299), the docstring should read:

       Unbind for this widget the event SEQUENCE.
       If FUNCID is given, only unbind the function identified with FUNCID
       and also delete that command.
    

    The main doc change is to specify what happens when funcid is not given while the code change in the patch is to no longer unbind everything when funcid *is* given.

    @terryjreedy
    Copy link
    Member

    Serhiy, given that the code did not match the docstring and that we are changing the code to match the clarified docstring, should we consider this a bugfix, and, should we backport it?

    @terryjreedy terryjreedy added 3.10 only security fixes and removed 3.7 (EOL) end of life labels May 26, 2020
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @matthiasschweikart
    Copy link

    Will there be a change to the code? I would like to use the "single function unbind".

    @terryjreedy
    Copy link
    Member

    The PR writer removed his cpython fork, and the PR was missing tests. So it must be rewritten and resubmitted by someone else. Since Serhiy is busy with other stuff, I may try to complete it some time, but I am not familiar with tkinter tests. There does seem to be enough material in the original report above to create one.

    @githubuser11123
    Copy link

    How is it going?

    @serhiy-storchaka serhiy-storchaka self-assigned this Oct 21, 2023
    @terryjreedy
    Copy link
    Member

    I am now working on other thing too, so anyone can try another patch.

    @serhiy-storchaka
    Copy link
    Member

    I am working on this. Other issues are that bind, unbind and similar methods can leave registered Tcl commands which no longer needed. It can look like "leak" if you rebind often enough.

    serhiy-storchaka pushed a commit to serhiy-storchaka/cpython that referenced this issue Oct 23, 2023
    serhiy-storchaka pushed a commit to serhiy-storchaka/cpython that referenced this issue Oct 23, 2023
    serhiy-storchaka pushed a commit to serhiy-storchaka/cpython that referenced this issue Oct 23, 2023
    miss-islington pushed a commit to miss-islington/cpython that referenced this issue Oct 24, 2023
    (cherry picked from commit 9bb202a)
    
    Co-authored-by: Serhiy Storchaka <[email protected]>
    miss-islington pushed a commit to miss-islington/cpython that referenced this issue Oct 24, 2023
    (cherry picked from commit 9bb202a)
    
    Co-authored-by: Serhiy Storchaka <[email protected]>
    serhiy-storchaka added a commit that referenced this issue Oct 24, 2023
    (cherry picked from commit 9bb202a)
    
    Co-authored-by: Serhiy Storchaka <[email protected]>
    serhiy-storchaka added a commit that referenced this issue Oct 24, 2023
    (cherry picked from commit 9bb202a)
    
    Co-authored-by: Serhiy Storchaka <[email protected]>
    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Oct 25, 2023

    I have split all the changes into three parts.

    • Add tests for binding related commands. Already merged.
    • Make unbind(sequence, funcid) only removing funcid from the current binding for sequence, keeping other commands, instead of destroying the whole binding for sequence. It is based on the PR gh-75666: Fix Misc.unbind behaviour when funcid is given #17954 by @GiovaLomba.
    • Make bind(sequence, func) and unbind(sequence), which destroy the current binding for sequence, also deleting the corresponding commands.

    Now, I am not sure what should be the target for the second change. Should it be backported? The current behavior only works as expected if there was only one callable bound to the sequence. If there was more than one, it can cause bad surprises. So it looks like a bug. What do you think, @terryjreedy?

    I am not going to backport the third change. In worst case the current code produces "leaks" of commands, creating more and more Tcl commands and references to Python callables until the widget be destroyed, if you constantly rebind the sequence.

    serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Oct 25, 2023
    …"funcid"
    
    Previously, "widget.unbind(sequence, funcid)" destroyed the current binding
    for "sequence", leaving "sequence" unbound, and deleted the "funcid"
    command.
    
    Now it removes only "funcid" from the binding for "sequence", keeping
    other commands, and deletes the "funcid" command.
    It leaves "sequence" unbound only if "funcid" was the last bound command.
    
    Co-authored-by: GiovanniL <[email protected]>
    @terryjreedy
    Copy link
    Member

    To me, the 2nd fix is a bugfix that should be backported. Beyond the docstring, the Shipman doc says

    w.unbind(sequence, funcid=None)
    This method deletes bindings on w for the event described by sequence. If the second argument is a callback bound to that sequence, that callback is removed and the rest, if any, are left in place. If the second argument is omitted, all bindings are deleted.

    Without checking back 20-30 years, I wonder if that ever was the case.

    @terryjreedy
    Copy link
    Member

    In any case, the bind docstring says "Bind will return an identifier to allow deletion of the bound function with
    unbind without memory leak." To me, this also implies the #2 behavior.

    serhiy-storchaka added a commit that referenced this issue Dec 6, 2023
    …d" (GH-111322)
    
    Previously, "widget.unbind(sequence, funcid)" destroyed the current binding
    for "sequence", leaving "sequence" unbound, and deleted the "funcid"
    command.
    
    Now it removes only "funcid" from the binding for "sequence", keeping
    other commands, and deletes the "funcid" command.
    It leaves "sequence" unbound only if "funcid" was the last bound command.
    
    Co-authored-by: GiovanniL <[email protected]>
    miss-islington pushed a commit to miss-islington/cpython that referenced this issue Dec 6, 2023
    …"funcid" (pythonGH-111322)
    
    Previously, "widget.unbind(sequence, funcid)" destroyed the current binding
    for "sequence", leaving "sequence" unbound, and deleted the "funcid"
    command.
    
    Now it removes only "funcid" from the binding for "sequence", keeping
    other commands, and deletes the "funcid" command.
    It leaves "sequence" unbound only if "funcid" was the last bound command.
    
    (cherry picked from commit cc7e45c)
    
    Co-authored-by: Serhiy Storchaka <[email protected]>
    Co-authored-by: GiovanniL <[email protected]>
    miss-islington pushed a commit to miss-islington/cpython that referenced this issue Dec 6, 2023
    …"funcid" (pythonGH-111322)
    
    Previously, "widget.unbind(sequence, funcid)" destroyed the current binding
    for "sequence", leaving "sequence" unbound, and deleted the "funcid"
    command.
    
    Now it removes only "funcid" from the binding for "sequence", keeping
    other commands, and deletes the "funcid" command.
    It leaves "sequence" unbound only if "funcid" was the last bound command.
    
    (cherry picked from commit cc7e45c)
    
    Co-authored-by: Serhiy Storchaka <[email protected]>
    Co-authored-by: GiovanniL <[email protected]>
    serhiy-storchaka added a commit that referenced this issue Dec 6, 2023
    … "funcid" (GH-111322) (GH-112801)
    
    Previously, "widget.unbind(sequence, funcid)" destroyed the current binding
    for "sequence", leaving "sequence" unbound, and deleted the "funcid"
    command.
    
    Now it removes only "funcid" from the binding for "sequence", keeping
    other commands, and deletes the "funcid" command.
    It leaves "sequence" unbound only if "funcid" was the last bound command.
    
    (cherry picked from commit cc7e45c)
    
    Co-authored-by: Serhiy Storchaka <[email protected]>
    Co-authored-by: GiovanniL <[email protected]>
    serhiy-storchaka added a commit that referenced this issue Dec 6, 2023
    … "funcid" (GH-111322) (GH-112802)
    
    Previously, "widget.unbind(sequence, funcid)" destroyed the current binding
    for "sequence", leaving "sequence" unbound, and deleted the "funcid"
    command.
    
    Now it removes only "funcid" from the binding for "sequence", keeping
    other commands, and deletes the "funcid" command.
    It leaves "sequence" unbound only if "funcid" was the last bound command.
    
    (cherry picked from commit cc7e45c)
    
    Co-authored-by: Serhiy Storchaka <[email protected]>
    Co-authored-by: GiovanniL <[email protected]>
    @antonin-acd
    Copy link

    I have found a workaround to unbind a single binding (inspired by https://mail.python.org/pipermail/tkinter-discuss/2012-May/003151.html):

    def unbind(widget, seq, funcid):
        bindings = {x.split()[1][3:]: x for x in widget.bind(seq).splitlines() if x.strip()}
        try:
            del bindings[funcid]
        except KeyError:
            raise tk.TclError('Binding "%s" not defined.' % funcid)
        widget.bind(seq, '\n'.join(list(bindings.values())))

    After many years, thanks you are a savior

    @hugovk
    Copy link
    Member

    hugovk commented Jan 11, 2024

    Triage: can we close this issue now or is there more to be done?

    @serhiy-storchaka
    Copy link
    Member

    Not yet.

    aisk pushed a commit to aisk/cpython that referenced this issue Feb 11, 2024
    …"funcid" (pythonGH-111322)
    
    Previously, "widget.unbind(sequence, funcid)" destroyed the current binding
    for "sequence", leaving "sequence" unbound, and deleted the "funcid"
    command.
    
    Now it removes only "funcid" from the binding for "sequence", keeping
    other commands, and deletes the "funcid" command.
    It leaves "sequence" unbound only if "funcid" was the last bound command.
    
    Co-authored-by: GiovanniL <[email protected]>
    Glyphack pushed a commit to Glyphack/cpython that referenced this issue Sep 2, 2024
    Glyphack pushed a commit to Glyphack/cpython that referenced this issue Sep 2, 2024
    …"funcid" (pythonGH-111322)
    
    Previously, "widget.unbind(sequence, funcid)" destroyed the current binding
    for "sequence", leaving "sequence" unbound, and deleted the "funcid"
    command.
    
    Now it removes only "funcid" from the binding for "sequence", keeping
    other commands, and deletes the "funcid" command.
    It leaves "sequence" unbound only if "funcid" was the last bound command.
    
    Co-authored-by: GiovanniL <[email protected]>
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    topic-tkinter type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants