Add keyboard support for esc and seek for imavid #4726
Conversation
WalkthroughThe modifications enhance the Changes
Poem
Tip New review modelWe have updated our review workflow to use the Anthropic's Claude family of models. Please share any feedback in the discussion post on our Discord. New featuresWalkthrough comment now includes:
Notes:
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/packages/looker/src/elements/common/actions.ts (3 hunks)
Additional context used
Path-based instructions (1)
app/packages/looker/src/elements/common/actions.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Additional comments not posted (3)
app/packages/looker/src/elements/common/actions.ts (3)
556-590: Verify frame calculation logic inseekTo.The function now handles both
VideoStateandImaVidState. Ensure that the frame calculation logic correctly computes the frame positions based on the state type and updates the frame number accordingly.#!/bin/bash # Description: Verify the frame calculation logic for correctness and efficiency. # Test: Search for similar logic in other parts of the codebase. Expect: Consistent and efficient frame calculation. rg --type typescript -A 5 $'frame calculation logic'
615-678: Simplify and verify new logic invideoEscape.The function has been adjusted to handle
ImaVidStatealongsideVideoState, increasing its complexity. Consider simplifying the logic and verify that it correctly handles different scenarios based on the state type.#!/bin/bash # Description: Verify the new logic for handling different scenarios based on the state type. # Test: Search for similar state handling logic in other parts of the codebase. Expect: Consistent and correct state handling. rg --type typescript -A 5 $'state handling logic'
Line range hint
5-678: Ensure TypeScript and React best practices are followed.Review the entire file to ensure that TypeScript typing is accurate and React concepts are properly utilized, especially in the context of the changes made for handling
ImaVidStateandVideoState.#!/bin/bash # Description: Verify TypeScript and React best practices. # Test: Search for TypeScript and React best practices in the codebase. Expect: Proper typing, use of hooks, and component lifecycle management. rg --type typescript -A 5 $'TypeScript and React best practices'
CamronStaley
force-pushed
the
imavid/controls
branch
from
8cd62f4
to
445899f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/packages/looker/src/elements/common/actions.ts (3 hunks)
Additional context used
Path-based instructions (1)
app/packages/looker/src/elements/common/actions.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
| const seekTo: Control<VideoState | ImaVidState> = { | ||
| title: "Seek to", | ||
| detail: "Seek to 0%, 10%, 20%... of the video", | ||
| shortcut: "0-9", | ||
| eventKeys: ["0", "1", "2", "3", "4", "5", "6", "7", "8", "9"], | ||
| action: (update, dispatchEvent, eventKey) => { | ||
| update(({ duration, config: { frameRate, support }, lockedToSupport }) => { | ||
| const frameCount = getFrameNumber(duration, duration, frameRate); | ||
| const total = lockedToSupport ? support[1] - support[0] : frameCount; | ||
| const base = lockedToSupport ? support[0] : 1; | ||
|
|
||
| update((state: ImaVidState | VideoState) => { | ||
| const isImavid = (state.config as ImaVidConfig) | ||
| .frameStoreController as ImaVidFramesController; | ||
| const frameName = isImavid ? "currentFrameNumber" : "frameNumber"; | ||
| let total = 0; | ||
| let base = 0; | ||
| if (isImavid) { | ||
| const { | ||
| config: { | ||
| frameStoreController: { totalFrameCount }, | ||
| }, | ||
| currentFrameNumber, | ||
| } = state as ImaVidState; | ||
| total = totalFrameCount; | ||
| base = currentFrameNumber < totalFrameCount ? currentFrameNumber : 1; | ||
| } else { | ||
| const { | ||
| lockedToSupport, | ||
| config: { support, frameRate }, | ||
| duration, | ||
| } = state as VideoState; | ||
| const frameCount = getFrameNumber(duration, duration, frameRate); | ||
| total = lockedToSupport ? support[1] - support[0] : frameCount; | ||
| base = lockedToSupport ? support[0] : 1; | ||
| } | ||
| const position = Math.round((parseInt(eventKey, 10) / 10) * total) + base; | ||
| dispatchEvent("options", { showJSON: false }); | ||
| return { | ||
| frameNumber: Math.max( | ||
| 1, | ||
| Math.round((parseInt(eventKey, 10) / 10) * total) + base | ||
| ), | ||
| [frameName]: Math.min(total, Math.max(1, position)), |
There was a problem hiding this comment.
Approve the seekTo control changes with a suggestion for readability.
The modifications to the seekTo control are well-aligned with the PR objectives and enhance the functionality by supporting both VideoState and ImaVidState. The logic to handle different state types and compute frame numbers based on the state is correctly implemented.
Suggestion for Improvement:
Consider adding comments within the action function to explain the logic, especially the calculations and conditions, to improve readability and maintainability.
| detail: "Escape the current context", | ||
| alwaysHandle: true, | ||
| action: (update, dispatchEvent, eventKey) => { | ||
| update( | ||
| ({ | ||
| action: (update, dispatchEvent) => { | ||
| update((state: ImaVidState | VideoState) => { | ||
| const isImavid = (state.config as ImaVidConfig) | ||
| .frameStoreController as ImaVidFramesController; | ||
|
|
||
| const frameName = isImavid ? "currentFrameNumber" : "frameNumber"; | ||
|
|
||
| const { | ||
| hasDefaultZoom, | ||
| showOptions, | ||
| frameNumber, | ||
| config: { support }, | ||
| options: { showHelp, showJSON, selectedLabels }, | ||
| lockedToSupport, | ||
| }) => { | ||
| if (showHelp) { | ||
| dispatchEvent("panels", { showHelp: "close" }); | ||
| return { showHelp: "close" }; | ||
| } | ||
| } = state as VideoState; | ||
|
|
||
| if (showOptions) { | ||
| return { showOptions: false }; | ||
| } | ||
| if (showHelp) { | ||
| dispatchEvent("panels", { showHelp: "close" }); | ||
| return { showHelp: "close" }; | ||
| } | ||
|
|
||
| if (showJSON) { | ||
| dispatchEvent("panels", { showJSON: "close" }); | ||
| dispatchEvent("options", { showJSON: false }); | ||
| return { options: { showJSON: false } }; | ||
| } | ||
| if (showOptions) { | ||
| return { showOptions: false }; | ||
| } | ||
|
|
||
| if (!lockedToSupport && Boolean(support)) { | ||
| return { | ||
| frameNumber: support[0], | ||
| lockedToSupport: true, | ||
| }; | ||
| } | ||
| if (showJSON) { | ||
| dispatchEvent("panels", { showJSON: "close" }); | ||
| dispatchEvent("options", { showJSON: false }); | ||
| return { options: { showJSON: false } }; | ||
| } | ||
|
|
||
| if (!hasDefaultZoom) { | ||
| return { | ||
| setZoom: true, | ||
| }; | ||
| } | ||
| if (!lockedToSupport && Boolean(support) && !isImavid) { | ||
| return { | ||
| frameNumber: support[0], | ||
| lockedToSupport: true, | ||
| }; | ||
| } | ||
|
|
||
| if (frameNumber !== 1) { | ||
| return { | ||
| frameNumber: 1, | ||
| playing: false, | ||
| }; | ||
| } | ||
| if (!hasDefaultZoom) { | ||
| return { | ||
| setZoom: true, | ||
| }; | ||
| } | ||
|
|
||
| if (selectedLabels.length) { | ||
| dispatchEvent("clear"); | ||
| return {}; | ||
| } | ||
| if (state[frameName] !== 1) { | ||
| return { | ||
| [frameName]: 1, | ||
| playing: false, | ||
| }; | ||
| } | ||
|
|
||
| dispatchEvent("close"); | ||
| if (selectedLabels.length) { | ||
| dispatchEvent("clear"); | ||
| return {}; | ||
| } | ||
| ); | ||
|
|
||
| dispatchEvent("close"); | ||
| return {}; | ||
| }); |
There was a problem hiding this comment.
Approve the videoEscape control changes with a suggestion for consistency.
The modifications to the videoEscape control enhance its functionality by supporting both VideoState and ImaVidState, allowing users to exit video playback and reset the state using the esc key. The comprehensive logic covers various scenarios and conditions, ensuring robust functionality.
Suggestion for Improvement:
Ensure consistency in the handling of different states and conditions. For example, the handling of showJSON could be streamlined to reduce redundancy in dispatching events and updating state.
CamronStaley
force-pushed
the
imavid/controls
branch
from
445899f
to
6a1c5e1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/packages/looker/src/elements/common/actions.ts (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/packages/looker/src/elements/common/actions.ts
What changes are proposed in this pull request?
Some of the keyboard controls offered in video datasets were not support for imavid so I am adding them here (also fixing an issue where you could go above the size of the video dataset when seeking).
Controls added:
How is this patch tested? If it is not, please explain why.
Local server
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
In imavid datasets you can now use esc to exit and number keys to seek through the videos
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
seekToandvideoEscapecontrols to manage various video states more effectively.Bug Fixes