Skip to content

Add tkinter Event class #4200

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

Merged
merged 4 commits into from
Jun 8, 2020
Merged

Add tkinter Event class #4200

merged 4 commits into from
Jun 8, 2020

Conversation

Hawk777
Copy link
Contributor

@Hawk777 Hawk777 commented Jun 7, 2020

Notes regarding this PR:

  • A lot of fields are set to the string ?? (literally two question marks) for events where those fields are not used and are not applicable. I didn’t bother making most of the fields Union[str, whatever], because that seemed more annoying than useful; most of the time you would be writing an event handler for the appropriate type of event and would only access those fields that are applicable for that event. I can make them all Union[str, whatever] if you think that’s more appropriate though.
  • send_event is not documented in the help output, but from inspection it appears to be a bool, so that’s what I made it.
  • I made the type of widget Misc because it can be an actual widget (of some subclass of BaseWidget) or it can be a tkinter.Tk which does not extend BaseWidget, and Misc is the common base class of those two. I could make it a Union[BaseWidget, Tk] if you prefer, or something else.

@hauntsaninja
Copy link
Collaborator

Could you take a look at CI? Need to fix the enum import or the base class :-)

@Hawk777
Copy link
Contributor Author

Hawk777 commented Jun 7, 2020

Right, base class fixed in 0ea4c3e. One question remaining, I gave all the enum members type int because it looks like that’s what Typeshed does with some other enums, but I don’t really know why. Hopefully that’s OK?

@Hawk777
Copy link
Contributor Author

Hawk777 commented Jun 7, 2020

Guess it’s not OK. CI seems to be suggesting that they ought to be declared as str. Let’s see how 8844c92 manages.

@Hawk777
Copy link
Contributor Author

Hawk777 commented Jun 7, 2020

Sigh. Looks like tkinter.EventType doesn’t exist in Python 3.5. The documentation claims that type is the event type “as a number”, though the enum is actually closer to a string than a number. While I can’t test it at runtime (my distro doesn’t offer an interpreter older than 3.6 these days), I suspect str is probably the more reasonable choice. 9ae1a17

@Hawk777
Copy link
Contributor Author

Hawk777 commented Jun 7, 2020

CI pass!

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Overall, this looks good to me, but I'm not familiar with the tkinter module.

  • enum members are typed by their value, which is often int, but in this case is str: https://github.com/python/cpython/blob/2efe18bf277dd0f38a1d248ae6bdd30947c26880/Lib/tkinter/__init__.py#L147
  • Debugging against CI is totally okay, but if you want to have access to older Python interpreters, pyenv works pretty well.
  • Misc is the class on which the relevant code seems to be defined too, so that lgtm.
  • send_event looks right.
  • Re "??", I'll trust you on whether Union[str, whatever] is a more helpful type than whatever. Note that generally typeshed prefers to stick to implementation, so if this causes issues down the road, we'll change it.

height: int
width: int
keycode: int
state: Union[int, str]
Copy link
Collaborator

Choose a reason for hiding this comment

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

How did you get the type for state? I'm not familiar with this module, but it might be just int: https://github.com/python/cpython/blob/d31b376bc85dded9d059259737b204e544707d07/Lib/tkinter/__init__.py#L1575
(note that calling repr on an Event will display the int by an or of bitmasks)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For events of type <Visibility>, state is a str (examples are VisibilityFullyObscured and VisibilityUnobscured). However for events of type <ButtonPress>, it is an int.

send_event: bool
keysym: str
keysym_num: int
if sys.version_info >= (3, 6):
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are at least some code paths where type and widget end up as str, but maybe this doesn't actually happen in practice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are those just the ?? case? Or are there non-?? cases where that can happen too?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, not familiar with the module, but I was checking against: https://github.com/python/cpython/blob/d31b376bc85dded9d059259737b204e544707d07/Lib/tkinter/__init__.py#L1587-L1592 where if some lookups fail, we end up with the (presumably) str argument.
But seems possible this might not really happen in practice.

@hauntsaninja hauntsaninja merged commit 51267a4 into python:master Jun 8, 2020
@Hawk777 Hawk777 deleted the tk-event branch June 8, 2020 23:17
vishalkuo pushed a commit to vishalkuo/typeshed that referenced this pull request Jun 26, 2020
@Akuli
Copy link
Collaborator

Akuli commented Jul 19, 2020

I often end up asserting the type of event.widget like this:

def save_geometry(event: tkinter.Event) -> None:
    assert isinstance(event.widget, tkinter.Tk)
    config['default_geometry'] = event.widget.geometry()

...

root.bind('<<Foo>>', save_geometry, add=True)

How about making tkinter.Event generic? This way the type of tkinter.Event[T].widget could be T, for any widget type T. And if root is known to have type Tk, I can define the event handler with event: tkinter.Event[tkinter.Tk] and get rid of the assert.

I wonder if it's possible to make plain tkinter.Event equivalent to tkinter.Event[tkinter.Misc] for backwards compatibility.

@Hawk777
Copy link
Contributor Author

Hawk777 commented Jul 19, 2020

How about making tkinter.Event generic? This way the type of tkinter.Event[T].widget could be T, for any widget type T. And if root is known to have type Tk, I can define the event handler with event: tkinter.Event[tkinter.Tk] and get rid of the assert.

I wonder if it's possible to make plain tkinter.Event equivalent to tkinter.Event[tkinter.Misc] for backwards compatibility.

