Textualize / textual

The lean application framework for Python. Build sophisticated user interfaces with a simple Python API. Run your apps in the terminal and a web browser.
https://textual.textualize.io/
MIT License
25.46k stars 782 forks source link

`@on` decorator matches widget subclass unexpectedly #4968

Open darrenburns opened 1 month ago

darrenburns commented 1 month ago

The code below fires a notification when you click the button.

from textual import on
from textual.app import App, ComposeResult
from textual.widgets import Button

class MyButton(Button):
    pass

class MyApp(App[None]):
    def compose(self) -> ComposeResult:
        yield Button("Normal Button")

    @on(MyButton.Pressed)
    def notify_on_press_my_button(self) -> None:
        self.notify("MyButton.Pressed matched!")

if __name__ == "__main__":
    MyApp().run()

On clicking a Button, I would not expect a handler decorated with @on(MyButton.Pressed) to fire.

If I click a MyButton, I'd expect a @on(Button.Pressed) handler to fire, and I've confirmed this works as I expect.

darrenburns commented 1 week ago

@willmcgugan this has caught me so many times now. It's a huge gotcha. @davidfokkema mentioned it's caught him out too on Discord. If you use @on with a subclass, I think it's almost always going to lead to a bug unless you use selector, as otherwise it's going to fire if you have any instances of the parent class mounted.

Just been bit by it again in my theme demo app - an @on(ThemeList.OptionHighlighted) handler is firing when a totally unrelated OptionList is used. (ThemeList is a subclass of OptionList).

willmcgugan commented 1 week ago

@darrenburns The current behavior fits my understanding, but I am not apposed to changing it.

If you subclass Button, you don't also subclass Button.Pressed

>>> from textual.widgets import Button
>>> class MyButton(Button):
...     pass
...
>>> MyButton.Pressed is Button.Pressed
True

So as far as the on decorator is concerned, they are the same event.

Feel free to propose a solution...

darrenburns commented 1 week ago

Yeah, I understand the mechanics, but if the identity check is is how the matching is happening, then I reckon we can do better. It's just such a huge footgun and so unintuitive that I don't think it could be considered anything other than a bug.

If I create a subclass like class DeleteAllFilesOnMyHardDriveButton(Button) with a handler like @on(DeleteAllFilesOnMyHardDriveButton.Pressed), I think it's reasonable to expect that would only fire when I click that specific type of button and not the "back up all my files safely" button on the other side of the screen. (Example intended to frighten 😂)

I have some ideas for how we can fix it but will wait until the theme stuff is out of the way.