Merge branch 'main' into ansi-fix

Author: willmcgugan

CHANGELOG.md

@@ -10,6 +10,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 ### Fixed
 
 - Fixed issue with ANSI colors not being converted to truecolor https://github.com/Textualize/textual/pull/4138
+- Fixed duplicate watch methods being attached to DOM nodes https://github.com/Textualize/textual/pull/4030
+- Fixed using `watch` to create additional watchers would trigger other watch methods https://github.com/Textualize/textual/issues/3878
 
 ## [0.49.0] - 2024-02-07
 

src/textual/_types.py

@@ -31,12 +31,24 @@ class UnusedParameter:
 """Type used for arbitrary callables used in callbacks."""
 IgnoreReturnCallbackType = Union[Callable[[], Awaitable[Any]], Callable[[], Any]]
 """A callback which ignores the return type."""
-WatchCallbackType = Union[
-    Callable[[], Awaitable[None]],
-    Callable[[Any], Awaitable[None]],
+WatchCallbackBothValuesType = Union[
     Callable[[Any, Any], Awaitable[None]],
-    Callable[[], None],
-    Callable[[Any], None],
     Callable[[Any, Any], None],
 ]
+"""Type for watch methods that accept the old and new values of reactive objects."""
+WatchCallbackNewValueType = Union[
+    Callable[[Any], Awaitable[None]],
+    Callable[[Any], None],
+]
+"""Type for watch methods that accept only the new value of reactive objects."""
+WatchCallbackNoArgsType = Union[
+    Callable[[], Awaitable[None]],
+    Callable[[], None],
+]
+"""Type for watch methods that do not require the explicit value of the reactive."""
+WatchCallbackType = Union[
+    WatchCallbackBothValuesType,
+    WatchCallbackNewValueType,
+    WatchCallbackNoArgsType,
+]
 """Type used for callbacks passed to the `watch` method of widgets."""

src/textual/reactive.py

@@ -16,14 +16,21 @@
     Generic,
     Type,
     TypeVar,
+    cast,
     overload,
 )
 
 import rich.repr
 
 from . import events
 from ._callback import count_parameters
-from ._types import MessageTarget, WatchCallbackType
+from ._types import (
+    MessageTarget,
+    WatchCallbackBothValuesType,
+    WatchCallbackNewValueType,
+    WatchCallbackNoArgsType,
+    WatchCallbackType,
+)
 
 if TYPE_CHECKING:
     from .dom import DOMNode
@@ -42,6 +49,43 @@ class TooManyComputesError(ReactiveError):
     """Raised when an attribute has public and private compute methods."""
 
 
+async def await_watcher(obj: Reactable, awaitable: Awaitable[object]) -> None:
+    """Coroutine to await an awaitable returned from a watcher"""
+    _rich_traceback_omit = True
+    await awaitable
+    # Watcher may have changed the state, so run compute again
+    obj.post_message(events.Callback(callback=partial(Reactive._compute, obj)))
+
+
+def invoke_watcher(
+    watcher_object: Reactable,
+    watch_function: WatchCallbackType,
+    old_value: object,
+    value: object,
+) -> None:
+    """Invoke a watch function.
+
+    Args:
+        watcher_object: The object watching for the changes.
+        watch_function: A watch function, which may be sync or async.
+        old_value: The old value of the attribute.
+        value: The new value of the attribute.
+    """
+    _rich_traceback_omit = True
+    param_count = count_parameters(watch_function)
+    if param_count == 2:
+        watch_result = cast(WatchCallbackBothValuesType, watch_function)(
+            old_value, value
+        )
+    elif param_count == 1:
+        watch_result = cast(WatchCallbackNewValueType, watch_function)(value)
+    else:
+        watch_result = cast(WatchCallbackNoArgsType, watch_function)()
+    if isawaitable(watch_result):
+        # Result is awaitable, so we need to await it within an async context
+        watcher_object.call_next(partial(await_watcher, watcher_object, watch_result))
+
+
 @rich.repr.auto
 class Reactive(Generic[ReactiveType]):
     """Reactive descriptor.
@@ -239,7 +283,7 @@ def __set__(self, obj: Reactable, value: ReactiveType) -> None:
                 obj.refresh(repaint=self._repaint, layout=self._layout)
 
     @classmethod
-    def _check_watchers(cls, obj: Reactable, name: str, old_value: Any):
+    def _check_watchers(cls, obj: Reactable, name: str, old_value: Any) -> None:
         """Check watchers, and call watch methods / computes
 
         Args:
@@ -252,39 +296,6 @@ def _check_watchers(cls, obj: Reactable, name: str, old_value: Any):
         internal_name = f"_reactive_{name}"
         value = getattr(obj, internal_name)
 
-        async def await_watcher(awaitable: Awaitable) -> None:
-            """Coroutine to await an awaitable returned from a watcher"""
-            _rich_traceback_omit = True
-            await awaitable
-            # Watcher may have changed the state, so run compute again
-            obj.post_message(events.Callback(callback=partial(Reactive._compute, obj)))
-
-        def invoke_watcher(
-            watcher_object: Reactable,
-            watch_function: Callable,
-            old_value: object,
-            value: object,
-        ) -> None:
-            """Invoke a watch function.
-
-            Args:
-                watcher_object: The object watching for the changes.
-                watch_function: A watch function, which may be sync or async.
-                old_value: The old value of the attribute.
-                value: The new value of the attribute.
-            """
-            _rich_traceback_omit = True
-            param_count = count_parameters(watch_function)
-            if param_count == 2:
-                watch_result = watch_function(old_value, value)
-            elif param_count == 1:
-                watch_result = watch_function(value)
-            else:
-                watch_result = watch_function()
-            if isawaitable(watch_result):
-                # Result is awaitable, so we need to await it within an async context
-                watcher_object.call_next(partial(await_watcher, watch_result))
-
         private_watch_function = getattr(obj, f"_watch_{name}", None)
         if callable(private_watch_function):
             invoke_watcher(obj, private_watch_function, old_value, value)
