Feature/contrast adjustment and image zoom issue #8800 #8881
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 pull request introduces enhanced canvas interaction capabilities in the CVAT UI, focusing on context image rendering and user interaction. The changes primarily involve adding a new Changes
Sequence DiagramsequenceDiagram
participant User
participant ContextImage
participant CanvasControl as useCanvasControl
User->>ContextImage: Interact with Canvas
alt Zoom
User->>ContextImage: Scroll Mouse Wheel
ContextImage->>CanvasControl: handleZoomChange()
CanvasControl-->>ContextImage: Update Zoom Level
end
alt Drag
User->>ContextImage: Press Left Mouse Button
ContextImage->>CanvasControl: handleMouseDown()
User->>ContextImage: Move Mouse
ContextImage->>CanvasControl: handleMouseMove()
User->>ContextImage: Release Mouse Button
ContextImage->>CanvasControl: handleMouseUp()
end
alt Color Adjustment
User->>ContextImage: Press Right Mouse Button
ContextImage->>CanvasControl: Adjust Brightness/Contrast
end
Poem
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
CodeRabbit Configuration File (
|
|
Added gamma brightness handling when right mouse button is held down. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (11)
cvat-ui/src/components/annotation-page/canvas/views/context-image/model/hooks/useCanvasControl.tsx (4)
28-33: Consider resetting zoom and offset on fullscreen exit
Currently,positionRef.currentis reset and thetransformstyle is applied to the canvas wheneverfullscreenKeychanges. However, if the user exits fullscreen after panning or zooming, the canvas remains in the transformed state. You might consider deciding whether to also reset the zoom/offset state on fullscreen exit to ensure the user sees a clean default view.
40-47: Avoid rapid zoom changes on trackpads
Usingevent.deltaY * 0.001can lead to rapid zoom changes on sensitive trackpads. Consider adding a configurable "zoom step" or applying an exponential factor for smoother zoom in/out experiences on different hardware.
67-79: Right-click brightness/contrast updates
Handling brightness/contrast changes on mouse movement with right-click is a nice UI trick. However, it might be more discoverable to provide some tooltip or visual indicator. Also, consider whether a large movement could inadvertently push the values to their extreme (50 or 200) too easily.
120-125: Consider enablinghandleMouseLeave
Commented-out code inhandleMouseLeavesuggests potential intention to reset drag state. If the user drags outside the canvas, it might be beneficial to stop the dragging to maintain consistent UX.cvat-ui/src/components/annotation-page/canvas/views/context-image/context-image.tsx (3)
12-12: Icon import usage
You’re importing more icons (SettingOutlined) than are used for a single minimal purpose. This is absolutely fine for clarity, but ensure they are all necessary to avoid bundling overhead if your build environment is sensitive to that.
100-104: Consider usingonMouseLeaveon the canvas
AttachingonMouseLeaveto the wrapper could inadvertently miss cases where the mouse leaves the<canvas>but not the wrapper. If it is the canvas area that matters, adding this to the<canvas>itself might be more precise.
114-117: Reset color button
AReloadOutlinedicon is used to reset color settings. The meaning might not be obvious to all users. A brief tooltip or “Reset color settings” label could improve discoverability.cvat-ui/src/components/annotation-page/canvas/grid-layout/canvas-layout.tsx (1)
204-204: DRY principle
The repeated pattern ofViewFabric(value, fullscreenKey)for each layout item is straightforward, but if further expansions appear, consider a more scalable approach such as mapping additional props in a single step insideViewFabric.cvat-ui/src/components/annotation-page/canvas/views/canvas2d/canvas-wrapper.tsx (2)
749-766: Potential user confusion on brightness/contrast
The code updates brightness and contrast on a right-click drag inonCanvasMouseMove. If not documented or indicated, some users may find it confusing. Consider providing a small hint or an in-app message the first time the user tries right-click dragging.
1078-1078: Event listener subscription
All event listeners are added at once ininitialSetup(). If your component grows in complexity, you might consider modularizing these listeners or grouping by feature to keep it cleaner.cvat-ui/src/components/annotation-page/canvas/views/context-image/styles.scss (1)
66-75: Consider standardizing button opacity valuesThe reset button's opacity behavior (0.6 to 1.0) differs slightly from the setup button's hover behavior (0.6 to 0.9). Consider standardizing these values for consistency.
&:hover { - opacity: 1; + opacity: 0.9; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cvat-ui/src/components/annotation-page/canvas/grid-layout/canvas-layout.tsx(3 hunks)cvat-ui/src/components/annotation-page/canvas/views/canvas2d/canvas-wrapper.tsx(3 hunks)cvat-ui/src/components/annotation-page/canvas/views/context-image/context-image.tsx(5 hunks)cvat-ui/src/components/annotation-page/canvas/views/context-image/model/hooks/index.ts(1 hunks)cvat-ui/src/components/annotation-page/canvas/views/context-image/model/hooks/useCanvasControl.tsx(1 hunks)cvat-ui/src/components/annotation-page/canvas/views/context-image/model/index.ts(1 hunks)cvat-ui/src/components/annotation-page/canvas/views/context-image/styles.scss(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- cvat-ui/src/components/annotation-page/canvas/views/context-image/model/hooks/index.ts
- cvat-ui/src/components/annotation-page/canvas/views/context-image/model/index.ts
🔇 Additional comments (7)
cvat-ui/src/components/annotation-page/canvas/views/context-image/context-image.tsx (3)
21-21: Fullscreen key optionality
fullscreenKey?: string; is helpful. Ensure that parent components properly check for existence before passing an undefined string to child components, preventing potential type confusion if deciding to do more advanced logic with fullscreenKey.
43-53: Correctness check
The destructured return from useCanvasControl includes items like canvasStyle and event handlers. Ensure that the wrapper <div className='cvat-context-image-wrapper'> is sized appropriately so that your canvas transformations (like zoom or drag) do not visually clip.
128-139: Conditionally render the draggable wrapper
The draggable-wrapper div is conditionally rendered if contextImageOffset < Object.keys(contextImageData).length. That’s good for preventing empty canvas usage. Make sure this condition correctly handles all edge cases where offset might be invalid or out-of-bounds (e.g., negative offset).
cvat-ui/src/components/annotation-page/canvas/grid-layout/canvas-layout.tsx (1)
Line range hint 40-52: Ensure type-safety and consistent usage
fullscreenKey?: string is passed to ViewFabric. Confirm that any usage respects the possibility of undefined so that no runtime errors occur if an undefined key is passed.
cvat-ui/src/components/annotation-page/canvas/views/canvas2d/canvas-wrapper.tsx (1)
594-594: Event listener removal
The removal of the mousemove listener here is crucial for preventing memory leaks. This looks correct, but be sure future additions of event listeners follow the same removal pattern in componentWillUnmount.
cvat-ui/src/components/annotation-page/canvas/views/context-image/styles.scss (2)
12-13: LGTM: Wrapper styling improvements
The addition of overflow: hidden properly contains the content, and the border-radius styling creates a cohesive design with rounded top corners.
78-91: LGTM: Draggable wrapper implementation
The flex layout with centered alignment and absolute positioning of the canvas provides a robust foundation for the zoom and drag functionality. The overflow handling prevents content spillage during interactions.
Let's verify that these styles work correctly with the associated React component:
✅ Verification successful
Let me try to find the associated component with these alternative search strategies.
LGTM: Draggable wrapper styles are correctly integrated
The styles are properly integrated with the React component in context-image.tsx. The component uses a canvas element within the draggable wrapper div, which aligns perfectly with the CSS implementation. The flex layout, absolute positioning, and overflow handling will work as intended for the canvas element referenced by canvasRef.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find the corresponding React component to ensure proper integration
ast-grep --pattern 'class $_ extends $_ {
$$$
render() {
$$$
draggable-wrapper
$$$
}
}'
Length of output: 107
Script:
#!/bin/bash
# Let's try a different approach to find the React component using ripgrep
rg -l "draggable-wrapper" --type ts --type tsx --type jsx
Length of output: 85
Script:
#!/bin/bash
# Also search in the specific directory where the styles are located
rg -l "draggable-wrapper" "cvat-ui/src/components/annotation-page/canvas/views/context-image/"
Length of output: 254
Script:
#!/bin/bash
# Search for any React component file in the context-image directory
fd -e tsx -e jsx "cvat-ui/src/components/annotation-page/canvas/views/context-image/"
Length of output: 655
Script:
#!/bin/bash
# Let's examine the context-image.tsx file to verify the implementation
rg "draggable-wrapper" -B 2 -A 2 "cvat-ui/src/components/annotation-page/canvas/views/context-image/context-image.tsx"
Length of output: 341
...i/src/components/annotation-page/canvas/views/context-image/model/hooks/useCanvasControl.tsx
Outdated
Show resolved
Hide resolved
…age/model/hooks/useCanvasControl.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…Contrast_adjustment_and_image_zoom_issue_#8800
|
Added zoom limitation when zooming out. |
|
Motivation and context
How has this been tested?
Checklist
developbranch(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
useCanvasControl, for managing canvas interactions.Bug Fixes
Style