NotificationsYou must be signed in to change notification settings
Fork32k
Star67.3k

bpo-32839: Add after_info to tkinter#5664

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

serhiy-storchaka merged 11 commits intopython:mainfromcsabella:bpo32839

Apr 26, 2024

Merged

bpo-32839: Add after_info to tkinter#5664

serhiy-storchaka merged 11 commits intopython:mainfromcsabella:bpo32839

Apr 26, 2024

Conversation

Copy link

Contributor

csabella commentedFeb 13, 2018•
edited by bedevere-bot
Loading

https://bugs.python.org/issue32839

the-knights-who-say-ni added the CLA signed label

Feb 13, 2018

bedevere-bot added the awaiting review label

Feb 13, 2018

serhiy-storchaka reviewed

Feb 13, 2018

View reviewed changes

Lib/tkinter/__init__.py OutdatedShow resolvedHide resolved

Lib/tkinter/test/test_tkinter/test_misc.py OutdatedShow resolvedHide resolved

Lib/tkinter/__init__.py OutdatedShow resolvedHide resolved

Lib/tkinter/test/test_tkinter/test_misc.py OutdatedShow resolvedHide resolved

terryjreedy requested changes

Feb 13, 2018

View reviewed changes

Lib/tkinter/__init__.py Outdated

		@@ -763,15 +763,31 @@ def after_cancel(self, id):

		Identifier returned by after or after_idle must be
		given as first parameter."""
		if id is None:
		return

Copy link

Member

Choose a reason for hiding this comment

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

This makes no sense to me.id is required and AFAKI should not be passed asNone. Doafter orafter_idle ever returnNone?

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

Please see my following comment with the Tcl code. I added this because I'm not sure if the original comments about the tuple changing were accurate. I think it was a result of id being None vs it having a value.

Lib/tkinter/__init__.py OutdatedShow resolvedHide resolved

Lib/tkinter/__init__.pyShow resolvedHide resolved

Copy link

bedevere-bot commentedFeb 13, 2018

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 phraseI 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.

bedevere-bot added awaiting changes and removed awaiting review labels

Feb 13, 2018

terryjreedy reviewed

Feb 14, 2018

View reviewed changes

Lib/tkinter/test/test_tkinter/test_misc.py OutdatedShow resolvedHide resolved

csabella added2 commits

March 4, 2018 08:40

Add after_info to tkinter

f3598e1

Changes per review.

51bfe2b

csabella force-pushed thebpo32839 branch from90c6408 to51bfe2bCompare

March 4, 2018 13:43

Copy link

ContributorAuthor

csabella commentedMar 4, 2018

I pushed a rebase and got the Appveyor error. I'm working on the other concerns about theafter_info functionality, so I'll push a fix once those are worked out. Although, this issue on Appveyor does answer the previous question about theafter_info() event list containing other events than the ones added in this test. It seems that the other newafter* tests are leaving something behind.

ZackerySpytz reviewed

Dec 27, 2019

View reviewed changes

Lib/tkinter/__init__.py OutdatedShow resolvedHide resolved

Misc/NEWS.d/next/Library/2018-02-13-10-02-54.bpo-32839.McbVz3.rst OutdatedShow resolvedHide resolved

Lib/tkinter/__init__.py OutdatedShow resolvedHide resolved

csabella closed this

Dec 31, 2019

Copy link

Member

terryjreedy commentedDec 31, 2019

Cheryl, can you explain the closing? I still think adding after_info would be good.

serhiy-storchaka reopened this

Dec 31, 2019

Merge branch 'master' into bpo32839

57806b7

Copy link

Member

terryjreedy commentedDec 31, 2019•
edited
Loading

The only reported error, only on Travis, on line 191 is an extra item, 'after#8' (the 'timer' of line 180, I presume), in the tuple returned from .after_info. (The Travis job page somehow disables copying with current Firefox.) On this system, with the tcl/tk used, the preceding root.update() did not remove the 'timer' callback from the list.

I restarted the failing job to see if the failure is repeatable on Travis.

Answer: yes.

Copy link

Contributor

ZackerySpytz commentedJan 4, 2020

FWIW, I also think addingafter_info() would be good.

Copy link

ContributorAuthor

csabella commentedJan 26, 2020

Sorry about closing this prematurely. I had thought there hadn't been much interest in adding the functionality when I originally proposed it, so I was just doing some housekeeping. Good to know there is interest. Having said that, the tests pass locally, so it may be tricky to get them to pass. I have aroot.update() to clear the timer event from a previous test, so it seems to work locally but not on Travis. I'll look into it more.