Skip to content
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

Event handler cleanups #3185

Open
mhsmith opened this issue Feb 11, 2025 · 1 comment
Open

Event handler cleanups #3185

mhsmith opened this issue Feb 11, 2025 · 1 comment
Labels
enhancement New features, or improvements to existing features.

Comments

@mhsmith
Copy link
Member

mhsmith commented Feb 11, 2025

While working on the Invent prototype in #3157, I found it necessary to pass each event handler a keyword argument identifying the event type. Although we don't have any need for that feature now, the way in which I did it may be useful for other reasons.

Specifically, I created an EventProperty class which allowed all this boilerplate:

    @property
    def on_press(self) -> OnPressHandler:
        """The handler to invoke when the button is pressed."""
        return self._on_press

    @on_press.setter
    def on_press(self, handler: toga.widgets.button.OnPressHandler) -> None:
        self._on_press = wrapped_handler(self, handler)

To be simplified to this:

on_press = EventProperty("The handler to invoke when the button is pressed.")

The only thing this omits is the handler protocol type. But as discussed in the links below, these didn't actually give the type-checking benefits we’d hoped for, because we couldn't find any clean way of making them accept both a regular and an async callable. What's more, the protocol classes add a huge amount of useless noise to the documentation – see Slider for a particularly bad example.

So I'd like to suggest the following simplifications:

  • Remove all the protocol classes. In the few cases where their docstring contains some information that isn't covered elsewhere, move it to the associated property. Or if the protocol is shared by multiple properties, put the details in a documentation section and link all the properties to it.

  • Switch to the EventProperty syntax as shown above.

  • Implement EventProperty such that, for type-checking and documentation purposes, all event properties would have a type called Handler. I'm not sure whether Handler should be a Protocol, TypeAlias, TypeVar, or something else, but it should:

    • Have a documentation section which explains the general rules about event handlers, which we're currently working on in #3170.
    • If possible, tell type-checkers that the property accepts both a regular and an async callable, with a single positional argument, and any keyword arguments.

Previous discussion of this subject:

@mhsmith mhsmith added the enhancement New features, or improvements to existing features. label Feb 11, 2025
@freakboy3742
Copy link
Member

Agreed that there's an opportunity to simplify repeated code here. You're not the first person to suggest it, either - see #797, and a draft implementation with #804.

I'm not 100% sure we can replace the protocol classes though. The protocols have 2 purposes - firstly documentation; and secondly, type annotation. A generic Handler(*args, **kwargs) definition won't provide a meaningful solution to either of those problems.

There's also the meta question of how Sphinx will handle events defined in this way. We clearly need to document the existence of on_X on a Widget; it's not clear to me how a class-level definition will render this in a way that makes sense. That said, it would give us an opportunity to clearly differentiate events into their own documentation block, as long as we can get Sphinx to play ball.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New features, or improvements to existing features.
Projects
None yet
Development

No branches or pull requests

2 participants