deephaven / deephaven-plugins

Deephaven Plugins
11 stars 15 forks source link

fix: Re-running ui code initially rendering the old document #1017

Closed mattrunyon closed 1 week ago

mattrunyon commented 1 week ago

This fixes a bug where re-running code causes the old document to be rendered first. This can cause initial mounting with old props for document trees that don't change between runs which can cause problems with components that rely on an initial prop value only.

This also fixes an issue where the old panels are shown until new panels/document is fetched. Now, it will show loading spinners on those panels instead. If the new document (re-assigned to the same variable in Python) has a new tree structure, then the old panels will be closed and new panels opened. If the document has the same tree, then the new panels will replace the old panels.

This Python example shows a slow loading component. Run the code, then change the props (I commented out the format_ prop) and re-run. The existing panel should turn into a loading spinner and then load without the formatting rules.

from deephaven import ui
import deephaven.plot.express as dx
import time

@ui.component
def my_t():
    time.sleep(2)
    return ui.table(
        dx.data.stocks().update("SymColor=Sym==`FISH` ? `positive` : `salmon`"),
        hidden_columns=["SymColor"],
        format_=[
            ui.TableFormat(value="0.00%"),
            ui.TableFormat(cols="Timestamp", value="E, dd MMM yyyy HH:mm:ss z"),
            ui.TableFormat(cols="Size", color="info", if_="Size < 10"),
            ui.TableFormat(cols="Size", color="notice", if_="Size > 100"),
            ui.TableFormat(cols=["Sym", "Exchange"], alignment="center"),
            ui.TableFormat(
                cols=["Sym", "Exchange"], background_color="negative", if_="Sym=`CAT`"
            ),
            ui.TableFormat(if_="Sym=`DOG`", color="oklab(0.6 -0.3 -0.25)"),
            ui.TableFormat(cols="Sym", color="SymColor"),
        ],
    )

t = my_t()
mattrunyon commented 1 week ago

If we wanted to show a loading panel immediately, we could modify the document to open a panel or show a toast when loading (if there's no document). We would maybe want to open a loading panel after some short delay so there's no flashing for quick running commands

mofojed commented 1 week ago

If we wanted to show a loading panel immediately, we could modify the document to open a panel or show a toast when loading (if there's no document). We would maybe want to open a loading panel after some short delay so there's no flashing for quick running commands

Showing the loading panel immediately would be fine in that once it loads, it would just fill that panel. However it gets dicey when you have components that have multiple panels.

mattrunyon commented 1 week ago

If we wanted to show a loading panel immediately, we could modify the document to open a panel or show a toast when loading (if there's no document). We would maybe want to open a loading panel after some short delay so there's no flashing for quick running commands

Showing the loading panel immediately would be fine in that once it loads, it would just fill that panel. However it gets dicey when you have components that have multiple panels.

Fair point. I can do this as a separate PR. I think it's fine if there's multiple panels because we have no way of knowing that, so showing 1 w/ an actual loading message instead of just a spinner will work fine I think. And then in the DocumentHandler we should be able to re-use the panelId and replace the loading panel w/ the 1st actual panel.

mofojed commented 1 week ago

Fair point. I can do this as a separate PR. I think it's fine if there's multiple panels because we have no way of knowing that, so showing 1 w/ an actual loading message instead of just a spinner will work fine I think. And then in the DocumentHandler we should be able to re-use the panelId and replace the loading panel w/ the 1st actual panel.

@mattrunyon This is easier said than done with the current setup... because the document structure is different going from "loading" to when the document is loaded (even with one panel, but especially with more than one panel), it means the ReactPanel unmounts, causing it to close a panel, then the new ReactPanel mounts, opening a new panel in a possibly new location.

I had a branch where I was playing around with this at one point. I think fix-panel-state-management changes to WidgetHandler. In that branch I'm ensuring a panel is opened immediately. I'm pretty sure the snag I hit was just with opening dashboards though, and didn't have time to resolve that.