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

self.watch ignores reactive updates when callback is async #3036

Closed
mzebrak opened this issue Jul 31, 2023 · 5 comments · Fixed by #3065
Closed

self.watch ignores reactive updates when callback is async #3036

mzebrak opened this issue Jul 31, 2023 · 5 comments · Fixed by #3065
Assignees

Comments

@mzebrak
Copy link

mzebrak commented Jul 31, 2023

Issue description

Consider such a snippet:

from textual.app import App, ComposeResult
from textual.binding import Binding
from textual.reactive import var
from textual.widget import Widget
from textual.widgets import Footer, Placeholder


class ReactiveHolder(Widget):
    trigger = var(False)


class WatchApp(App):
    BINDINGS = [
        Binding("f2", "update", "Update"),
    ]

    counter = 0

    def __init__(self):
        super().__init__()
        self.reactive_holder = ReactiveHolder()

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

    def on_mount(self) -> None:
        self.watch(self.reactive_holder, "trigger", self.__callback)

    def action_update(self) -> None:
        self.__class__.counter += 1
        self.reactive_holder.trigger = not self.reactive_holder.trigger

    async def __callback(self) -> None:
        await self.query(Placeholder).remove()
        await self.mount(Placeholder(f"Callback called {self.counter} times"))


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

While running it - after pressing F2 nothing happens, when Placeholder should be mounted. This is because self.watch cannot call a callback if it is:

  1. asynchronous AND 2. reactive is in a different object than this application.

Because either:

removing the async and await keywords makes this code to run just fine.

from textual.app import App, ComposeResult
from textual.binding import Binding
from textual.reactive import var
from textual.widget import Widget
from textual.widgets import Footer, Placeholder


class ReactiveHolder(Widget):
    trigger = var(False)


class WatchApp(App):
    BINDINGS = [
        Binding("f2", "update", "Update"),
    ]

    counter = 0

    def __init__(self):
        super().__init__()
        self.reactive_holder = ReactiveHolder()

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

    def on_mount(self) -> None:
        self.watch(self.reactive_holder, "trigger", self.__callback)

    def action_update(self) -> None:
        self.__class__.counter += 1
        self.reactive_holder.trigger = not self.reactive_holder.trigger

    def __callback(self) -> None:
        self.query(Placeholder).remove()
        self.mount(Placeholder(f"Callback called {self.counter} times"))


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

also placing the reactive in App makes this run just fine

from textual.app import App, ComposeResult
from textual.binding import Binding
from textual.reactive import var
from textual.widgets import Footer, Placeholder


class WatchApp(App):
    BINDINGS = [
        Binding("f2", "update", "Update"),
    ]

    trigger = var(False)

    counter = 0

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

    def on_mount(self) -> None:
        self.watch(self, "trigger", self.__callback)

    def action_update(self) -> None:
        self.__class__.counter += 1
        self.trigger = not self.trigger

    async def __callback(self) -> None:
        await self.query(Placeholder).remove()
        await self.mount(Placeholder(f"Callback called {self.counter} times"))


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

Textual Diagnostics

Versions

Name Value
Textual 0.30.0
Rich 13.5.1

Python

Name Value
Version 3.10.6
Implementation CPython
Compiler GCC 11.3.0
Executable /home/mzebrak/.pyenv/versions/3.10.6/envs/textual-tests/bin/python3.10

Operating System

Name Value
System Linux
Release 6.2.6-76060206-generic
Version #202303130630167942497222.04~4a8cde1 SMP PREEMPT_DYNAMIC Tue M

Terminal

Name Value
Terminal Application Unknown
TERM xterm-256color
COLORTERM truecolor
FORCE_COLOR Not set
NO_COLOR Not set

Rich Console options

Name Value
size width=198, height=43
legacy_windows False
min_width 1
max_width 198
is_terminal True
encoding utf-8
max_height 43
justify None
overflow None
no_wrap False
highlight None
markup None
height None
@github-actions
Copy link

We found the following entry in the FAQ which you may find helpful:

Feel free to close this issue if you found an answer in the FAQ. Otherwise, please give us a little time to review.

This is an automated reply, generated by FAQtory

@mzebrak mzebrak changed the title self.watch ignores reactive updates self.watch ignores reactive updates when callback is async Jul 31, 2023
@rodrigogiraoserrao
Copy link
Contributor

This looks vaguely related to an issue I found when implementing the progress bar where I also couldn't watch changes on a reactive outside of the progress bar widget. Sadly, I can't find an open issue about that; maybe I didn't file any.

I'll take a look at this.

@rodrigogiraoserrao
Copy link
Contributor

rodrigogiraoserrao commented Aug 7, 2023

Alright, I took a look and what I found is that arguably V1 is working as expected and V2 is the buggy one.

V1 "doesn't work" because you created a reactive attribute in a widget (ReactiveHolder) that is never mounted, so it's like the widget wasn't "turned ON", which means that Textual isn't "running" the widget and its message queue isn't being processed.
As you press F2 repeatedly, Textual wants to call the callback and puts it in the message queue of the reactive_holder attribute of the app, but because the widget isn't running, the message queue doesn't get processed.

E.g., if you add CSS to the ReactiveHolder so that it doesn't take any space and if you yield it inside the method compose of the app, then V1 will work:

Modified example
from textual.app import App, ComposeResult
from textual.binding import Binding
from textual.reactive import var
from textual.widget import Widget
from textual.widgets import Footer, Placeholder


class ReactiveHolder(Widget):
    ### NEW: Added `DEFAULT_CSS` here:
    DEFAULT_CSS = """ReactiveHolder { height: 0; width: 0; }"""
    trigger = var(False)


class WatchApp(App):
    BINDINGS = [
        Binding("f2", "update", "Update"),
    ]

    counter = 0

    def __init__(self):
        super().__init__()
        self.reactive_holder = ReactiveHolder()

    def compose(self) -> ComposeResult:
        ### NEW: Yielded the reactive holder here:
        yield self.reactive_holder
        yield Footer()

    def on_mount(self) -> None:
        self.watch(self.reactive_holder, "trigger", self.__callback)

    def action_update(self) -> None:
        self.__class__.counter += 1
        self.reactive_holder.trigger = not self.reactive_holder.trigger

    async def __callback(self) -> None:
        await self.query(Placeholder).remove()
        await self.mount(Placeholder(f"Callback called {self.counter} times"))


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

But if that's the case, then why does V2 work?

That's because, in V2, the callback is a synchronous function and thus Textual calls it right away when the reactive attribute is changed, whereas in V1 it had to schedule the callback (because it is asynchronous).

These are just my findings for the moment.
We'll figure out the best way to proceed from here and we'll update you ASAP.

@rodrigogiraoserrao
Copy link
Contributor

There's an argument to be made in favour of V1 working, too.
Because you're using self.watch inside the app, you are saying that the app is the one watching for changes in the trigger reactive, so the callbacks that are being scheduled into the message queue of the owner of the reactive should probably be scheduled into the message queue of the watcher (in this case, the app).

Maybe this will be "better", for some definition of "better".

rodrigogiraoserrao added a commit that referenced this issue Aug 7, 2023
The async reactive callbacks are now scheduled on the message pump of the watcher of the reactive instead of on the owner of the reactive attribute.
Related issues: #3036.
@rodrigogiraoserrao rodrigogiraoserrao linked a pull request Aug 24, 2023 that will close this issue
@github-actions
Copy link

Don't forget to star the repository!

Follow @textualizeio for Textual updates.

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 a pull request may close this issue.

2 participants