toggling a RadioButton temporarily sets the RadioSet's pressed_button to None

[sjoerdtimmer]

tested with 0.15.1

I have a button assigned to cycle through some states.
The relevant code looks something like this:

def action_cycle(self) -> None:
    statelist: RadioSet = self.query_one('#statelist', RadioSet)
    current = statelist.pressed_button
    if current.id == 'foo':
        goto = 'bar'
    elif current.id == 'bar':
        goto = 'baz'
    elif current.id == 'baz':
        goto = 'foo'
    button: RadioButton = self.query_one('#' + goto, RadioButton)
    button.toggle()

I found that this causes an issue if I hold down the assigned key long enough.
It seems that the RadioSet temporarily has no pressed_button and that it stays that way longer than it takes the eventloop to start processing the next keystroke. This is a bit suspicious because normally asyncio avoid this kind of race conditions. I suspect that the background rendering thread is involved but I didn't dive too much into that.

It would be better if the radioset had an update method, or if the button toggle was completely processed before yielding again to the event loop, or if there was some kind of async with construct that could be used to avoid race conditions.

Of course, in my case it is not that difficult to keep track of the state outside of the ButtonSet but I prefer to keep my code as simple as possible and with a single source of truth.

[davep]

In trying to follow the problem here, I tried to make a minimal reproduction of what you seem to be saying you're seeing; this is what I came up with:

from textual.app        import App, ComposeResult
from textual.containers import Vertical
from textual.widgets    import Header, Footer, RadioSet, RadioButton

class RSExternalChangeApp( App[ None ] ):

    CSS = """
    Vertical {
        align: center middle;
    }

    RadioSet {
        padding: 2;
    }
    """

    BINDINGS = [
        ( "space", "next", "Next Radio Button" )
    ]

    def compose( self ) -> ComposeResult:
        yield Header()
        with Vertical():
            yield RadioSet(
                *[ RadioButton( f"This is item {n}", not n, id=f"rb{n}" ) for n in range( 10 ) ]
            )
        yield Footer()

    def action_next( self ):
        pressed = self.query_one( RadioSet ).pressed_button
        next_rb = int( pressed.id.split( "rb" )[ 1 ] )
        if next_rb == 9:
            next_rb = 0
        else:
            next_rb += 1
        self.query_one( f"#rb{next_rb}", RadioButton ).toggle()

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

The idea being, if I camp on the Space key the pressed button in the radio set will cycle through the set. Is that correct?

The first thing I'd point out is that pressed_button is typed as RadioButton | None, so it would always be a good idea to handle the case where it returns None anyway. In my development environment I get the following warning about my test code:

Moreover, I can't actually seem to reproduce the issue you're seeing. Admittedly I am testing with Textual 0.17.0, but I suspect it wouldn't make a difference.

I'd say it would always be a good idea to handle the None case in your action_cycle code; but that aside, do you think you could provide a minimal self-contained version of the problem so I might see it in action?

[sjoerdtimmer]

You are correct on all accounts.
Your counter example does it. If I hold space for a few seconds, eventually it will crash.
https://asciinema.org/a/eqB6At9xwFSrbYCHPhc38JqQR

Note that using an ssh connection or while recording the session using asciinema it is much faster to trigger than without. This corroborates my suspicion that it has something to do with the background rendering.

The reason for bringing up the discussion is that it is counter intuitive that toggling a button doesn't make that button the pressed one... This is bound to cause unexpected behavior for more developers. Arguably is is also against the asyncio philosophy where programs can yield to the event loop after they are done processing the current input. In this case the input is still being processed and textual is yielding too quickly. I realize now that this goes much deeper than radio buttons, but I believe that the parallelization introduced in 0.4 moves slightly too much work to the background worker. It should update the state and then defer the rendering to the worker and then yield to the event loop. Contrary to that, it seems that currently the whole update is deferred to the background worker and the yield to the event loop happens before the state could be updated. With this new insight I understand if you don't want to pursue this any further. But it's matter for thought and consideration for future rewrites.

[davep]

Right, thanks for testing that. With that extra bit of environmental information it's clearer where the issue is. It looks like, for you, there's a brief moment where two RadioButtons have the .-on class applied, resulting in pressed_button returning None.

We'll give RadioSet.on_radio_button_changed a wee refresh to eliminate the moment where that's possible.

[davep]

See #2203