@@ -294,7 +305,7 @@ def invoke_watcher(
             invoke_watcher(obj, public_watch_function, old_value, value)
 
         # Process "global" watchers
-        watchers: list[tuple[Reactable, Callable]]
+        watchers: list[tuple[Reactable, WatchCallbackType]]
         watchers = getattr(obj, "__watchers", {}).get(name, [])
         # Remove any watchers for reactables that have since closed
         if watchers:
@@ -404,11 +415,13 @@ def _watch(
     """
     if not hasattr(obj, "__watchers"):
         setattr(obj, "__watchers", {})
-    watchers: dict[str, list[tuple[Reactable, Callable]]] = getattr(obj, "__watchers")
+    watchers: dict[str, list[tuple[Reactable, WatchCallbackType]]] = getattr(
+        obj, "__watchers"
+    )
     watcher_list = watchers.setdefault(attribute_name, [])
-    if callback in watcher_list:
+    if any(callback == callback_from_list for _, callback_from_list in watcher_list):
         return
-    watcher_list.append((node, callback))
     if init:
         current_value = getattr(obj, attribute_name, None)
-        Reactive._check_watchers(obj, attribute_name, current_value)
+        invoke_watcher(obj, callback, current_value, current_value)
+    watcher_list.append((node, callback))

tests/test_reactive.py

@@ -598,3 +598,110 @@ def compose(self) -> ComposeResult:
     app = MyApp()
     async with app.run_test():
         assert app.query_one(MyWidget).foo == "foobar"
+
+
+async def test_no_duplicate_external_watchers() -> None:
+    """Make sure we skip duplicated watchers."""
+
+    counter = 0
+
+    class Holder(Widget):
+        attr = var(None)
+
+    class MyApp(App[None]):
+        def __init__(self) -> None:
+            super().__init__()
+            self.holder = Holder()
+
+        def on_mount(self) -> None:
+            self.watch(self.holder, "attr", self.callback)
+            self.watch(self.holder, "attr", self.callback)
+
+        def callback(self) -> None:
+            nonlocal counter
+            counter += 1
+
+    app = MyApp()
+    async with app.run_test():
+        assert counter == 1
+        app.holder.attr = 73
+        assert counter == 2
+
+
+async def test_external_watch_init_does_not_propagate() -> None:
+    """Regression test for https://github.com/Textualize/textual/issues/3878.
+
+    Make sure that when setting an extra watcher programmatically and `init` is set,
+    we init only the new watcher and not the other ones, but at the same
+    time make sure both watchers work in regular circumstances.
+    """
+
+    logs: list[str] = []
+
+    class SomeWidget(Widget):
+        test_1: var[int] = var(0)
+        test_2: var[int] = var(0, init=False)
+
+        def watch_test_1(self) -> None:
+            logs.append("test_1")
+
+        def watch_test_2(self) -> None:
+            logs.append("test_2")
+
+    class InitOverrideApp(App[None]):
+        def compose(self) -> ComposeResult:
+            yield SomeWidget()
+
+        def on_mount(self) -> None:
+            def watch_test_2_extra() -> None:
+                logs.append("test_2_extra")
+
+            self.watch(self.query_one(SomeWidget), "test_2", watch_test_2_extra)
+
+    app = InitOverrideApp()
+    async with app.run_test():
+        assert logs == ["test_1", "test_2_extra"]
+        app.query_one(SomeWidget).test_2 = 73
+        assert logs.count("test_2_extra") == 2
+        assert logs.count("test_2") == 1
+
+
+async def test_external_watch_init_does_not_propagate_to_externals() -> None:
+    """Regression test for https://github.com/Textualize/textual/issues/3878.
+
+    Make sure that when setting an extra watcher programmatically and `init` is set,
+    we init only the new watcher and not the other ones (even if they were
+    added dynamically with `watch`), but at the same time make sure all watchers
+    work in regular circumstances.
+    """
+
+    logs: list[str] = []
+
+    class SomeWidget(Widget):
+        test_var: var[int] = var(0)
+
+    class MyApp(App[None]):
+        def compose(self) -> ComposeResult:
+            yield SomeWidget()
+
+        def add_first_watcher(self) -> None:
+            def first_callback() -> None:
+                logs.append("first")
+
+            self.watch(self.query_one(SomeWidget), "test_var", first_callback)
+
+        def add_second_watcher(self) -> None:
+            def second_callback() -> None:
+                logs.append("second")
+
+            self.watch(self.query_one(SomeWidget), "test_var", second_callback)
+
+    app = MyApp()
+    async with app.run_test():
+        assert logs == []
+        app.add_first_watcher()
+        assert logs == ["first"]
+        app.add_second_watcher()
+        assert logs == ["first", "second"]
+        app.query_one(SomeWidget).test_var = 73
+        assert logs == ["first", "second", "first", "second"]