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.

Sign up for GitHub

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

Release/v1.0.1 #4914

Merged
merged 18 commits into from
Oct 14, 2024
Merged

Release/v1.0.1 #4914

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
f08b8cc
fix COCO category IDs
brimoor Oct 2, 2024
221d807
fixing add_coco_labels
brimoor Oct 3, 2024
cc430f4
bumping compatibility version (#4883)
brimoor Oct 3, 2024
b375477
remove deprecated fiftyone-desktop refs
brimoor Oct 3, 2024
2496795
remove remote check
benjaminpkane Oct 4, 2024
2580fb5
Merge pull request #4888 from voxel51/bug/quickstart
brimoor Oct 4, 2024
b481bf3
fix temporal detections and classifications bubbles (#4891)
benjaminpkane Oct 4, 2024
17e8e53
Merge pull request #4884 from voxel51/bugfix/iss-4615-4795
brimoor Oct 4, 2024
ac6f451
Add utils for supporting rerun panel plugin (#4876)
sashankaryal Oct 4, 2024
35c13f7
Video playback (#4878)
benjaminpkane Oct 4, 2024
a2d26f0
wait until buffering is done (#4892)
sashankaryal Oct 4, 2024
46c1929
externalize fiftyone plugins too (#4897)
sashankaryal Oct 5, 2024
cd62462
updateSample with raw sample data (#4905)
benjaminpkane Oct 8, 2024
cad8d42
Release notes v1.0.1 (#4898)
brimoor Oct 10, 2024
37634ad
time marches forward (#4913)
findtopher Oct 10, 2024
233689b
bump version for release
findtopher Oct 10, 2024
2adc94e
Fix video tooltip (#4916)
benjaminpkane Oct 11, 2024
b48fbce
release is now Monday (#4919)
findtopher Oct 11, 2024
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
25 changes: 7 additions & 18 deletions app/packages/core/src/components/Modal/ImaVidLooker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,14 @@ import React, {
import { useErrorHandler } from "react-error-boundary";
import { useRecoilValue, useSetRecoilState } from "recoil";
import { v4 as uuid } from "uuid";
import { useInitializeImaVidSubscriptions, useModalContext } from "./hooks";
import { useClearSelectedLabels, useShowOverlays } from "./ModalLooker";
import {
shortcutToHelpItems,
useClearSelectedLabels,
useInitializeImaVidSubscriptions,
useLookerOptionsUpdate,
useShowOverlays,
} from "./ModalLooker";
useModalContext,
} from "./hooks";
import useKeyEvents from "./use-key-events";
import { shortcutToHelpItems } from "./utils";

interface ImaVidLookerReactProps {
sample: fos.ModalSample;
Expand Down Expand Up @@ -132,19 +133,7 @@ export const ImaVidLookerReact = React.memo(

useEventHandler(looker, "clear", useClearSelectedLabels());

const hoveredSample = useRecoilValue(fos.hoveredSample);

useEffect(() => {
const hoveredSampleId = hoveredSample?._id;
looker.updater((state) => ({
...state,
// todo: always setting it to true might not be wise
shouldHandleKeyEvents: true,
options: {
...state.options,
},
}));
}, [hoveredSample, sample, looker]);
useKeyEvents(initialRef, sample._id, looker);

const ref = useRef<HTMLDivElement>(null);
useEffect(() => {
Expand Down
203 changes: 39 additions & 164 deletions app/packages/core/src/components/Modal/ModalLooker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,35 +1,12 @@
import { useTheme } from "@fiftyone/components";
import { AbstractLooker } from "@fiftyone/looker";
import { BaseState } from "@fiftyone/looker/src/state";
import type { ImageLooker } from "@fiftyone/looker";
import * as fos from "@fiftyone/state";
import { useEventHandler, useOnSelectLabel } from "@fiftyone/state";
import React, { useEffect, useMemo, useRef, useState } from "react";
import { useErrorHandler } from "react-error-boundary";
import React, { useEffect, useMemo } from "react";
import { useRecoilCallback, useRecoilValue, useSetRecoilState } from "recoil";
import { v4 as uuid } from "uuid";
import { useModalContext } from "./hooks";
import { ImaVidLookerReact } from "./ImaVidLooker";

export const useLookerOptionsUpdate = () => {
return useRecoilCallback(
({ snapshot, set }) =>
async (update: object, updater?: (updated: {}) => void) => {
const currentOptions = await snapshot.getPromise(
fos.savedLookerOptions
);

const panels = await snapshot.getPromise(fos.lookerPanels);
const updated = {
...currentOptions,
...update,
showJSON: panels.json.isOpen,
showHelp: panels.help.isOpen,
};
set(fos.savedLookerOptions, updated);
if (updater) updater(updated);
}
);
};
import { VideoLookerReact } from "./VideoLooker";
import { useModalContext } from "./hooks";
import useLooker from "./use-looker";

export const useShowOverlays = () => {
return useRecoilCallback(({ set }) => async (event: CustomEvent) => {
Expand All @@ -47,137 +24,40 @@ export const useClearSelectedLabels = () => {
};

interface LookerProps {
sample?: fos.ModalSample;
onClick?: React.MouseEventHandler<HTMLDivElement>;
sample: fos.ModalSample;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Missing sample Property in Multiple Components Using LookerProps

Several components utilizing LookerProps do not provide the required sample property, which may lead to potential runtime errors. Please ensure that all relevant components are updated to include the sample property.

Affected Files Include:

  • app/packages/core/src/components/Modal/ModalNavigation.tsx
  • app/packages/core/src/components/Modal/Renderer.tsx
  • app/packages/core/src/components/Sync.tsx
  • app/packages/core/src/components/Analytics.tsx
  • app/packages/core/src/components/Button/Button.tsx
  • (and many others)
🔗 Analysis chain

Ensure all components using LookerProps provide the required sample property

The sample property in LookerProps has been changed from optional to required. Please verify that all components using LookerProps have been updated to include the sample property to prevent potential runtime errors.

Run the following script to find all usages of LookerProps and check if they provide the sample property:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find all TypeScript files using 'LookerProps' and check if 'sample' is provided.

# Find all components that use 'LookerProps'
rg --type tsx ':\s*LookerProps' -l | while read file; do
  echo "Checking $file"
  # Check if 'sample' is used in the component's props
  rg 'function\s+\w+\s*\(.*sample' $file || echo "'sample' not found in $file"
done

Length of output: 76

Script:

#!/bin/bash
# Description: Find all .tsx files using 'LookerProps' and check if 'sample' is provided.

# Find all .tsx files that use 'LookerProps'
fd --extension tsx | while read file; do
  echo "Checking $file"
  # Check if 'LookerProps' is used in the component's props and if 'sample' is provided
  rg 'LookerProps' "$file" | rg 'sample' || echo "'sample' not found in $file"
done

Length of output: 176635

}

const ModalLookerNoTimeline = React.memo(
({ sample: sampleDataWithExtraParams }: LookerProps) => {
const [id] = useState(() => uuid());
const colorScheme = useRecoilValue(fos.colorScheme);

const { sample } = sampleDataWithExtraParams;

const theme = useTheme();
const initialRef = useRef<boolean>(true);
const lookerOptions = fos.useLookerOptions(true);
const [reset, setReset] = useState(false);
const selectedMediaField = useRecoilValue(fos.selectedMediaField(true));
const setModalLooker = useSetRecoilState(fos.modalLooker);

const createLooker = fos.useCreateLooker(true, false, {
...lookerOptions,
});

const { setActiveLookerRef } = useModalContext();

const looker = React.useMemo(
() => createLooker.current(sampleDataWithExtraParams),
[reset, createLooker, selectedMediaField]
) as AbstractLooker<BaseState>;

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

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

useEffect(() => {
!initialRef.current && looker.updateOptions(lookerOptions);
}, [lookerOptions]);

useEffect(() => {
!initialRef.current && looker.updateSample(sample);
}, [sample, colorScheme]);

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

const handleError = useErrorHandler();
const ModalLookerNoTimeline = React.memo((props: LookerProps) => {
const { id, looker, ref } = useLooker<ImageLooker>(props);
const theme = useTheme();
const setModalLooker = useSetRecoilState(fos.modalLooker);

const updateLookerOptions = useLookerOptionsUpdate();
useEventHandler(looker, "options", (e) => updateLookerOptions(e.detail));
useEventHandler(looker, "showOverlays", useShowOverlays());
useEventHandler(looker, "reset", () => {
setReset((c) => !c);
});
const { setActiveLookerRef } = useModalContext();

const jsonPanel = fos.useJSONPanel();
const helpPanel = fos.useHelpPanel();
useEffect(() => {
setModalLooker(looker);
}, [looker, setModalLooker]);

useEventHandler(looker, "select", useOnSelectLabel());
useEventHandler(looker, "error", (event) => handleError(event.detail));
useEventHandler(
looker,
"panels",
async ({ detail: { showJSON, showHelp, SHORTCUTS } }) => {
if (showJSON) {
jsonPanel[showJSON](sample);
}
if (showHelp) {
if (showHelp == "close") {
helpPanel.close();
} else {
helpPanel[showHelp](shortcutToHelpItems(SHORTCUTS));
}
}

updateLookerOptions({}, (updatedOptions) =>
looker.updateOptions(updatedOptions)
);
}
);

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

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

useEventHandler(looker, "clear", useClearSelectedLabels());

const hoveredSample = useRecoilValue(fos.hoveredSample);

useEffect(() => {
const hoveredSampleId = hoveredSample?._id;
looker.updater((state) => ({
...state,
shouldHandleKeyEvents: hoveredSampleId === sample._id,
options: {
...state.options,
},
}));
}, [hoveredSample, sample, looker]);

const ref = useRef<HTMLDivElement>(null);
useEffect(() => {
ref.current?.dispatchEvent(
new CustomEvent(`looker-attached`, { bubbles: true })
);
}, [ref]);

return (
<div
ref={ref}
id={id}
data-cy="modal-looker-container"
style={{
width: "100%",
height: "100%",
background: theme.background.level2,
position: "relative",
}}
/>
);
}
);
useEffect(() => {
if (looker) {
setActiveLookerRef(looker as fos.Lookers);
}
}, [looker, setActiveLookerRef]);

return (
<div
ref={ref}
id={id}
data-cy="modal-looker-container"
style={{
width: "100%",
height: "100%",
background: theme.background.level2,
position: "relative",
}}
/>
);
});

export const ModalLooker = React.memo(
({ sample: propsSampleData }: LookerProps) => {
Expand All @@ -197,21 +77,16 @@ export const ModalLooker = React.memo(
const shouldRenderImavid = useRecoilValue(
fos.shouldRenderImaVidLooker(true)
);
const video = useRecoilValue(fos.isVideoDataset);

if (shouldRenderImavid) {
return <ImaVidLookerReact sample={sample} />;
}

if (video) {
return <VideoLookerReact sample={sample} />;
}

return <ModalLookerNoTimeline sample={sample} />;
}
);

export function shortcutToHelpItems(SHORTCUTS) {
return Object.values(
Object.values(SHORTCUTS).reduce((acc, v) => {
acc[v.shortcut] = v;

return acc;
}, {})
);
}
80 changes: 80 additions & 0 deletions app/packages/core/src/components/Modal/VideoLooker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
import { useTheme } from "@fiftyone/components";
import type { VideoLooker } from "@fiftyone/looker";
import { getFrameNumber } from "@fiftyone/looker";
import {
useCreateTimeline,
useDefaultTimelineNameImperative,
useTimeline,
} from "@fiftyone/playback";
import * as fos from "@fiftyone/state";
import React, { useEffect, useMemo, useState } from "react";
import useLooker from "./use-looker";

interface VideoLookerReactProps {
sample: fos.ModalSample;
}

export const VideoLookerReact = (props: VideoLookerReactProps) => {
const theme = useTheme();
const { id, looker, sample } = useLooker<VideoLooker>(props);
const [totalFrames, setTotalFrames] = useState<number>();
const frameRate = useMemo(() => {
return sample.frameRate;
}, [sample]);

useEffect(() => {
const load = () => {
const duration = looker.getVideo().duration;
setTotalFrames(getFrameNumber(duration, duration, frameRate));
looker.removeEventListener("load", load);
};
looker.addEventListener("load", load);
}, [frameRate, looker]);
Comment on lines +25 to +32
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider using fos.useEventHandler for consistent event handling

In the useEffect hook, you're adding an event listener to the looker object using looker.addEventListener. For consistency and better resource management, consider using fos.useEventHandler, which is used elsewhere in the code (e.g., in TimelineController) for handling events. This ensures that event listeners are properly cleaned up when the component unmounts.

Apply this diff to refactor the event handling:

-useEffect(() => {
-  const load = () => {
-    const duration = looker.getVideo().duration;
-    setTotalFrames(getFrameNumber(duration, duration, frameRate));
-    looker.removeEventListener("load", load);
-  };
-  looker.addEventListener("load", load);
-}, [frameRate, looker]);
+fos.useEventHandler(looker, "load", () => {
+  const duration = looker.getVideo().duration;
+  setTotalFrames(getFrameNumber(duration, duration, frameRate));
+});
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
useEffect(() => {
const load = () => {
const duration = looker.getVideo().duration;
setTotalFrames(getFrameNumber(duration, duration, frameRate));
looker.removeEventListener("load", load);
};
looker.addEventListener("load", load);
}, [frameRate, looker]);
fos.useEventHandler(looker, "load", () => {
const duration = looker.getVideo().duration;
setTotalFrames(getFrameNumber(duration, duration, frameRate));
});


return (
<>
<div
id={id}
data-cy="modal-looker-container"
style={{
width: "100%",
height: "100%",
background: theme.background.level2,
position: "relative",
}}
/>
{totalFrames !== undefined && (
<TimelineController looker={looker} totalFrames={totalFrames} />
)}
</>
);
};

const TimelineController = ({
looker,
totalFrames,
}: {
looker: VideoLooker;
totalFrames: number;
}) => {
Comment on lines +53 to +59
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Define a separate interface for TimelineController props for clarity

Currently, the props for TimelineController are typed inline within the parameter destructuring. For better readability and maintainability, consider defining a separate interface for the props.

Apply this diff to define a TimelineControllerProps interface:

+interface TimelineControllerProps {
+  looker: VideoLooker;
+  totalFrames: number;
+}
+
-const TimelineController = ({
-  looker,
-  totalFrames,
-}: {
-  looker: VideoLooker;
-  totalFrames: number;
-}) => {
+const TimelineController = ({ looker, totalFrames }: TimelineControllerProps) => {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const TimelineController = ({
looker,
totalFrames,
}: {
looker: VideoLooker;
totalFrames: number;
}) => {
interface TimelineControllerProps {
looker: VideoLooker;
totalFrames: number;
}
const TimelineController = ({ looker, totalFrames }: TimelineControllerProps) => {

const { getName } = useDefaultTimelineNameImperative();
const timelineName = React.useMemo(() => getName(), [getName]);

useCreateTimeline({
name: timelineName,
config: totalFrames
? {
totalFrames,
loop: true,
}
: undefined,
optOutOfAnimation: true,
});

const { pause, play } = useTimeline(timelineName);

fos.useEventHandler(looker, "pause", pause);
fos.useEventHandler(looker, "play", play);

return null;
};
21 changes: 21 additions & 0 deletions app/packages/core/src/components/Modal/hooks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,27 @@ export const useLookerHelpers = () => {
};
};

export const useLookerOptionsUpdate = () => {
return useRecoilCallback(
({ snapshot, set }) =>
async (update: object, updater?: (updated: {}) => void) => {
const currentOptions = await snapshot.getPromise(
fos.savedLookerOptions
);

const panels = await snapshot.getPromise(fos.lookerPanels);
const updated = {
...currentOptions,
...update,
showJSON: panels.json.isOpen,
showHelp: panels.help.isOpen,
};
set(fos.savedLookerOptions, updated);
if (updater) updater(updated);
}
);
};

export const useInitializeImaVidSubscriptions = () => {
const subscribeToImaVidStateChanges = useRecoilCallback(
({ set }) =>
Expand Down
Loading
Loading