classification
Title: Variable.__init__ raises obscure AttributeError
Type: behavior Stage: resolved
Components: Tkinter Versions: Python 3.10, Python 3.9, Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: epaine, serhiy.storchaka, shippo_
Priority: normal Keywords: patch

Created on 2020-12-13 23:24 by shippo_, last changed 2020-12-19 15:36 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
setup_master.diff epaine, 2020-12-14 20:42
Pull Requests
URL Status Linked Edit
PR 23781 merged serhiy.storchaka, 2020-12-15 15:56
PR 23853 merged serhiy.storchaka, 2020-12-19 10:33
PR 23854 merged serhiy.storchaka, 2020-12-19 11:15
Messages (18)
msg382944 - (view) Author: Ivo Shipkaliev (shippo_) * Date: 2020-12-13 23:24
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
msg382946 - (view) Author: Ivo Shipkaliev (shippo_) * Date: 2020-12-13 23:49
https://mail.python.org/archives/list/python-ideas@python.org/thread/FSQUFJJQDNSRN4HI7VFXWCNO46YLXQDS/
msg382949 - (view) Author: Ivo Shipkaliev (shippo_) * Date: 2020-12-14 00:09
Or maybe:

333 >         if not master:
334 >             if not _default_root:
335 >                 _default_root = Tk()
336 >             master = _default_root
337 >         self._root = master._root()
msg382951 - (view) Author: Ivo Shipkaliev (shippo_) * Date: 2020-12-14 02:07
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()
msg382974 - (view) Author: E. Paine (epaine) * Date: 2020-12-14 10:20
+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.
msg382978 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-12-14 10:41
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.
msg383002 - (view) Author: Ivo Shipkaliev (shippo_) * Date: 2020-12-14 19:22
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.
msg383003 - (view) Author: E. Paine (epaine) * Date: 2020-12-14 20:42
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).
msg383018 - (view) Author: Ivo Shipkaliev (shippo_) * Date: 2020-12-14 23:28
"Attached is a diff which ..." -- Much nicer!

Are you gonna submit a PR so I can eventually use _setup_master() in my PR?
msg383041 - (view) Author: E. Paine (epaine) * Date: 2020-12-15 10:08
> 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?
msg383043 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-12-15 11:10
Don't haste. I am currently working on a large PR with many tests.
msg383081 - (view) Author: Ivo Shipkaliev (shippo_) * Date: 2020-12-15 17:24
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
msg383082 - (view) Author: E. Paine (epaine) * Date: 2020-12-15 17:34
> 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.
msg383087 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-12-15 18:42
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.
msg383106 - (view) Author: Ivo Shipkaliev (shippo_) * Date: 2020-12-15 23:34
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
msg383366 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-12-19 10:17
New changeset 3d569fd6dccf9f582bafaca04d3535094cae393e by Serhiy Storchaka in branch 'master':
bpo-42630: Improve error reporting in Tkinter for absent default root (GH-23781)
https://github.com/python/cpython/commit/3d569fd6dccf9f582bafaca04d3535094cae393e
msg383369 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-12-19 11:08
New changeset 87e7a14ee3bd7dc495e51166598453114342d0bf by Serhiy Storchaka in branch '3.9':
[3.9] bpo-42630: Improve error reporting in Tkinter for absent default root (GH-23781) (GH-23853)
https://github.com/python/cpython/commit/87e7a14ee3bd7dc495e51166598453114342d0bf
msg383381 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-12-19 14:38
New changeset 80c445cafbdfb16c4a882e3ff6fe28b471aacdfc by Serhiy Storchaka in branch '3.8':
[3.8] bpo-42630: Improve error reporting in Tkinter for absent default root (GH-23781) (GH-23854)
https://github.com/python/cpython/commit/80c445cafbdfb16c4a882e3ff6fe28b471aacdfc
History
Date User Action Args
2020-12-19 15:36:43serhiy.storchakasetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2020-12-19 14:38:52serhiy.storchakasetmessages: + msg383381
2020-12-19 11:15:59serhiy.storchakasetpull_requests: + pull_request22719
2020-12-19 11:08:31serhiy.storchakasetmessages: + msg383369
2020-12-19 10:33:28serhiy.storchakasetpull_requests: + pull_request22718
2020-12-19 10:17:11serhiy.storchakasetmessages: + msg383366
2020-12-19 08:43:06serhiy.storchakalinkissue42672 superseder
2020-12-15 23:34:21shippo_setmessages: + msg383106
2020-12-15 18:42:15serhiy.storchakasetmessages: + msg383087
2020-12-15 17:34:14epainesetmessages: + msg383082
2020-12-15 17:24:18shippo_setmessages: + msg383081
2020-12-15 15:56:12serhiy.storchakasetstage: patch review
pull_requests: + pull_request22639
2020-12-15 11:10:48serhiy.storchakasetmessages: + msg383043
2020-12-15 10:08:02epainesetmessages: + msg383041
2020-12-14 23:28:56shippo_setmessages: + msg383018
2020-12-14 20:42:08epainesetfiles: + setup_master.diff
keywords: + patch
messages: + msg383003
2020-12-14 19:22:58shippo_setmessages: + msg383002
2020-12-14 15:14:46shippo_setresolution: works for me -> (no value)
2020-12-14 10:41:37serhiy.storchakasetassignee: serhiy.storchaka
messages: + msg382978
2020-12-14 10:20:57epainesetversions: + Python 3.8, Python 3.9, Python 3.10
nosy: + serhiy.storchaka, epaine
title: Variable.__init__ to raise a RuntimeError instead of obscure AttributeError -> Variable.__init__ raises obscure AttributeError
messages: + msg382974

2020-12-14 02:07:43shippo_setresolution: works for me
messages: + msg382951
2020-12-14 00:09:57shippo_setmessages: + msg382949
title: Variable.__init__ raise a RuntimeError instead of obscure AttributeError -> Variable.__init__ to raise a RuntimeError instead of obscure AttributeError
2020-12-13 23:49:35shippo_setmessages: + msg382946
2020-12-13 23:24:00shippo_create