bpo-42630: Improve error reporting in Tkinter for absent default root by serhiy-storchaka · Pull Request #23781 · python/cpython

bpo-42630: Improve error reporting in Tkinter for absent default root by serhiy-storchaka · Pull Request #23781 · python/cpython

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.

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

Conversation

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.

		@@ -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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

	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)

Choose a reason for hiding this comment

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

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

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

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

Choose a reason for hiding this comment

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

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

See above comment.

		@@ -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')

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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.


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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

It does not do it if _default_root was not set.

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.

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

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.

Thanks for making the requested changes!

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

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.

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

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

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

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request

…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 <storchaka@gmail.com>

serhiy-storchaka added a commit that referenced this pull request

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

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request

…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 <storchaka@gmail.com>

serhiy-storchaka added a commit that referenced this pull request

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

adorilson pushed a commit to adorilson/cpython that referenced this pull request

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