feat: Jim/GRWT-6600/implement fibonacci fan drawing tool in interactive layer #399
+3,248
−105
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 30 commits
March 7, 2025 15:33
…_drawing_tools_attempt_control_repainting
- Add FibfanDragState enum with explicit drag states - Replace bool? isDraggingStartPoint with FibfanDragState? _dragState - Refactor drag logic to use switch statements for better readability - Improve visual feedback and alignment guide logic - Enhance code maintainability and type safety Addresses sourcery-ai suggestion to improve drag state management in Fibonacci Fan drawing tool. Files changed: - lib/src/deriv_chart/interactive_layer/interactable_drawings/fibfan/drag_state.dart (new) - lib/src/deriv_chart/interactive_layer/interactable_drawings/fibfan/fibfan_interactable_drawing.dart
- Replace linear interpolation with angle-based calculations in mobile preview - Add missing dart:math import for trigonometric functions - Use math.atan2() and math.tan() for consistent fan line positioning - Ensure preview matches final fan exactly, especially for steep angles - Add coordinate validation before drawing operations Fixes inconsistency where mobile preview used different calculation method than main fan implementation, causing visual mismatches particularly for steep Fibonacci fans.
|
@sourcery-ai review |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @jim-deriv - I've reviewed your changes and they look great!
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…grwt-6600/implement-fibonacci-fan-drawing-tool-in-interactive-layer
…n callbacks - Fix method signature mismatches in FibfanInteractableDrawing - Add missing onAddingStateChange parameter to preview methods - Fix constructor issues in both desktop and mobile preview classes - Remove extra VoidCallback onDone parameter from onCreateTap methods - Implement proper completion callbacks: - Desktop: 2-step process (start point -> end point) - Mobile: 1-step process (auto-positioned points) - Add required imports for AddingStateInfo - Fix code style issues (TODO comments) All compilation errors resolved, no analysis issues found.
…ation and fix default thickness - Replace custom toolbar with proper dropdown components (ColorPickerDropdownButton, LineThicknessDropdownButton) - Update imports to use standard widget components - Fix default thickness from 0.9 to 1 (removes '0 px' display issue) - Remove redundant thickness arguments that match defaults - Ensure toolbar now shows '1 px' instead of '0 px' - Maintain consistent styling with other drawing tools
…wing tool - Update fibfan_drawing_tool_config.g.dart with latest generated code - Add chart_axis_config.g.dart and overlay_style.g.dart generated files - Update chart_config.g.dart with new configurations - Modify chart_axis_config.dart and overlay_style.dart source files
- Add Top & Bottom Lines (0% & 100%) shared color selector - Add individual color selectors for 38.2%, 50%, and 61.8% fibonacci levels - Update buildDrawingToolBarMenu method in fibfan_interactable_drawing.dart - Add tooltips for each color selector for better UX - Maintain existing fibonacciLevelColors map structure - Ensure proper color synchronization between shared levels
- Update drawFanFills method in helpers.dart to accept fibonacciLevelColors parameter - Modify fill color logic to use level0 color instead of fillStyle.color - Update fibfan_interactable_drawing.dart to pass fibonacciLevelColors to drawFanFills - Fix linting issues by adding const keywords where appropriate - Ensure fill areas now match the 0% and 100% line colors for visual consistency - Maintain backward compatibility with fallback to fillStyle.color if level0 not available
- Updated helpers.dart with corrected angle calculation logic - Modified fibfan_drawing_tool_config.dart for improved functionality
…grwt-6600/implement-fibonacci-fan-drawing-tool-in-interactive-layer
…ag operations - Add drawLabelsWithZIndex helper function to paint_helpers.dart for reusability - Implement z-index logic where dragged edge point labels appear above non-dragged labels - Refactor fibfan drawing tool to use the new helper function - Reduce code duplication by centralizing label z-index logic - Ensure consistent behavior across both Y-axis (quote) and X-axis (epoch) labels This improves visual feedback during drag operations by preventing label overlap and clearly indicating which point is being actively manipulated.
- Update trend line to use the reusable drawLabelsWithZIndex helper function - Implement z-index logic for both Y-axis (quote) and X-axis (epoch) labels - Ensure dragged edge point labels appear above non-dragged labels - Maintain consistent behavior with fibfan drawing tool - Reduce code duplication by leveraging centralized helper function This extends the label z-index functionality to trend lines, providing better visual feedback during drag operations across all two-point drawing tools.
- Add missing blank line for consistent code formatting
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @jim-deriv - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `lib/src/deriv_chart/interactive_layer/interactable_drawings/fibfan/helpers.dart:230` </location>
<code_context>
+/// **Performance Optimization:**
+/// This class implements paint object caching to improve rendering performance
+/// by reusing Paint objects instead of creating new ones for each draw operation.
+class FibonacciFanHelpers {
+ /// Cache for line paint objects to improve performance.
+ ///
</code_context>
<issue_to_address>
Paint and text painter caches are never cleared automatically, which could lead to memory bloat.
Since caches are only cleared manually, frequent style changes in long-lived charts may cause unbounded memory use. Consider tying cache clearing to theme or style changes, or implementing automated cache management.
</issue_to_address>
### Comment 2
<location> `lib/src/deriv_chart/interactive_layer/interactable_drawings/fibfan/helpers.dart:357` </location>
<code_context>
+ /// **Performance Benefit:** Eliminates TextPainter creation and layout
+ /// overhead for repeated label rendering, significantly improving
+ /// performance during animations and frequent redraws.
+ static TextPainter getCachedTextPainter(
+ String text, Color color, double fontSize) {
+ final String key = 'text_${text}_${color.value}_$fontSize';
</code_context>
<issue_to_address>
TextPainter cache key does not account for font family or weight beyond hardcoded w500.
If labelStyle uses a different font family or weight, the cache key may cause incorrect rendering. Include the full TextStyle or its hash in the cache key.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
lib/src/deriv_chart/interactive_layer/interactable_drawings/fibfan/helpers.dart
Show resolved
Hide resolved
lib/src/deriv_chart/interactive_layer/interactable_drawings/fibfan/helpers.dart
Show resolved
Hide resolved
…tool - Add automated paint and text painter cache management to prevent memory bloat - Implement size-based eviction (100 entries max) and time-based expiration (5 minutes) - Add cache wrapper classes with timestamp tracking for intelligent cleanup - Integrate configuration change detection for automatic cache invalidation - Add comprehensive cache management API with selective clearing methods - Ensure cached objects always reflect current styling and configuration - Maintain optimal drawing performance while preventing unbounded memory growth Addresses performance issue where caches were never cleared automatically, which could lead to memory bloat in long-lived chart applications.
- Add import for DrawingToolConfig - Add 'T extends DrawingToolConfig' constraint to generic type parameter - Resolves Dart error: 'T' doesn't conform to the bound 'DrawingToolConfig'
- Add optional fontWeight and fontFamily parameters to getCachedTextPainter method - Update cache key format to include font weight index and font family - Prevent incorrect text rendering when different font families or weights are used - Add new cache clearing methods for font weight and font family - Maintain backward compatibility with existing code - Addresses sourcery-ai bug risk about incomplete cache key This fixes the issue where TextPainter cache key did not account for font family or weight beyond hardcoded w500, which could cause incorrect rendering when labelStyle uses different font properties.
- Updated edge point rendering to use level0 color (same as level100) - Simplified color logic by using single edgePointColor variable - Applied changes to main drawing, desktop preview, and mobile preview - Ensures visual consistency between edge points and fibonacci lines
- Updated drawPointAlignmentGuides calls to use level0 color (same as edge points) - Applied changes to main drawing, desktop preview, and mobile preview - Ensures visual consistency between alignment guides and fibonacci lines - Alignment guides now match the top/bottom line colors for better coherence
…points - Updated drawValueLabel and drawEpochLabel calls to use level0 color (same as edge points) - Applied changes to main drawing, desktop preview, and mobile preview - Ensures complete visual consistency across all fibfan UI elements - All labels (value, epoch, alignment guides) now match the top/bottom line colors
…awValueLabel - Updated drawEpochLabel to use color parameter for text instead of textStyle.color - Updated drawValueLabel to use color parameter for text instead of textStyle.color - Ensures consistent visual appearance where text and border colors are synchronized
- Hide crosshair when hovering over a selected drawing tool - Show crosshair when hovering over non-selected drawing tools or empty space - Maintain normal crosshair behavior in all other scenarios - Use existing _findHoveredDrawing method for hit detection
- Rewrite _hitTestFanArea to use exact same calculations as drawFanFills - Add _isPointInTriangle method using barycentric coordinates for precise hit testing - Test each triangular fill area between adjacent fan lines (0%-38.2%, 38.2%-50%, etc.) - Ensure perfect coverage of all visible fan areas including upper regions - Maintain existing endpoint and fan line hit testing for precise interactions - Fix issue where parts of the fan (especially upper areas) were not draggable
- Add calculateFanAreaPolygons() method to helpers for shared polygon calculations - Update drawFanFills() to use shared calculation method - Update _hitTestFanArea() to use shared calculation method - Eliminates code duplication between hit testing and drawing operations - Ensures perfect consistency between visual fan areas and hit testing - Improves maintainability by centralizing geometric calculations
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @jim-deriv - I've reviewed your changes and they look great!
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…grwt-6600/implement-fibonacci-fan-drawing-tool-in-interactive-layer
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Clickup link: https://app.clickup.com/t/20696747/GRWT-6600
This PR contains the following changes:
Developers Note (Optional)
Pre-launch Checklist (For PR creator)
As a creator of this PR:
Reviewers
Pre-launch Checklist (For Reviewers)
As a reviewer I ensure that:
Pre-launch Checklist (For QA)
Pre-launch Checklist (For Maintainer)
Summary by Sourcery
Implement the Fibonacci Fan drawing tool in the interactive layer, complete with configuration, rendering helpers, and mobile/desktop preview behaviors, while adding new epoch labeling and a UI control toggle.
New Features:
Enhancements:
Documentation:
Summary by Sourcery
Implement Fibonacci Fan drawing tool in the interactive layer with full configuration, rendering helpers, performance caching, and mobile/desktop creation workflows.
New Features:
Enhancements:
Build:
Documentation: