gh-75666: Tkinter: "unbind(sequence, funcid)" now only unbinds "funcid" #111322

serhiy-storchaka · 2023-10-25T13:50:43Z

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.

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

…"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 · 2023-10-26T03:13:41Z

Lib/test/test_tkinter/test_misc.py

+        self.assertIn(funcid2, script)
+        self.assertEqual(f.bind(), (event,))


To properly test the unbind code (see comment), I think this should bind 3 scripts and unbind the middle one.

I would also like to test that the other two funcids still work in unbind.

There is a separate test, test_bind_events(), that tests that registered functions are called when the event occurs. Although it does not cover unbinding.

It is not so easy to make it working. For example, currently I cannot make it working in this test, an I do not know why. Maybe it does not work with keyboard events.

Well, I found how to do this. Will see whether it works on Windows.

The expanded test, not visible above, is what I proposed. When I just ran test_misc, whether by itself or with test_tkinter, whether directly or with regrtest, test_bind_events hangs with two blank tk windows on the screen. Hence the CI test timed out. When I close the first of the two windows, tests 'continue' after this error:

ERROR: test_bind_events (__main__.BindTest.test_bind_events) ---------------------------------------------------------------------- Traceback (most recent call last): File "F:\dev\3x\Lib\test\test_tkinter\test_misc.py", line 709, in test_bind_events root.wait_visibility() # needed on Windows ^^^^^^^^^^^^^^^^^^^^^^ File "F:\dev\3x\Lib\tkinter\__init__.py", line 752, in wait_visibility self.tk.call('tkwait', 'visibility', window._w) _tkinter.TclError: window "." was deleted before its visibility changed

This is due to the added "self.frame.wait_visibility()" in BindTest.setUp. With root deleted to let tests continue, the remaining BindTest functions fail with various messages.

DefaultRootTest.test_default_root also fails, with
"AttributeError: module 'tkinter' has no attribute '_default_root'.". A bad inter-testcase dependency? The following DefaultRootTest.test_getboolean and everything after passes.

With the the test_bind_events wait_visibility commented out, there is this error.

