bpo-42630: Improve error reporting in Tkinter for absent default root #23781

serhiy-storchaka · 2020-12-15T15:55:57Z

Tkinter functions and constructors which need a default root window
raise now RuntimeError with descriptive message instead of obscure
AttributeError or NameError if it is not created yet or cannot
be created automatically.

Also add tests for all functions which use default root window.

https://bugs.python.org/issue42630

Tkinter functions and constructors which need a default root window raise now RuntimeError with descriptive message instead of obscure AttributeError or NameError if it is not created yet or cannot be created automatically. Also add tests for all functions which use default root window.

E-Paine · 2020-12-15T19:42:55Z

Lib/tkinter/__init__.py

@@ -2262,6 +2273,9 @@ def __init__(self, screenName=None, baseName=None, className='Tk',
        self.tk = _tkinter.create(screenName, baseName, className, interactive, wantobjects, useTk, sync, use)
        if useTk:
            self._loadtk()
+            global _default_root


IMO it makes sense for root = tk.Tk() to be equivalent to:

root = tk.Tcl() root.loadtk()

This change means that the following now fails:

root = tk.Tcl() root.loadtk() tk.getboolean("true")

Agree. This change is a product of evolution. Initially it was more different from the original code, but finally the original code was restored, just in different place.

E-Paine · 2020-12-15T19:45:31Z

Lib/tkinter/font.py

@@ -184,7 +184,7 @@ def metrics(self, *options, **kw):
 def families(root=None, displayof=None):
    "Get font families (as a tuple)"
    if not root:
-        root = tkinter._default_root
+        root = tkinter._get_default_root('use font.families()')


Suggested change

root = tkinter._get_default_root('use font.families()')

tk = tkinter._get_default_root('use font.families()').tk

Similar to what you did in __init__.py (which I thought was a good idea)

This code differs from the code in __init__.py, because we have the root parameter. So we would need to add a branch:

else: tk = root.tk

It is larger change of code and I am not sure it is worth.

E-Paine · 2020-12-15T19:45:48Z

Lib/tkinter/font.py

@@ -194,7 +194,7 @@ def families(root=None, displayof=None):
 def names(root=None):
    "Get names of defined fonts (as a tuple)"
    if not root:
-        root = tkinter._default_root
+        root = tkinter._get_default_root('use font.names()')


Suggested change

root = tkinter._get_default_root('use font.names()')

tk = tkinter._get_default_root('use font.names()').tk

See above comment.

E-Paine · 2020-12-15T19:46:24Z

Lib/tkinter/simpledialog.py

@@ -260,7 +258,7 @@ def __init__(self, title, prompt,
                 parent = None):

        if not parent:
-            parent = tkinter._default_root
+            parent = _get_default_root('create query dialog')


I'm not sure this whole check for parent is needed as parent is ultimately passed to Toplevel which handles it.

But parent is also used directly in Dialog.__init__. Although there are other issues in Dialog.__init__. I'll make that code working according to the initial intention.

Lib/tkinter/test/support.py

E-Paine · 2020-12-15T19:47:17Z

Lib/tkinter/test/support.py

+
+    def tearDown(self):
+        destroy_default_root()
+        tkinter._default_root = None


Do we need this, as destroy_default_root set _default_root to None there as well?

It does not do it if _default_root was not set.

E-Paine · 2020-12-16T11:21:32Z

See also issue 39171

terryjreedy

test.test_idle ends with

    tk._support_default_root = 1
    tk._default_root = None

Since you are backporting, feel free to change this also.
idlelib.pyshell 1065 has tkinter._default_root = None # 03Jan04 KBK What's this?
(The comment should go.) Should _default_root be set to None or deleted? I am a bit confused since NoDefaultRoot does both.

There are no other references to (_support)_default_root in /LIb outside of /tkinter.

Lib/tkinter/__init__.py

bedevere-bot · 2020-12-18T22:52:05Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

serhiy-storchaka · 2020-12-19T08:31:17Z

tkinter._default_root = None # 03Jan04 KBK What's this?

This is for running IDLE in the same process as the user code. IDLE already created a root window and set it as default, but the user code should use separate root window.

I have made the requested changes; please review again.

bedevere-bot · 2020-12-19T08:31:20Z

Thanks for making the requested changes!

@terryjreedy: please review the changes made to this pull request.

terryjreedy

Almost no one runs user code in the IDLE process, but until the option is removed, the new comment is OK, so IDLE changes are approved.

miss-islington · 2020-12-19T10:17:11Z

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8, 3.9.
🐍🍒⛏🤖

miss-islington · 2020-12-19T10:17:17Z

Sorry, @serhiy-storchaka, I could not cleanly backport this to 3.9 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 3d569fd6dccf9f582bafaca04d3535094cae393e 3.9

miss-islington · 2020-12-19T10:17:18Z

Sorry @serhiy-storchaka, I had trouble checking out the 3.8 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 3d569fd6dccf9f582bafaca04d3535094cae393e 3.8

…t root (pythonGH-23781) * Tkinter functions and constructors which need a default root window raise now RuntimeError with descriptive message instead of obscure AttributeError or NameError if it is not created yet or cannot be created automatically. * Add tests for all functions which use default root window. * Fix import in the pynche script.. (cherry picked from commit 3d569fd) Co-authored-by: Serhiy Storchaka <[email protected]>

bedevere-bot · 2020-12-19T10:33:30Z

GH-23853 is a backport of this pull request to the 3.9 branch.

…t root (GH-23781) (GH-23853) * Tkinter functions and constructors which need a default root window raise now RuntimeError with descriptive message instead of obscure AttributeError or NameError if it is not created yet or cannot be created automatically. * Add tests for all functions which use default root window. * Fix import in the pynche script. (cherry picked from commit 3d569fd)

…t root (pythonGH-23781) * Tkinter functions and constructors which need a default root window raise now RuntimeError with descriptive message instead of obscure AttributeError or NameError if it is not created yet or cannot be created automatically. * Add tests for all functions which use default root window. * Fix import in the pynche script.. (cherry picked from commit 3d569fd) Co-authored-by: Serhiy Storchaka <[email protected]>

bedevere-bot · 2020-12-19T11:16:01Z

GH-23854 is a backport of this pull request to the 3.8 branch.

…t root (GH-23781) (GH-23854) * Tkinter functions and constructors which need a default root window raise now RuntimeError with descriptive message instead of obscure AttributeError or NameError if it is not created yet or cannot be created automatically. * Add tests for all functions which use default root window. * Fix import in the pynche script. (cherry picked from commit 3d569fd)

…pythonGH-23781) * Tkinter functions and constructors which need a default root window raise now RuntimeError with descriptive message instead of obscure AttributeError or NameError if it is not created yet or cannot be created automatically. * Add tests for all functions which use default root window. * Fix import in the pynche script.

