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.

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

bpo-32857: tkinter after_cancel to raise error if called with None #5701

Merged
merged 4 commits into from Mar 4, 2018
Merged

bpo-32857: tkinter after_cancel to raise error if called with None #5701

merged 4 commits into from Mar 4, 2018

Conversation

csabella
Copy link
Contributor

@csabella csabella commented Feb 16, 2018 •
edited by bedevere-bot

serhiy-storchaka
serhiy-storchaka reviewed Feb 19, 2018
View reviewed changes
Lib/tkinter/test/test_tkinter/test_misc.py Outdated

# Cancel timer event.
(script, _) = root.tk.splitlist(root.tk.call('after', 'info', timer1))
self.assertIn(script, root._tclCommands)
Copy link
Member

Choose a reason for hiding this comment

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

_tclCommands is an implementation detail. It would be better to not use it in tests. Otherwise changing the implementation (it already was changed in the past) will break tests.

But you can test a side effect of the script by calling root.tk.call(script).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for this suggestion. I wasn't quite sure how to test that the script still existed after the cancel.

Lib/tkinter/test/test_tkinter/test_misc.py Outdated
with self.assertRaises(ValueError):
root.after_cancel(None)

# A non-existent id raises a TclError, which is caught in after_cancel.
Copy link
Member

Choose a reason for hiding this comment

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

The comment looks misleading. This is the test for after_cancel() which doesn't raise a TclError.

Actually I think that checking that root.tk.call('after', 'info', 'spam') raises a TclError is not needed. Using this command in after_cancel() is an implementation detail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

Lib/tkinter/test/test_tkinter/test_misc.py Outdated
@@ -48,6 +48,39 @@ def test_tk_setPalette(self):
'^must specify a background color$',
root.tk_setPalette, highlightColor='blue')

def test_after_cancel(self):
root = self.root
timer1 = root.after(5000, lambda: 'break')
Copy link
Member

Choose a reason for hiding this comment

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

Make commands having a side effect (for example adding an item to the list) and check that after canceling events and calling update() they are not executed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made a counter within a callback function. Is that OK or is a list a better solution?

Lib/tkinter/test/test_tkinter/test_misc.py Outdated
(script, _) = root.tk.splitlist(root.tk.call('after', 'info', timer1))
self.assertIn(script, root._tclCommands)
root.after_cancel(timer1)
self.assertNotIn(script, root._tclCommands)
Copy link
Member

Choose a reason for hiding this comment

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

Here root.tk.call(script) should raise a TclError and shouldn't have a side effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! This is very helpful in my understanding.

@serhiy-storchaka serhiy-storchaka added type-bug An unexpected behavior, bug, or error needs backport to 3.6 labels Feb 20, 2018
serhiy-storchaka
serhiy-storchaka approved these changes Feb 20, 2018
View reviewed changes
Misc/NEWS.d/next/Library/2018-02-16-14-37-14.bpo-32857.-XljAx.rst Outdated
@@ -0,0 +1 @@
:mod:`tkinter` change handling of ``None`` parameter call to after_cancel.
Copy link
Member

Choose a reason for hiding this comment

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

Could you please provide more useful information for readers? Like "after_cancel(None) now is error instead of canceling the first scheduled function."

And please add "Patch by your name."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@serhiy-storchaka serhiy-storchaka merged commit 74382a3 into python:master Mar 4, 2018
@miss-islington
Copy link
Contributor

Thanks @csabella for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6, 3.7.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 4, 2018
@csabella @miss-islington
bpo-32857: Raise error when tkinter after_cancel() is called with Non…
262c137 
…e. (pythonGH-5701)

(cherry picked from commit 74382a3)

Co-authored-by: Cheryl Sabella <cheryl.sabella@gmail.com>
@bedevere-bot
Copy link

GH-5972 is a backport of this pull request to the 3.7 branch.

@miss-islington
Copy link
Contributor

Sorry, @csabella and @serhiy-storchaka, I could not cleanly backport this to 2.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 74382a3f175ac285cc924a73fd758e8dc3cc41bb 2.7

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 4, 2018
@csabella @miss-islington
bpo-32857: Raise error when tkinter after_cancel() is called with Non…
9c2ed48 
…e. (pythonGH-5701)

(cherry picked from commit 74382a3)

Co-authored-by: Cheryl Sabella <cheryl.sabella@gmail.com>
@bedevere-bot
Copy link

GH-5973 is a backport of this pull request to the 3.6 branch.

miss-islington added a commit that referenced this pull request Mar 4, 2018
@miss-islington @csabella
bpo-32857: Raise error when tkinter after_cancel() is called with Non…
73a4396 
…e. (GH-5701)

(cherry picked from commit 74382a3)

Co-authored-by: Cheryl Sabella <cheryl.sabella@gmail.com>
miss-islington added a commit that referenced this pull request Mar 4, 2018
@miss-islington @csabella
bpo-32857: Raise error when tkinter after_cancel() is called with Non…
a5303dd 
…e. (GH-5701)

(cherry picked from commit 74382a3)

Co-authored-by: Cheryl Sabella <cheryl.sabella@gmail.com>
@csabella csabella deleted the bpo32857 branch March 4, 2018 12:54
@csabella
Copy link
Contributor Author

csabella commented Mar 4, 2018

@serhiy-storchaka Would you like me to cherry pick this to 2.7?

@serhiy-storchaka
Copy link
Member

Yes, it would be nice.

csabella added a commit to csabella/cpython that referenced this pull request Apr 27, 2018
@csabella
bpo-32857: Raise error when tkinter after_cancel() is called with Non…
aeabb7a 
…e. (pythonGH-5701)

(cherry picked from commit 74382a3)
@bedevere-bot
Copy link

GH-6620 is a backport of this pull request to the 2.7 branch.

serhiy-storchaka pushed a commit that referenced this pull request May 5, 2018
@csabella @serhiy-storchaka
bpo-32857: Raise error when tkinter after_cancel() is called with Non…
3a04598 
…e. (GH-5701) (GH-6620)

(cherry picked from commit 74382a3)
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 approved these changes

Assignees
No one assigned
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@csabella @miss-islington @bedevere-bot @serhiy-storchaka @the-knights-who-say-ni