Fixed issue: fit:canvas may not generate in some cases #8750
Conversation
WalkthroughThis update addresses issues related to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CanvasView
participant CanvasModel
User->>CanvasView: Trigger fit action
CanvasView->>CanvasModel: Call fit method
CanvasModel->>CanvasModel: Calculate updatedScale, updatedTop, updatedLeft
CanvasModel->>CanvasView: Notify changes if necessary
CanvasView->>User: Update canvas view
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 (
|
![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
🧹 Outside diff range and nitpick comments (3)
changelog.d/20241128_112750_sekachev.bs_fixed_fit.md (1)
1-4: Consider enhancing the changelog entry with additional details.While the current entry correctly describes the fix, consider adding more context to help users understand:
- What causes this issue (e.g., specific user actions or conditions)
- The impact on users (e.g., visual or functional consequences)
- The technical solution implemented (e.g., event handling improvements)
Here's a suggested enhancement:
### Fixed -fit:canvas event is not generated if to fit it from the controls sidebar -(<https://github.com/cvat-ai/cvat/pull/8750>) +- Fixed an issue where the fit:canvas event was not being generated when using the fit + operation from the controls sidebar. This could prevent proper canvas fitting and + affect the viewport adjustment. The fix improves event handling to ensure consistent + behavior across all canvas fitting operations. + (<https://github.com/cvat-ai/cvat/pull/8750>)cvat-canvas/src/typescript/canvasModel.ts (1)
708-717: Good optimization! Added check before updating valuesThe new condition prevents unnecessary updates and notifications by checking if values have actually changed. The added comment about
fittedScalehelps explain its relationship with zooming.Consider adding type annotations to the temporary variables for better type safety:
-const updatedScale = Math.min(Math.max(updatedScale, FrameZoom.MIN), FrameZoom.MAX); -const updatedTop = this.data.canvasSize.height / 2 - this.data.imageSize.height / 2; -const updatedLeft = this.data.canvasSize.width / 2 - this.data.imageSize.width / 2; +const updatedScale: number = Math.min(Math.max(updatedScale, FrameZoom.MIN), FrameZoom.MAX); +const updatedTop: number = this.data.canvasSize.height / 2 - this.data.imageSize.height / 2; +const updatedLeft: number = this.data.canvasSize.width / 2 - this.data.imageSize.width / 2;cvat-canvas/src/typescript/canvasView.ts (1)
1893-1901: Dispatch 'canvas.fit' event when image is fittedThis change correctly ensures the
canvas.fitevent is dispatched when the image is fitted, addressing the issue where the event was not generated in some cases.Consider adding unit tests to verify the
canvas.fitevent dispatch to prevent future regressions.Would you like assistance in creating the unit tests or opening a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
changelog.d/20241128_112750_sekachev.bs_fixed_fit.md(1 hunks)cvat-canvas/src/typescript/canvasModel.ts(1 hunks)cvat-canvas/src/typescript/canvasView.ts(1 hunks)
🔇 Additional comments (1)
cvat-canvas/src/typescript/canvasModel.ts (1)
690-706: LGTM! Improved readability with temporary variables
The refactoring improves code clarity by:
- Using descriptive temporary variables for scale and position calculations
- Maintaining clear separation between angle-dependent scale calculations
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #8750 +/- ##
========================================
Coverage 74.05% 74.06%
========================================
Files 409 409
Lines 43783 43790 +7
Branches 3984 3986 +2
========================================
+ Hits 32425 32433 +8
+ Misses 11358 11357 -1
|
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
Bug Fixes
Improvements
Refactor