diff --git a/plugins/ui/DESIGN.md b/plugins/ui/DESIGN.md index 8ec65fd3a..477718abe 100644 --- a/plugins/ui/DESIGN.md +++ b/plugins/ui/DESIGN.md @@ -1438,13 +1438,13 @@ use_table_listener( ###### Parameters -| Parameter | Type | Description | -|---------------|--------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -| `table` | `Table` | The table to listen to. | -| `listener` | `Callable[[TableUpdate, bool], None] \| TableListener` | Either a function or a [TableListener](https://deephaven.io/core/pydoc/code/deephaven.table_listener.html#deephaven.table_listener.TableListener) with an on_update function. The function must take a [TableUpdate](https://deephaven.io/core/pydoc/code/deephaven.table_listener.html#deephaven.table_listener.TableUpdate) and is_replay bool. [More table listener info](https://deephaven.io/core/docs/how-to-guides/table-listeners-python/) | -| `description` | `str \| None` | An optional description for the UpdatePerformanceTracker to append to the listener’s entry description, default is None. -| `do_replay` | `bool` | Whether to replay the initial snapshot of the table, default is False. | -| `replay_lock` | `LockType` | The lock type used during replay, default is ‘shared’, can also be ‘exclusive’. | +| Parameter | Type | Description | +| ------------- | ------------------------------------------------------ | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `table` | `Table` | The table to listen to. | +| `listener` | `Callable[[TableUpdate, bool], None] \| TableListener` | Either a function or a [TableListener](https://deephaven.io/core/pydoc/code/deephaven.table_listener.html#deephaven.table_listener.TableListener) with an on_update function. The function must take a [TableUpdate](https://deephaven.io/core/pydoc/code/deephaven.table_listener.html#deephaven.table_listener.TableUpdate) and is_replay bool. [More table listener info](https://deephaven.io/core/docs/how-to-guides/table-listeners-python/) | +| `description` | `str \| None` | An optional description for the UpdatePerformanceTracker to append to the listener’s entry description, default is None. | +| `do_replay` | `bool` | Whether to replay the initial snapshot of the table, default is False. | +| `replay_lock` | `LockType` | The lock type used during replay, default is ‘shared’, can also be ‘exclusive’. | ##### use_table_data @@ -1637,7 +1637,6 @@ class LinkPoint(TypedDict): - #### Context By default, the context of a `@ui.component` will be created per client session (same as [Parameterized Query's "parallel universe" today](https://github.com/deephaven-ent/iris/blob/868b868fc9e180ee948137b10b6addbac043605e/ParameterizedQuery/src/main/java/io/deephaven/query/parameterized/impl/ParameterizedQueryServerImpl.java#L140)). However, it would be interesting if it were possible to share a context among all sessions for the current user, and/or share a context with other users even; e.g. if one user selects and applies a filter, it updates immediately for all other users with that dashboard open. So three cases: @@ -1772,15 +1771,15 @@ sequenceDiagram UIP->>SP: Render tft SP->>SP: Run sym_exchange Note over SP: sym_exchange executes, running text_filter_table twice - SP-->>UIP: Result (flex([tft1, tft2])) - UIP-->>W: Display (flex([tft1, tft2])) + SP-->>UIP: Result (document=flex([tft1, tft2]), exported_objects=[tft1, tft2]) + UIP-->>W: Display Result U->>UIP: Change text input 1 UIP->>SP: Change state SP->>SP: Run sym_exchange - Note over SP: sym_exchange executes, text_filter_table only
runs once for the one changed input - SP-->>UIP: Result (flex([tft1', tft2])) - UIP-->>W: Display (flex([tft1', tft2])) + Note over SP: sym_exchange executes, text_filter_table only
runs once for the one changed input
only exports the new table, as client already has previous tables + SP-->>UIP: Result (document=flex([tft1', tft2], exported_objects=[tft1'])) + UIP-->>W: Display Result ``` ##### Communication/Callbacks @@ -1807,12 +1806,11 @@ sequenceDiagram A component that is created on the server side runs through a few steps before it is rendered on the client side: -1. Element - The basis for all UI components. Generally a `FunctionElement`, and does not run the function until it is requested by the UI. The result can change depending on the context that it is rendered in (e.g. what "state" is set). -2. RenderedNode - After an element has been rendered using a renderer, it becomes a `RenderedNode`. This is an immutable representation of the document. -3. JSONEncodedNode - The `RenderedNode` is then encoded into JSON using `NodeEncoder`. It pulls out all the objects and maps them to exported objects, and all the callables to be mapped to commands that can be accepted by JSON-RPC. This is the final representation of the document that is sent to the client. -4. ElementPanel - Client side where it's receiving the `documentUpdated` from the server plugin, and then rendering the `JSONEncodedNode` into a `ElementPanel` (e.g. a `GoldenLayout` panel). Decodes the JSON, maps all the exported objects to the actual objects, and all the callables to async methods that will call to the server. -5. ElementView - Renders the decoded panel into the UI. Picks the element based on the name of it. -6. ObjectView - Render an exported object +1. [Element](./src/deephaven/ui/elements/Element.py) - The basis for all UI components. Generally a [FunctionElement](./src/deephaven/ui/elements/FunctionElement.py) created by a script using the [@ui.component](./src/deephaven/ui/components/make_component.py) decorator, and does not run the function until it is rendered. The result can change depending on the context that it is rendered in (e.g. what "state" is set). +2. [ElementMessageStream](./src/deephaven/ui/object_types/ElementMessageStream.py) - The `ElementMessageStream` is responsible for rendering one instance of an element in a specific rendering context and handling the server-client communication. The element is rendered to create a [RenderedNode](./src/deephaven/ui/renderer/RenderedNode.py), which is an immutable representation of a rendered document. The `RenderedNode` is then encoded into JSON using [NodeEncoder](./src/deephaven/ui/renderer/NodeEncoder.py), which pulls out all the non-serializable objects (such as Tables) and maps them to exported objects, and all the callables to be mapped to commands that can be accepted by JSON-RPC. This is the final representation of the document that is sent to the client, and ultimately handled by the `WidgetHandler`. +3. [DashboardPlugin](./src/js/src/DashboardPlugin.tsx) - Client side `DashboardPlugin` that listens for when a widget of type `Element` is opened, and manage the `WidgetHandler` instances that are created for each widget. +4. [WidgetHandler](./src/js/src/WidgetHandler.tsx) - Uses JSON-RPC communication with an `ElementMessageStream` instance to load the initial rendered document and associated exported objects. Listens for any changes and updates the document accordingly. +5. [DocumentHandler](./src/js/src/DocumentHandler.tsx) - Handles the root of a rendered document, laying out the appropriate panels or dashboard specified. #### Other Decisions diff --git a/plugins/ui/src/deephaven/ui/components/make_component.py b/plugins/ui/src/deephaven/ui/components/make_component.py index 326873686..b7e43c652 100644 --- a/plugins/ui/src/deephaven/ui/components/make_component.py +++ b/plugins/ui/src/deephaven/ui/components/make_component.py @@ -1,18 +1,23 @@ import functools import logging +from typing import Any, Callable from .._internal import get_component_qualname from ..elements import FunctionElement logger = logging.getLogger(__name__) -def make_component(func): +def make_component(func: Callable[..., Any]): """ Create a FunctionalElement from the passed in function. + + Args: + func: The function to create a FunctionalElement from. + Runs when the component is being rendered. """ @functools.wraps(func) - def make_component_node(*args, **kwargs): + def make_component_node(*args: Any, **kwargs: Any): component_type = get_component_qualname(func) return FunctionElement(component_type, lambda: func(*args, **kwargs)) diff --git a/plugins/ui/src/deephaven/ui/elements/Element.py b/plugins/ui/src/deephaven/ui/elements/Element.py index c889b6459..e0c00ad0c 100644 --- a/plugins/ui/src/deephaven/ui/elements/Element.py +++ b/plugins/ui/src/deephaven/ui/elements/Element.py @@ -1,9 +1,11 @@ from __future__ import annotations from abc import ABC, abstractmethod -from typing import Any +from typing import Any, Dict from .._internal import RenderContext +PropsType = Dict[str, Any] + class Element(ABC): """ @@ -21,7 +23,7 @@ def name(self) -> str: return "deephaven.ui.Element" @abstractmethod - def render(self, context: RenderContext) -> dict[str, Any]: + def render(self, context: RenderContext) -> PropsType: """ Renders this element, and returns the result as a dictionary of props for the element. If you just want to render children, pass back a dict with children only, e.g. { "children": ... } diff --git a/plugins/ui/src/deephaven/ui/elements/__init__.py b/plugins/ui/src/deephaven/ui/elements/__init__.py index 1fada21ca..9003ffad9 100644 --- a/plugins/ui/src/deephaven/ui/elements/__init__.py +++ b/plugins/ui/src/deephaven/ui/elements/__init__.py @@ -1,6 +1,6 @@ -from .Element import Element +from .Element import Element, PropsType from .BaseElement import BaseElement from .FunctionElement import FunctionElement from .UITable import UITable -__all__ = ["BaseElement", "Element", "FunctionElement", "UITable"] +__all__ = ["BaseElement", "Element", "FunctionElement", "PropsType", "UITable"] diff --git a/plugins/ui/src/deephaven/ui/object_types/ElementMessageStream.py b/plugins/ui/src/deephaven/ui/object_types/ElementMessageStream.py index 6448f9f43..1c1887702 100644 --- a/plugins/ui/src/deephaven/ui/object_types/ElementMessageStream.py +++ b/plugins/ui/src/deephaven/ui/object_types/ElementMessageStream.py @@ -1,6 +1,6 @@ from __future__ import annotations -import json import io +import json from jsonrpc import JSONRPCResponseManager, Dispatcher import logging from typing import Any @@ -14,6 +14,36 @@ class ElementMessageStream(MessageStream): + _manager: JSONRPCResponseManager + """ + Handle incoming requests from the client. + """ + + _dispatcher: Dispatcher + """ + The dispatcher to use when client calls callables. + """ + + _encoder: NodeEncoder + """ + Encoder to use to encode the document. + """ + + _message_id: int + """ + The next message ID to use. + """ + + _element: Element + """ + The element to render. + """ + + _connection: MessageStream + """ + The connection to send the rendered element to. + """ + def __init__(self, element: Element, connection: MessageStream): """ Create a new ElementMessageStream. Renders the element in a render context, and sends the rendered result to the @@ -25,10 +55,10 @@ def __init__(self, element: Element, connection: MessageStream): """ self._element = element self._connection = connection - self._update_count = 0 self._message_id = 0 self._manager = JSONRPCResponseManager() self._dispatcher = Dispatcher() + self._encoder = NodeEncoder(separators=(",", ":")) def start(self) -> None: context = RenderContext() @@ -54,15 +84,15 @@ def on_data(self, payload: bytes, references: list[Any]) -> None: if response is None: return - payload = response.json - logger.debug("Response: %s, %s", type(payload), payload) - self._connection.on_data(payload.encode(), []) + response_payload = response.json + logger.debug("Response: %s, %s", type(response_payload), response_payload) + self._connection.on_data(response_payload.encode(), []) def _get_next_message_id(self) -> int: self._message_id += 1 return self._message_id - def _make_notification(self, method: str, *params: list[Any]) -> None: + def _make_notification(self, method: str, *params: Any) -> dict[str, Any]: """ Make a JSON-RPC notification. Can notify the client without expecting a response. @@ -76,7 +106,7 @@ def _make_notification(self, method: str, *params: list[Any]) -> None: "params": params, } - def _make_request(self, method: str, *params: list[Any]) -> None: + def _make_request(self, method: str, *params: Any) -> dict[str, Any]: """ Make a JSON-RPC request. Messages the client and expects a response. @@ -98,22 +128,20 @@ def _send_document_update(self, root: RenderedNode) -> None: Args: root: The root node of the document to send """ - # We use an ID prefix to ensure that the callable ids are unique across each document render/update - # That way we don't have to worry about callables from previous renders being called accidentally - self._update_count += 1 - id_prefix = f"cb_{self._update_count}_" # TODO(#67): Send a diff of the document instead of the entire document. - request = self._make_notification("documentUpdated", root) - encoder = NodeEncoder(callable_id_prefix=id_prefix, separators=(",", ":")) - payload = encoder.encode(request) + encoder_result = self._encoder.encode_node(root) + encoded_document = encoder_result["encoded_node"] + new_objects = encoder_result["new_objects"] + callable_id_dict = encoder_result["callable_id_dict"] + request = self._make_notification("documentUpdated", encoded_document) + payload = json.dumps(request) logger.debug(f"Sending payload: {payload}") dispatcher = Dispatcher() - for i, callable in enumerate(encoder.callables): - key = f"{id_prefix}{i}" - logger.debug("Registering callable %s", key) - dispatcher[key] = callable + for callable, callable_id in callable_id_dict.items(): + logger.debug("Registering callable %s", callable_id) + dispatcher[callable_id] = callable self._dispatcher = dispatcher - self._connection.on_data(payload.encode(), encoder.objects) + self._connection.on_data(payload.encode(), new_objects) diff --git a/plugins/ui/src/deephaven/ui/renderer/NodeEncoder.py b/plugins/ui/src/deephaven/ui/renderer/NodeEncoder.py index c7730d45e..ea0064067 100644 --- a/plugins/ui/src/deephaven/ui/renderer/NodeEncoder.py +++ b/plugins/ui/src/deephaven/ui/renderer/NodeEncoder.py @@ -1,14 +1,43 @@ from __future__ import annotations -from collections.abc import Iterator import json -from typing import Any, Callable +from typing import Any, Callable, TypedDict +from weakref import WeakKeyDictionary from .RenderedNode import RenderedNode CALLABLE_KEY = "__dhCbid" OBJECT_KEY = "__dhObid" ELEMENT_KEY = "__dhElemName" +DEFAULT_CALLABLE_ID_PREFIX = "cb" + +# IDs for callables are a string of a callable prefix set and an incrementing ID +CallableId = str + +# IDs for objects is just an incrementing ID. We should only send new exported objects with each render +ObjectId = int + + +class NodeEncoderResult(TypedDict): + """ + Result of encoding a node. Contains the encoded node, list of new objects, and callables dictionary. + """ + + encoded_node: str + """ + The encoded node. + """ + + new_objects: list[Any] + """ + The list of new objects. + """ + + callable_id_dict: WeakKeyDictionary[Callable[..., Any], CallableId] + """ + Dictionary from a callable to the ID assigned to the callable. + """ + class NodeEncoder(json.JSONEncoder): """ @@ -20,30 +49,40 @@ class NodeEncoder(json.JSONEncoder): _callable_id_prefix: str """ - Prefix to use for callable ids. Used to ensure callables used in stream are unique. + Prefix to use for callable ids. + """ + + _next_callable_id: int + """ + Incrementing ID that is used to assign IDs to callables. Needs to be prefixed with the `_callable_id_prefix` to get an ID. """ - _callables: list[Callable] + _callable_dict: WeakKeyDictionary[Callable[..., Any], CallableId] """ - List of callables parsed out of the document + Dictionary from a callable to the ID assigned to the callable. """ - _callable_id_dict: dict[int, int] + _new_objects: list[Any] """ - Dictionary from a callables id to the index in the callables array. + List of objects parsed out of the most recently encoded document. """ - _objects: list[Any] + _next_object_id: int """ - List of objects parsed out of the document + The next object id to use. Increment for each new object encountered. """ - _object_id_dict: dict[int, int] + _object_id_dict: WeakKeyDictionary[Any, int] """ - Dictionary from an objects id to the index in the objects array. + Dictionary from an object to the ID assigned to it """ - def __init__(self, *args, callable_id_prefix: str = "cb", **kwargs): + def __init__( + self, + callable_id_prefix: str = DEFAULT_CALLABLE_ID_PREFIX, + *args: Any, + **kwargs: Any, + ): """ Create a new NodeEncoder. @@ -54,55 +93,69 @@ def __init__(self, *args, callable_id_prefix: str = "cb", **kwargs): """ super().__init__(*args, **kwargs) self._callable_id_prefix = callable_id_prefix - self._callables = [] - self._callable_id_dict = {} - self._objects = [] - self._object_id_dict = {} - - def default(self, node: Any): - if isinstance(node, RenderedNode): - return self._convert_rendered_node(node) - elif callable(node): - return self._convert_callable(node) + self._next_callable_id = 0 + self._callable_dict = WeakKeyDictionary() + self._new_objects = [] + self._next_object_id = 0 + self._object_id_dict = WeakKeyDictionary() + + def default(self, o: Any): + if isinstance(o, RenderedNode): + return self._convert_rendered_node(o) + elif callable(o): + return self._convert_callable(o) else: try: - return super().default(node) + return super().default(o) except TypeError: # This is a non-serializable object. We'll store a reference to the object in the objects array. - return self._convert_object(node) + return self._convert_object(o) - @property - def callables(self) -> list[Callable]: - return self._callables + def encode(self, o: Any) -> str: + # Raise an error - should call encode_node instead + raise NotImplementedError("Use encode_node instead") - @property - def objects(self) -> list[Any]: - return self._objects + def encode_node(self, node: RenderedNode) -> NodeEncoderResult: + """ + Encode the document, and return the encoded document and the list of new objects. + + Args: + o: The document to encode + """ + # Reset the new objects list - they will get set when encoding + self._new_objects = [] + encoded_node = super().encode(node) + return { + "encoded_node": encoded_node, + "new_objects": self._new_objects, + "callable_id_dict": self._callable_dict, + } def _convert_rendered_node(self, node: RenderedNode): - result = {ELEMENT_KEY: node.name} + result: dict[str, Any] = {ELEMENT_KEY: node.name} if node.props is not None: result["props"] = node.props return result - def _convert_callable(self, cb: callable): - callable_id = id(cb) - callable_index = self._callable_id_dict.get(callable_id, len(self._callables)) - if callable_index == len(self._callables): - self._callables.append(cb) - self._callable_id_dict[callable_id] = callable_index + def _convert_callable(self, cb: Callable[..., Any]): + callable_id = self._callable_dict.get(cb) + if callable_id == None: + callable_id = f"{self._callable_id_prefix}{self._next_callable_id}" + self._next_callable_id += 1 + self._callable_dict[cb] = callable_id return { - CALLABLE_KEY: f"{self._callable_id_prefix}{callable_index}", + CALLABLE_KEY: callable_id, } def _convert_object(self, obj: Any): - object_id = id(obj) - object_index = self._object_id_dict.get(object_id, len(self._objects)) - if object_index == len(self._objects): - self._objects.append(obj) - self._object_id_dict[object_id] = object_index + object_id = self._object_id_dict.get(obj) + if object_id == None: + object_id = self._next_object_id + self._next_object_id += 1 + self._object_id_dict[obj] = object_id + self._new_objects.append(obj) return { - OBJECT_KEY: object_index, + OBJECT_KEY: object_id, } diff --git a/plugins/ui/src/deephaven/ui/renderer/RenderedNode.py b/plugins/ui/src/deephaven/ui/renderer/RenderedNode.py index 809a7c8fa..db9b71555 100644 --- a/plugins/ui/src/deephaven/ui/renderer/RenderedNode.py +++ b/plugins/ui/src/deephaven/ui/renderer/RenderedNode.py @@ -1,4 +1,6 @@ from __future__ import annotations +from typing import Optional +from ..elements import PropsType class RenderedNode: @@ -7,9 +9,9 @@ class RenderedNode: """ _name: str - _props: dict | None + _props: Optional[PropsType] - def __init__(self, name: str, props: dict = None): + def __init__(self, name: str, props: Optional[PropsType] = None): """ Stores the result of a rendered node @@ -28,7 +30,7 @@ def name(self) -> str: return self._name @property - def props(self) -> dict | None: + def props(self) -> Optional[PropsType]: """ Get the props of the node. """ diff --git a/plugins/ui/src/js/src/ObjectView.tsx b/plugins/ui/src/js/src/ObjectView.tsx index 987068ef5..108d42d68 100644 --- a/plugins/ui/src/js/src/ObjectView.tsx +++ b/plugins/ui/src/js/src/ObjectView.tsx @@ -13,7 +13,11 @@ function ObjectView(props: ObjectViewProps) { const { object } = props; log.info('Object is', object); - const fetch = useCallback(() => object.fetch() as Promise, [object]); + const fetch = useCallback(async () => { + // We re-export the object in case this object is used in multiple places or closed/opened multiple times + const reexportedObject = await object.reexport(); + return reexportedObject.fetch() as Promise; + }, [object]); const plugins = usePlugins(); diff --git a/plugins/ui/src/js/src/UITable.tsx b/plugins/ui/src/js/src/UITable.tsx index 7106650b8..d1f971df5 100644 --- a/plugins/ui/src/js/src/UITable.tsx +++ b/plugins/ui/src/js/src/UITable.tsx @@ -19,12 +19,20 @@ function UITable({ element }: UITableProps) { // Just load the object on mount useEffect(() => { + let isCancelled = false; async function loadModel() { log.debug('Loading table from props', element.props); - const newTable = (await element.props.table.fetch()) as Table; + const reexportedTable = await element.props.table.reexport(); + const newTable = (await reexportedTable.fetch()) as Table; + if (isCancelled) { + newTable.close(); + } setTable(newTable); } loadModel(); + return () => { + isCancelled = true; + }; }, [dh, element]); const irisGridProps: Partial = useMemo(() => { diff --git a/plugins/ui/src/js/src/WidgetHandler.tsx b/plugins/ui/src/js/src/WidgetHandler.tsx index ffdc64fa3..9f2df8dfb 100644 --- a/plugins/ui/src/js/src/WidgetHandler.tsx +++ b/plugins/ui/src/js/src/WidgetHandler.tsx @@ -1,7 +1,13 @@ /** * Handles document events for one widget. */ -import React, { useCallback, useEffect, useMemo, useState } from 'react'; +import React, { + useCallback, + useEffect, + useMemo, + useRef, + useState, +} from 'react'; import { JSONRPCClient, JSONRPCServer, @@ -36,12 +42,13 @@ function WidgetHandler({ onClose, widget: wrapper }: WidgetHandlerProps) { const [widget, setWidget] = useState(); const [element, setElement] = useState(); - useEffect( - () => () => { - widget?.close(); - }, - [widget] + // When we fetch a widget, the client is then responsible for the exported objects. + // These objects could stay alive even after the widget is closed if we wanted to, + // but for our use case we want to close them when the widget is closed, so we close them all on unmount. + const exportedObjectMap = useRef>( + new Map() ); + const exportedObjectCount = useRef(0); // Bi-directional communication as defined in https://www.npmjs.com/package/json-rpc-2.0 const jsonClient = useMemo( @@ -57,7 +64,7 @@ function WidgetHandler({ onClose, widget: wrapper }: WidgetHandlerProps) { : null, [widget] ); - const parseData = useCallback( + const parseDocument = useCallback( /** * Parse the data from the server, replacing any callable nodes with functions that call the server. * Replaces all Callables with an async callback that will automatically call the server use JSON-RPC. @@ -65,11 +72,14 @@ function WidgetHandler({ onClose, widget: wrapper }: WidgetHandlerProps) { * Element nodes are not replaced. Those are handled in `DocumentHandler`. * * @param data The data to parse - * @param exportedObjects The exported objects to use for re-hydrating objects * @returns The parsed data */ - (data: string, exportedObjects: WidgetExportedObject[]) => - JSON.parse(data, (key, value) => { + (data: string) => { + // Keep track of exported objects that are no longer in use after this render. + // We close those objects that are no longer referenced, as they will never be referenced again. + const deadObjectMap = new Map(exportedObjectMap.current); + + const parsedData = JSON.parse(data, (key, value) => { // Need to re-hydrate any objects that are defined if (isCallableNode(value)) { const callableId = value[CALLABLE_KEY]; @@ -81,14 +91,51 @@ function WidgetHandler({ onClose, widget: wrapper }: WidgetHandlerProps) { } if (isObjectNode(value)) { // Replace this node with the exported object - return exportedObjects[value[OBJECT_KEY]]; + const objectKey = value[OBJECT_KEY]; + const exportedObject = exportedObjectMap.current.get(objectKey); + if (exportedObject === undefined) { + // The map should always have the exported object for a key, otherwise the protocol is broken + throw new Error(`Invalid exported object key ${objectKey}`); + } + deadObjectMap.delete(objectKey); + return exportedObject; } return value; - }), + }); + + // Close any objects that are no longer referenced + deadObjectMap.forEach((deadObject, objectKey) => { + log.debug('Closing dead object', objectKey); + deadObject.close(); + exportedObjectMap.current.delete(objectKey); + }); + + log.debug2( + 'Parsed data', + parsedData, + 'exportedObjectMap', + exportedObjectMap.current, + 'deadObjectMap', + deadObjectMap + ); + return parsedData; + }, [jsonClient] ); + const updateExportedObjects = useCallback( + (newExportedObjects: WidgetExportedObject[]) => { + for (let i = 0; i < newExportedObjects.length; i += 1) { + const exportedObject = newExportedObjects[i]; + const exportedObjectKey = exportedObjectCount.current; + exportedObjectCount.current += 1; + exportedObjectMap.current.set(exportedObjectKey, exportedObject); + } + }, + [] + ); + useEffect( function initMethods() { if (jsonClient == null) { @@ -96,29 +143,34 @@ function WidgetHandler({ onClose, widget: wrapper }: WidgetHandlerProps) { } log.debug('Adding methods to jsonClient'); - jsonClient.addMethod('documentUpdated', async (params: [ElementNode]) => { + jsonClient.addMethod('documentUpdated', async (params: [string]) => { log.debug2('documentUpdated', params[0]); - setElement(params[0]); + const newDocument = parseDocument(params[0]); + setElement(newDocument); }); return () => { jsonClient.rejectAllPendingRequests('Widget was changed'); }; }, - [jsonClient] + [jsonClient, parseDocument] ); useEffect(() => { if (widget == null) { return; } + // Need to reset the exported object map and count + const widgetExportedObjectMap = new Map(); + exportedObjectMap.current = widgetExportedObjectMap; + exportedObjectCount.current = 0; function receiveData( data: string, - exportedObjects: WidgetExportedObject[] + newExportedObjects: WidgetExportedObject[] ) { - log.debug2('Data received', data, exportedObjects); - const parsedData = parseData(data, exportedObjects); - jsonClient?.receiveAndSend(parsedData); + log.debug2('Data received', data, newExportedObjects); + updateExportedObjects(newExportedObjects); + jsonClient?.receiveAndSend(JSON.parse(data)); } const cleanup = widget.addEventListener( @@ -136,10 +188,16 @@ function WidgetHandler({ onClose, widget: wrapper }: WidgetHandlerProps) { receiveData(widget.getDataAsString(), widget.exportedObjects); return () => { - log.debug('Cleaning up listener'); + log.debug('Cleaning up widget', widget); cleanup(); + widget.close(); + + // Clean up any exported objects that haven't been closed yet + Array.from(widgetExportedObjectMap.values()).forEach(exportedObject => { + exportedObject.close(); + }); }; - }, [dh, jsonClient, parseData, widget]); + }, [dh, jsonClient, parseDocument, updateExportedObjects, widget]); useEffect( function loadWidget() { @@ -149,6 +207,9 @@ function WidgetHandler({ onClose, widget: wrapper }: WidgetHandlerProps) { const newWidget = await wrapper.fetch(); if (isCancelled) { newWidget.close(); + newWidget.exportedObjects.forEach(exportedObject => { + exportedObject.close(); + }); return; } log.debug('newWidget', wrapper.id, wrapper.definition, newWidget); diff --git a/plugins/ui/src/js/src/WidgetTestUtils.ts b/plugins/ui/src/js/src/WidgetTestUtils.ts index 1db3930f0..ad11e9264 100644 --- a/plugins/ui/src/js/src/WidgetTestUtils.ts +++ b/plugins/ui/src/js/src/WidgetTestUtils.ts @@ -9,7 +9,7 @@ export function makeDocumentUpdatedJsonRpc( return { jsonrpc: '2.0', method: 'documentUpdated', - params: [document], + params: [JSON.stringify(document)], }; } @@ -34,10 +34,12 @@ export function makeWidgetDefinition({ export function makeWidget({ addEventListener = jest.fn(() => jest.fn()), getDataAsString = () => makeDocumentUpdatedJsonRpcString(), + exportedObjects = [], }: Partial = {}): Widget { return TestUtils.createMockProxy({ addEventListener, getDataAsString, + exportedObjects, }); } diff --git a/plugins/ui/src/js/src/WidgetTypes.ts b/plugins/ui/src/js/src/WidgetTypes.ts index f0ca0ca3c..c4ed147a3 100644 --- a/plugins/ui/src/js/src/WidgetTypes.ts +++ b/plugins/ui/src/js/src/WidgetTypes.ts @@ -9,7 +9,7 @@ export interface WidgetMessageDetails { export type WidgetMessageEvent = CustomEvent; -export type WidgetFetch = () => Promise; +export type WidgetFetch = (takeOwnership?: boolean) => Promise; export type WidgetWrapper = { definition: WidgetDefinition; diff --git a/plugins/ui/test/deephaven/ui/test_encoder.py b/plugins/ui/test/deephaven/ui/test_encoder.py index 2c8854d10..2d06e4f51 100644 --- a/plugins/ui/test/deephaven/ui/test_encoder.py +++ b/plugins/ui/test/deephaven/ui/test_encoder.py @@ -30,14 +30,18 @@ def expect_result( from deephaven.ui.renderer import NodeEncoder encoder = NodeEncoder(callable_id_prefix=callable_id_prefix) - payload = encoder.encode(node) + result = encoder.encode_node(node) self.assertDictEqual( - json.loads(payload), expected_payload, "payloads don't match" + json.loads(result["encoded_node"]), expected_payload, "payloads don't match" ) self.assertListEqual( - encoder.callables, expected_callables, "callables don't match" + list(result["callable_id_dict"].keys()), + expected_callables, + "callables don't match", + ) + self.assertListEqual( + result["new_objects"], expected_objects, "objects don't match" ) - self.assertListEqual(encoder.objects, expected_objects, "objects don't match") def test_empty_document(self): self.expect_result(make_node(""), {"__dhElemName": ""}) @@ -354,6 +358,139 @@ def test_callable_id_prefix(self): callable_id_prefix="d2c", ) + def test_delta_objects(self): + """ + Test that the encoder retains IDs for objects and callables that are the same between encodings. + Also check that the encoder only sends new objects in the most recent encoding. + """ + + from deephaven.ui.renderer import NodeEncoder + + objs = [TestObject(), TestObject(), TestObject()] + cbs = [lambda: None, lambda: None, lambda: None] + + # Should use a depth-first traversal to find all exported objects and their indices + encoder = NodeEncoder() + node = make_node( + "test0", + { + "children": [ + make_node("test1", {"foo": [cbs[0]]}), + cbs[1], + make_node("test3", {"bar": cbs[2], "children": [objs[0], objs[1]]}), + make_node("test4", {"children": [objs[0], objs[2]]}), + objs[1], + make_node("test6", {"children": [objs[2]]}), + ] + }, + ) + + result = encoder.encode_node(node) + + expected_payload = { + "__dhElemName": "test0", + "props": { + "children": [ + { + "__dhElemName": "test1", + "props": {"foo": [{"__dhCbid": "cb0"}]}, + }, + {"__dhCbid": "cb1"}, + { + "__dhElemName": "test3", + "props": { + "bar": {"__dhCbid": "cb2"}, + "children": [ + {"__dhObid": 0}, + {"__dhObid": 1}, + ], + }, + }, + { + "__dhElemName": "test4", + "props": {"children": [{"__dhObid": 0}, {"__dhObid": 2}]}, + }, + {"__dhObid": 1}, + { + "__dhElemName": "test6", + "props": {"children": [{"__dhObid": 2}]}, + }, + ], + }, + } + + self.assertDictEqual( + json.loads(result["encoded_node"]), expected_payload, "payloads don't match" + ) + self.assertListEqual( + list(result["callable_id_dict"].keys()), cbs, "callables don't match" + ) + self.assertListEqual(result["new_objects"], objs, "objects don't match") + + # Add some new objects and callables to the mix for next encoding + delta_objs = [TestObject()] + delta_cbs = [lambda: None] + objs = [objs[0], None, objs[2], delta_objs[0]] + cbs = [cbs[0], None, cbs[2], delta_cbs[0]] + + # Replace cb[1] and obj[1] with the new callable/object and encode again + node = make_node( + "test0", + { + "children": [ + make_node("test1", {"foo": [cbs[0]]}), + cbs[3], + make_node("test3", {"bar": cbs[2], "children": [objs[0], objs[3]]}), + make_node("test4", {"children": [objs[0], objs[2]]}), + objs[3], + make_node("test6", {"children": [objs[2]]}), + ] + }, + ) + + result = encoder.encode_node(node) + expected_payload = { + "__dhElemName": "test0", + "props": { + "children": [ + { + "__dhElemName": "test1", + "props": {"foo": [{"__dhCbid": "cb0"}]}, + }, + {"__dhCbid": "cb3"}, + { + "__dhElemName": "test3", + "props": { + "bar": {"__dhCbid": "cb2"}, + "children": [ + {"__dhObid": 0}, + {"__dhObid": 3}, + ], + }, + }, + { + "__dhElemName": "test4", + "props": {"children": [{"__dhObid": 0}, {"__dhObid": 2}]}, + }, + {"__dhObid": 3}, + { + "__dhElemName": "test6", + "props": {"children": [{"__dhObid": 2}]}, + }, + ], + }, + } + + self.assertDictEqual( + json.loads(result["encoded_node"]), expected_payload, "payloads don't match" + ) + self.assertListEqual( + list(result["callable_id_dict"].keys()), + [cbs[0], cbs[2], cbs[3]], + "callables don't match", + ) + self.assertListEqual(result["new_objects"], delta_objs, "objects don't match") + if __name__ == "__main__": unittest.main()