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

wip: improve modal perf #4986

Draft
wants to merge 9 commits into
base: develop
Choose a base branch
from
Draft
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
3 changes: 1 addition & 2 deletions app/packages/core/src/components/Grid/Grid.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -125,12 +125,11 @@ function Grid() {

element && spotlight.attach(element);
spotlight.addEventListener("load", mount);
spotlight.addEventListener("rowchange", set);

return () => {
freeVideos();
spotlight.removeEventListener("load", mount);
spotlight.removeEventListener("rowchange", set);

spotlight.destroy();
document.dispatchEvent(new CustomEvent("grid-unmount"));
};
Expand Down
1 change: 1 addition & 0 deletions app/packages/core/src/components/Grid/useAt.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ export default function useAt(pageReset: string) {
const set = useRecoilTransaction_UNSTABLE(
({ set }) =>
({ at, page, offset }: { at: ID; page: number; offset: number }) => {
console.log("SCROLL");
set(gridPage, page);
set(gridAt, at.description);
set(gridOffset, offset);
Expand Down
12 changes: 0 additions & 12 deletions app/packages/core/src/components/Grid/useSelect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,18 +13,6 @@ export default function useSelect(
const { init, deferred } = fos.useDeferrer();

const selected = useRecoilValue(fos.selectedSamples);
useEffect(() => {
deferred(() => {
const fontSize = getFontSize();
spotlight?.updateItems((id) => {
store.get(id)?.updateOptions({
...options,
fontSize,
selected: selected.has(id.description),
});
});
});
}, [deferred, getFontSize, options, selected, spotlight, store]);

useEffect(() => {
return spotlight ? init() : undefined;
Expand Down
26 changes: 12 additions & 14 deletions app/packages/core/src/components/Modal/Modal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,13 @@ import { useRecoilCallback, useRecoilValue } from "recoil";
import styled from "styled-components";
import { ModalActionsRow } from "../Actions";
import Sidebar from "../Sidebar";
import { useLookerHelpers } from "./hooks";
import { modalContext } from "./modal-context";
import ModalNavigation from "./ModalNavigation";
import { ModalSpace } from "./ModalSpace";
import { TooltipInfo } from "./TooltipInfo";
import { useLookerHelpers } from "./hooks";
import { modalContext } from "./modal-context";
import { useModalSidebarRenderEntry } from "./use-sidebar-render-entry";
import { ModalSample } from "./ModalSamplePlugin";

const ModalWrapper = styled.div`
position: fixed;
Expand Down Expand Up @@ -104,6 +105,10 @@ const Modal = () => {
}

clearModal();
activeLookerRef.current?.removeEventListener(
"close",
modalCloseHandler
);
},
[clearModal, jsonPanel, helpPanel]
);
Expand Down Expand Up @@ -178,21 +183,13 @@ const Modal = () => {
const onLookerSetSubscribers = useRef<((looker: fos.Lookers) => void)[]>([]);

const onLookerSet = useCallback((looker: fos.Lookers) => {
onLookerSetSubscribers.current.forEach((sub) => sub(looker));

looker.addEventListener("close", modalCloseHandler);
}, []);

// cleanup effect
useEffect(() => {
return () => {
activeLookerRef.current?.removeEventListener("close", modalCloseHandler);
};
// onLookerSetSubscribers.current.forEach((sub) => sub(looker));
// looker.addEventListener("close", modalCloseHandler);
}, []);

const setActiveLookerRef = useCallback(
(looker: fos.Lookers) => {
activeLookerRef.current = looker;
// activeLookerRef.current = looker;
onLookerSet(looker);
},
[onLookerSet]
Expand All @@ -217,7 +214,8 @@ const Modal = () => {
<OperatorPromptArea area={OPERATOR_PROMPT_AREAS.DRAWER_LEFT} />
<ModalNavigation onNavigate={onNavigate} />
<SpacesContainer>
<ModalSpace />
{/* <ModalSpace /> */}
<ModalSample />
</SpacesContainer>
<SidebarContainer>
<SidebarPanelBlendInDiv />
Expand Down
2 changes: 1 addition & 1 deletion app/packages/core/src/components/Modal/ModalNavigation.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ const ModalNavigation = ({ onNavigate }: { onNavigate: () => void }) => {
onNavigate();
const result = await navigation?.previous();
setModal(result);
}, [onNavigate, navigation, setModal]);
}, [navigation, onNavigate, setModal]);

const keyboardHandler = useCallback(
(e: KeyboardEvent) => {
Expand Down
17 changes: 13 additions & 4 deletions app/packages/core/src/components/Modal/hooks.ts
Original file line number Diff line number Diff line change
@@ -1,16 +1,25 @@
import * as fos from "@fiftyone/state";
import { useHelpPanel, useJSONPanel } from "@fiftyone/state";
import { useCallback, useContext } from "react";
import { useCallback, useContext, useRef } from "react";
import { useRecoilCallback } from "recoil";
import { modalContext } from "./modal-context";

export const useLookerHelpers = () => {
const jsonPanel = useJSONPanel();
const helpPanel = useHelpPanel();

// todo: jsonPanel and helpPanel are not referentially stable
// so use refs here
const jsonPanelRef = useRef(jsonPanel);
const helpPanelRef = useRef(helpPanel);

jsonPanelRef.current = jsonPanel;
helpPanelRef.current = helpPanel;

const onNavigate = useCallback(() => {
jsonPanel.close();
helpPanel.close();
}, [helpPanel, jsonPanel]);
jsonPanelRef.current?.close();
helpPanelRef.current?.close();
}, []);

return {
jsonPanel,
Expand Down
43 changes: 20 additions & 23 deletions app/packages/core/src/components/Modal/use-key-events.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
import type { Lookers } from "@fiftyone/state";
import { hoveredSample } from "@fiftyone/state";
import type { MutableRefObject } from "react";
import { useEffect, useRef } from "react";
import { selector, useRecoilValue } from "recoil";
import { selector } from "recoil";

export const hoveredSampleId = selector<string>({
key: "hoveredSampleId",
Expand All @@ -16,25 +15,23 @@ export default function (
id: string,
looker: Lookers
) {
const hoveredId = useRecoilValue(hoveredSampleId);
const ready = useRef(false);

useEffect(() => {
if (ref.current) {
// initial call should wait for load event
const update = () => {
looker.updateOptions({
shouldHandleKeyEvents: id === hoveredId,
});
ready.current = true;

looker.removeEventListener("load", update);
};
looker.addEventListener("load", update);
} else if (ready.current) {
looker.updateOptions({
shouldHandleKeyEvents: id === hoveredId,
});
}
}, [hoveredId, id, looker, ref]);
// const hoveredId = useRecoilValue(hoveredSampleId);
// const ready = useRef(false);
// useEffect(() => {
// if (ref.current) {
// // initial call should wait for load event
// const update = () => {
// looker.updateOptions({
// shouldHandleKeyEvents: id === hoveredId,
// });
// ready.current = true;
// looker.removeEventListener("load", update);
// };
// looker.addEventListener("load", update);
// } else if (ready.current) {
// looker.updateOptions({
// shouldHandleKeyEvents: id === hoveredId,
// });
// }
// }, [hoveredId, id, looker, ref]);
}
61 changes: 24 additions & 37 deletions app/packages/core/src/components/Modal/use-looker.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import * as fos from "@fiftyone/state";
import React, { useEffect, useRef, useState } from "react";
import { useErrorHandler } from "react-error-boundary";
import { useRecoilValue, useSetRecoilState } from "recoil";
import { useRecoilValue } from "recoil";
import { v4 as uuid } from "uuid";
import { useClearSelectedLabels, useShowOverlays } from "./ModalLooker";
import { useLookerOptionsUpdate, useModalContext } from "./hooks";
import { useLookerOptionsUpdate } from "./hooks";
import useKeyEvents from "./use-key-events";
import { shortcutToHelpItems } from "./utils";

Expand All @@ -18,28 +18,26 @@ function useLooker<L extends fos.Lookers>({
const [id] = useState(() => uuid());
const initialRef = useRef<boolean>(true);
const ref = useRef<HTMLDivElement>(null);
const [reset, setReset] = useState(false);
const lookerOptions = fos.useLookerOptions(true);
const [reset, setReset] = useState(false);
const createLooker = fos.useCreateLooker(
true,
false,
lookerOptions,
undefined,
true
);
const selectedMediaField = useRecoilValue(fos.selectedMediaField(true));
const colorScheme = useRecoilValue(fos.colorScheme);
const looker = React.useMemo(() => {
/** start refreshers */
reset;
selectedMediaField;
/** end refreshers */

return createLooker.current(sample);
}, [createLooker, reset, sample, selectedMediaField]) as L;
}, [createLooker, reset, sample]) as L;
const handleError = useErrorHandler();
const updateLookerOptions = useLookerOptionsUpdate();

const selectedMediaField = useRecoilValue(fos.selectedMediaField(true));
const colorScheme = useRecoilValue(fos.colorScheme);
fos.useEventHandler(looker, "clear", useClearSelectedLabels());
fos.useEventHandler(looker, "error", (event) => handleError(event.detail));
fos.useEventHandler(looker, "options", (e) => updateLookerOptions(e.detail));
Expand All @@ -59,27 +57,8 @@ function useLooker<L extends fos.Lookers>({
!initialRef.current && looker.updateSample(sample.sample);
}, [colorScheme, looker, sample]);

useEffect(() => {
initialRef.current = false;
}, []);

useEffect(() => {
ref.current?.dispatchEvent(
new CustomEvent("looker-attached", { bubbles: true })
);
}, []);

useEffect(() => {
looker.attach(id);
}, [looker, id]);

useEffect(() => {
return () => looker?.destroy();
}, [looker]);

const jsonPanel = fos.useJSONPanel();
const helpPanel = fos.useHelpPanel();

fos.useEventHandler(
looker,
"panels",
Expand All @@ -94,7 +73,6 @@ function useLooker<L extends fos.Lookers>({
helpPanel[showHelp](shortcutToHelpItems(SHORTCUTS));
}
}

updateLookerOptions({}, (updatedOptions) =>
looker.updateOptions(updatedOptions)
);
Expand All @@ -103,21 +81,30 @@ function useLooker<L extends fos.Lookers>({

useKeyEvents(initialRef, sample.sample._id, looker);

const setModalLooker = useSetRecoilState(fos.modalLooker);
useEffect(() => {
ref.current?.dispatchEvent(
new CustomEvent("looker-attached", { bubbles: true })
);
}, []);
useEffect(() => {
initialRef.current = false;
}, []);

const { setActiveLookerRef } = useModalContext();
useEffect(() => {
ref.current?.dispatchEvent(
new CustomEvent("looker-attached", { bubbles: true })
);
}, []);

useEffect(() => {
setModalLooker(looker);
}, [looker, setModalLooker]);
looker.attach(id);
}, [looker, id]);

useEffect(() => {
if (looker) {
setActiveLookerRef(looker as fos.Lookers);
}
}, [looker, setActiveLookerRef]);
return () => looker?.destroy();
}, [looker]);

return { id, looker, ref, sample, updateLookerOptions };
return { id, looker, ref, sample };
}

export default useLooker;
1 change: 1 addition & 0 deletions app/packages/core/src/plugins/samples.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ registerComponent({
name: "Samples",
label: "Samples",
component: () => {
console.log("RENDER");
return (
<Container>
<Grid key={"grid"} />
Expand Down
Loading
Loading