Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

bpo-39171: Missing root in tkinter simpledialog.py _QueryDialog #17775

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

Closed
Dominic-Mayers wants to merge 2 commits into from
Closed

bpo-39171: Missing root in tkinter simpledialog.py _QueryDialog #17775

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
03a075b
bpo-39171: In simpledialog.py _QueryDialog, missing _default_root
Dominic-Mayers Dec 31, 2019
e4a5efc
Merge remote-tracking branch 'upstream/master'
Dominic-Mayers Jan 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion 10 Lib/tkinter/simpledialog.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,15 @@ def __init__(self, title, prompt,
parent = None):

if not parent:
parent = tkinter._default_root
if tkinter._default_root:

@E-Paine E-Paine Jun 14, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would recommend changing this to an explicit check for "not None"

@Dominic-Mayers Dominic-Mayers Jun 15, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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()

@E-Paine E-Paine Jun 14, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@E-Paine E-Paine Jun 14, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

On second thought, would it be better to mimic tkinter.commondialog (for consistency), which doesn't withdraw or delete a new window?

@Dominic-Mayers Dominic-Mayers Jun 15, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

else:
raise RuntimeError(
"No parent specified and tkinter is "
"configured to not support default root")

self.prompt = prompt
self.minvalue = minvalue
Expand Down
1 change: 1 addition & 0 deletions 1 Misc/NEWS.d/next/Core and Builtins/2019-12-31-19-06-02.bpo-39171.cmXDsX.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Creating missing parent in tkinter simpledialog when possible.

@E-Paine E-Paine Jun 14, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would this be better suited to the "library" part of the news?

@Dominic-Mayers Dominic-Mayers Jun 15, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I don't know much about these aspects.

Morty Proxy This is a proxified and sanitized view of the page, visit original site.