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

Jump to bottom

Merge main #4920

Closed
wants to merge 5 commits into from
Closed

Merge main #4920

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
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
7e73fec
ID_LIKE fallback for fiftyone-db (#4903)
benjaminpkane Oct 11, 2024
1f5eaa8
Merge pull request #4914 from voxel51/release/v1.0.1
findtopher Oct 14, 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
9 changes: 4 additions & 5 deletions app/packages/core/src/components/ColorModal/ColorFooter.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import * as foq from "@fiftyone/relay";
import * as fos from "@fiftyone/state";
import React, { useEffect } from "react";
import { useMutation } from "react-relay";
import { useRecoilState, useRecoilValue } from "recoil";
import { useRecoilValue } from "recoil";
import { ButtonGroup, ModalActionButtonContainer } from "./ShareStyledDiv";
import { activeColorEntry } from "./state";

Expand All @@ -14,8 +14,7 @@ const ColorFooter: React.FC = () => {
? canEditCustomColors.message
: "Save to dataset app config";
const setColorScheme = fos.useSetSessionColorScheme();
const [activeColorModalField, setActiveColorModalField] =
useRecoilState(activeColorEntry);
const activeColorModalField = useRecoilValue(activeColorEntry);
const [setDatasetColorScheme] =
useMutation<foq.setDatasetColorSchemeMutation>(foq.setDatasetColorScheme);
const colorScheme = useRecoilValue(fos.colorScheme);
Expand All @@ -26,8 +25,8 @@ const ColorFooter: React.FC = () => {
const subscription = useRecoilValue(fos.stateSubscription);

useEffect(
() => foq.subscribe(() => setActiveColorModalField(null)),
[setActiveColorModalField]
() => foq.subscribe((_, { set }) => set(activeColorEntry, null)),
[]
Comment on lines +28 to +29
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 the implications of the useEffect changes

The useEffect hook has been modified:

  1. The dependency array is now empty, meaning this effect will only run once on component mount.
  2. The callback now directly sets activeColorEntry to null using Recoil's set function.

While this simplifies the state update logic, consider the following:

  • Is it intentional to only reset activeColorEntry on component mount?
  • Are there any scenarios where activeColorEntry needs to be reset based on other state changes?

If the current behavior is intended, you might want to add a comment explaining why the effect only runs once. If not, consider adding appropriate dependencies to the dependency array.

Additionally, to improve code clarity, you could extract the subscription logic into a custom hook:

const useResetActiveColorEntry = () => {
  useEffect(() => foq.subscribe((_, { set }) => set(activeColorEntry, null)), []);
};

Then use it in the component:

const ColorFooter: React.FC = () => {
  // ... other code ...
  useResetActiveColorEntry();
  // ... rest of the component ...
};

This would make the component's logic more modular and easier to test.

);

if (!activeColorModalField) return null;
Expand Down
20 changes: 3 additions & 17 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,11 +1,10 @@
import { useTheme } from "@fiftyone/components";
import type { ImageLooker } from "@fiftyone/looker";
import * as fos from "@fiftyone/state";
import React, { useEffect, useMemo } from "react";
import { useRecoilCallback, useRecoilValue, useSetRecoilState } from "recoil";
import React, { useMemo } from "react";
import { useRecoilCallback, useRecoilValue } from "recoil";
import { ImaVidLookerReact } from "./ImaVidLooker";
import { VideoLookerReact } from "./VideoLooker";
import { useModalContext } from "./hooks";
import useLooker from "./use-looker";

export const useShowOverlays = () => {
Expand All @@ -28,21 +27,8 @@ interface LookerProps {
}

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

const { setActiveLookerRef } = useModalContext();

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

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

return (
<div
Expand Down
18 changes: 16 additions & 2 deletions app/packages/core/src/components/Modal/use-looker.ts
View file
Open in desktop
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 } from "recoil";
import { useRecoilValue, useSetRecoilState } from "recoil";
import { v4 as uuid } from "uuid";
import { useClearSelectedLabels, useShowOverlays } from "./ModalLooker";
import { useLookerOptionsUpdate } from "./hooks";
import { useLookerOptionsUpdate, useModalContext } from "./hooks";
import useKeyEvents from "./use-key-events";
import { shortcutToHelpItems } from "./utils";

Expand Down Expand Up @@ -103,6 +103,20 @@ function useLooker<L extends fos.Lookers>({

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

const setModalLooker = useSetRecoilState(fos.modalLooker);

const { setActiveLookerRef } = useModalContext();

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

Comment on lines +110 to +113
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 cleaning up modalLooker state on unmount

Currently, modalLooker is set when looker changes, but there is no cleanup when the component unmounts. To prevent potential stale references or memory leaks, consider resetting modalLooker when the component unmounts.

Add a cleanup function to the useEffect:

useEffect(() => {
  setModalLooker(looker);
+  return () => {
+    setModalLooker(null);
+  };
}, [looker, setModalLooker]);
📝 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(() => {
setModalLooker(looker);
}, [looker, setModalLooker]);
useEffect(() => {
setModalLooker(looker);
return () => {
setModalLooker(null);
};
}, [looker, setModalLooker]);

useEffect(() => {
if (looker) {
setActiveLookerRef(looker as fos.Lookers);
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

Avoid unnecessary type assertion

Using as fos.Lookers in setActiveLookerRef(looker as fos.Lookers); may mask potential type safety issues. Since looker is of type L extending fos.Lookers, the cast might be unnecessary. Consider ensuring that looker is correctly typed to eliminate the need for casting.

Adjust the code to avoid the type assertion:

- setActiveLookerRef(looker as fos.Lookers);
+ setActiveLookerRef(looker);
📝 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
setActiveLookerRef(looker as fos.Lookers);
setActiveLookerRef(looker);

}
}, [looker, setActiveLookerRef]);

Comment on lines +114 to +119
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

Ensure activeLookerRef is cleaned up on unmount

In the useEffect, you set activeLookerRef when looker is available, but you should also reset it when the component unmounts to avoid holding onto stale references.

Modify the useEffect to include a cleanup function:

useEffect(() => {
  if (looker) {
    setActiveLookerRef(looker as fos.Lookers);
  }
+  return () => {
+    setActiveLookerRef(null);
+  };
}, [looker, setActiveLookerRef]);
📝 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(() => {
if (looker) {
setActiveLookerRef(looker as fos.Lookers);
}
}, [looker, setActiveLookerRef]);
useEffect(() => {
if (looker) {
setActiveLookerRef(looker as fos.Lookers);
}
return () => {
setActiveLookerRef(null);
};
}, [looker, setActiveLookerRef]);

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

Expand Down
27 changes: 18 additions & 9 deletions app/packages/looker/src/lookers/video.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
FrameSample,
LabelData,
StateUpdate,
VideoConfig,
VideoSample,
VideoState,
} from "../state";
Expand Down Expand Up @@ -318,7 +319,7 @@ export class VideoLooker extends AbstractLooker<VideoState, VideoSample> {
...this.getDefaultOptions(),
...options,
},
buffers: [[firstFrame, firstFrame]] as Buffers,
buffers: this.initialBuffers(config),
seekBarHovering: false,
SHORTCUTS: VIDEO_SHORTCUTS,
hasPoster: false,
Expand All @@ -328,8 +329,8 @@ export class VideoLooker extends AbstractLooker<VideoState, VideoSample> {
}

hasDefaultZoom(state: VideoState, overlays: Overlay<VideoState>[]): boolean {
let pan = [0, 0];
let scale = 1;
const pan = [0, 0];
const scale = 1;

return (
scale === state.scale &&
Expand Down Expand Up @@ -402,10 +403,9 @@ export class VideoLooker extends AbstractLooker<VideoState, VideoSample> {
lookerWithReader !== this &&
frameCount !== null
) {
lookerWithReader && lookerWithReader.pause();
this.setReader();
lookerWithReader?.pause();
lookerWithReader = this;
this.state.buffers = [[1, 1]];
this.setReader();
Comment on lines +406 to +408
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Potential race condition with shared lookerWithReader variable

Using the shared variable lookerWithReader may lead to unexpected behavior if multiple VideoLooker instances are active simultaneously. This can cause instances to pause or overwrite each other's readers.

Consider refactoring to avoid shared mutable state across instances. Encapsulating lookerWithReader within the class or managing instance-specific readers can prevent conflicts.

} else if (lookerWithReader !== this && frameCount) {
this.state.buffering && this.dispatchEvent("buffering", false);
this.state.playing = false;
Expand Down Expand Up @@ -549,10 +549,14 @@ export class VideoLooker extends AbstractLooker<VideoState, VideoSample> {
}

updateSample(sample: VideoSample) {
this.state.buffers = [[1, 1]];
this.frames.clear();
if (lookerWithReader === this) {
lookerWithReader?.pause();
lookerWithReader = null;
}

this.frames = new Map();
this.state.buffers = this.initialBuffers(this.state.config);
super.updateSample(sample);
this.setReader();
}

getVideo() {
Expand All @@ -565,6 +569,11 @@ export class VideoLooker extends AbstractLooker<VideoState, VideoSample> {
this.frames.get(frameNumber)?.deref() !== undefined
);
}

private initialBuffers(config: VideoConfig) {
const firstFrame = config.support ? config.support[0] : 1;
return [[firstFrame, firstFrame]] as Buffers;
}
}

const withFrames = <T extends { [key: string]: unknown }>(obj: T): T =>
Expand Down
1 change: 1 addition & 0 deletions app/packages/looker/src/state.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,7 @@ export interface ImageState extends BaseState {
}

export interface VideoState extends BaseState {
buffers: Buffers;
config: VideoConfig;
options: VideoOptions;
seeking: boolean;
Expand Down
4 changes: 2 additions & 2 deletions docs/source/release-notes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ FiftyOne Release Notes

FiftyOne Teams 2.1.1
--------------------
*Released October 11, 2024*
*Released October 14, 2024*

Includes all updates from :ref:`FiftyOne 1.0.1 <release-notes-v1.0.1>`, plus:

Expand All @@ -16,7 +16,7 @@ Includes all updates from :ref:`FiftyOne 1.0.1 <release-notes-v1.0.1>`, plus:

FiftyOne 1.0.1
--------------
*Released October 11, 2024*
*Released October 14, 2024*

App

Expand Down
8 changes: 6 additions & 2 deletions package/db/setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,11 @@ def _get_linux_download():
# filter empty lines
d = dict(line for line in reader if line)

for k, v in LINUX_DOWNLOADS[d["ID"].lower()].items():
key = d["ID"].lower()
if key not in LINUX_DOWNLOADS:
key = d["ID_LIKE"].lower()

for k, v in LINUX_DOWNLOADS[key].items():
if d["VERSION_ID"].startswith(k):
return v[MACHINE]

Expand All @@ -171,7 +175,7 @@ def _get_download():
MONGODB_BINARIES = ["mongod"]


VERSION = "1.1.6"
VERSION = "1.1.7"
benjaminpkane marked this conversation as resolved.
Show resolved Hide resolved


def get_version():
Expand Down
2 changes: 1 addition & 1 deletion setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
from setuptools import setup, find_packages


VERSION = "1.0.0"
VERSION = "1.0.1"


def get_version():
Expand Down
Loading