textual icon indicating copy to clipboard operation
textual copied to clipboard
verified publisher icon Textualize

Ensure all widget messages are posted even when triggered by the developer

Open rodrigogiraoserrao opened this issue 2 years ago • 2 comments

We should not prevent a message from being posted just because the state of the widget was changed programmatically, and even though the programmer already knows the state changed.

E.g., TabbedContent should post TabActivated when the reactive attribute active is assigned to.

See #2500 & #3417 for more examples and context.

rodrigogiraoserrao avatar Dec 14 '23 11:12 rodrigogiraoserrao

There's currently a number of issues related to this, so I thought it might be a good idea to start a list of widgets/messages that might need looking at:

  • [x] TabbedContent.TabActivated - completed in #4159
  • [ ] Select.Changed
  • [ ] TextArea.Changed

TomJGooding avatar Feb 12 '24 21:02 TomJGooding

Presumably a Changed event shouldn't post on initialisation? How would you approach this when init=False isn't an option because there's other updates that need to happen in the watch method?

TomJGooding avatar Feb 15 '24 18:02 TomJGooding

Looking at every widget, working out which are N/A for this and which have properties that can cause events:

  • [x] Button - N/A
  • [x] Checkbox
    • [x] - Changed - Works as expected
  • [x] Collapsible
    • [x] Expanded - Doesn't get posted when toggled in code
    • [x] Collapsed - Doesn't get posted when toggled in code
  • [x] ContentSwitcher - N/A
  • [x] DataTable
    • [x] CellHighlighted - Works as expected
    • [x] CellSelected - N/A
    • [x] RowHighlighted - Works as expected
    • [x] RowSelected - N/A
    • [x] ColumnHighlighted - Works as expected
    • [x] ColumnSelected - N/A
    • [x] HeaderSelected - N/A
    • [x] RowLabelSelected - N/A
  • [x] Digits - N/A
  • [x] DirectoryTree
    • [x] FileSelected - TBD
    • [x] DirectorySelected - TBD
  • [x] Footer - N/A
  • [x] Header - N/A
  • [x] Input
    • [x] Changed - Works as expected
    • [x] Submitted - N/A
  • [x] Label - N/A
  • [x] ListItem - N/A
  • [X] ListView
    • [x] Highlighted - Works as expected
    • [X] Selected - N/A
  • [x] LoadingIndicator - N/A
  • [x] Log - N/A
  • [x] Markdown
    • [X] TableOfContentsUpdated - Doesn't get posted when you update a Markdown.
    • [x] TableOfContentsSelected - N/A
    • [x] LinkClicked - N/A
  • [x] MarkdownViewer - N/A
  • [x] OptionList
    • [x] OptionHighlighted - Works as expected
    • [x] OptionSelected - N/A
  • [x] Placeholder - N/A
  • [x] Pretty - N/A
  • [x] ProgressBar - N/A
  • [x] RadioButton
    • [x] Changed - Works as expected
  • [x] RadioSet
    • [x] Changed - Works as expected
  • [x] RichLog - N/A
  • [x] Rule - N/A
  • [x] Select
    • [x] Changed - Doesn't get posted when value is changed
  • [x] SelectionList
    • [x] SelectionHighlighted - Works as expected.
    • [x] SelectionToggled - Doesn't get posted when a check is toggled from code
    • [x] SelectedChanged - Doesn't get posted when the selected set is changed from code
  • [x] Sparkline - N/A
  • [x] Static - N/A
  • [x] Switch
    • [x] Changed - Works as expected
  • [x] TabbedContent
    • [x] TabActivated - Not bubbled due to wrong sender issue
    • [x] Cleared - Not bubbled due to wrong sender issue
  • [x] TabPane
    • [x] Disabled - N/A
    • [x] Enabled - N/A
  • [x] Tabs
    • [x] TabActivated - Can get stopped from bubbling by Textual.
    • [x] TabDisabled - Works as expected.
    • [x] TabEnabled - Works as expected.
    • [x] TabHidden - Not bubbled due to wrong sender issue
    • [x] TabShown - Not bubbled due to wrong sender issue
    • [x] Cleared - Not bubbled due to wrong sender issue depending on the route to triggering it
  • [x] TextArea
    • [x] Changed - Not sent when text is set or load_text is called.
    • [x] SelectionChanged - Works as expected
  • [x] ToggleButton
    • [x] Changed - Works as expected (albeit not an exposed widget)
  • [x] Tree
    • [x] NodeCollapsed - Not sent when collapse is called on a node.
    • [x] NodeExpanded - Not sent when expand is called on a node.
    • [x] NodeHighlighted - Not bubbled when select_node is called.
    • [x] NodeSelected - N/A

