Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upbpo-31485: Fix Misc.unbind behaviour when funcid is given #17954
Conversation
This comment has been minimized.
This comment has been minimized.
the-knights-who-say-ni
commented
Jan 11, 2020
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA). Recognized GitHub usernameWe couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames: This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. You can check yourself to see if the CLA has been received. Thanks again for the contribution, we look forward to reviewing it! |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
About the ubuntu test failure: it happens at |
I think a new test is needed to verify that the revision does what we want and expect it to. |
self.tk.call('bind', self._w, sequence, '') | ||
"""Unbind for this widget the event SEQUENCE. if | ||
FUNCID is given, delete the command also.""" | ||
bound = '' | ||
if funcid: | ||
self.deletecommand(funcid) |
This comment has been minimized.
This comment has been minimized.
terryjreedy
Jan 14, 2020
•
Member
The test failure is here, _tkinter.TclError: can't delete Tcl command
. In the absence of more specific information, I interpret this as tk, on Linux, refusing to delete a command that is in use. Try moving the deletion back to after the rebinding, as it originally was. It is OK to have two if funcid
clauses.
This comment has been minimized.
This comment has been minimized.
GiovaLomba
Jan 14, 2020
Author
Should this work? I just give it a try with ubuntu tests and it worked.
def unbind(self, sequence, funcid=None):
"""Unbind for this widget the event SEQUENCE. if
FUNCID is given, delete the command also."""
funcs = self.tk.call('bind', self._w, sequence, '')
if funcid:
self.deletecommand(funcid)
keep = '\n'.join(f for f in funcs.split('\n') if not f.startswith(f'if {{"[{funcid}'))
self.tk.call('bind', self._w, sequence, keep)
Where should the test be written? Would the test just check that after unbind
with given funcid
, other bound function are there and the given one is gone?
self.tk.call('bind', self._w, sequence, '') | ||
"""Unbind for this widget the event SEQUENCE. if | ||
FUNCID is given, delete the command also.""" | ||
bound = '' |
This comment has been minimized.
This comment has been minimized.
terryjreedy
Jan 14, 2020
Member
keep
or rebind
strike me as better names, but follow Serhiy if he disagrees.
This comment has been minimized.
This comment has been minimized.
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(f'if {{"[{funcid}')]) |
This comment has been minimized.
This comment has been minimized.
terryjreedy
Jan 14, 2020
Member
join
does not need the intermediate list and the second iteration. Remove '[' and ']' to leave a generator expression (comprehension).
I don't understand f.startswith(f'if {{"[{funcid}')
, ie, why the func corresponding to funcid should necessarily be a string starting with something like if {"[<funcid>
. Does this have something to do with how tcl wraps python functions? If so, should we assume that the bound function is such?
This comment has been minimized.
This comment has been minimized.
GiovaLomba
Jan 14, 2020
Author
You'right for the generator.
Neither do I. After having done several tests to check how callbacks are bound, it came out that they always start with that pattern, so I assumed this is "how tcl wraps python functions".
@@ -0,0 +1 @@ | |||
On the tkinter library, the widget.unbind(<Sequence>, funcid) call unbinds all bindings for <Sequence>, while documentation states that when funcid is given all other bindings remains untuched. Apparently this behaviour is not offered by tkinter itself and, in case of non-empty funcid argument value be given, should be obtained by, at first, unbinding all bindings and then bind again all the ones not having the given funcid. This is basically what the proposed patch does. It takes account of the modifications done into the unbind method docstring, as described by other PSF members on the issue tracker. |
This comment has been minimized.
This comment has been minimized.
terryjreedy
Jan 14, 2020
Member
This is one line of about 500 chars. I think we usually comment with line breaks, but I don't know if it makes much difference. In any case, this seems like more explanation than needed by users, who are the audience for news items.
The technical details can be appropriate for the commit message. You can edit the initial message on the PR to include a suggested commit message (about the bpo link).
This comment has been minimized.
This comment has been minimized.
bedevere-bot
commented
Jan 14, 2020
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
GiovaLomba commentedJan 11, 2020
•
edited by bedevere-bot
https://bugs.python.org/issue31485