Skip to content

gh-75666: Fix Misc.unbind behaviour when funcid is given #17954

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 9 commits into from
Closed

gh-75666: Fix Misc.unbind behaviour when funcid is given #17954

wants to merge 9 commits into from

Conversation

GiovaLomba
Copy link
Contributor

@GiovaLomba GiovaLomba commented Jan 11, 2020 •
edited by terryjreedy
Loading

@GiovaLomba
Fix: https://bugs.python.org/issue31485
95c4404 
Allows Misc.unbind to delete only the bound callback
having the given funcid. See the issue above for more.
@GiovaLomba
Merge pull request #1 from GiovaLomba/issue31485
ac6aeef 
bpo-31485: Fix Misc.unbind behaviour when funcid is given
@GiovaLomba GiovaLomba closed this Jan 13, 2020
@GiovaLomba GiovaLomba reopened this Jan 14, 2020
@vstinner
Copy link
Member

cc @serhiy-storchaka @terryjreedy @csabella

@GiovaLomba
Copy link
Contributor Author

About the ubuntu test failure: it happens at cpython/Lib/idlelib/multicall.py, line 241 in __del__. Having a look at the code it appears that, for reasons unknown to me, on Ubuntu when the destructor is called functions in tk have already been gone and hence the deletecommand(funcid) raises Can't delete Tcl command. As a workaround to Ubuntu specific behaviour, and maybe future changes in the tk library itself, exception handling could be added around the deletecommand(funcid) call, avoiding exception being raised when funcid does not exists anymore. What's your opinion?

terryjreedy
terryjreedy requested changes Jan 14, 2020
View reviewed changes
Copy link
Member

@terryjreedy terryjreedy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a new test is needed to verify that the revision does what we want and expect it to.

@bedevere-bot
Copy link

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@GiovaLomba
Fix: tests on Ubuntu bionic
8e03203 
Fix `_tkinter.TclError: can't delete Tcl command` on Ubuntu bionic.
Change `bound` to `keep` as suggested.
@GiovaLomba
Update 2020-01-11-22-09-29.bpo-31485.DSvjsY.rst
0583640
@terryjreedy
Update __init__.py
597ef39 
Revise docstring to match behavior specified by Serhiy.  Rearrange code to match.
@terryjreedy
Fix whitespace, delete extra '.'
082db44
@terryjreedy
Leave no-funcid call as it was.
af41122
GiovaLomba
GiovaLomba commented Jan 16, 2020
View reviewed changes
@terryjreedy
Copy link
Member

terryjreedy commented Jan 17, 2020 •
edited
Loading

The Travis failure is bogus as far as this issue is concerned: 1. a warning about a future docutils change, treated as an error; 2. news.gmane.io not responding. The Travis code tests (Ubuntu) passed.

@serhiy-storchaka Do you want to add tests for this? On this issue? There seem to be no existing bind tests. ('bind' only occurs, 8 times, in test_ttk.test_widgets.) It would seem that Misc._bind could also stand being tested.

I think the code is ready to merge, pending your review.

@python python deleted a comment from the-knights-who-say-ni Jan 23, 2020
@csabella csabella removed the request for review from serhiy-storchaka May 25, 2020 15:39
@csabella csabella requested a review from serhiy-storchaka May 25, 2020 15:40
@serhiy-storchaka
Copy link
Member

Oh, yes, such non-trivial code needs tests.

serhiy-storchaka
serhiy-storchaka reviewed Dec 15, 2020
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks correct to me, but it would be nice to add tests.

Misc/NEWS.d/next/Library/2020-01-11-22-09-29.bpo-31485.DSvjsY.rst
@@ -0,0 +1 @@
https://github.com/python/cpython/pull/17954#
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a short description, not a link.

There will be a link to the corresponded issue on the tracker in any case.

@vstinner vstinner closed this May 3, 2021
@vstinner vstinner deleted the branch python:master May 3, 2021 21:29
@j4321 j4321 mannequin mentioned this pull request Apr 18, 2023
Open
@terryjreedy terryjreedy changed the title bpo-31485: Fix Misc.unbind behaviour when funcid is given gh-75666: Fix Misc.unbind behaviour when funcid is given Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka left review comments

@terryjreedy terryjreedy terryjreedy approved these changes

Assignees
No one assigned
Labels
awaiting merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@GiovaLomba @vstinner @bedevere-bot @terryjreedy @serhiy-storchaka @the-knights-who-say-ni