Code feedback from joe/winston;

Author: schloerke

shiny/render/_dataframe.py

@@ -35,28 +35,29 @@
 DataFrameResult = Union[None, "pd.DataFrame", "DataGrid", "DataTable"]
 
 
-class OnCellUpdateFn(Protocol):
+class CellUpdateFn(Protocol):
     async def __call__(
         self,
         *,
-        row_index: int,
-        column_index: int,
-        value: str,
-        prev: str,
+        info: CellUpdateInfo,
+        # TODO-barret; Remove `**kwargs`
         **kwargs: Any,  # future proofing
     ) -> Any: ...
-class OnCellUpdateParams(TypedDict):
+
+
+class CellUpdateInfo(TypedDict):
     row_index: int
     column_index: int
     value: str
     prev: str
 
 
-class OnCellsUpdateFn(Protocol):
+class CellsUpdateFn(Protocol):
     async def __call__(
         self,
         *,
-        update_infos: list[OnCellUpdateParams],
+        infos: list[CellUpdateInfo],
+        # TODO-barret; Remove `**kwargs`
         **kwargs: Any,  # future proofing
     ) -> Any: ...
 @dataclass
@@ -403,8 +404,8 @@ class data_frame(Renderer[DataFrameResult]):
     _value: reactive.Value[DataFrameResult | None]
     # _data: reactive.Value[pd.DataFrame | None]
 
-    handle_cell_update: OnCellUpdateFn
-    handle_cells_update: OnCellsUpdateFn
+    handle_cell_update: CellUpdateFn
+    handle_cells_update: CellsUpdateFn
 
     cell_patches: reactive.Value[list[CellPatch]]
 
@@ -455,6 +456,7 @@ def _init_reactives(self) -> None:
 
         # Init
         self._value: reactive.Value[Union[DataFrameResult, None]] = reactive.Value(None)
+        self.cell_patches: reactive.Value[list[CellPatch]] = reactive.Value([])
 
         @reactive.calc
         def self_data() -> pd.DataFrame:
@@ -514,8 +516,6 @@ def self_data_selected() -> pd.DataFrame:
 
         self.data_selected = self_data_selected
 
-        self.cell_patches: reactive.Value[list[CellPatch]] = reactive.Value([])
-
         @reactive.calc
         def self_data_patched() -> pd.DataFrame:
             data = self.data()
@@ -537,44 +537,46 @@ def _get_session(self) -> Session:
             )
         return self._session
 
-    def on_cell_update(self, fn: OnCellUpdateFn) -> Self:
+    # TODO-barret; `on` sounds like a registration. Maybe use `set` instead? `set_cell_update_fn`?
+    def on_cell_update(self, fn: CellUpdateFn) -> Self:
         self.handle_cell_update = fn
         return self
 
-    def on_cells_update(self, fn: OnCellsUpdateFn) -> Self:
+    def on_cells_update(self, fn: CellsUpdateFn) -> Self:
         self.handle_cells_update = fn
         return self
 
     def _init_handlers(self) -> None:
         async def _on_cell_update_default(
             *,
-            row_index: int,
-            column_index: int,
-            value: str,
-            prev: str,
+            info: CellUpdateInfo,
+            # row_index: int,
+            # column_index: int,
+            # value: str,
+            # prev: str,
             **kwargs: Any,
         ) -> str:
-            return value
+            return info["value"]
 
         async def _on_cells_update_default(
             *,
-            update_infos: list[OnCellUpdateParams],
+            infos: list[CellUpdateInfo],
             **kwargs: Any,
         ):
             with reactive.isolate():
                 formatted_values: list[Any] = []
-                for update_info in update_infos:
-                    row_index = update_info["row_index"]
-                    column_index = update_info["column_index"]
-                    value = update_info["value"]
-                    prev = update_info["prev"]
-
-                    formatted_value = await self.handle_cell_update(
-                        row_index=row_index,
-                        column_index=column_index,
-                        value=value,
-                        prev=prev,
-                    )
+                for update_info in infos:
+                    # row_index = update_info["row_index"]
+                    # column_index = update_info["column_index"]
+                    # value = update_info["value"]
+                    # prev = update_info["prev"]
+
+                    formatted_value = await self.handle_cell_update(info=update_info)
+                    #     row_index=row_index,
+                    #     column_index=column_index,
+                    #     value=value,
+                    #     prev=prev,
+                    # )
                     # TODO-barret; check type here?
                     # TODO-barret; The return value should be coerced by pandas to the correct type
                     formatted_values.append(formatted_value)
