Add OSD preview rulers #4567
Add OSD preview rulers #4567
Conversation
WalkthroughAdds a canvas-based ruler overlay to the OSD preview with a default-enabled toggle, updates preview DOM and styles to host the overlay, implements ruler drawing and lifecycle (resize, toggle, render hook, cleanup), and adds a localization entry for the toggle. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant OSD UI
participant OSD.drawRulers
participant Canvas
User->>OSD UI: Toggle "Rulers" checkbox
OSD UI->>OSD.drawRulers: invoke (enabled)
OSD.drawRulers->>Canvas: measure preview, size canvas, draw axes/ticks/labels
sequenceDiagram
participant Window
participant OSD UI
participant OSD.drawRulers
participant Canvas
Window-->>OSD UI: resize.osd-rulers event
OSD UI->>OSD.drawRulers: redraw (throttled/requestAnimationFrame)
OSD.drawRulers->>Canvas: recompute bounds, resize canvas, redraw rulers
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 6
🧹 Nitpick comments (1)
src/css/tabs/osd.less (1)
211-212: Consider using CSS variables for margin valuesThe hard-coded 20px margins might not scale well across different screen sizes or zoom levels. Consider using CSS variables or relative units.
- margin-top: 20px; - margin-left: 20px; + margin-top: var(--ruler-margin, 20px); + margin-left: var(--ruler-margin, 20px);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/css/tabs/osd.less(5 hunks)src/js/tabs/osd.js(4 hunks)src/tabs/osd.html(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-14T13:42:41.217Z
Learnt from: syahmizul
PR: betaflight/betaflight-configurator#4516
File: src/js/tabs/osd.js:3544-3559
Timestamp: 2025-06-14T13:42:41.217Z
Learning: In OSD positioning systems, width/height calculations like `limits.maxX - limits.minX` work in conjunction with centering coordinate formulas like `Math.floor((OSD.data.displaySize.x - w) / 2)`. Adding +1 to width calculations breaks centering by making elements off-center by half a character position. The entire positioning system must be considered holistically when making changes.
Applied to files:
src/js/tabs/osd.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy
🔇 Additional comments (4)
src/css/tabs/osd.less (2)
31-31: Good: Using standard CSS propertyRemoving the vendor prefix in favor of the standard
appearanceproperty improves cross-browser compatibility.
192-206: LGTM: Well-structured overlay implementationThe preview container and ruler overlay CSS is properly structured with appropriate positioning and pointer-events handling.
src/js/tabs/osd.js (1)
4230-4243: LGTM: Proper resize throttling implementationThe window resize listener properly uses requestAnimationFrame for throttling and includes a timeout to ensure the preview has resized first. Good implementation.
src/tabs/osd.html (1)
63-67: LGTM: Well-structured DOM for overlayThe preview container structure with the overlay canvas is properly implemented. The initial hidden state and DOM hierarchy support the overlay rendering approach.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
locales/en/messages.json(1 hunks)src/js/tabs/osd.js(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-14T13:43:10.231Z
Learnt from: syahmizul
PR: betaflight/betaflight-configurator#4516
File: src/js/tabs/osd.js:3544-3559
Timestamp: 2025-06-14T13:43:10.231Z
Learning: In the Betaflight Configurator OSD positioning feature (src/js/tabs/osd.js), the elementWidth and elementHeight calculations use limits.maxX - limits.minX and limits.maxY - limits.minY without adding 1. While adding 1 would be mathematically correct for inclusive bounds, it causes visual centering issues where elements appear off-center. The current calculation produces better visual centering results, so the +1 correction should be avoided to maintain proper visual alignment.
Applied to files:
src/js/tabs/osd.js
📚 Learning: 2025-06-14T13:42:41.217Z
Learnt from: syahmizul
PR: betaflight/betaflight-configurator#4516
File: src/js/tabs/osd.js:3544-3559
Timestamp: 2025-06-14T13:42:41.217Z
Learning: In OSD positioning systems, width/height calculations like `limits.maxX - limits.minX` work in conjunction with centering coordinate formulas like `Math.floor((OSD.data.displaySize.x - w) / 2)`. Adding +1 to width calculations breaks centering by making elements off-center by half a character position. The entire positioning system must be considered holistically when making changes.
Applied to files:
src/js/tabs/osd.js
🔇 Additional comments (9)
src/js/tabs/osd.js (9)
1-36: LGTM! Well-organized constants with clear documentationThe ruler constants are properly organized at the file level with clear naming and helpful comments. This follows the feedback from previous reviews to extract magic numbers into named constants.
2363-2383: Good consolidation of ruler configurationThe
OSD.rulerConfigobject effectively consolidates all ruler-related configuration in one place, making it easy to maintain and modify. The backwards compatibility comment forlabelPaddingis helpful.
2408-2457: Robust context setup with proper error handlingThe
setupRulerContextfunction properly validates DOM elements and handles edge cases like empty element arrays. The canvas resizing logic and context configuration are appropriate.
2459-2466: Simple and effective coordinate helpersThe column and row center calculation helpers are concise and correctly handle the coordinate transformations using
getBoundingClientRect().
2468-2656: Well-structured axis drawing functionsThe four axis drawing functions (
_drawTopAxis,_drawBottomAxis,_drawLeftAxis,_drawRightAxis) follow a consistent pattern and properly handle:
- Major/minor tick distinctions
- Center line highlighting
- Label positioning with edge constraints
- Text stroke effects for visibility
The logic for determining major ticks and label positioning is appropriate for each axis orientation.
2658-2670: Clean coordinator function with proper error handlingThe main
drawRulersfunction acts as a clean coordinator that:
- Uses the configuration object
- Handles initialization failures gracefully
- Orchestrates the four axis drawing functions
- Maintains a clear separation of concerns
4281-4282: Proper integration with preview rendering lifecycleThe ruler drawing is correctly placed after the preview is rendered, ensuring the DOM elements are available for measurement and positioning.
4317-4336: Well-implemented event handling with proper throttlingThe event handlers are properly implemented:
- Change handler for the rulers toggle
- Resize handler with proper throttling using
requestAnimationFrame- Cleanup to prevent memory leaks
- Timeout to allow preview to resize before redrawing rulers
The throttling mechanism prevents excessive redraw calls during window resizing.
4458-4458: Proper cleanup prevents memory leaksThe cleanup properly unbinds the resize event handler with the correct namespace, preventing memory leaks when the tab is closed.
|
|
Preview URL: https://654d71e0.betaflight-configurator.pages.dev |
Summary by CodeRabbit
New Features
UI Improvements
Localization