Merge pull request #5079 from Textualize/loading-redux

willmcgugan · web-flow · commit d6e2718487b4 · 2024-10-02T17:26:54.000+01:00
new loading indicator method
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,13 +10,18 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 ### Fixed
 
 - Fixed issue with screen not updating when auto_refresh was enabled https://github.com/Textualize/textual/pull/5063
+- Fixed issues regarding loading indicator https://github.com/Textualize/textual/pull/5079
 
 ### Added
 
 - Added `DOMNode.is_on_screen` property https://github.com/Textualize/textual/pull/5063
 - Added support for keymaps (user configurable key bindings) https://github.com/Textualize/textual/pull/5038
 - Added descriptions to bindings for all internal widgets, and updated casing to be consistent https://github.com/Textualize/textual/pull/5062
 
+### Changed
+
+- Breaking change: `Widget.set_loading` no longer return an awaitable https://github.com/Textualize/textual/pull/5079
+
 ## [0.81.0] - 2024-09-25
 
 ### Added
diff --git a/src/textual/_compositor.py b/src/textual/_compositor.py
@@ -664,6 +664,17 @@ def add_widget(
 
                     get_layer_index = layers_to_index.get
 
+                    if widget._cover_widget is not None:
+                        map[widget._cover_widget] = _MapGeometry(
+                            region.shrink(widget.styles.gutter),
+                            order,
+                            clip,
+                            region.size,
+                            container_size,
+                            virtual_region,
+                            dock_gutter,
+                        )
+
                     # Add all the widgets
                     for sub_region, _, sub_widget, z, fixed, overlay in reversed(
                         placements
@@ -681,18 +692,17 @@ def add_widget(
                             widget_region = self._constrain(
                                 sub_widget.styles, widget_region, no_clip
                             )
-
-                        add_widget(
-                            sub_widget,
-                            sub_region,
-                            widget_region,
-                            ((1, 0, 0),) if overlay else widget_order,
-                            layer_order,
-                            no_clip if overlay else sub_clip,
-                            visible,
-                            arrange_result.scroll_spacing,
-                        )
-
+                        if widget._cover_widget is None:
+                            add_widget(
+                                sub_widget,
+                                sub_region,
+                                widget_region,
+                                ((1, 0, 0),) if overlay else widget_order,
+                                layer_order,
+                                no_clip if overlay else sub_clip,
+                                visible,
+                                arrange_result.scroll_spacing,
+                            )
                         layer_order -= 1
 
                 if visible:
@@ -737,7 +747,7 @@ def add_widget(
                 if styles.constrain != "none":
                     widget_region = self._constrain(styles, widget_region, no_clip)
 
-                map[widget] = _MapGeometry(
+                map[widget._render_widget] = _MapGeometry(
                     widget_region,
                     order,
                     clip,
diff --git a/src/textual/widget.py b/src/textual/widget.py
@@ -14,7 +14,6 @@
 from typing import (
     TYPE_CHECKING,
     AsyncGenerator,
-    Awaitable,
     ClassVar,
     Collection,
     Generator,
@@ -58,7 +57,6 @@
 from textual._styles_cache import StylesCache
 from textual._types import AnimationLevel
 from textual.actions import SkipAction
-from textual.await_complete import AwaitComplete
 from textual.await_remove import AwaitRemove
 from textual.box_model import BoxModel
 from textual.cache import FIFOCache
@@ -333,6 +331,38 @@ class Widget(DOMNode):
     loading: Reactive[bool] = Reactive(False)
     """If set to `True` this widget will temporarily be replaced with a loading indicator."""
 
+    virtual_size: Reactive[Size] = Reactive(Size(0, 0), layout=True)
+    """The virtual (scrollable) [size][textual.geometry.Size] of the widget."""
+
+    has_focus: Reactive[bool] = Reactive(False, repaint=False)
+    """Does this widget have focus? Read only."""
+
+    mouse_hover: Reactive[bool] = Reactive(False, repaint=False)
+    """Is the mouse over this widget? Read only."""
+
+    scroll_x: Reactive[float] = Reactive(0.0, repaint=False, layout=False)
+    """The scroll position on the X axis."""
+
+    scroll_y: Reactive[float] = Reactive(0.0, repaint=False, layout=False)
+    """The scroll position on the Y axis."""
+
+    scroll_target_x = Reactive(0.0, repaint=False)
+    """Scroll target destination, X coord."""
+
+    scroll_target_y = Reactive(0.0, repaint=False)
+    """Scroll target destination, Y coord."""
+
+    show_vertical_scrollbar: Reactive[bool] = Reactive(False, layout=True)
+    """Show a vertical scrollbar?"""
+
+    show_horizontal_scrollbar: Reactive[bool] = Reactive(False, layout=True)
+    """Show a horizontal scrollbar?"""
+
+    border_title: str | Text | None = _BorderTitle()  # type: ignore
+    """A title to show in the top border (if there is one)."""
+    border_subtitle: str | Text | None = _BorderTitle()  # type: ignore
+    """A title to show in the bottom border (if there is one)."""
+
     # Default sort order, incremented by constructor
     _sort_order: ClassVar[int] = 0
 
@@ -430,38 +460,8 @@ def __init__(
         """An anchored child widget, or `None` if no child is anchored."""
         self._anchor_animate: bool = False
         """Flag to enable animation when scrolling anchored widgets."""
-
-    virtual_size: Reactive[Size] = Reactive(Size(0, 0), layout=True)
-    """The virtual (scrollable) [size][textual.geometry.Size] of the widget."""
-
-    has_focus: Reactive[bool] = Reactive(False, repaint=False)
-    """Does this widget have focus? Read only."""
-
-    mouse_hover: Reactive[bool] = Reactive(False, repaint=False)
-    """Is the mouse over this widget? Read only."""
-
-    scroll_x: Reactive[float] = Reactive(0.0, repaint=False, layout=False)
-    """The scroll position on the X axis."""
-
-    scroll_y: Reactive[float] = Reactive(0.0, repaint=False, layout=False)
-    """The scroll position on the Y axis."""
-
-    scroll_target_x = Reactive(0.0, repaint=False)
-    """Scroll target destination, X coord."""
-
-    scroll_target_y = Reactive(0.0, repaint=False)
-    """Scroll target destination, Y coord."""
-
-    show_vertical_scrollbar: Reactive[bool] = Reactive(False, layout=True)
-    """Show a vertical scrollbar?"""
-
-    show_horizontal_scrollbar: Reactive[bool] = Reactive(False, layout=True)
-    """Show a horizontal scrollbar?"""
-
-    border_title: str | Text | None = _BorderTitle()  # type: ignore
-    """A title to show in the top border (if there is one)."""
-    border_subtitle: str | Text | None = _BorderTitle()  # type: ignore
-    """A title to show in the bottom border (if there is one)."""
+        self._cover_widget: Widget | None = None
+        """Widget to render over this widget (used by loading indicator)."""
 
     @property
     def is_mounted(self) -> bool:
@@ -587,6 +587,33 @@ def is_maximized(self) -> bool:
         except NoScreen:
             return False
 
+    @property
+    def _render_widget(self) -> Widget:
+        """The widget the compositor should render."""
+        # Will return the "cover widget" if one is set, otherwise self.
+        return self._cover_widget if self._cover_widget is not None else self
+
+    def _cover(self, widget: Widget) -> None:
+        """Set a widget used to replace the visuals of this widget (used for loading indicator).
+
+        Args:
+            widget: A newly constructed, but unmounted widget.
+        """
+        self._uncover()
+        self._cover_widget = widget
+        widget._parent = self
+        widget._start_messages()
+        widget._post_register(self.app)
+        self.app.stylesheet.apply(widget)
+        self.refresh(layout=True)
+
+    def _uncover(self) -> None:
+        """Remove any widget, previously set via [`_cover`][textual.widget.Widget._cover]."""
+        if self._cover_widget is not None:
+            self._cover_widget.remove()
+            self._cover_widget = None
+            self.refresh(layout=True)
+
     def anchor(self, *, animate: bool = False) -> None:
         """Anchor the widget, which scrolls it into view (like [scroll_visible][textual.widget.Widget.scroll_visible]),
         but also keeps it in view if the widget's size changes, or the size of its container changes.
@@ -716,7 +743,7 @@ def get_loading_widget(self) -> Widget:
         loading_widget = self.app.get_loading_widget()
         return loading_widget
 
-    def set_loading(self, loading: bool) -> Awaitable:
+    def set_loading(self, loading: bool) -> None:
         """Set or reset the loading state of this widget.
 
         A widget in a loading state will display a LoadingIndicator that obscures the widget.
@@ -728,19 +755,16 @@ def set_loading(self, loading: bool) -> Awaitable:
             An optional awaitable.
         """
         LOADING_INDICATOR_CLASS = "-textual-loading-indicator"
-        LOADING_INDICATOR_QUERY = f".{LOADING_INDICATOR_CLASS}"
-        remove_indicator = self.query_children(LOADING_INDICATOR_QUERY).remove()
         if loading:
             loading_indicator = self.get_loading_widget()
             loading_indicator.add_class(LOADING_INDICATOR_CLASS)
-            await_mount = self.mount(loading_indicator)
-            return AwaitComplete(remove_indicator, await_mount).call_next(self)
+            self._cover(loading_indicator)
         else:
-            return remove_indicator
+            self._uncover()
 
-    async def _watch_loading(self, loading: bool) -> None:
+    def _watch_loading(self, loading: bool) -> None:
         """Called when the 'loading' reactive is changed."""
-        await self.set_loading(loading)
+        self.set_loading(loading)
 
     ExpectType = TypeVar("ExpectType", bound="Widget")
 
@@ -3993,6 +4017,7 @@ def _on_scroll_to_region(self, message: messages.ScrollToRegion) -> None:
         self.scroll_to_region(message.region, animate=True)
 
     def _on_unmount(self) -> None:
+        self._uncover()
         self.workers.cancel_node(self)
 
     def action_scroll_home(self) -> None:
diff --git a/src/textual/widgets/_loading_indicator.py b/src/textual/widgets/_loading_indicator.py
@@ -23,6 +23,7 @@ class LoadingIndicator(Widget):
         min-height: 1;
         content-align: center middle;
         color: $accent;
+        text-style: not reverse;
     }
     LoadingIndicator.-textual-loading-indicator {
         layer: _loading;
diff --git a/tests/animations/test_loading_indicator_animation.py b/tests/animations/test_loading_indicator_animation.py
@@ -4,7 +4,6 @@
 """
 
 from textual.app import App
-from textual.widgets import LoadingIndicator
 
 
 async def test_loading_indicator_is_not_static_on_full() -> None:
@@ -15,7 +14,7 @@ async def test_loading_indicator_is_not_static_on_full() -> None:
     async with app.run_test() as pilot:
         app.screen.loading = True
         await pilot.pause()
-        indicator = app.query_one(LoadingIndicator)
+        indicator = app.screen._cover_widget
         assert str(indicator.render()) != "Loading..."
 
 
@@ -27,7 +26,7 @@ async def test_loading_indicator_is_not_static_on_basic() -> None:
     async with app.run_test() as pilot:
         app.screen.loading = True
         await pilot.pause()
-        indicator = app.query_one(LoadingIndicator)
+        indicator = app.screen._cover_widget
         assert str(indicator.render()) != "Loading..."
 
 
@@ -39,5 +38,5 @@ async def test_loading_indicator_is_static_on_none() -> None:
     async with app.run_test() as pilot:
         app.screen.loading = True
         await pilot.pause()
-        indicator = app.query_one(LoadingIndicator)
+        indicator = app.screen._cover_widget
         assert str(indicator.render()) == "Loading..."
diff --git a/tests/test_widget.py b/tests/test_widget.py
@@ -425,23 +425,23 @@ def compose(self) -> ComposeResult:
         app = pilot.app
         label = app.query_one(Label)
         assert label.loading == False
-        assert len(label.query(LoadingIndicator)) == 0
+        assert label._cover_widget is None
 
         label.loading = True
         await pilot.pause()
-        assert len(label.query(LoadingIndicator)) == 1
+        assert label._cover_widget is not None
 
         label.loading = True  # Setting to same value is a null-op
         await pilot.pause()
-        assert len(label.query(LoadingIndicator)) == 1
+        assert label._cover_widget is not None
 
         label.loading = False
         await pilot.pause()
-        assert len(label.query(LoadingIndicator)) == 0
+        assert label._cover_widget is None
 
         label.loading = False  # Setting to same value is a null-op
         await pilot.pause()
-        assert len(label.query(LoadingIndicator)) == 0
+        assert label._cover_widget is None
 
 
 async def test_is_mounted_property():

Original file line number	Diff line number	Diff line change
`@@ -23,6 +23,7 @@ class LoadingIndicator(Widget):`
`23`	`23`	`min-height: 1;`
`24`	`24`	`content-align: center middle;`
`25`	`25`	`color: $accent;`
	`26`	`+ text-style: not reverse;`
`26`	`27`	`}`
`27`	`28`	`LoadingIndicator.-textual-loading-indicator {`
`28`	`29`	`layer: _loading;`