serhiy-storchaka added needs backport to 3.8 needs backport to 3.9 only security fixes labels Dec 15, 2020

the-knights-who-say-ni added the CLA signed label Dec 15, 2020

bedevere-bot added the awaiting core review label Dec 15, 2020

E-Paine mentioned this pull request Dec 15, 2020

gh-86017: Improve font actual docs #22434

Open

E-Paine reviewed Dec 15, 2020

View reviewed changes

Address review comments and fix also Dialog.__init__.

98a47b9

serhiy-storchaka added 4 commits December 18, 2020 08:49

Merge branch 'master' into tkinter-default-root

ba335c6

Add test_simpledialog.

c5db1f7

Merge branch 'master' into tkinter-default-root

6284e46

Merge branch 'master' into tkinter-default-root

0a5db85

terryjreedy requested changes Dec 18, 2020

View reviewed changes

Lib/tkinter/__init__.py Show resolved Hide resolved

bedevere-bot added awaiting changes and removed awaiting core review labels Dec 18, 2020

serhiy-storchaka added 5 commits December 19, 2020 09:44

Merge branch 'master' into tkinter-default-root

7209007

Fix import in pynche and simplify the code.

70dd5c9

Clarify setting of _default_root in IDLE.

007c61f

Add tests for NoDefaultRoot and setting default root in Tk().

2d2c89f

Add comments for NoDefaultRoot().

9b31415

bedevere-bot added awaiting change review and removed awaiting changes labels Dec 19, 2020

bedevere-bot requested a review from terryjreedy December 19, 2020 08:31

terryjreedy approved these changes Dec 19, 2020

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Dec 19, 2020

serhiy-storchaka merged commit 3d569fd into python:master Dec 19, 2020

bedevere-bot removed the awaiting merge label Dec 19, 2020

serhiy-storchaka deleted the tkinter-default-root branch December 19, 2020 10:17

miss-islington assigned serhiy-storchaka Dec 19, 2020

bedevere-bot removed the needs backport to 3.9 only security fixes label Dec 19, 2020

bedevere-bot removed the needs backport to 3.8 label Dec 19, 2020

serhiy-storchaka removed their assignment Dec 29, 2020

	root = tkinter._get_default_root('use font.families()')
	tk = tkinter._get_default_root('use font.families()').tk

Uh oh!

bpo-42630: Improve error reporting in Tkinter for absent default root #23781

bpo-42630: Improve error reporting in Tkinter for absent default root #23781

Uh oh!

Conversation

serhiy-storchaka commented Dec 15, 2020 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

E-Paine Dec 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Dec 16, 2020

Choose a reason for hiding this comment

Uh oh!

E-Paine Dec 15, 2020

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Dec 16, 2020

Choose a reason for hiding this comment

Uh oh!

E-Paine Dec 15, 2020

Choose a reason for hiding this comment

Uh oh!

E-Paine Dec 15, 2020

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Dec 16, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

E-Paine Dec 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Dec 16, 2020

Choose a reason for hiding this comment

Uh oh!

E-Paine commented Dec 16, 2020

Uh oh!

terryjreedy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bedevere-bot commented Dec 18, 2020

Uh oh!

serhiy-storchaka commented Dec 19, 2020

Uh oh!

bedevere-bot commented Dec 19, 2020

Uh oh!

terryjreedy left a comment

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Dec 19, 2020

Uh oh!

miss-islington commented Dec 19, 2020

Uh oh!

miss-islington commented Dec 19, 2020

Uh oh!

bedevere-bot commented Dec 19, 2020

Uh oh!

bedevere-bot commented Dec 19, 2020

Uh oh!

Uh oh!

serhiy-storchaka commented Dec 15, 2020 •

edited by bedevere-bot

Loading

E-Paine Dec 15, 2020 •

edited

Loading

E-Paine Dec 15, 2020 •

edited

Loading