I suppose bind would take a Callable[[tkinter.Event[TheTypeOfTheWidgetYouCalledBindOn]], Optional[str]], though I guess it would require overriding bind in every concrete widget class in order to change the type signature. And also it would really be ideal if we could have some variance, so you can pass def f(e: Event[Widget]) to Button.bind, in case f only needs Widget stuff rather than Button stuff. bind_all and bind_class would be problematic, too; I’m not sure what their callback parameters should be. I’m not an expert at type specifications; if you can make this work, it seems reasonable to me.

@Akuli
Copy link
Collaborator

Akuli commented Jul 19, 2020

I'm currently working on a script that auto-generates some tkinter stubs (hopefully it won't be too insane for creating a pull request to typeshed, lol). If the script ever gets merged to typeshed, then adding a custom bind to every widget would be easy.

I agree that it would be great if Event[Button] was acceptable when Event[Widget] is expected. We shouldn't run into the issue of how List[Button] is not compatible with List[Widget] because event.widget is never assigned to.

bind_all callback should be typed with Event[Misc], because it really applies to all widgets. Tk's widget classes aren't guaranteed to have nice correspondence with tkinter's widget classes, but they pretty much correspond to each other anyway, so I think it would be fine if Label.bind_class used Event[Label].

What's your opinion on typing.Literal and tkinter? I like to use Optional[Literal['break']] for the return value of bind, for example, which means that it is an error to accidentally return "breakk" or something. Tkinter's source code literally has code like this:

cmd = ('%sif {"[%s %s]" == "break"} break\n'
...

i.e. call the bind callback from Tcl and check if it returns the string 'break' or something else.

@Akuli
Copy link
Collaborator

Akuli commented Jul 19, 2020

It seems like putting something to every widget is not even necessary:

from typing import TypeVar, List

T = TypeVar('T')

class Widget:
    def bind(self: T) -> List[T]:
        return [self]

class Label(Widget):
    pass

reveal_type(Label().bind)    # mypy says: Revealed type is 'def () -> builtins.list[foo.Label*]'

@Hawk777
Copy link
Contributor Author

Hawk777 commented Jul 19, 2020

bind_all callback should be typed with Event[Misc], because it really applies to all widgets. Tk's widget classes aren't guaranteed to have nice correspondence with tkinter's widget classes, but they pretty much correspond to each other anyway, so I think it would be fine if Label.bind_class used Event[Label].

I suppose Label.bind_class could take a callable taking Event[Label], but for any container that could contain other widgets, I think it would have to be Event[Misc], right?

What's your opinion on typing.Literal and tkinter? I like to use Optional[Literal['break']] for the return value of bind, for example, which means that it is an error to accidentally return "breakk" or something. Tkinter's source code literally has code like this:

I didn’t know it existed :) It seems great, other than that it’s limited to 3.8+ and therefore requires some Python version checks.

@hauntsaninja
Copy link
Collaborator

I think an upper bound to tkinter.Misc on the type variable used to make Event generic might do what you want? I see you've found a good solution using self types for your other problem.

@Akuli
Copy link
Collaborator

Akuli commented Jul 20, 2020

There is a backwards-compatible way to use Literal (taken from #1631):

if sys.version_info >= (3, 8):
    from typing import Literal
else:
    from typing_extensions import Literal

Now that you mention container widgets, I remembered that Toplevel and Tk (aka toplevel windows) get notified from their child widgets' events. Consider this, for example:

>>> import tkinter
>>> root = tkinter.Tk()
>>> root.bind('<Button-1>', lambda e: print(repr(e.widget)))
'140329698363464<lambda>'
>>> tkinter.Label(root, text="click me").pack()

Now clicking on the label triggers a callback bound with root.bind and prints the label widget. You can also make it print the root widget if you resize the window and click below the label.

This behaviour is documented in bind manual page (the second bullet):

  •  If  a  tag  is the name of an internal window the binding applies to
     that window.

  •  If the tag is the name of a toplevel window the binding  applies  to
     the toplevel window and all its internal windows.

  •  If  the  tag  is the name of a class of widgets, such as Button, the
     binding applies to all widgets in that class;

  •  If tag has the value all, the binding applies to all windows in  the
     application.

One problem with this is that it's possible for a Frame or LabelFrame to essentially become a Toplevel.

>>> f = tkinter.Frame()
>>> f.tk.call('wm', 'manage', f)
''
>>> f.tk.call('wm', 'geometry', f, '200x300')
''
>>> f.bind('<Button-1>', lambda e: print(repr(e.widget)))
'140329697914120<lambda>'
>>> tkinter.Label(f, text="click me too").pack()

Now clicking on the frame runs the bind callback with event.widget set to the Label. This feature is basically not supported at all in tkinter though, as you can see from the need to use tk.call directly, so it's fine to assume that this isn't done.

@JelleZijlstra
Copy link
Member

In typeshed you can just always use from typing_extensions import Literal, no need for the version check.

@Akuli
Copy link
Collaborator

Akuli commented Jul 20, 2020

Is there a way to make plain Event equivalent to Event[Misc]?

The typing module does something similar where Sequence is equivalent to Sequence[Any], but reading the source code of the typing module isn't exactly very enlightening.

@Akuli
Copy link
Collaborator

Akuli commented Jul 20, 2020

Hmm. If I do

_W = TypeVar('_W', covariant=True, bound=Misc)
class Event(Generic[_W]):
    widget: _W

then plain Event is equivalent to Event[Any], which to me seems like the bound isn't actually doing anything.

@Akuli
Copy link
Collaborator

Akuli commented Jul 20, 2020

Also, bind_class doesn't work at all like I thought it would work. You need to pass the Tk widget class name as an argument, so it actually has nothing to do with Python classes and needs to be Event[Misc]-typed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants