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

compute_* methods don't run unless the changed reactive has a watch_* method #1274

Closed
JoshKarpel opened this issue Nov 24, 2022 · 7 comments
Closed
Assignees
Labels
bug Something isn't working

Comments

@JoshKarpel
Copy link
Contributor

JoshKarpel commented Nov 24, 2022

It looks like compute_* methods aren't called unless there's a watch_* method for the attribute that is actually changing, i.e., this statement in the docs from https://textual.textualize.io/guide/reactivity/#compute-methods is wrong:

and update them when any other reactive attribute changes.

I noticed this in my own app but you can see it in the computed01.py example in the docs linked above. If you try it as-is, the background will never update because compute_color is never called. Adding a watch_* method for one of the three colors makes it update when that color is changed.

For example, make this modification:

from textual.app import App, ComposeResult
from textual.color import Color
from textual.containers import Horizontal
from textual.reactive import reactive
from textual.widgets import Input, Static


class ComputedApp(App):
    CSS_PATH = "computed01.css"

    red = reactive(0)
    green = reactive(0)
    blue = reactive(0)
    color = reactive(Color.parse("transparent"))

    def compose(self) -> ComposeResult:
        yield Horizontal(
            Input("0", placeholder="Enter red 0-255", id="red"),
            Input("0", placeholder="Enter green 0-255", id="green"),
            Input("0", placeholder="Enter blue 0-255", id="blue"),
            id="color-inputs",
        )
        yield Static(id="color")

    def watch_blue(self, blue: int) -> None:  # this is new!
        pass # you don't need to do anything here, there just needs to be a watch method to call to trigger the compute

    def compute_color(self) -> Color:  
        return Color(self.red, self.green, self.blue).clamped

    def watch_color(self, color: Color) -> None:  # 
        self.query_one("#color").styles.background = color

    def on_input_changed(self, event: Input.Changed) -> None:
        try:
            component = int(event.value)
        except ValueError:
            self.bell()
        else:
            if event.input.id == "red":
                self.red = component
            elif event.input.id == "green":
                self.green = component
            else:
                self.blue = component


if __name__ == "__main__":
    app = ComputedApp()
    app.run()

Then the background will actually update, but only when the blue input changes.

I suspect the problem is in this chunk of logic which decides when to run computes, where it seems like the decision is only based on whether any watchers are running synchronously, not on whether the reactive attribute value changed:

# Compute is only required if a watcher runs immediately, not if they were posted.
require_compute = False
watch_function = getattr(obj, f"watch_{name}", None)
if callable(watch_function):
require_compute = require_compute or invoke_watcher(
watch_function, old_value, value
)
watchers: list[Callable] = getattr(obj, "__watchers", {}).get(name, [])
for watcher in watchers:
require_compute = require_compute or invoke_watcher(
watcher, old_value, value
)
if require_compute:
# Run computes
obj.post_message_no_wait(
events.Callback(sender=obj, callback=partial(Reactive._compute, obj))
)

Potentially related to some issues @darrenburns filed recently around compute methods: #1218 #1227

@davep davep added the bug Something isn't working label Nov 24, 2022
@JoshKarpel
Copy link
Contributor Author

JoshKarpel commented Nov 26, 2022

I just tested Textual v0.5.0 and realized there's another issue with the require_compute code (which is new as of #1145, so it isn't in v0.4.0) - because of the short-circuiting in require_compute = require_compute or invoke_watcher(...), invoke_watcher() isn't called if require_compute is already True, so watchers added via e.g. textual.reactive.watch are skipped:

watchers: list[Callable] = getattr(obj, "__watchers", {}).get(name, [])
for watcher in watchers:
require_compute = require_compute or invoke_watcher(
watcher, old_value, value
)

@jayaddison
Copy link

Further investigation required for this, but 395616f could be relevant; with that commit reverted, the computed01.py demo application behaviour matches the documentation.

Note: I haven't yet checked whether that affects the linked issues.

@JoshKarpel
Copy link
Contributor Author

@willmcgugan would you mind clarifying the intended behavior? I'd be happy to put up a PR to get things into the desired state (changing either the docs or the code as needed).

I imagine 395616f was intended to optimize compute_* methods, but it seems like it has broken the behavior described in the docs, since they aren't always run when any reactive attributes change - they would only be run if there is an explicitly-defined watch_* method for the attribute that is changing.

Either way, I think I'll need to swap the or ordering to fix the short-circuiting problem I noted in #1274 (comment) .

@willmcgugan
Copy link
Collaborator

We're tracking the issue here #1227 But we may go with some changes to the behaviour.

Compute methods should be called regardless if there is a corresponding watch method.

Compute methods look like properties, but weren't intended to be so. The idea was that you could watch computed properties and get notified when something changes.

In retrospect, perhaps they should additionally be called on demand. So they behave more like properties.

@JoshKarpel
Copy link
Contributor Author

Gotcha, I'll follow along on #1227 ! Thanks for the update.

@rodrigogiraoserrao
Copy link
Contributor

I'm closing this as the original code already works without having to add a dummy watch_x method.

@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
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants