-
Notifications
You must be signed in to change notification settings - Fork 557
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
Merge main #4920
Merge main #4920
Changes from all commits
233689b
2adc94e
b48fbce
7e73fec
1f5eaa8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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"; | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider cleaning up Currently, Add a cleanup function to the useEffect(() => {
setModalLooker(looker);
+ return () => {
+ setModalLooker(null);
+ };
}, [looker, setModalLooker]); 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||
useEffect(() => { | ||||||||||||||||||||||||||||
if (looker) { | ||||||||||||||||||||||||||||
setActiveLookerRef(looker as fos.Lookers); | ||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Avoid unnecessary type assertion Using Adjust the code to avoid the type assertion: - setActiveLookerRef(looker as fos.Lookers);
+ setActiveLookerRef(looker); 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
}, [looker, setActiveLookerRef]); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
Comment on lines
+114
to
+119
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Ensure In the Modify the useEffect(() => {
if (looker) {
setActiveLookerRef(looker as fos.Lookers);
}
+ return () => {
+ setActiveLookerRef(null);
+ };
}, [looker, setActiveLookerRef]); 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||
return { id, looker, ref, sample, updateLookerOptions }; | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,7 @@ import { | |
FrameSample, | ||
LabelData, | ||
StateUpdate, | ||
VideoConfig, | ||
VideoSample, | ||
VideoState, | ||
} from "../state"; | ||
|
@@ -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, | ||
|
@@ -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 && | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential race condition with shared Using the shared variable Consider refactoring to avoid shared mutable state across instances. Encapsulating |
||
} else if (lookerWithReader !== this && frameCount) { | ||
this.state.buffering && this.dispatchEvent("buffering", false); | ||
this.state.playing = false; | ||
|
@@ -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() { | ||
|
@@ -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 => | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,7 +13,7 @@ | |
from setuptools import setup, find_packages | ||
|
||
|
||
VERSION = "1.0.0" | ||
VERSION = "1.0.1" | ||
|
||
|
||
def get_version(): | ||
|
There was a problem hiding this comment.
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:activeColorEntry
to null using Recoil'sset
function.While this simplifies the state update logic, consider the following:
activeColorEntry
on component mount?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:
Then use it in the component:
This would make the component's logic more modular and easier to test.