Variable.__init__ raises obscure AttributeError

[shippo16]

BPO	42630
Nosy	@serhiy-storchaka, @E-Paine, @shippo16
PRs	bpo-42630: Improve error reporting in Tkinter for absent default root #23781 [3.9] bpo-42630: Improve error reporting in Tkinter for absent default root (GH-23781) #23853 [3.8] bpo-42630: Improve error reporting in Tkinter for absent default root (GH-23781) #23854
Files	setup_master.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2020-12-19.15:36:43.728>
created_at = <Date 2020-12-13.23:24:00.603>
labels = ['3.8', 'type-bug', 'expert-tkinter', '3.9', '3.10']
title = 'Variable.__init__ raises obscure AttributeError'
updated_at = <Date 2020-12-19.15:36:43.728>
user = 'https://github.com/shippo16'

bugs.python.org fields:

activity = <Date 2020-12-19.15:36:43.728>
actor = 'serhiy.storchaka'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2020-12-19.15:36:43.728>
closer = 'serhiy.storchaka'
components = ['Tkinter']
creation = <Date 2020-12-13.23:24:00.603>
creator = 'shippo_'
dependencies = []
files = ['49681']
hgrepos = []
issue_num = 42630
keywords = ['patch']
message_count = 18.0
messages = ['382944', '382946', '382949', '382951', '382974', '382978', '383002', '383003', '383018', '383041', '383043', '383081', '383082', '383087', '383106', '383366', '383369', '383381']
nosy_count = 3.0
nosy_names = ['serhiy.storchaka', 'epaine', 'shippo_']
pr_nums = ['23781', '23853', '23854']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue42630'
versions = ['Python 3.8', 'Python 3.9', 'Python 3.10']

[shippo16]

Hello.

I think it would be nice to add:

335 > if not master:
336 > raise RuntimeError('a valid Tk instance is required.')

to lib/tkinter/init.py, and not rely on this unclear AttributeError.

Could it be assigned to me, please?

Best Regards
Ivo Shipkaliev

[shippo16]

https://mail.python.org/archives/list/python-ideas@python.org/thread/FSQUFJJQDNSRN4HI7VFXWCNO46YLXQDS/

[shippo16]

Or maybe:

333 > if not master:
334 > if not _default_root:
335 > _default_root = Tk()
336 > master = _default_root
337 > self._root = master._root()

[shippo16]

Sorry, we need "global" too:

333 > if not master:
334 > global _default_root
335 > if not _default_root:
336 > _default_root = Tk()
337 > master = _default_root
338 > self._root = master._root()

[E-Paine]

+1 I agree the current AttributeError is not suitable.

I would just copy the code from Lib/tkinter/init.py:2524 (or even better: refactor it into its own method to avoid duplication). The code there, though, would raise a similar AttributeError if the default root is disabled, so I suggest that needs a changing to possibly a TypeError (i.e. missing 'master' argument). I assume you are alright to create a PR for this?

Please revert the "resolution" field as this is intended to be the reason for issue closure.

[serhiy-storchaka]

Actually it may be worth to reuse setup_master() from tkinter.ttk.

But I am not sure what is better: raise error (RuntimeError) if the global function uses _default_root which is not initialized, or create the root widget implicitly. Currently AttributeError or NameError is raised.

[shippo16]

The current implementation is already relying on _some_ master anyway:

335 > self._root = master._root()

So, initializing a default root, if one isn't present, shouldn't break anything. And reusing setup_master() is a good idea. Maybe:

333 > master = setup_master(master)
334 > self._root = master._root()

But:

from tkinter.ttk import setup_master

leads to a circular import error. I'll look into this.

[E-Paine]

Attached is a diff which moves the logic for setup_master to the tkinter module while allowing it to still be imported from the tkinter.ttk module (in case someone uses it...) The diff also replaces the logic in a few other places to:
A. make behaviour more consistent
B. give nicer errors in other methods (i.e. avoiding what this issue is about but in other places)

I guess my question is whether we are limiting most changes to just __init__.py or whether we want to do more of a cleanup throughout the tkinter module (e.g. tkinter.dialog.Dialog can be neatened and no longer needs to inherit the Widget class).

[shippo16]

"Attached is a diff which ..." -- Much nicer!

Are you gonna submit a PR so I can eventually use _setup_master() in my PR?

[E-Paine]

Are you gonna submit a PR

I think I assumed you would incorporate it into your PR unless you would prefer it to be separate?

