Review the order in which a Mount event is received and render is called

[davep]

Following on from #2912, we should look to see if we can guarantee that any freshly-mounted widget receives its Mount event before any call on render is made.

[MorningLightMountain713]

Have been caught out by this. Watching. Would be good to guarantee on_mount is called before any calls to render Cheers!

[darrenburns]

I've changed the label to bug, because it's basically considered idiomatic Textual at this point to initialise state inside your on_mount. If you use that state inside a render method, Textual should not crash.

This currently causes sporadic crashes in the LoadingIndicator widget. The crash in LoadingIndicator was fixed/worked around in a release on 25/Oct/23, but the fundamental problem still exists.

[rodrigogiraoserrao]

FYI I have a way to repro a situation where render gets called before on_mount.
Just run the app below and click the button twice in under one second:

import asyncio


from textual.app import App
from textual.widget import Widget
from textual.widgets import Button


class W(Widget):
    DEFAULT_CSS = "W { height: 1; }"
    def render(self):
        return self.renderable

    async def on_mount(self):
        await asyncio.sleep(1)
        self.renderable = "1234"
        print("on mount")


class MyApp(App[None]):
    def compose(self):
        yield Button()

    async def on_button_pressed(self):
        self.mount(W())


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

[rodrigogiraoserrao]

Progress being made. Widgets that haven't been mounted yet are being considered as "visible widgets" by the compositor too soon. E.g., changing Compositor.visible_widgets to only consider widgets for which the _mounted_event has been set seems to fix the problem.

Next up I'll figure out why unmounted widgets are making it to the compositor map so soon.

[rodrigogiraoserrao]

It seems that the issue is in the way the compositor builds the map, which does not take into account which widgets have been mounted yet.

Consider this tentative fix:

class Compositor:
    def _arrange_root(...) -> tuple[CompositorMap, set[Widget]]:
        # ...

        def add_widget(...) -> None:
            if not widget._mounted_event.is_set():  # <-- new
                return                              # <-- new

This passes the full test suite and it takes care of the problem by preventing unmounted events from being considered when building the map.

Another possible alternative would be to register widgets only after they're mounted but that doesn't sound correct.

Another possibly possible alternative is to check if the “registering” process can be split into two in a way that makes sense:

• some “registering” happens before mounting
• we mount the widget
• and the AwaitMount would finish the “registering” process after the event Mount is processed

I'll investigate if this third idea is a possibility.

[rodrigogiraoserrao]

Just for the sake of completeness and explicitness, the exact issue is that the AwaitMount will refresh the parent of the widget that got mounted.
If that call happens while there are other widgets that are in the process of being mounted, the compositor will be asked to do work and at that point in time there are widgets that have been registered in the DOM but not mounted, which is what eventually triggers the calls to render on widgets that haven't been mounted.

[willmcgugan]

@rodrigogiraoserrao I guess we can close this?

[rodrigogiraoserrao]

Yes, I forgot to link the PR to this issue.

[github-actions]

Don't forget to star the repository!

Follow @textualizeio for Textual updates.