FAIL: test_bind_events (__main__.BindTest.test_bind_events) ---------------------------------------------------------------------- Traceback (most recent call last): File "F:\dev\3x\Lib\test\test_tkinter\test_misc.py", line 729, in test_bind_events self.assertEqual(events, [ AssertionError: Lists differ: [] != [('frame', <tkinter.Frame object .!topleve[172 chars]me>)]

With the setUp wait_visibility instead commented out, there is this error:

>FAIL: test_unbind2 (__main__.BindTest.test_unbind2) Traceback (most recent call last): File "F:\dev\3x\Lib\test\test_tkinter\test_misc.py", line 496, in test_unbind2 self.assertEqual(events, ['a', 'b', 'c']) AssertionError: Lists differ: [] != ['a', 'b', 'c']

With the setUp call instead moved into test_unbind2 either at the top or as f.wait_visibility() before the generate events call, there is no error.

terryjreedy · 2023-10-26T03:13:48Z

Lib/tkinter/__init__.py

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


Make clearer that this will not delete the Python function passed to the binding call, but only the binding-specific (tcl) wrapper (as the bind docstring promises).

Suggested change

and also delete that command.

and also delete the binding-specific tcl wrapper function.

What is "the binding-specific tcl wrapper function"? This term is not mentioned in the documentation and will confuse users.

It deletes the Tcl command and removes a reference to Python function. These details are not documented, but experienced users can discover this. I do not know what to write here.

Yes, it does not work with keyboard events.

I used different events for different tests to avoid possible order dependencies or chain reaction of failures after failing in one test. It was easy with keyboard events, it also minimizes risk of generating even by a user during running tests. But it may be not so easy to find a dozen different non-keyboard events (and we can add more tests in future). Is it necessary?

I withdraw the suggestion above. Either insert 'tcl' before 'command' or wait until someone reports being confused.

terryjreedy · 2023-10-26T03:16:07Z

Lib/tkinter/__init__.py

+        if funcid is None:
+            self.tk.call('bind', self._w, sequence, '')
+        else:
+            lines = self.tk.call('bind', self._w, sequence).split('\n')


Without seeing what lines looks like for various scenarios, I cannot verify the code below. Hence would have to trust test. Hence would like testing of what seems like a harder scenario.

Usually it looks like:

if {"[140011831004144test1 %# %b %f %h %k %s %t %w %x %y %A %E %K %N %W %T %X %Y %D]" == "break"} break if {"[140011830917168test2 %# %b %f %h %k %s %t %w %x %y %A %E %K %N %W %T %X %Y %D]" == "break"} break

if it only contains bindings added by bind(). It can also contain arbitrary Tcl commands, including multiline commands, if it is standard binding or bind() was called with a string instead of callable (AFAIK IDLE does such hacking). It does not matter, because it is unlikely that somebody add commands in the above form, using generated funcid.

I checked -- IDLE uses this in fix_x11_paste().

It's useful to manually register a string (to reuse functions multiple times or to directly call a Tk method), but that would entirely reasonably preclude using unbind() with that function. Should there be a test or something to verify it leaves other lines intact?

Right, you wrote that. This comment only serves to explain what I did not review in detail and why I requested the test change.

@TeamSpen210 If you manually register a string to directly call a Tk method, unbind() with funcid will not touch it, and unbind() without second argument will remove it. If you use funcid in other binding, it will stop working when unbind() deletes the command. If you change the line that calls funcid, unbind() may not find it or break the script after deleting the line. In any case, this feature is not documented, and doing so, you are at a risk. Of course, we will try to not break it, but our expectation of possible use cases can be wrong if you do something unusual.

terryjreedy · 2023-10-26T03:17:48Z

Lib/tkinter/__init__.py

+                             if not line.startswith(prefix))
+            if not keep.strip():
+                keep = ''
+            self.tk.call('bind', self._w, sequence, keep)


I have no idea if it is legal to bind a multiple-script script, hence test should require that.

It is supported, but neither documented nor tested. I have not touched this, because it can mean doubling or tripling the number of tests.

Misc/NEWS.d/next/Library/2023-10-25-16-37-13.gh-issue-75666.BpsWut.rst

bedevere-app · 2023-10-26T03:19:02Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

terryjreedy · 2023-10-28T00:49:29Z

My response above explains how I fixed the test failure on Windows.

serhiy-storchaka · 2023-10-28T06:59:52Z

Thank you @terryjreedy. It is not convenient to me to test it on Windows (start a VM, update the workthree, build, and it is with less convenient and slower CLI), so you helped me.

But it still fails on Ubuntu. I did not notice it because tests are passed when run only test_tkinter.test_misc, but fail when run all test_tkinter tests.

terryjreedy · 2023-10-28T08:15:45Z

After 'rearrange the code', test_misc passes for me both alone and with test_tkinter.

serhiy-storchaka · 2023-10-29T07:29:29Z

I have made the requested changes; please review again.

serhiy-storchaka

Oh, I wrote comments a month ago, but did not submit them.

serhiy-storchaka · 2023-10-28T05:58:21Z

Lib/tkinter/__init__.py

+        if funcid is None:
+            self.tk.call('bind', self._w, sequence, '')
+        else:
+            lines = self.tk.call('bind', self._w, sequence).split('\n')


@TeamSpen210 If you manually register a string to directly call a Tk method, unbind() with funcid will not touch it, and unbind() without second argument will remove it. If you use funcid in other binding, it will stop working when unbind() deletes the command. If you change the line that calls funcid, unbind() may not find it or break the script after deleting the line. In any case, this feature is not documented, and doing so, you are at a risk. Of course, we will try to not break it, but our expectation of possible use cases can be wrong if you do something unusual.

serhiy-storchaka · 2023-10-28T06:00:40Z

Lib/tkinter/__init__.py

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


miss-islington-app · 2023-12-06T14:44:58Z

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

miss-islington-app · 2023-12-06T14:44:58Z

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

…"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]>

bedevere-app · 2023-12-06T14:45:09Z

GH-112801 is a backport of this pull request to the 3.11 branch.

…"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]>

bedevere-app · 2023-12-06T14:45:14Z

GH-112802 is a backport of this pull request to the 3.12 branch.

… "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]>

… "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]>

…"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]>

serhiy-storchaka requested a review from terryjreedy October 25, 2023 13:50

bedevere-app bot mentioned this pull request Oct 25, 2023

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

Open

bedevere-app bot added the awaiting core review label Oct 25, 2023

terryjreedy requested changes Oct 26, 2023

View reviewed changes

bedevere-app bot added awaiting changes and removed awaiting core review labels Oct 26, 2023

Address some of review comments.

ba53020

serhiy-storchaka added 3 commits October 28, 2023 09:00

Merge branch 'main' into tkinter-unbind-funcid

776293a

Update the docstring.

3e71c69

Try to fix tests on Windows.

f8b58bc

serhiy-storchaka added 2 commits October 28, 2023 10:03

Fix tests on Ubuntu.

be3ba6a

Try to rearrange the code.

c5b05e2

skirpichev mentioned this pull request Nov 3, 2023

Is Bedevere retired? python/bedevere#604

Closed

serhiy-storchaka requested a review from terryjreedy November 8, 2023 17:56

serhiy-storchaka commented Dec 1, 2023

View reviewed changes

serhiy-storchaka removed the awaiting changes label Dec 4, 2023

serhiy-storchaka merged commit cc7e45c into python:main Dec 6, 2023
1 check passed

serhiy-storchaka deleted the tkinter-unbind-funcid branch December 6, 2023 14:42

serhiy-storchaka added needs backport to 3.11 only security fixes needs backport to 3.12 bug and security fixes labels Dec 6, 2023

bedevere-app bot removed the needs backport to 3.11 only security fixes label Dec 6, 2023

bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Dec 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-75666: Tkinter: "unbind(sequence, funcid)" now only unbinds "funcid" #111322

gh-75666: Tkinter: "unbind(sequence, funcid)" now only unbinds "funcid" #111322

serhiy-storchaka commented Oct 25, 2023 •

edited by bedevere-app bot

Loading

terryjreedy Oct 26, 2023

terryjreedy Oct 26, 2023

serhiy-storchaka Oct 26, 2023

serhiy-storchaka Oct 26, 2023

terryjreedy Oct 28, 2023

terryjreedy Oct 26, 2023

serhiy-storchaka Oct 26, 2023

serhiy-storchaka Oct 26, 2023

terryjreedy Oct 27, 2023

serhiy-storchaka Oct 28, 2023

terryjreedy Oct 26, 2023

serhiy-storchaka Oct 26, 2023

serhiy-storchaka Oct 26, 2023

TeamSpen210 Oct 28, 2023

terryjreedy Oct 28, 2023

serhiy-storchaka Oct 28, 2023

terryjreedy Oct 26, 2023

serhiy-storchaka Oct 26, 2023

bedevere-app bot commented Oct 26, 2023

terryjreedy commented Oct 28, 2023

serhiy-storchaka commented Oct 28, 2023

terryjreedy commented Oct 28, 2023

serhiy-storchaka commented Oct 29, 2023

serhiy-storchaka left a comment

serhiy-storchaka Oct 28, 2023

serhiy-storchaka Oct 28, 2023

miss-islington-app bot commented Dec 6, 2023

miss-islington-app bot commented Dec 6, 2023

bedevere-app bot commented Dec 6, 2023

bedevere-app bot commented Dec 6, 2023

		self.assertIn(funcid2, script)
		self.assertEqual(f.bind(), (event,))

	and also delete that command.
	and also delete the binding-specific tcl wrapper function.

gh-75666: Tkinter: "unbind(sequence, funcid)" now only unbinds "funcid" #111322

gh-75666: Tkinter: "unbind(sequence, funcid)" now only unbinds "funcid" #111322

Conversation

serhiy-storchaka commented Oct 25, 2023 • edited by bedevere-app bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-app bot commented Oct 26, 2023

terryjreedy commented Oct 28, 2023

serhiy-storchaka commented Oct 28, 2023

terryjreedy commented Oct 28, 2023

serhiy-storchaka commented Oct 29, 2023

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miss-islington-app bot commented Dec 6, 2023

miss-islington-app bot commented Dec 6, 2023

bedevere-app bot commented Dec 6, 2023

bedevere-app bot commented Dec 6, 2023

serhiy-storchaka commented Oct 25, 2023 •

edited by bedevere-app bot

Loading