diff --git a/packages/suite-base/src/components/PanelExtensionAdapter/PanelExtensionAdapter.test.tsx b/packages/suite-base/src/components/PanelExtensionAdapter/PanelExtensionAdapter.test.tsx index cb0b358421..3d5ac01d0d 100644 --- a/packages/suite-base/src/components/PanelExtensionAdapter/PanelExtensionAdapter.test.tsx +++ b/packages/suite-base/src/components/PanelExtensionAdapter/PanelExtensionAdapter.test.tsx @@ -22,6 +22,7 @@ import { MessageEvent, Immutable, Subscription, + SettingsTreeAction, } from "@lichtblick/suite"; import MockPanelContextProvider from "@lichtblick/suite-base/components/MockPanelContextProvider"; import { AlertsContext, AlertsContextStore } from "@lichtblick/suite-base/context/AlertsContext"; @@ -71,6 +72,83 @@ describe("PanelExtensionAdapter", () => { await sig; }); + it("routes nested converter settings through the panel updater", () => { + // GIVEN a panel that handles converter settings updates + const saveConfig = jest.fn(); + const updateSettingsTree = jest.fn(); + const settingsTreeSpy = jest + .spyOn(PanelStateContextProvider, "usePanelSettingsTreeUpdate") + .mockReturnValue(updateSettingsTree); + const config = { topics: { topic: { child: { enabled: false } } } }; + const panelActionHandler = jest.fn(); + const extensionHandler = jest.fn(); + const updateTopicSettings = jest.fn((updateSettings) => updateSettings(config)); + const initPanel = (context: BuiltinPanelExtensionContext) => { + context.updatePanelSettingsEditor( + { actionHandler: panelActionHandler, nodes: {} }, + updateTopicSettings, + ); + }; + + const view = render( + + ({}), + panelSettings: { + "3D": { + settings: () => ({}), + handler: extensionHandler, + }, + }, + }, + ], + }} + > + + + , + ); + + // WHEN a nested setting is enabled + const action: SettingsTreeAction = { + action: "update", + payload: { path: ["topics", "topic", "child", "enabled"], input: "boolean", value: true }, + }; + act(() => { + updateSettingsTree.mock.calls[0]![0].actionHandler(action); + }); + + // THEN the panel applies the edit once without a duplicate adapter save + expect(updateTopicSettings).toHaveBeenCalledTimes(1); + expect(extensionHandler).toHaveBeenCalledWith(action, config.topics.topic); + expect(extensionHandler).toHaveBeenCalledTimes(1); + expect(saveConfig).not.toHaveBeenCalled(); + + // WHEN a normal panel setting is changed + const panelAction: SettingsTreeAction = { + action: "update", + payload: { path: ["scene", "enableStats"], input: "boolean", value: true }, + }; + act(() => { + updateSettingsTree.mock.calls.at(-1)![0].actionHandler(panelAction); + }); + + // THEN only the panel handler receives the edit without another converter update or save + expect(panelActionHandler).toHaveBeenLastCalledWith(panelAction); + expect(panelActionHandler).toHaveBeenCalledTimes(2); + expect(updateTopicSettings).toHaveBeenCalledTimes(1); + expect(extensionHandler).toHaveBeenCalledTimes(1); + expect(saveConfig).not.toHaveBeenCalled(); + view.unmount(); + settingsTreeSpy.mockRestore(); + }); + it("should expose point-in-time message lookups through the panel context", async () => { // GIVEN - a panel that captures its extension context let panelContext: PanelExtensionContext | undefined; diff --git a/packages/suite-base/src/components/PanelExtensionAdapter/PanelExtensionAdapter.tsx b/packages/suite-base/src/components/PanelExtensionAdapter/PanelExtensionAdapter.tsx index 5d44bd5384..5d44a3c0ba 100644 --- a/packages/suite-base/src/components/PanelExtensionAdapter/PanelExtensionAdapter.tsx +++ b/packages/suite-base/src/components/PanelExtensionAdapter/PanelExtensionAdapter.tsx @@ -69,7 +69,11 @@ import { maybeCast } from "@lichtblick/suite-base/util/maybeCast"; import { PanelConfigVersionError } from "./PanelConfigVersionError"; import { RenderStateConfig, initRenderStateBuilder } from "./renderState"; -import { BuiltinPanelExtensionContext, MessageConverterAlertHandler } from "./types"; +import { + BuiltinPanelExtensionContext, + MessageConverterAlertHandler, + TopicSettingsUpdater, +} from "./types"; import { useSharedPanelState } from "./useSharedPanelState"; import { useSubscribeMessageRange } from "./useSubscribeMessageRange"; @@ -392,7 +396,12 @@ function PanelExtensionAdapter( }, }; - const extensionSettingsActionHandler = (action: SettingsTreeAction) => { + const extensionSettingsActionHandler = ( + action: SettingsTreeAction, + updateTopicSettings: TopicSettingsUpdater = (updateSettings) => { + saveConfig(produce(updateSettings)); + }, + ) => { if (action.action === "reorder-node") { return; // Extensions don't support reordering } @@ -400,25 +409,25 @@ function PanelExtensionAdapter( payload: { path }, } = action; - saveConfig( - produce<{ topics: Record }>((draft) => { - const [category, topicName] = path; + const [category, topicName] = path; - if (category === "topics" && topicName != undefined) { - const topicToSchemaNameMap = getTopicToSchemaNameMap(messagePipelineState()); - const schemaName = topicToSchemaNameMap[topicName]; + if (category === "topics" && topicName != undefined) { + const topicToSchemaNameMap = getTopicToSchemaNameMap(messagePipelineState()); + const schemaName = topicToSchemaNameMap[topicName]; - if (schemaName == undefined) { - return; - } + if (schemaName == undefined) { + return; + } - extensionsSettings[panelName]?.[schemaName]?.handler(action, draft.topics[topicName]); - setForceConversion((_old) => { - return new Set([topicName]); - }); - } - }), - ); + const settings = extensionsSettings[panelName]?.[schemaName]; + if (settings) { + updateTopicSettings((draft) => { + draft.topics[topicName] ??= {}; + settings.handler(action, draft.topics[topicName]); + }); + setForceConversion(() => new Set([topicName])); + } + } }; return { @@ -643,13 +652,16 @@ function PanelExtensionAdapter( setSubscribedAppSettings(settings); }, - updatePanelSettingsEditor: (settings: SettingsTree) => { + updatePanelSettingsEditor: ( + settings: SettingsTree, + updateTopicSettings?: TopicSettingsUpdater, + ) => { if (!isMounted()) { return; } const actionHandler: typeof settings.actionHandler = (action) => { settings.actionHandler(action); - extensionSettingsActionHandler(action); + extensionSettingsActionHandler(action, updateTopicSettings); }; updatePanelSettingsTree({ ...settings, actionHandler }); }, diff --git a/packages/suite-base/src/components/PanelExtensionAdapter/types.ts b/packages/suite-base/src/components/PanelExtensionAdapter/types.ts index 4bac4ec421..46ac11a8d0 100644 --- a/packages/suite-base/src/components/PanelExtensionAdapter/types.ts +++ b/packages/suite-base/src/components/PanelExtensionAdapter/types.ts @@ -5,7 +5,12 @@ // License, v2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at http://mozilla.org/MPL/2.0/ -import { Immutable, MessageConverterAlert, PanelExtensionContext } from "@lichtblick/suite"; +import { + Immutable, + MessageConverterAlert, + PanelExtensionContext, + SettingsTree, +} from "@lichtblick/suite"; import { IteratorResult } from "@lichtblick/suite-base/players/IterablePlayer/IIterableSource"; import { PlayerAlert, Topic } from "@lichtblick/suite-base/players/types"; import { InstalledMessageConverter } from "@lichtblick/suite-base/types/messageConverters"; @@ -64,6 +69,12 @@ export type MessagePathDropConfig = { * extensions. */ export type BuiltinPanelExtensionContext = { + /** Apply converter settings to the panel's current config before saving. */ + updatePanelSettingsEditor: ( + settings: SettingsTree, + updateTopicSettings?: TopicSettingsUpdater, + ) => void; + /** * Fetch an asset from Studio's asset manager. * @@ -95,7 +106,11 @@ export type BuiltinPanelExtensionContext = { * clears the alert with the given `alertId`. */ unstable_setAlert?: (alertId: string, alert: Immutable | undefined) => void; -} & PanelExtensionContext; +} & Omit; + +export type TopicSettingsUpdater = ( + updateSettings: (config: { topics: Record }) => void, +) => void; export type MessageConverterAlertHandler = ( converter: InstalledMessageConverter, diff --git a/packages/suite-base/src/panels/ThreeDeeRender/ThreeDeeRender.test.tsx b/packages/suite-base/src/panels/ThreeDeeRender/ThreeDeeRender.test.tsx index 0ba2cb2c03..96f845e467 100644 --- a/packages/suite-base/src/panels/ThreeDeeRender/ThreeDeeRender.test.tsx +++ b/packages/suite-base/src/panels/ThreeDeeRender/ThreeDeeRender.test.tsx @@ -9,18 +9,27 @@ import "@testing-library/jest-dom"; import { act, render, waitFor } from "@testing-library/react"; +import { produce } from "immer"; -import { Topic } from "@lichtblick/suite"; -import { BuiltinPanelExtensionContext } from "@lichtblick/suite-base/components/PanelExtensionAdapter"; +import { SettingsTreeAction, Topic } from "@lichtblick/suite"; +import MockPanelContextProvider from "@lichtblick/suite-base/components/MockPanelContextProvider"; +import { + BuiltinPanelExtensionContext, + PanelExtensionAdapter, +} from "@lichtblick/suite-base/components/PanelExtensionAdapter"; import { useAnalytics } from "@lichtblick/suite-base/context/AnalyticsContext"; import { DEFAULT_FOLLOW_MODE, MAX_TRANSFORM_MESSAGES, } from "@lichtblick/suite-base/panels/ThreeDeeRender/constants"; import type { MessageEvent } from "@lichtblick/suite-base/players/types"; +import * as PanelStateContextProvider from "@lichtblick/suite-base/providers/PanelStateContextProvider"; +import PanelSetup from "@lichtblick/suite-base/stories/PanelSetup"; import MessageEventBuilder from "@lichtblick/suite-base/testing/builders/MessageEventBuilder"; import RenderStateBuilder from "@lichtblick/suite-base/testing/builders/RenderStateBuilder"; +import { SaveConfig } from "@lichtblick/suite-base/types/panels"; +import { RendererConfig } from "./IRenderer"; import { Renderer } from "./Renderer"; import { ThreeDeeRender } from "./ThreeDeeRender"; import { DEFAULT_CAMERA_STATE } from "./camera"; @@ -225,6 +234,139 @@ describe("ThreeDeeRender", () => { expect(container).toBeInTheDocument(); }); + it("saves direct and nested converter edits immediately and preserves them after a later save", () => { + // GIVEN a panel with a disabled nested setting + jest.useFakeTimers(); + const renderer = { + ...createMockRenderer(), + updateConfig: (handler: (draft: RendererConfig) => void) => { + renderer.config = produce(renderer.config as RendererConfig, handler); + renderer.emit("configChange", renderer); + }, + }; + mockedRenderer.mockImplementationOnce(() => renderer as unknown as Renderer); + const initialConfig = { topics: { topic: { text: "", child: { enabled: false } } } }; + // The scene handler applies direct fields before the converter handles the same edit. + renderer.settings.handleAction.mockImplementation((action: SettingsTreeAction) => { + if (action.action === "update" && action.payload.path.length === 3) { + renderer.updateConfig((draft) => { + (draft.topics.topic as typeof initialConfig.topics.topic).text = action.payload + .value as string; + }); + } + }); + let savedConfig: unknown = initialConfig; + const saveConfig: SaveConfig = (update) => { + savedConfig = typeof update === "function" ? update(savedConfig) : update; + }; + + // AND GIVEN an adapter with a converter that handles nested settings + const updateSettingsTree = jest.fn(); + const settingsTreeSpy = jest + .spyOn(PanelStateContextProvider, "usePanelSettingsTreeUpdate") + .mockReturnValue(updateSettingsTree); + const initPanel = jest.fn(); + const adapterView = render( + + ({}), + panelSettings: { + "3D": { + settings: () => ({}), + handler: (action, config) => { + if (action.action === "update") { + const topicConfig = config as typeof initialConfig.topics.topic; + if (action.payload.path[2] === "text") { + topicConfig.text = action.payload.value as string; + } else { + topicConfig.child.enabled = action.payload.value as boolean; + } + } + }, + }, + }, + }, + ], + }} + > + + + , + ); + + // AND GIVEN a 3D panel using the adapter's settings and save methods + const context = initPanel.mock.calls.at(-1)![0]; + const props = setup(undefined, { + initialState: context.initialState, + saveState: context.saveState.bind(context), + updatePanelSettingsEditor: context.updatePanelSettingsEditor.bind(context), + }); + const view = render(); + + try { + // WHEN the nested setting is enabled + act(() => { + updateSettingsTree.mock.calls.at(-1)![0].actionHandler({ + action: "update", + payload: { + path: ["topics", "topic", "child", "enabled"], + input: "boolean", + value: true, + }, + }); + }); + + // THEN the edit is saved + expect(savedConfig).toMatchObject({ + topics: { topic: { child: { enabled: true } } }, + }); + + // WHEN text is typed in a direct field without waiting for the debounced save + for (const text of ["h", "he", "hello", "hello world"]) { + act(() => { + updateSettingsTree.mock.calls.at(-1)![0].actionHandler({ + action: "update", + payload: { path: ["topics", "topic", "text"], input: "string", value: text }, + }); + }); + + // THEN the settings editor can read the latest text after every edit + expect(savedConfig).toMatchObject({ + topics: { topic: { text, child: { enabled: true } } }, + }); + } + + // WHEN camera movement triggers a later save + act(() => { + renderer.getCameraState.mockReturnValue({ ...DEFAULT_CAMERA_STATE, distance: 10 }); + renderer.emit("cameraMove"); + }); + act(() => { + jest.advanceTimersByTime(100); + }); + + // THEN both converter settings survive the later save + expect(savedConfig).toMatchObject({ + topics: { topic: { text: "hello world", child: { enabled: true } } }, + }); + } finally { + view.unmount(); + adapterView.unmount(); + settingsTreeSpy.mockRestore(); + jest.useRealTimers(); + } + }); + it("renders a canvas element", () => { // Given const props = setup(); diff --git a/packages/suite-base/src/panels/ThreeDeeRender/ThreeDeeRender.tsx b/packages/suite-base/src/panels/ThreeDeeRender/ThreeDeeRender.tsx index 1bf7879add..5aec84f1af 100644 --- a/packages/suite-base/src/panels/ThreeDeeRender/ThreeDeeRender.tsx +++ b/packages/suite-base/src/panels/ThreeDeeRender/ThreeDeeRender.tsx @@ -26,6 +26,7 @@ import { Topic, } from "@lichtblick/suite"; import { AppSetting } from "@lichtblick/suite-base/AppSetting"; +import { TopicSettingsUpdater } from "@lichtblick/suite-base/components/PanelExtensionAdapter/types"; import { useAnalytics } from "@lichtblick/suite-base/context/AnalyticsContext"; import { DEFAULT_SCENE_EXTENSION_CONFIG } from "@lichtblick/suite-base/panels/ThreeDeeRender/SceneExtensionConfig"; import { @@ -132,6 +133,7 @@ export function ThreeDeeRender(props: Readonly): React.JSX. }; }); const configRef = useLatest(config); + const lastSavedConfig = useRef>(); const { cameraState } = config; const backgroundColor = config.scene.backgroundColor; @@ -358,13 +360,35 @@ export function ThreeDeeRender(props: Readonly): React.JSX. // Rebuild the settings sidebar tree as needed useEffect(() => { - context.updatePanelSettingsEditor({ - actionHandler, - enableFilter: true, - focusedPath: focusedSettingsPath, - nodes: settingsTree ?? {}, - }); - }, [actionHandler, context, focusedSettingsPath, settingsTree]); + let topicsBeforeAction = renderer?.config.topics; + + // Detect direct field edits even when the scene handler applies them first. + const handleSettingsAction = (action: SettingsTreeAction) => { + topicsBeforeAction = renderer?.config.topics; + actionHandler(action); + }; + + // Save immediately so stale editor values cannot overwrite the latest edits. + const applyConverterSettings: TopicSettingsUpdater = (updateSettings) => { + if (renderer) { + renderer.updateConfig(updateSettings); + if (renderer.config.topics !== topicsBeforeAction) { + saveState(renderer.config); + lastSavedConfig.current = renderer.config; + } + } + }; + + context.updatePanelSettingsEditor( + { + actionHandler: handleSettingsAction, + enableFilter: true, + focusedPath: focusedSettingsPath, + nodes: settingsTree ?? {}, + }, + applyConverterSettings, + ); + }, [actionHandler, context, focusedSettingsPath, settingsTree, renderer, saveState]); // Update the renderer's reference to `config` when it changes. Note that this does *not* // automatically update the settings tree. @@ -412,7 +436,11 @@ export function ThreeDeeRender(props: Readonly): React.JSX. // Save panel settings whenever they change const throttledSave = useDebouncedCallback( (newConfig: Immutable) => { - saveState(newConfig); + // Immediate converter saves already persist this config. + if (newConfig !== lastSavedConfig.current) { + saveState(newConfig); + lastSavedConfig.current = newConfig; + } }, 100, { leading: false, trailing: true, maxWait: 1000 },