bpo-41176: revise Tkinter mainloop dispatching flag behavior #21299
Conversation
new method `dispatching` on tkinter, Misc, and _tkinter.tkapp
DeprecationWarning issued on implicit wait Docstring changes Comment stubs for eventual removal of WaitForMainloop Add setmainloopwaitattempts method to _tkinter.tkapp
Replace all usages of update_idletasks in tests with update. update_idletasks does not in fact process all pending events. It updates the idle tasks only, without touching the normal event queue. This is not normaly a problem... until it is. Update docstrings to clarify.
|
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). CLA MissingOur records indicate the following people have not signed the CLA: For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. If you have recently signed the CLA, please wait at least one business day You can check yourself to see if the CLA has been received. Thanks again for the contribution, we look forward to reviewing it! |
There was a problem hiding this comment.
This is generally looking very good, though there are a few things I feel need addressing:
- Please sign your CLA! - Unfortunately, we cannot legally accept the patch until the CLA has been signed. Once it has been approved, an asterisk will appear next to your name on bugs.python and the bots should change the PR labels automatically.
- New methods = new docs - As we are adding new methods, these should be fully documented in the docs (probably in Doc/library/tkinter.rst)
- Whatsnew? - I feel this is definitely a big enough change to be worth adding to the 3.10 whatsnew
- ACKS - Because this is your first non-trivial patch, please feel free to add yourself to Misc/ACKS
This is just an initial look at the code and I have not tested the patch yet, though I hope to do this soon.
Thank you for this PR: as I have already said it looks very good and the code has now persuaded me in favour of deprecating WaitForMainloop. I may, at some point, drag Terry into this for his opinion on the changes (and how they could affect IDLE)
|
Thanks @E-Paine! Re: CLA, I have 2 separate signed agreements in my inbox now, I hope I'm not doing something wrong! T_T I'll see what I can do about these other points this weekend, otherwise it may have to wait another week. |
|
I forgot to mention this previously, but you will also need to add a news entry. The easiest way is probably to click "details" on bedevere/news, "sign-in" and then install |
Exceptions raised in the thread will be ignored, so capture the conditions of interest and assert them after joining. Add WaitForMainloop warning on timeout too.
Still need to raise if not dispatching
Too hard to tell which flag broke if using the loop
brackets
In preparation for supporting multiple dispatchers with an IntEnum
failing with timeout gives no helpful debugging info, so try to eliminate all possible hangs
Seems to cause CI to never exit mainloop at least on Ubuntu. No idea why yet.
|
@E-Paine sorry, I need help with CI since I can't reproduce this myself. Ubuntu suddenly began hanging in mainloop but it persists even after I reverted the changes. See this diff between 648434f and b58f8a1. |
prevent subsequent calls to setmainloopwaitattempts from stepping on the behavior of current WaitForMainloop calls.
|
Great! I just ask (hopefully) one last change before I approve the changes (and we call in a core-dev): please indent the Tk class methods (in the docs) so it looks clearer and Sphinx registers them as methods of that class (you can then also remove |
|
I stumbled into https://bugs.python.org/issue39093 in another project. I tried to make a comment on the bug tracker but submitting the comment there only produces errors. I want to +1 the uncommenting solution. The problem occurs on del rather than specifically in the gc somewhere (it happens when refs drop to zero too), so I wouldn't worry too much about killing the garbage collector. It also looks like fixing the python part would be about 3 lines of non-user-facing code with weakrefs. Are you sure that's no-go? Would it be any help to roll https://bugs.python.org/issue39093 into this PR or is keeping them separate best? |
current status: Prone to races and hard-to-diagnose timeouts (see python/cpython#21299)
| def mainloop(n=0): | ||
| """Run the main loop of Tcl.""" | ||
| _default_root.tk.mainloop(n) | ||
| def mainloop(threshold=0): |
There was a problem hiding this comment.
This is an API change that should be flagged?
There was a problem hiding this comment.
Good point, this was just meant to make the variable name a bit more descriptive, but it could break (rare) user code that relied on setting n as a keyword argument.
current status: Prone to races and hard-to-diagnose timeouts (see python/cpython#21299)
|
Could this be re-opened and reconsidered? |
|
If you can get the attention of a core dev for me, I can help with any last touches. But I can't invest the time to do a rewrite if that's needed. |
https://bugs.python.org/issue41176