Repository: Textualize/textual
Pull request: Rework `RadioSet` so it no longer leans on the DOM for state
See #2202 and subsequently #2203. While unable to reproduce locally, it does look like there are some environments where it was possible for a `RadioSet` to briefly appear to have two buttons pressed, which in turn would result in `pressed_button` evaluating to `None`.

This PR changes the internals of `RadioSet` so that it no longer uses the DOM to track state, which should remove the issue.

Problem statement:
RadioSet should not query the DOM to get its state.
With the current implementation, there is a brief window where there can be two radio buttons checked. This can lead to the `pressed_button` property returning None.

We should not use the DOM in this instance. Better to keep track of the pressed button in an attribute.

Hints from issue discussion:
See https://github.com/Textualize/textual/discussions/2202

Commit message: Update the ChangeLog

Patch to review:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 91ac354e51..b5ada56dc6 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -5,6 +5,12 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](http://keepachangelog.com/)
 and this project adheres to [Semantic Versioning](http://semver.org/).
 
+## Unreleased
+
+### [Fixed]
+
+- `RadioSet` is now far less likely to report `pressed_button` as `None` https://github.com/Textualize/textual/issues/2203
+
 ## [0.17.3] - 2023-04-02
 
 ### [Fixed]
diff --git a/src/textual/widgets/_radio_set.py b/src/textual/widgets/_radio_set.py
index 2d1f2e304e..f0c4170ac6 100644
--- a/src/textual/widgets/_radio_set.py
+++ b/src/textual/widgets/_radio_set.py
@@ -2,8 +2,9 @@
 
 from __future__ import annotations
 
+from rich.repr import Result
+
 from ..containers import Container
-from ..css.query import DOMQuery, QueryError
 from ..message import Message
 from ._radio_button import RadioButton
 
@@ -46,14 +47,14 @@ def __init__(self, radio_set: RadioSet, pressed: RadioButton) -> None:
             """A reference to the `RadioSet` that was changed."""
             self.pressed = pressed
             """The `RadioButton` that was pressed to make the change."""
-            # Note: it would be cleaner to use `sender.pressed_index` here,
-            # but we can't be 100% sure all of the updates have happened at
-            # this point, and so we can't go looking for the index of the
-            # pressed button via the normal route. So here we go under the
-            # hood.
-            self.index = radio_set._nodes.index(pressed)
+            self.index = radio_set.pressed_index
             """The index of the `RadioButton` that was pressed to make the change."""
 
+        def __rich_repr__(self) -> Result:
+            yield "radio_set", self.radio_set
+            yield "pressed", self.pressed
+            yield "index", self.index
+
     def __init__(
         self,
         *buttons: str | RadioButton,
@@ -76,6 +77,10 @@ def __init__(
             [RadioButton][textual.widgets.RadioButton] will be created from
             it.
         """
+        self._pressed_button: RadioButton | None = None
+        """Keeps track of which radio button is pressed."""
+        self._buttons: list[RadioButton] = []
+        """Holds the radio buttons we're responsible for."""
         super().__init__(
             *[
                 (button if isinstance(button, RadioButton) else RadioButton(button))
@@ -87,22 +92,28 @@ def __init__(
             disabled=disabled,
         )
 
-    @property
-    def _buttons(self) -> DOMQuery[RadioButton]:
-        """The buttons within the set."""
-        return self.query(RadioButton)
-
     def on_mount(self) -> None:
         """Perform some processing once mounted in the DOM."""
+
+        # Get the collection of buttons we're directly responsible for. We
+        # do this in here rather than in __init__ because someone might be
+        # composing the RadioSet using it as a context manager; point being
+        # there won't be any buttons being passed to __init__.
+        self._buttons = list(self.query(RadioButton))
+
         # It's possible for the user to pass in a collection of radio
-        # buttons, with more than one set to on. So here we check for that
-        # and, for want of a better approach, we keep the first one on and
-        # turn all the others off.
-        switched_on = self._buttons.filter(".-on")
-        if len(switched_on) > 1:
-            with self.prevent(RadioButton.Changed):
-                for button in switched_on[1:]:
-                    button.value = False
+        # buttons, with more than one set to on; they shouldn't, but we
+        # can't stop them. So here we check for that and, for want of a
+        # better approach, we keep the first one on and turn all the others
+        # off.
+        switched_on = [button for button in self._buttons if button.value]
+        with self.prevent(RadioButton.Changed):
+            for button in switched_on[1:]:
+                button.value = False
+
+        # Keep track of which button is initially pressed.
+        if switched_on:
+            self._pressed_button = switched_on[0]
 
     def on_radio_button_changed(self, event: RadioButton.Changed) -> None:
         """Respond to the value of a button in the set being changed.
@@ -110,39 +121,39 @@ def on_radio_button_changed(self, event: RadioButton.Changed) -> None:
         Args:
             event: The event.
         """
-        # If the button is changing to be the pressed button...
-        if event.radio_button.value:
-            # ...send off a message to say that the pressed state has
-            # changed.
-            self.post_message(self.Changed(self, event.radio_button))
-            # ...then look for the button that was previously the pressed
-            # one and unpress it.
-            for button in self._buttons.filter(".-on"):
-                if button != event.radio_button:
-                    button.value = False
-                    break
-        else:
-            # If this leaves us with no buttons checked, disallow that. Note
-            # that we stop the current event and (see below) we also prevent
-            # another Changed event being emitted. This should all be seen
-            # as a non-operation.
-            event.stop()
-            if not self._buttons.filter(".-on"):
-                with self.prevent(RadioButton.Changed):
-                    event.radio_button.value = True
+        # We're going to consume the underlying radio button events, making
+        # it appear as if they don't emit their own, as far as the caller is
+        # concerned. As such, stop the event bubbling and also prohibit the
+        # same event being sent out if/when we make a value change in here.
+        event.stop()
+        with self.prevent(RadioButton.Changed):
+            # If the message pertains to a button being clicked to on...
+            if event.radio_button.value:
+                # If there's a button pressed right now and it's not really a
+                # case of the user mashing on the same button...
+                if (
+                    self._pressed_button is not None
+                    and self._pressed_button != event.radio_button
+                ):
+                    self._pressed_button.value = False
+                # Make the pressed button this new button.
+                self._pressed_button = event.radio_button
+                # Emit a message to say our state has changed.
+                self.post_message(self.Changed(self, event.radio_button))
+            else:
+                # We're being clicked off, we don't want that.
+                event.radio_button.value = True
 
     @property
     def pressed_button(self) -> RadioButton | None:
         """The currently-pressed button, or `None` if none are pressed."""
-        try:
-            return self.query_one("RadioButton.-on", RadioButton)
-        except QueryError:
-            return None
+        return self._pressed_button
 
     @property
     def pressed_index(self) -> int:
         """The index of the currently-pressed button, or -1 if none are pressed."""
-        try:
-            return self._nodes.index(self.pressed_button)
-        except ValueError:
-            return -1
+        return (
+            self._nodes.index(self._pressed_button)
+            if self._pressed_button is not None
+            else -1
+        )


Code review comments:
diff_hunk: @@ -87,62 +92,68 @@ def __init__(
             disabled=disabled,
         )
 
-    @property
-    def _buttons(self) -> DOMQuery[RadioButton]:
-        """The buttons within the set."""
-        return self.query(RadioButton)
-
     def on_mount(self) -> None:
         """Perform some processing once mounted in the DOM."""
+
+        # Get the collection of buttons we're directly responsible for. We
+        # do this in here rather than in __init__ because someone might be
+        # composing the RadioSet using it as a context manager; point being
+        # there won't be any buttons being passed to __init__.
+        self._buttons = list(self.query(RadioButton))
path: src/textual/widgets/_radio_set.py
text: @user1:
Are we creating a limitation by calculating this on mount? What if a RadioButton is removed or added after the mount?

@author:
Yup, this was a choice, albeit one I'm not wedded to. It raises the question of what we should do with compound widgets where we don't really want the content changed via the normal DOM-modification mechanisms (see ListView, and I think a couple of others, for different but related issues). 

Happy to return this aspect back to pure DOM querying. 

Meta question though: do you think we should have the concept of a sealed container for times like this, when it makes sense to have full control over the content?

@user1:
That's not a bad idea. A widget might want to manage the DOM itself. 

@author:
Cool, I'll raise an issue to this effect when I'm next at a physical keyboard.

diff_hunk: @@ -2,8 +2,9 @@
 
 from __future__ import annotations
 
+from rich.repr import Result
path: src/textual/widgets/_radio_set.py
text: @user1:
Could you use the `rich.repr.auto` decorator with Rich reprs. See other files for an example of use.

@author:
Will do, thanks. 

@author:
I tried this but, I think I may be misunderstanding [how to apply it](https://rich.readthedocs.io/en/stable/pretty.html#automatic-rich-repr) correctly; it seemed to result in the `Changed` `repr`ing as if there was nothing to show; which is where I started. Also, looking through Textual, there seems to be a *lot* of cases where `__rich_repr__` is used anyway, sometimes even in conjunction with the decorator.

Could I get more of a pointer here?

@user1:
You need both. The decorator also generates a `__repr__` which will work if you don't print it with Rich.

@author:
Ahhh! I'd read it as a thing that generates Rich reprs! Okay, that makes more sense now.