bpo-39171: Missing root in tkinter simpledialog.py _QueryDialog#17775
bpo-39171: Missing root in tkinter simpledialog.py _QueryDialog#17775Dominic-Mayers wants to merge 2 commits into
Conversation
|
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). Recognized GitHub usernameWe couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames: @MaharishiCanada This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. You can check yourself to see if the CLA has been received. Thanks again for the contribution, we look forward to reviewing it! |
|
I signed the CLA and added a blurb. I hope these are OK now. |
* The programmer has to create the root himself and then withdraw it. * In a similar situation, the moduke ttk creates the root automatically.
|
|
||
| if not parent: | ||
| parent = tkinter._default_root | ||
| if tkinter._default_root: |
There was a problem hiding this comment.
I would recommend changing this to an explicit check for "not None"
There was a problem hiding this comment.
Yes. The only reason why I did not do it, is to make as little change as possible.
| parent = tkinter._default_root | ||
| elif tkinter._support_default_root: | ||
| parent = tkinter.Tk() | ||
| parent.withdraw() |
There was a problem hiding this comment.
If a new Tk instance is being created, I am of the opinion it should be explicitly deleted once we are done with the dialog (we shouldn't rely on the GC to clean it).
There was a problem hiding this comment.
On second thought, would it be better to mimic tkinter.commondialog (for consistency), which doesn't withdraw or delete a new window?
There was a problem hiding this comment.
I thought about that too. It was a few months ago, but I remember to have compared with what is done elsewhere and came to the same conclusion.
| @@ -0,0 +1 @@ | ||
| Creating missing parent in tkinter simpledialog when possible. |
There was a problem hiding this comment.
Would this be better suited to the "library" part of the news?
There was a problem hiding this comment.
I don't know much about these aspects.
|
I really appreciate the effort that has clearly gone into this patch, however I fear it is needlessly over-complicated. It may be better to set the parent to a dummy frame, similar to 'commondialog', which will then handle all your logic in a single line ( |
|
It is not clear to me that it will be equivalent. I suspect that to have the expected behavior with regard to the location of the widget, one may want to reuse the current main window, if it exists. Honestly, the current code is something that I wrote few months ago, so I am relying on vague memory, but what I recall is that location with respect to the current parent was important. But, if you are sure that you are taking care of all the logic with respect to location, go ahead. |
|
Sorry, by a dummy widget, I meant create a blank frame but not add it to the window. This way the window supplied by "parent" is used if it exists, otherwise the default root and falling back to create a new window. The widget is never added to the window using a display manager. |
|
I propose this for the news: |
|
It was done in more general way in other issues. |
|
Great ! |
My first pull request. I hope this will be obviously linked to https://bugs.python.org/issue39171.
https://bugs.python.org/issue39171