davep avatar Feb 22 '24 11:02 davep

Now whittling things down; here are widgets and their messages that don't post messages when something happens in code that mimics something the user might do:

Collapsible

Neither Collapsible.Expanded nor Collapsible.Collapsed are posted when Collapsible.collapsed is changed.

Markdown

TableOfContentsUpdated should get posted when you update a Markdown, but it doesn't.

Select

The Changed message isn't posted when Select.value is assigned to.

SelectionList

  • SelectionToggled isn't posted when toggle is called.
  • SelectedChanged isn't posted when select, deselect or toggle are called.

TabbedContent

  • TabActivated can be made to not bubble depending on the DOM layout. Fixed by #4222
  • Cleared can be made to not bubble depending on the DOM layout; this likely needs the Message.set_sender fix (note also that in the code some effort was made to handle something akin to this anyway; but it looks like it didn't go far enough -- that effort can likely be made cleaner now).

Tabs

  • TabActivated can be made to not bubble depending on the DOM layout. Fixed by #4222
  • TabHidden gets stopped from bubbling depending on DOM layout, needs Message.set_sender fix.
  • TabShown gets stopped from bubbling depending on DOM layout, needs Message.set_sender fix.
  • Cleared can be made to not bubble when doing active = "" depending on the DOM layout. Fixed by #4222
  • Cleared can be made to not bubble when calling Tabs.clear depending on the DOM layout. Fixed by #4222
  • Cleared can be made to not bubble when using Tabs.hide to hide all tabs depending on the DOM layout. Fixed by #4222

TextArea

  • Changed isn't sent when text is assigned to or when load_text is called.

Tree

  • NodeExpanded isn't posted if a node's expand method is called (this looks to be down to the event in my test app being stopped from bubbling because it looks like it's going down the DOM, not up, due to the active message pump). Needs the Message.set_sender fix.
  • NodeCollapsed isn't posted if a node's collapse method is called (this looks to be down to the event in my test app being stopped from bubbling because it looks like it's going down the DOM, not up, due to the active message pump). Needs the Message.set_sender fix.
  • NodeHighlighted isn't posted/bubbled (need to be sure which once work takes place) when Tree.select_node is called. While working on the fixes I can't actually get this to fail now.

DirectoryTree

It looks like FileSelected and DirectorySelected are masked too, but it's not quite clear how or why and what part of this is down to the issues with Tree or not. I'm making both as "not working" and will review once Tree is all tidy.

davep avatar Feb 26 '24 10:02 davep

Starting to work through the widgets (as documented above), I seemed to run into a problem where, when looking at the "missing" messages from DataTable, it made no sense that they weren't getting posted when updating the cursor_coordinate from code. Reading the code there was no difference between that reactive being changed by code run from bindings, vs that being changed by any other code.

I've since gone on to see the same issue with Input too. Given that Input is more straightforward, I think I can illustrate what I'm seeing with an example built around that.

Consider this code:

from textual import on
from textual.app import App, ComposeResult
from textual.containers import Vertical
from textual.widgets import Button, Input

class Child(Vertical):

    def compose(self) -> ComposeResult:
        yield Button("More input")
        yield Input()

    @on(Button.Pressed)
    def more_input(self) -> None:
        self.query_one(Input).value += "x"

    @on(Input.Changed)
    def note_change(self) -> None:
        self.notify("Changed")

class InputMessageApp(App[None]):

    def compose(self) -> ComposeResult:
        yield Child()

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

If I type into the Input field, I get a notification saying Changed. If I press the button, I get a notification saying Changed.

Now if I change the code so that the event handler that handles the Input.Changed message goes into the App:

from textual import on
from textual.app import App, ComposeResult
from textual.containers import Vertical
from textual.widgets import Button, Input

class Child(Vertical):

    def compose(self) -> ComposeResult:
        yield Button("More input")
        yield Input()

    @on(Button.Pressed)
    def more_input(self) -> None:
        self.query_one(Input).value += "x"

class InputMessageApp(App[None]):

    def compose(self) -> ComposeResult:
        yield Child()

    @on(Input.Changed)
    def note_change(self) -> None:
        self.notify("Changed")

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

things work differently. If I type into the Input I get a notification saying Changed. However, if I press the button I get no notification at all; but the content (the value) of the Input does change.

I did start to wonder if this was an "defined on app" vs "defined on screen" thing, but that doesn't seem to make a difference:

from textual import on
from textual.app import App, ComposeResult
from textual.containers import Vertical
from textual.screen import Screen
from textual.widgets import Button, Input

class Child(Vertical):

    def compose(self) -> ComposeResult:
        yield Button("More input")
        yield Input()

    @on(Button.Pressed)
    def more_input(self) -> None:
        self.query_one(Input).value += "x"

class Main(Screen):

    def compose(self) -> ComposeResult:
        yield Child()

    @on(Input.Changed)
    def note_change(self) -> None:
        self.notify("Changed")

class InputMessageApp(App[None]):

    def on_mount(self) -> None:
        self.push_screen(Main())

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

This too has the message handled when directly typed, but not handled when the button is pressed.

This feels like a bug; I'm finding it unexpected (or I'm missing the obvious here). It does explain why my test application is showing that (for example) DataTable isn't sending messages when really it should be (according to my reading of the code).

davep avatar Feb 26 '24 14:02 davep

The difference above seems to be Message._stop_propagation:

from textual import on
from textual.app import App, ComposeResult
from textual.containers import Vertical
from textual.widgets import Button, Input

class Child(Vertical):

    def compose(self) -> ComposeResult:
        yield Button("More input")
        yield Input()

    @on(Button.Pressed)
    def more_input(self) -> None:
        self.query_one(Input).value += "x"

    @on(Input.Changed)
    def note_change(self, event: Input.Changed) -> None:
        self.notify(f"Input changed; stop propagation: {event._stop_propagation}")

class InputMessageApp(App[None]):

    def compose(self) -> ComposeResult:
        yield Child()

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

If the button is pressed, Message._stop_propagation is True; if the Input is typed into, Message._stop_propagation is False.

davep avatar Feb 26 '24 15:02 davep

Taking it a little further, the Message._sender differs depending on how the Input.value is changed:

from textual import on
from textual.app import App, ComposeResult
from textual.containers import Vertical
from textual.widgets import Button, Input

class Child(Vertical):

    def compose(self) -> ComposeResult:
        yield Button("More input")
        yield Input()

    @on(Button.Pressed)
    def more_input(self) -> None:
        self.query_one(Input).value += "x"

    @on(Input.Changed)
    def note_change(self, event: Input.Changed) -> None:
        self.notify(
            f"Input changed; stop propagation: {event._stop_propagation}; "
            f"Sender: {event._sender}"
        )

class InputMessageApp(App[None]):

    def compose(self) -> ComposeResult:
        yield Child()

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

When the Input is typed into, the Message._sender is the Input; when the Button is pressed the Message._sender is Child. This difference is likely playing into the code that decides if bubbling should happen.

https://github.com/Textualize/textual/blob/7327987a180b41547a7af688b81f012c43a3538c/src/textual/message_pump.py#L715-L721

davep avatar Feb 26 '24 15:02 davep

(I should probably add here that this isn't a direct "cause" of what this issue is about, but it does illustrate what seems to be an edge case where, even if we make all of the required changes so that messages aren't prevented; it could still appear that they are)

davep avatar Feb 26 '24 15:02 davep

More or less there now with testing as many widgets and messages as possible. The one thing remaining is all things Tab, Tabs and TabbedContent related; going to move onto this last. This got left aside as the "sandbox" I made for testing used TabbedContent for easy switching between each of the sandboxen and... there's a message issue or two kicking about when Tabs and/or TabbedContent are within TabbedContent. So I'll be testing that standalone.

davep avatar Feb 29 '24 11:02 davep

Although roughly mentioned above; I think this is worth a highlight as it's a subtle difference that will need to be taken into account when building widgets and also when constructing tests for messages. This code will produce a Tabs.TabsActivated message when Tabs.active is modified in code:

from textual.app import App, ComposeResult
from textual.widgets import Tabs

class GoodTabsMessageApp(App[None]):

    def compose(self) -> ComposeResult:
        yield Tabs("1", "2", "3", "4")

    def on_mount(self) -> None:
        self.query_one(Tabs).active = "tab-3"

    def on_tabs_tab_activated(self, event: Tabs.TabActivated) -> None:
        self.notify(f"{event!r}")

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

In fact the notification fires twice: once for the activation of tab-1, and again for the activation of tab-3. It's the latter we're most interested in.

This code, however, makes it appear that the message isn't being sent when Tabs.active is changed:

from textual.app import App, ComposeResult
from textual.containers import Container
from textual.widgets import Tabs

class TabsContainer(Container):

    def compose(self) -> ComposeResult:
        yield Tabs("1", "2", "3", "4")

    def on_mount(self) -> None:
        self.query_one(Tabs).active = "tab-3"

class BadTabsMessageApp(App[None]):

    def compose(self) -> ComposeResult:
        yield TabsContainer()

    def on_tabs_tab_activated(self, event: Tabs.TabActivated) -> None:
        self.notify(f"{event!r}")

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

We get a notification for tab-1, but not when we set it to tab-3.

In this specific case (where it's down to a reactive) this is fixed in main by #4222; in other cases (see mentions above) it will need correction by the use of Message.set_sender in the widget posting the message to ensure that the correct sender is recorded.

As I say, I mention this here as the use of the container is important here to how things bubble, or get stopped from bubblign if they appear to be heading "down" the DOM rather than "up".

davep avatar Feb 29 '24 12:02 davep

As best as I can tell that's every relevant combination of widget and message tested, in as many scenarios as I can come up with. Next job come Monday is to write a suite of tests that cover these issues (albeit some of them will be non-issues already now thanks to #4222 already been in main) and then make the relevant fix. We seem to have a split now between code that just needs to do a set_sender on the message to ensure we don't get into a fight with the code that ensures messages bubble up and not down, and code that just simply isn't posting a message in a particular circumstance.

All look fairly straightforward to resolve.

davep avatar Feb 29 '24 17:02 davep

Good work Dave! I'll be interested to see how you approach the Changed messages as I wasn't sure how to handle these when I had a quick attempt at this.

TomJGooding avatar Feb 29 '24 18:02 TomJGooding

For tracking what needs tests and fixes, etc:

  • Collapsible
    • Expanded
      • [x] Tests
      • [x] Fix
    • Collapsed
      • [x] Tests
      • [x] Fix
  • Markdown
    • TableOfContentsUpdated
      • [x] Tests
      • [x] Fix
  • Select
    • Changed
      • [x] Tests
      • [x] Fix
  • SelectionList
    • SelectionToggled
      • [x] Tests
      • [x] Fix
    • SelectedChanged
      • [x] Tests
      • [x] Fix
  • TabbedContent
    • Cleared
      • [x] Tests
      • [x] Fix
  • Tabs
    • TabHidden
      • [ ] Tests
      • [x] Fix
    • TabShown
      • [ ] Tests
      • [x] Fix
  • TextArea
    • Changed
      • [x] Tests
      • [x] Fix
  • Tree
    • NodeExpanded
      • [x] Tests
      • [x] Fix
    • NodeCollapsed
      • [x] Tests
      • [x] Fix
  • DirectoryTree
    • [x] Final checks after Tree is fixed

davep avatar Mar 04 '24 10:03 davep

Don't forget to star the repository!

Follow @textualizeio for Textual updates.

github-actions[bot] avatar Mar 07 '24 14:03 github-actions[bot]