From d2c6eabb1fe313708fadd6676858466710159fda Mon Sep 17 00:00:00 2001 From: Matthew Runyon Date: Wed, 2 Oct 2024 18:22:41 -0500 Subject: [PATCH] fix: Reuse dashboard tabs when reassigning the variable (#2243) Fixes #1971 Tested with this code run 1 line at a time ```python import deephaven.ui as ui a = ui.dashboard(ui.text("Hello")) a = ui.dashboard(ui.text("World")) a = ui.text("Hello") a = ui.dashboard(ui.text("Hello")) del a ``` Also tested dashboards still load in embed-widget. There was a race condition since the `makeUseListenerFunction` hook was using `useEffect`. This caused the event to emit between the event hub being set and the listener being added. --- .../code-studio/src/main/AppMainContainer.tsx | 76 ++++++++++++++----- .../src/panels/ConsolePanel.tsx | 3 + packages/dashboard/src/DashboardEvents.ts | 38 ++++------ packages/embed-widget/src/App.tsx | 33 +++----- .../golden-layout/src/utils/EventUtils.ts | 21 +++-- 5 files changed, 95 insertions(+), 76 deletions(-) diff --git a/packages/code-studio/src/main/AppMainContainer.tsx b/packages/code-studio/src/main/AppMainContainer.tsx index d80063a935..02718496b2 100644 --- a/packages/code-studio/src/main/AppMainContainer.tsx +++ b/packages/code-studio/src/main/AppMainContainer.tsx @@ -34,10 +34,10 @@ import { getAllDashboardsData, getDashboardData, listenForCreateDashboard, + listenForCloseDashboard, PanelEvent, setDashboardData as setDashboardDataAction, setDashboardPluginData as setDashboardPluginDataAction, - stopListenForCreateDashboard, updateDashboardData as updateDashboardDataAction, } from '@deephaven/dashboard'; import { @@ -189,6 +189,8 @@ export class AppMainContainer extends Component< const { allDashboardData } = this.props; this.dashboardLayouts = new Map(); + this.createDashboardListenerRemovers = new Map(); + this.closeDashboardListenerRemovers = new Map(); this.state = { contextActions: [ @@ -275,9 +277,8 @@ export class AppMainContainer extends Component< componentWillUnmount(): void { this.deinitWidgets(); - this.dashboardLayouts.forEach(layout => { - stopListenForCreateDashboard(layout.eventHub, this.handleCreateDashboard); - }); + this.createDashboardListenerRemovers.forEach(rm => rm()); + this.closeDashboardListenerRemovers.forEach(rm => rm()); window.removeEventListener( 'beforeunload', @@ -288,6 +289,12 @@ export class AppMainContainer extends Component< /** Map from the dashboard ID to the GoldenLayout instance for that dashboard */ dashboardLayouts: Map; + /** Map from dashboard ID to remover functions for create dashboard listener */ + createDashboardListenerRemovers: Map void>; + + /** Map from dashboard ID to remover functions for close dashboard listener */ + closeDashboardListenerRemovers: Map void>; + importElement: RefObject; widgetListenerRemover?: () => void; @@ -504,16 +511,21 @@ export class AppMainContainer extends Component< const oldLayout = this.dashboardLayouts.get(activeTabKey); if (oldLayout === newLayout) return; + this.dashboardLayouts.set(activeTabKey, newLayout); + if (oldLayout != null) { - stopListenForCreateDashboard( - oldLayout.eventHub, - this.handleCreateDashboard - ); + this.createDashboardListenerRemovers.get(activeTabKey)?.(); + this.closeDashboardListenerRemovers.get(activeTabKey)?.(); } - this.dashboardLayouts.set(activeTabKey, newLayout); - - listenForCreateDashboard(newLayout.eventHub, this.handleCreateDashboard); + this.createDashboardListenerRemovers.set( + activeTabKey, + listenForCreateDashboard(newLayout.eventHub, this.handleCreateDashboard) + ); + this.closeDashboardListenerRemovers.set( + activeTabKey, + listenForCloseDashboard(newLayout.eventHub, this.handleCloseDashboard) + ); } handleCreateDashboard({ @@ -524,16 +536,38 @@ export class AppMainContainer extends Component< const newId = nanoid(); const { setDashboardPluginData } = this.props; setDashboardPluginData(newId, pluginId, data); - this.setState(({ tabs }) => ({ - tabs: [ - ...tabs, - { - key: newId, - title, - }, - ], - activeTabKey: newId, - })); + this.setState(({ tabs }) => { + const existingTab = tabs.findIndex( + ({ title: tabTitle }) => tabTitle === title + ); + if (existingTab !== -1) { + // Replace the existing tab + return { + tabs: tabs.map((tab, index) => + index === existingTab ? { key: newId, title } : tab + ), + activeTabKey: newId, + }; + } + return { + tabs: [ + ...tabs, + { + key: newId, + title, + }, + ], + activeTabKey: newId, + }; + }); + } + + handleCloseDashboard(title: string): void { + const { tabs } = this.state; + const tab = tabs.find(t => t.title === title); + if (tab != null) { + this.handleTabClose(tab.key); + } } handleWidgetMenuClick(): void { diff --git a/packages/dashboard-core-plugins/src/panels/ConsolePanel.tsx b/packages/dashboard-core-plugins/src/panels/ConsolePanel.tsx index 7299fa1e98..39d6599793 100644 --- a/packages/dashboard-core-plugins/src/panels/ConsolePanel.tsx +++ b/packages/dashboard-core-plugins/src/panels/ConsolePanel.tsx @@ -14,6 +14,7 @@ import { } from '@deephaven/console'; import { type DashboardPanelProps, + emitCloseDashboard, emitPanelOpen, LayoutManagerContext, LayoutUtils, @@ -278,6 +279,8 @@ export class ConsolePanel extends PureComponent< if (id != null) { const { glEventHub } = this.props; glEventHub.emit(PanelEvent.CLOSE, id); + // Just emit for all panels since there shouldn't be dashboard and panel name conflicts + emitCloseDashboard(glEventHub, title); } } } diff --git a/packages/dashboard/src/DashboardEvents.ts b/packages/dashboard/src/DashboardEvents.ts index 68759ae456..de4437818d 100644 --- a/packages/dashboard/src/DashboardEvents.ts +++ b/packages/dashboard/src/DashboardEvents.ts @@ -1,35 +1,23 @@ -import type { EventHub } from '@deephaven/golden-layout'; +import { makeEventFunctions } from '@deephaven/golden-layout'; export const CREATE_DASHBOARD = 'CREATE_DASHBOARD'; +export const CLOSE_DASHBOARD = 'CLOSE_DASHBOARD'; + export interface CreateDashboardPayload { pluginId: string; title: string; data: T; } -export function stopListenForCreateDashboard( - eventHub: EventHub, - handler: (p: CreateDashboardPayload) => void -): void { - try { - eventHub.off(CREATE_DASHBOARD, handler); - } catch { - // golden-layout throws if the handler is not found. Instead catch it and no-op - } -} +export const { + listen: listenForCreateDashboard, + emit: emitCreateDashboard, + useListener: useCreateDashboardListener, +} = makeEventFunctions<[detail: CreateDashboardPayload]>(CREATE_DASHBOARD); -export function listenForCreateDashboard( - eventHub: EventHub, - handler: (p: CreateDashboardPayload) => void -): () => void { - eventHub.on(CREATE_DASHBOARD, handler); - return () => stopListenForCreateDashboard(eventHub, handler); -} - -export function emitCreateDashboard( - eventHub: EventHub, - payload: CreateDashboardPayload -): void { - eventHub.emit(CREATE_DASHBOARD, payload); -} +export const { + listen: listenForCloseDashboard, + emit: emitCloseDashboard, + useListener: useCloseDashboardListener, +} = makeEventFunctions<[title: string]>(CLOSE_DASHBOARD); diff --git a/packages/embed-widget/src/App.tsx b/packages/embed-widget/src/App.tsx index ae68053cb1..9deaf26c9e 100644 --- a/packages/embed-widget/src/App.tsx +++ b/packages/embed-widget/src/App.tsx @@ -24,11 +24,10 @@ import Log from '@deephaven/log'; import { useDashboardPlugins } from '@deephaven/plugin'; import { getAllDashboardsData, - listenForCreateDashboard, type CreateDashboardPayload, setDashboardPluginData, - stopListenForCreateDashboard, emitPanelOpen, + useCreateDashboardListener, } from '@deephaven/dashboard'; import { getVariableDescriptor, @@ -147,31 +146,17 @@ function App(): JSX.Element { const [goldenLayout, setGoldenLayout] = useState(null); const [dashboardId, setDashboardId] = useState('default-embed-widget'); // Can't be DEFAULT_DASHBOARD_ID because its dashboard layout is not stored in dashboardData - const handleGoldenLayoutChange = useCallback( - (newLayout: GoldenLayout) => { - function handleCreateDashboard({ - pluginId, - data, - }: CreateDashboardPayload) { - const id = nanoid(); - dispatch(setDashboardPluginData(id, pluginId, data)); - setDashboardId(id); - } - - setGoldenLayout(oldLayout => { - if (oldLayout != null) { - stopListenForCreateDashboard( - oldLayout.eventHub, - handleCreateDashboard - ); - } - listenForCreateDashboard(newLayout.eventHub, handleCreateDashboard); - return newLayout; - }); + const handleCreateDashboard = useCallback( + ({ pluginId, data }: CreateDashboardPayload) => { + const id = nanoid(); + dispatch(setDashboardPluginData(id, pluginId, data)); + setDashboardId(id); }, [dispatch] ); + useCreateDashboardListener(goldenLayout?.eventHub, handleCreateDashboard); + const [hasEmittedWidget, setHasEmittedWidget] = useState(false); const handleDashboardInitialized = useCallback(() => { @@ -243,7 +228,7 @@ function App(): JSX.Element { ]} activeDashboard={dashboardId} onLayoutInitialized={handleDashboardInitialized} - onGoldenLayoutChange={handleGoldenLayoutChange} + onGoldenLayoutChange={setGoldenLayout} plugins={dashboardPlugins} /> diff --git a/packages/golden-layout/src/utils/EventUtils.ts b/packages/golden-layout/src/utils/EventUtils.ts index 7862d77270..83b29c546e 100644 --- a/packages/golden-layout/src/utils/EventUtils.ts +++ b/packages/golden-layout/src/utils/EventUtils.ts @@ -1,4 +1,4 @@ -import { useEffect } from 'react'; +import { useEffect, useLayoutEffect, useRef } from 'react'; import EventEmitter from './EventEmitter'; type AsArray

= P extends unknown[] ? P : [P]; @@ -16,7 +16,7 @@ export type EventEmitFunction = ( ) => void; export type EventListenerHook = ( - eventEmitter: EventEmitter, + eventEmitter: EventEmitter | null | undefined, handler: EventHandlerFunction ) => void; @@ -57,10 +57,19 @@ export function makeUseListenerFunction( event: string ): EventListenerHook { return (eventEmitter, handler) => { - useEffect( - () => listenForEvent(eventEmitter, event, handler), - [eventEmitter, handler] - ); + const eventEmitterRef = useRef(null); + const handlerRef = useRef(() => false); + + if ( + eventEmitterRef.current != eventEmitter || + handlerRef.current != handler + ) { + eventEmitterRef.current?.off(event, handlerRef.current); + eventEmitter?.on(event, handler); + } + + eventEmitterRef.current = eventEmitter; + handlerRef.current = handler; }; }