Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Including missing improvement that was available on the original project #29

Merged
merged 2 commits into from
May 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "foxbox",
"version": "1.0.2",
"version": "1.0.3",
"license": "MPL-2.0",
"private": true,
"productName": "Foxbox",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

/* eslint-disable jest/no-conditional-expect */

import { renderHook, act } from "@testing-library/react";
import { act, renderHook } from "@testing-library/react";
import { PropsWithChildren, useCallback, useState } from "react";
import { DeepPartial } from "ts-essentials";

Expand All @@ -29,7 +29,7 @@ import MockCurrentLayoutProvider from "@foxglove/studio-base/providers/CurrentLa
import delay from "@foxglove/studio-base/util/delay";
import { makeMockAppConfiguration } from "@foxglove/studio-base/util/makeMockAppConfiguration";

import { MessagePipelineProvider, useMessagePipeline, MessagePipelineContext } from ".";
import { MessagePipelineContext, MessagePipelineProvider, useMessagePipeline } from ".";
import FakePlayer from "./FakePlayer";
import { MAX_PROMISE_TIMEOUT_TIME_MS } from "./pauseFrameForPromise";

Expand Down Expand Up @@ -136,7 +136,7 @@ describe("MessagePipelineProvider/useMessagePipeline", () => {
playerState: {
activeData: undefined,
capabilities: [],
presence: PlayerPresence.NOT_PRESENT,
presence: PlayerPresence.INITIALIZING,
playerId: "",
progress: {},
},
Expand Down Expand Up @@ -199,7 +199,7 @@ describe("MessagePipelineProvider/useMessagePipeline", () => {
expect(all[0]!.playerState).toEqual({
activeData: undefined,
capabilities: [],
presence: PlayerPresence.NOT_PRESENT,
presence: PlayerPresence.INITIALIZING,
playerId: "",
progress: {},
});
Expand Down Expand Up @@ -259,7 +259,7 @@ describe("MessagePipelineProvider/useMessagePipeline", () => {
playerState: {
activeData: undefined,
capabilities: [],
presence: PlayerPresence.NOT_PRESENT,
presence: PlayerPresence.INITIALIZING,
playerId: "",
progress: {},
},
Expand Down
14 changes: 5 additions & 9 deletions packages/studio-base/src/components/MessagePipeline/store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,16 @@ import {
PlayerState,
SubscribePayload,
} from "@foxglove/studio-base/players/types";
import { assertNever } from "@foxglove/studio-base/util/assertNever";
import isDesktopApp from "@foxglove/studio-base/util/isDesktopApp";

import { FramePromise } from "./pauseFrameForPromise";
import { MessagePipelineContext } from "./types";

export function defaultPlayerState(): PlayerState {
export function defaultPlayerState(player?: Player): PlayerState {
return {
presence: PlayerPresence.NOT_PRESENT,
// when there is a player we default to initializing, to prevent thrashing in the UI when
// the player is initialized.
presence: player ? PlayerPresence.INITIALIZING : PlayerPresence.NOT_PRESENT,
progress: {},
capabilities: [],
profile: undefined,
Expand Down Expand Up @@ -138,7 +139,7 @@ export function createMessagePipelineStore({
},

public: {
playerState: defaultPlayerState(),
playerState: defaultPlayerState(initialPlayer),
messageEventsBySubscriberId: new Map(),
subscriptions: [],
sortedTopics: [],
Expand Down Expand Up @@ -423,11 +424,6 @@ export function reducer(
};
}
}

assertNever(
action,
`Unhandled message pipeline action type ${(action as MessagePipelineStateAction).type}`,
);
}

async function builtinFetch(url: string, opts?: { signal?: AbortSignal }) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { CSSProperties, useEffect, useLayoutEffect, useMemo, useRef, useState }
import { useLatest } from "react-use";
import { v4 as uuid } from "uuid";

import { useValueChangedDebugLog, useSynchronousMountedState } from "@foxglove/hooks";
import { useSynchronousMountedState, useValueChangedDebugLog } from "@foxglove/hooks";
import Logger from "@foxglove/log";
import { fromSec, toSec } from "@foxglove/rostime";
import {
Expand Down Expand Up @@ -42,11 +42,12 @@ import useGlobalVariables from "@foxglove/studio-base/hooks/useGlobalVariables";
import {
AdvertiseOptions,
PlayerCapabilities,
PlayerPresence,
SubscribePayload,
} from "@foxglove/studio-base/players/types";
import {
usePanelSettingsTreeUpdate,
useDefaultPanelTitle,
usePanelSettingsTreeUpdate,
} from "@foxglove/studio-base/providers/PanelStateContextProvider";
import { PanelConfig, SaveConfig } from "@foxglove/studio-base/types/panels";
import { assertNever } from "@foxglove/studio-base/util/assertNever";
Expand Down Expand Up @@ -118,7 +119,7 @@ function PanelExtensionAdapter(
const { playerState, pauseFrame, setSubscriptions, seekPlayback, sortedTopics } =
messagePipelineContext;

const { capabilities, profile: dataSourceProfile } = playerState;
const { capabilities, profile: dataSourceProfile, presence: playerPresence } = playerState;

const { openSiblingPanel, setMessagePathDropConfig } = usePanelContext();

Expand Down Expand Up @@ -547,6 +548,8 @@ function PanelExtensionAdapter(
);
}, [initialState, highestSupportedConfigVersion]);

const playerIsInitializing = playerPresence === PlayerPresence.INITIALIZING;

// Manage extension lifecycle by calling initPanel() when the panel context changes.
//
// If we useEffect here instead of useLayoutEffect, the prevRenderState can get polluted with data
Expand All @@ -556,9 +559,12 @@ function PanelExtensionAdapter(
throw new Error("Expected panel container to be mounted");
}

// If the config is too new for this panel to support we bail and don't do any panel initialization
// We will instead show a warning message to the user
if (configTooNew) {
// Also don't show panel when the player is initializing. The initializing state is temporary for
// players to go through to load their sources. Once a player has completed initialization `initPanel` is called again (or even a few times),
// because parts of the player context have changed. This cleans up the old panel that was present
// during initialization. So there can be no state held between extension panels between initialization and
// whatever follows it. To prevent this unnecessary render, we do not render the panel during initialization.
if (configTooNew || playerIsInitializing) {
return;
}

Expand Down Expand Up @@ -596,7 +602,14 @@ function PanelExtensionAdapter(
getMessagePipelineContext().setSubscriptions(panelId, []);
getMessagePipelineContext().setPublishers(panelId, []);
};
}, [initPanel, panelId, partialExtensionContext, getMessagePipelineContext, configTooNew]);
}, [
initPanel,
panelId,
partialExtensionContext,
getMessagePipelineContext,
configTooNew,
playerIsInitializing,
]);

const style: CSSProperties = {};
if (slowRender) {
Expand Down
2 changes: 1 addition & 1 deletion packages/studio/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@foxglove/studio",
"version": "1.0.2",
"version": "1.0.3",
"license": "MPL-2.0",
"repository": {
"type": "git",
Expand Down
Loading