-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Do not block canvas navigation when frame is being fetched #8284
Conversation
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe recent updates enhance the functionality and performance of the CVAT application by incrementing version numbers and refining logic within key components. Notable improvements include better image loading state management, streamlined event handling, and clearer control flows in annotation management. These changes aim to optimize user interaction, improve responsiveness, and maintain robust state management, ensuring a smoother experience during image annotation tasks. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CanvasViewImpl
participant CanvasModelImpl
participant AnnotationReducer
User->>CanvasViewImpl: Load Image
CanvasViewImpl-->>CanvasModelImpl: Set isImageLoading(true)
CanvasModelImpl-->>CanvasViewImpl: Notify image loading
CanvasViewImpl-->>User: Display loading state
CanvasModelImpl->>CanvasModelImpl: Check image existence
CanvasModelImpl-->>CanvasViewImpl: Set isImageLoading(false)
CanvasViewImpl-->>User: Display image
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
cvat-ui/src/containers/annotation-page/top-bar/top-bar.tsx (1)
Reintroduce
canvasIsReady
Check inonPrevFrame
MethodThe
canvasIsReady
state is crucial for ensuring that frame navigation and other canvas-dependent actions occur only when the canvas is fully loaded. Its removal from theonPrevFrame
method could lead to unexpected behavior. Consider reintroducing this check to maintain stability and prevent errors.
- File:
cvat-ui/src/containers/annotation-page/top-bar/top-bar.tsx
- Method:
onPrevFrame
Analysis chain
Line range hint
374-385
: Verify the impact of removingcanvasIsReady
check inonPrevFrame
.The method now navigates frames based on other conditions. Ensure that the removal of
canvasIsReady
does not lead to unexpected behavior when the canvas is not ready.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `canvasIsReady` in conjunction with frame navigation methods across the codebase. # Test: Search for conditions involving `canvasIsReady` and frame navigation. Expect: Ensure proper handling when canvas is not ready. rg --type js --type ts 'canvasIsReady.*frame'Length of output: 164
Script:
#!/bin/bash # Search for the `onPrevFrame` method definition and its usage across the codebase to understand its context and conditions. ast-grep --lang ts --pattern 'onPrevFrame($_) { $$$ }' # Additionally, search for invocations of `onPrevFrame` to see how it is used. rg 'onPrevFrame'Length of output: 1744
Script:
#!/bin/bash # Search for canvas-related state management to understand how canvas readiness is ensured in the codebase. rg 'canvasIsReady'Length of output: 3558
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- cvat-canvas/package.json (1 hunks)
- cvat-canvas/src/typescript/canvasModel.ts (2 hunks)
- cvat-canvas/src/typescript/canvasView.ts (5 hunks)
- cvat-ui/package.json (1 hunks)
- cvat-ui/src/components/annotation-page/canvas/views/canvas2d/canvas-wrapper.tsx (2 hunks)
- cvat-ui/src/containers/annotation-page/top-bar/top-bar.tsx (13 hunks)
- cvat-ui/src/reducers/annotation-reducer.ts (2 hunks)
Files skipped from review due to trivial changes (2)
- cvat-canvas/package.json
- cvat-ui/package.json
Additional comments not posted (20)
cvat-ui/src/containers/annotation-page/top-bar/top-bar.tsx (9)
Line range hint
354-363
: Verify the impact of removingcanvasIsReady
check inonBackward
.The method now navigates frames based on other conditions. Ensure that the removal of
canvasIsReady
does not lead to unexpected behavior when the canvas is not ready.
298-300
: Verify the impact of removingcanvasIsReady
check inundo
.The
undo
method now only checks if the frame can be changed and if there is anundoAction
. Ensure that this change does not lead to issues when the canvas is not ready.
306-308
: Verify the impact of removingcanvasIsReady
check inredo
.The
redo
method now only checks if the frame can be changed and if there is aredoAction
. Ensure that this change does not lead to issues when the canvas is not ready.
466-470
: Verify the impact of removingcanvasIsReady
check insearchAnnotations
.The method now searches annotations based on other conditions. Ensure that the removal of
canvasIsReady
does not lead to unexpected behavior when the canvas is not ready.
Line range hint
403-413
: Verify the impact of removingcanvasIsReady
check inonNextFrame
.The method now navigates frames based on other conditions. Ensure that the removal of
canvasIsReady
does not lead to unexpected behavior when the canvas is not ready.
338-343
: Verify the impact of removingcanvasIsReady
check inonFirstFrame
.The method now navigates frames based on other conditions. Ensure that the removal of
canvasIsReady
does not lead to unexpected behavior when the canvas is not ready.Verification successful
Removal of
canvasIsReady
fromonFirstFrame
is unlikely to cause issues.The
canvasIsReady
check is still used in other parts of the code to control frame navigation and playback, ensuring that actions are only performed when the canvas is ready. This suggests that the change does not introduce unexpected behavior.
- The
canvasIsReady
condition is used in conjunction withplaying
andframeFetching
to control frame navigation.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `canvasIsReady` in conjunction with frame navigation methods across the codebase. # Test: Search for conditions involving `canvasIsReady` and frame navigation. Expect: Ensure proper handling when canvas is not ready. rg --type js --type ts 'canvasIsReady.*frame'Length of output: 164
Script:
#!/bin/bash # Description: Explore the usage of `canvasIsReady` throughout the file to understand its role in frame navigation. # Test: Search for all occurrences of `canvasIsReady` in the file to analyze its usage context. rg --type ts 'canvasIsReady' cvat-ui/src/containers/annotation-page/top-bar/top-bar.tsx -A 5 -B 5Length of output: 1710
559-561
: Verify the impact of removingcanvasIsReady
check inonDeleteFrame
.The method now deletes frames based on the job type. Ensure that the removal of
canvasIsReady
does not lead to unexpected behavior when the canvas is not ready.
451-456
: Verify the impact of removingcanvasIsReady
check inonLastFrame
.The method now navigates frames based on other conditions. Ensure that the removal of
canvasIsReady
does not lead to unexpected behavior when the canvas is not ready.
Line range hint
431-440
: Verify the impact of removingcanvasIsReady
check inonForward
.The method now navigates frames based on other conditions. Ensure that the removal of
canvasIsReady
does not lead to unexpected behavior when the canvas is not ready.Verification successful
Verification of
canvasIsReady
Removal inonForward
:The removal of the
canvasIsReady
check from theonForward
method does not directly impact frame navigation. ThecanvasIsReady
state is used elsewhere to ensure the canvas is ready for operations, but its absence inonForward
is not linked to any critical dependencies that would cause issues.
- The
canvasIsReady
variable is derived from the Redux state and is used in other conditions to ensure readiness for playing frames.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `canvasIsReady` in conjunction with frame navigation methods across the codebase. # Test: Search for conditions involving `canvasIsReady` and frame navigation. Expect: Ensure proper handling when canvas is not ready. rg --type js --type ts 'canvasIsReady.*frame'Length of output: 164
Script:
#!/bin/bash # Description: Find where `canvasIsReady` is set or modified in the codebase to understand its role. # Test: Search for assignments or modifications to `canvasIsReady`. rg --type js --type ts 'canvasIsReady\s*='Length of output: 772
cvat-ui/src/reducers/annotation-reducer.ts (3)
592-596
: Efficient check for object existence inACTIVATE_OBJECT
.The addition of the
requiredObjectExists
check before proceeding with state updates is a good optimization, preventing unnecessary operations.
1013-1021
: Simplified state update inDELETE_FRAME_SUCCESS
.Directly assigning
data
toframe.data
streamlines the state update process, reducing complexity and improving clarity.
1013-1021
: Simplified state update inRESTORE_FRAME_SUCCESS
.Directly assigning
data
toframe.data
streamlines the state update process, reducing complexity and improving clarity.cvat-canvas/src/typescript/canvasModel.ts (2)
552-556
: Conditional notification forOBJECTS_UPDATED
.Adding a check to ensure
OBJECTS_UPDATED
is only triggered if an image exists improves performance by avoiding unnecessary updates.
561-564
: Improved state management forimageIsDeleted
.Setting
imageIsDeleted
immediately and resettingangle
if the image is deleted ensures accurate internal state representation.cvat-ui/src/components/annotation-page/canvas/views/canvas2d/canvas-wrapper.tsx (2)
929-931
: Good addition ofactivatedState
check.The added check for
activatedState
before callingfocus
improves robustness by preventing potential errors whenactivatedState
is null or undefined.
962-963
: Flexible argument handling inframeData
proxy.Modifying the
get
method to accept variable arguments (...args: any[]
) increases flexibility in handling different data retrieval scenarios, aligning with the PR's objective of improving responsiveness.cvat-canvas/src/typescript/canvasView.ts (4)
65-65
: Good addition:isImageLoading
property.The introduction of the
isImageLoading
property is a well-structured approach to manage the image loading state and control user interactions accordingly.
1442-1442
: Appropriate initialization ofisImageLoading
.Initializing
isImageLoading
totrue
in the constructor is appropriate, as it reflects the initial loading state of the canvas.
1649-1663
: Improvement inmousemove
event handling.The check for
isImageLoading
before dispatching themousemove
event is a smart optimization to prevent unnecessary processing during image loading.
Line range hint
1795-1829
: Correct handling ofisImageLoading
innotify
method.The logic for updating
isImageLoading
in response toIMAGE_CHANGED
events is correctly implemented, ensuring that the canvas accurately reflects the loading state.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #8284 +/- ##
========================================
Coverage 83.37% 83.37%
========================================
Files 390 390
Lines 41549 41551 +2
Branches 3861 3861
========================================
+ Hits 34642 34644 +2
Misses 6907 6907
|
Quality Gate passedIssues Measures |
Motivation and context
How has this been tested?
Checklist
develop
branch(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
New Features
cvat-canvas
andcvat-ui
packages, indicating minor improvements and bug fixes.Bug Fixes
CanvasWrapperComponent
to prevent errors related to undefined states.annotation-reducer
, ensuring more predictable updates.Improvements