-
Notifications
You must be signed in to change notification settings - Fork 590
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 release/v1.0.0
to develop
#4858
Changes from 14 commits
b305252
1e05d11
81037fa
f37884d
d3960f8
f5e8a5a
473b8d7
ba40b0e
3e1a9b1
58c2ae6
7ebfd94
a3f47e5
887654f
9ff80ab
f5cdc88
be1e074
a38596d
543ecf3
ab426cb
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,6 +1,5 @@ | ||||||||||||||||||||||||||
import { useTheme } from "@fiftyone/components"; | ||||||||||||||||||||||||||
import { AbstractLooker, ImaVidLooker } from "@fiftyone/looker"; | ||||||||||||||||||||||||||
import { BUFFERING_PAUSE_TIMEOUT } from "@fiftyone/looker/src/lookers/imavid/constants"; | ||||||||||||||||||||||||||
import { BaseState } from "@fiftyone/looker/src/state"; | ||||||||||||||||||||||||||
import { FoTimelineConfig, useCreateTimeline } from "@fiftyone/playback"; | ||||||||||||||||||||||||||
import { useDefaultTimelineNameImperative } from "@fiftyone/playback/src/lib/use-default-timeline-name"; | ||||||||||||||||||||||||||
|
@@ -54,6 +53,8 @@ export const ImaVidLookerReact = React.memo( | |||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
const { activeLookerRef, setActiveLookerRef } = useModalContext(); | ||||||||||||||||||||||||||
const imaVidLookerRef = | ||||||||||||||||||||||||||
activeLookerRef as unknown as React.MutableRefObject<ImaVidLooker>; | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
const looker = React.useMemo( | ||||||||||||||||||||||||||
() => createLooker.current(sampleDataWithExtraParams), | ||||||||||||||||||||||||||
|
@@ -152,19 +153,59 @@ export const ImaVidLookerReact = React.memo( | |||||||||||||||||||||||||
); | ||||||||||||||||||||||||||
}, [ref]); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
const loadRange = React.useCallback(async (range: BufferRange) => { | ||||||||||||||||||||||||||
// todo: implement | ||||||||||||||||||||||||||
return new Promise<void>((resolve) => { | ||||||||||||||||||||||||||
setTimeout(() => { | ||||||||||||||||||||||||||
resolve(); | ||||||||||||||||||||||||||
}, BUFFERING_PAUSE_TIMEOUT); | ||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||
}, []); | ||||||||||||||||||||||||||
const loadRange = React.useCallback( | ||||||||||||||||||||||||||
async (range: Readonly<BufferRange>) => { | ||||||||||||||||||||||||||
const storeBufferManager = | ||||||||||||||||||||||||||
imaVidLookerRef.current.frameStoreController.storeBufferManager; | ||||||||||||||||||||||||||
const fetchBufferManager = | ||||||||||||||||||||||||||
imaVidLookerRef.current.frameStoreController.fetchBufferManager; | ||||||||||||||||||||||||||
Comment on lines
+158
to
+161
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. Add null checks for Accessing Update the code to safely access const loadRange = React.useCallback(
async (range: Readonly<BufferRange>) => {
- const storeBufferManager =
- imaVidLookerRef.current.frameStoreController.storeBufferManager;
- const fetchBufferManager =
- imaVidLookerRef.current.frameStoreController.fetchBufferManager;
+ if (!imaVidLookerRef.current) {
+ // Handle the case when current is null or undefined
+ return;
+ }
+ const storeBufferManager =
+ imaVidLookerRef.current.frameStoreController.storeBufferManager;
+ const fetchBufferManager =
+ imaVidLookerRef.current.frameStoreController.fetchBufferManager;
// Rest of the code... 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
if (storeBufferManager.containsRange(range)) { | ||||||||||||||||||||||||||
return; | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
const unprocessedStoreBufferRange = | ||||||||||||||||||||||||||
storeBufferManager.getUnprocessedBufferRange(range); | ||||||||||||||||||||||||||
const unprocessedBufferRange = | ||||||||||||||||||||||||||
fetchBufferManager.getUnprocessedBufferRange( | ||||||||||||||||||||||||||
unprocessedStoreBufferRange | ||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
if (!unprocessedBufferRange) { | ||||||||||||||||||||||||||
return; | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
imaVidLookerRef.current.frameStoreController.enqueueFetch( | ||||||||||||||||||||||||||
unprocessedBufferRange | ||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
return new Promise<void>((resolve) => { | ||||||||||||||||||||||||||
const fetchMoreListener = (e: CustomEvent) => { | ||||||||||||||||||||||||||
if ( | ||||||||||||||||||||||||||
e.detail.id === imaVidLookerRef.current.frameStoreController.key | ||||||||||||||||||||||||||
) { | ||||||||||||||||||||||||||
if (storeBufferManager.containsRange(unprocessedBufferRange)) { | ||||||||||||||||||||||||||
resolve(); | ||||||||||||||||||||||||||
window.removeEventListener( | ||||||||||||||||||||||||||
"fetchMore", | ||||||||||||||||||||||||||
fetchMoreListener as EventListener | ||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
window.addEventListener( | ||||||||||||||||||||||||||
"fetchMore", | ||||||||||||||||||||||||||
fetchMoreListener as EventListener, | ||||||||||||||||||||||||||
{ once: true } | ||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||
[] | ||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
const renderFrame = React.useCallback((frameNumber: number) => { | ||||||||||||||||||||||||||
( | ||||||||||||||||||||||||||
activeLookerRef.current as unknown as ImaVidLooker | ||||||||||||||||||||||||||
)?.element.drawFrameNoAnimation(frameNumber); | ||||||||||||||||||||||||||
imaVidLookerRef.current?.element.drawFrameNoAnimation(frameNumber); | ||||||||||||||||||||||||||
}, []); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
const { getName } = useDefaultTimelineNameImperative(); | ||||||||||||||||||||||||||
|
@@ -189,7 +230,7 @@ export const ImaVidLookerReact = React.memo( | |||||||||||||||||||||||||
|
||||||||||||||||||||||||||
const readyWhen = useCallback(async () => { | ||||||||||||||||||||||||||
return new Promise<void>((resolve) => { | ||||||||||||||||||||||||||
// wait for total frame count to be resolved | ||||||||||||||||||||||||||
// hack: wait for total frame count to be resolved | ||||||||||||||||||||||||||
let intervalId; | ||||||||||||||||||||||||||
intervalId = setInterval(() => { | ||||||||||||||||||||||||||
if (totalFrameCountRef.current) { | ||||||||||||||||||||||||||
|
@@ -200,6 +241,10 @@ export const ImaVidLookerReact = React.memo( | |||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||
}, []); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
const onAnimationStutter = useCallback(() => { | ||||||||||||||||||||||||||
imaVidLookerRef.current?.element.checkFetchBufferManager(); | ||||||||||||||||||||||||||
}, []); | ||||||||||||||||||||||||||
Comment on lines
+244
to
+246
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. Improve handling of In the Update the code to include a null check: const onAnimationStutter = useCallback(() => {
- imaVidLookerRef.current?.element.checkFetchBufferManager();
+ imaVidLookerRef.current?.element?.checkFetchBufferManager();
}, []); This ensures that both 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
const { | ||||||||||||||||||||||||||
isTimelineInitialized, | ||||||||||||||||||||||||||
registerOnPauseCallback, | ||||||||||||||||||||||||||
|
@@ -210,6 +255,10 @@ export const ImaVidLookerReact = React.memo( | |||||||||||||||||||||||||
name: timelineName, | ||||||||||||||||||||||||||
config: timelineCreationConfig, | ||||||||||||||||||||||||||
waitUntilInitialized: readyWhen, | ||||||||||||||||||||||||||
// using this mechanism to resume fetch if it was paused | ||||||||||||||||||||||||||
// ideally we have control of fetch in this component but can't do that yet | ||||||||||||||||||||||||||
// since imavid is part of the grid too | ||||||||||||||||||||||||||
onAnimationStutter, | ||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||
|
@@ -224,35 +273,27 @@ export const ImaVidLookerReact = React.memo( | |||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
registerOnPlayCallback(() => { | ||||||||||||||||||||||||||
(activeLookerRef.current as unknown as ImaVidLooker).element.update( | ||||||||||||||||||||||||||
() => ({ | ||||||||||||||||||||||||||
playing: true, | ||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||
imaVidLookerRef.current?.element?.update(() => ({ | ||||||||||||||||||||||||||
playing: true, | ||||||||||||||||||||||||||
})); | ||||||||||||||||||||||||||
Comment on lines
+276
to
+278
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. Ensure consistent null checks when accessing In some instances, optional chaining is used (e.g., Review and update the following code segments: registerOnPlayCallback(() => {
- imaVidLookerRef.current?.element?.update(() => ({
+ imaVidLookerRef.current?.element?.update(() => ({
playing: true,
}));
});
registerOnPauseCallback(() => {
- imaVidLookerRef.current?.element?.update(() => ({
+ imaVidLookerRef.current?.element?.update(() => ({
playing: false,
}));
});
registerOnSeekCallbacks({
start: () => {
- imaVidLookerRef.current?.element?.update(() => ({
+ imaVidLookerRef.current?.element?.update(() => ({
seeking: true,
}));
},
end: () => {
- imaVidLookerRef.current?.element?.update(() => ({
+ imaVidLookerRef.current?.element?.update(() => ({
seeking: false,
}));
},
}); Ensure that all accesses to Also applies to: 282-284, 289-296 |
||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
registerOnPauseCallback(() => { | ||||||||||||||||||||||||||
(activeLookerRef.current as unknown as ImaVidLooker).element.update( | ||||||||||||||||||||||||||
() => ({ | ||||||||||||||||||||||||||
playing: false, | ||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||
imaVidLookerRef.current?.element?.update(() => ({ | ||||||||||||||||||||||||||
playing: false, | ||||||||||||||||||||||||||
})); | ||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
registerOnSeekCallbacks({ | ||||||||||||||||||||||||||
start: () => { | ||||||||||||||||||||||||||
(activeLookerRef.current as unknown as ImaVidLooker).element.update( | ||||||||||||||||||||||||||
() => ({ | ||||||||||||||||||||||||||
seeking: true, | ||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||
imaVidLookerRef.current?.element?.update(() => ({ | ||||||||||||||||||||||||||
seeking: true, | ||||||||||||||||||||||||||
})); | ||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||
end: () => { | ||||||||||||||||||||||||||
(activeLookerRef.current as unknown as ImaVidLooker).element.update( | ||||||||||||||||||||||||||
() => ({ | ||||||||||||||||||||||||||
seeking: false, | ||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||
imaVidLookerRef.current?.element?.update(() => ({ | ||||||||||||||||||||||||||
seeking: false, | ||||||||||||||||||||||||||
})); | ||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
@@ -265,9 +306,8 @@ export const ImaVidLookerReact = React.memo( | |||||||||||||||||||||||||
// hack: poll every 10ms for total frame count | ||||||||||||||||||||||||||
// replace with event listener or callback | ||||||||||||||||||||||||||
let intervalId = setInterval(() => { | ||||||||||||||||||||||||||
const totalFrameCount = ( | ||||||||||||||||||||||||||
activeLookerRef.current as unknown as ImaVidLooker | ||||||||||||||||||||||||||
).frameStoreController.totalFrameCount; | ||||||||||||||||||||||||||
const totalFrameCount = | ||||||||||||||||||||||||||
imaVidLookerRef.current.frameStoreController.totalFrameCount; | ||||||||||||||||||||||||||
if (totalFrameCount) { | ||||||||||||||||||||||||||
setTotalFrameCount(totalFrameCount); | ||||||||||||||||||||||||||
clearInterval(intervalId); | ||||||||||||||||||||||||||
|
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
Simplify type assertion for
imaVidLookerRef
.The current type assertion uses
as unknown as
, which is unnecessarily complex. You can simplify it by directly castingactiveLookerRef
to the desired type.Apply this diff to simplify the type assertion:
📝 Committable suggestion