@@ -587,43 +589,45 @@ async def _on_cells_update_default(
 
     # To be called by session's outputRPC message handler on this data_frame
     # Do not change this method unless you update corresponding code in `/js/dataframe/`!!
-    async def _handle_cells_update(self, update_infos: list[OnCellUpdateParams]):
-        with session_context(self._get_session()):
-            with reactive.isolate():
-                # Make new array to trigger reactive update
-                patches = [p for p in self.cell_patches()]
+    # @outputRPC_handler
+    async def _handle_cells_update(self, update_infos: list[CellUpdateInfo]):
+
+        # if len(self.cell_patches()) > 1:
+        #     raise RuntimeError("Barret testing!")
+
+        # Make new array to trigger reactive update
+        patches = [p for p in self.cell_patches()]
 
-                # Call on_cells_update
-                formatted_values = await self.handle_cells_update(
-                    update_infos=update_infos
+        # Call user's on_cells_update method to retrieve formatted values
+        # TODO-barret; REname `formatted` to `raw_values`
+        formatted_values = await self.handle_cells_update(infos=update_infos)
+
+        if len(formatted_values) != len(update_infos):
+            raise ValueError(
+                f"The return value of {self.output_id}'s `handle_cells_update()` (typically set by `@{self.output_id}.on_cells_update`) must be a list of the same length as the input list of cell updates. Received {len(formatted_values)} items and expected {len(update_infos)}."
+            )
+
+        # Add new patches
+        for formatted_value, update_info in zip(formatted_values, update_infos):
+            patches.append(
+                CellPatch(
+                    row_index=update_info["row_index"],
+                    column_index=update_info["column_index"],
+                    value=formatted_value,
+                    prev=update_info["prev"],
                 )
+            )
 
-                if len(formatted_values) != len(update_infos):
-                    raise ValueError(
-                        f"The return value of {self.output_id}'s `handle_cells_update()` (typically set by `@{self.output_id}.on_cells_update`) must be a list of the same length as the input list of cell updates. Received {len(formatted_values)} items and expected {len(update_infos)}."
-                    )
-
-                # Add new patches
-                for formatted_value, update_info in zip(formatted_values, update_infos):
-                    patches.append(
-                        CellPatch(
-                            row_index=update_info["row_index"],
-                            column_id=update_info["column_id"],
-                            value=formatted_value,
-                            prev=update_info["prev"],
-                        )
-                    )
-
-                # Remove duplicate patches
-                patch_map: dict[tuple[int, str], CellPatch] = {}
-                for patch in patches:
-                    patch_map[(patch.row_index, patch.column_id)] = patch
-                patches = list(patch_map.values())
-
-                # Set new patches
-                self.cell_patches.set(patches)
+        # Remove duplicate patches
+        patch_map: dict[tuple[int, int], CellPatch] = {}
+        for patch in patches:
+            patch_map[(patch.row_index, patch.column_index)] = patch
+        patches = list(patch_map.values())
 
-                return formatted_values
+        # Set new patches
+        self.cell_patches.set(patches)
+
+        return formatted_values
 
     def auto_output_ui(self) -> Tag:
         return ui.output_data_frame(id=self.output_id)
@@ -661,11 +665,10 @@ def _set_output_metadata(self, *, output_id: str) -> None:
     async def render(self) -> Jsonifiable:
         # Reset value
         self._value.set(None)
+        self.cell_patches.set([])
 
         value = await self.fn()
         if value is None:
-            # Quit early
-            self._value.set(None)
             return None
 
         if not isinstance(value, AbstractTabularData):

tests/playwright/shiny/components/data_frame/edit/app.py

@@ -3,78 +3,71 @@
 import pandas as pd
 from palmerpenguins import load_penguins_raw  # pyright: ignore[reportMissingTypeStubs]
 
-from shiny import App, Inputs, reactive, render, ui
+from shiny import App, Inputs, Outputs, Session, module, reactive, render, ui
+from shiny.render._dataframe import CellUpdateInfo
+
+# TODO-barret; Make an example that uses a dataframe that then updates a higher level reactive, that causes the df to update... which causes the table to render completely
+# TODO-barret-future; When "updating" data, try to maintain the scroll, filter info when a new `df` is supplied;
+
 
 # Load the dataset
-df = reactive.value(load_penguins_raw())
+penguins = load_penguins_raw()
+
+df1 = pd.DataFrame(data={"a": [1, 2]})
+df1.insert(1, "a", [3, 4], True)  # pyright: ignore
+
+df = penguins
+# df = df1
+
+
+# df = reactive.value(df1)df = reactive.value(load_penguins_raw())
+
+MOD_ID = "testing"
+
+
+@module.ui
+def mod_ui():
+    return ui.TagList(
+        ui.card(
+            ui.output_data_frame("summary_data"),
+            height="400px",
+        ),
+    )
 
 
 app_ui = ui.page_fillable(
     {"class": "p-3"},
     # ui.markdown(
     #     "**Instructions**: Select one or more countries in the table below to see more information."
     # ),
-    ui.card(
-        ui.output_data_frame("summary_data"),
-        height="400px",
-    ),
-    ui.pre(id="barret"),
-    ui.tags.script(
-        """
-        // const window_console_log = console.log;
-        // console.log = function() {
-        //     window_console_log.apply(console, arguments);
-        //     let txt = "log - " + Date.now() + ": ";
-        //     for (let i = 0; i < arguments.length; i++) {
-        //         if (i > 0) txt += ", ";
-        //         txt += arguments[i];
-        //     }
-        //     $("#barret").append(txt + "\\n");
-        // };
-        const window_console_trace = console.trace;
-        console.trace = function() {
-            window_console_trace.apply(console, arguments);
-            let txt = "trace - " + Date.now() + ": ";
-            for (let i = 0; i < arguments.length; i++) {
-                if (i > 0) txt += ", ";
-                txt += arguments[i];
-            }
-            $("#barret").append(txt + "\\n");
-        };
-        const window_console_error = console.error;
-        console.error = function() {
-            window_console_error.apply(console, arguments);
-            let txt = "error - " + Date.now() + ": ";
-            for (let i = 0; i < arguments.length; i++) {
-                if (i > 0) txt += ", ";
-                txt += arguments[i];
-            }
-            $("#barret").append(txt + "\\n");
-        };
-        // console.log("Hello, World!");
-        // console.error("Hello, World!");
-        """
-    ),
+    mod_ui(MOD_ID),
 )
 
 
-def server(input: Inputs):
+@module.server
+def mod_server(input: Inputs, output: Outputs, session: Session):
     @render.data_frame
     def summary_data():
         # return df
-        return render.DataGrid(df(), mode="edit")
-        # return render.DataTable(df, row_selection_mode="multiple")
+        return render.DataGrid(df, mode="edit")
+        return render.DataTable(df, mode="edit")
 
     @summary_data.on_cell_update
     async def handle_edit(
         *,
-        row_index: int,
-        column_id: str,
-        value: str,
-        prev: str,
-        **kwargs: Any,
+        info: CellUpdateInfo,
+        # info: int,
+        # column_index: int,
+        # value: str,
+        # prev: str,
     ):
-        return "demo_" + value
+        return "demo_" + info["value"]
+
+    @reactive.effect
+    def _():
+        summary_data.data()
+        summary_data.data_patched()
+        print(summary_data.data_patched(), summary_data.cell_patches())
 
     # summary_data.data_patched()  # ~ patched_df()
 
@@ -152,4 +145,8 @@ async def handle_edit_simple(
             # return df_copy
 
 
-app = App(app_ui, server)
+def server(input: Inputs, output: Outputs, session: Session):
+    mod_server(MOD_ID)
+
+
+app = App(app_ui, server, debug=False)