[serhiy-storchaka]

Don't haste. I am currently working on a large PR with many tests.

[shippo16]

Thank you very much, fellows!

Serhiy, I'm lloking at Lib/tkinter/init.py changes. I'd like to share my thoughts on this:

I understand your concern: "But I am not sure what is better: raise error (RuntimeError) if the global function uses _default_root which is not initialized, or create the root widget implicitly."

In the new _get_default_root() function, first we check if Support Default Root in on:

290 > if not _support_default_root:

If we get passed this block, we know that Support Default Root is on, meaning that all entities that require a master, but didn't receive one passed-in, must receive a default one. That's how I perceive the whole idea behind Support Default Root.

But later on we do:

293 > if not _default_root:
294 > if what:
295 > raise RuntimeError(f"Too early to {what}: no default root window")

At this point, if "what" evaluates to True, we raise a RuntimeError. But at this same time Support Default Root is on, and there is no default root. And clearly: ".. no default root window" error contradicts the "_support_default_root" idea.

So further on, assuming Support Default Root is on, if we instantiate a Variable with master=None, we would get: "Too early to create variable: no default root window", which makes no sense.

In my view, we should always create a default root if it's needed, provided that _support_default_root is True. Simply: Support Default Root must lead to a default root.

Best Regards

[E-Paine]

In my view, we should always create a default root if it's needed

I somewhat disagree. I think Serhiy has done a very good job (in what I've reviewed so far) of balancing when a new root should or shouldn't be created (e.g. does it make sense to create a new root when calling getboolean as this should only be called once there is a Tcl object to be processed?).

We also have the consideration of backwards compatibility as some weird script may rely upon such errors to, for example, detect when a root has not already been created.

[serhiy-storchaka]

Currently, a root window is created implicitly only when you create a Tkinter or Ttk widget. i.e. things which should be visible to user. When you create image, variable, or use global utility function like getbool() or mainloop() or image_types() it raises an error: AttributeError or NamedError, or sometimes RuntimeError with error message like "Too early to create image: no default root window". With PR 23781 it will always raise RuntimeError instead of AttributeError or NamedError with corresponding error message.

"no default root window" is correct. There is no yet default root window required by the function. After you create it, explicitly or implicitly, you could use that function.

It could be odd if image_type() will successfully return a result with a side effect of popping up a window.

[shippo16]

First: thank you!

I think Serhiy has done a very good job ...

I'm not saying he ain't! More so, I greatly appreciate everyone's time and effort. But I'm discussing the implementation here, not somebody's work.

Apparently I haven't been precise enough in conveying my message. Let me try to clarify what I mean. Consider the following:

An object gets initialized. The object's constructor accepts a "master" optional parameter (e.g. Variable.__init__). So, every time:
-- "master" is None
and
-- _support_default_root is True
a default root must be instantiated. A master is optional, and when it's not given and _support_default_root switch is on, a default root should be supplied. That's the whole point of _support_default_root after all.

My understanding of the module is not as deep as yours', but getboolean(), mainloop() and image_types() shouldn't be affected.

"Too early to create image: no default root window": Why isn't there? When _support_default_root is on.

Again, I can see that:

"no default root window" is correct

:But why is there no default window? Support default root is on, right?

There is no yet default root window required by the function.

:A default root is required when you instantiate an Image without a "master". It's not required as an argument, but it is required for the operation of Image.
I'm suggesting something like:

class Variable:
...
def __init__(self, master=None, value=None, name=None):
...
master = master or _get_default_root()
self._root = master._root()
...

class Image:
...
def __init__(self, imgtype, name=None, cnf={}, master=None, **kw):
...
master = master or _get_default_root()
self.tk = getattr(master, 'tk', master)
...

Best Wishes
Ivo Shipkaliev

[serhiy-storchaka]

New changeset 3d569fd by Serhiy Storchaka in branch 'master':
bpo-42630: Improve error reporting in Tkinter for absent default root (GH-23781)
3d569fd

[serhiy-storchaka]

New changeset 87e7a14 by Serhiy Storchaka in branch '3.9':
[3.9] bpo-42630: Improve error reporting in Tkinter for absent default root (GH-23781) (GH-23853)
87e7a14

[serhiy-storchaka]

New changeset 80c445c by Serhiy Storchaka in branch '3.8':
[3.8] bpo-42630: Improve error reporting in Tkinter for absent default root (GH-23781) (GH-23854)
80c445c