feat: Add checkpoint based trade type markers #461
Conversation
…t parameter for vertical line markers
…e bottom of vertical lines if provided
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Files |
|
Claude finished @behnam-deriv's task —— View job Code Review: Add checkpoint based trade type markersI've completed a thorough review of this PR. Here are my findings: SummaryThis PR adds support for checkpoint-based markers in multi-stage contracts (e.g., Double Rise/Fall). The implementation introduces two new marker types ( Overall Assessment: The code is generally well-structured and follows the repository's patterns. However, there are several issues that need attention before merging. 🔴 High Priority Issues1. Missing Test CoverageLocation: Entire PR According to the PR checklist and repository guidelines, tests are required for new features. The new Recommendation: Add unit tests covering:
2. Potential Null Pointer ExceptionLocation: for (final ChartMarker marker in markerGroup.markers) {
final Offset center = points[marker.markerType!]!; // ⚠️ Double bang operatorIssue: Using Recommendation: final Offset? center = points[marker.markerType];
if (center == null) continue;
// or provide a fallback calculation3. Inconsistent Null Safety PatternLocation: final Offset center = points[marker.markerType!] ??
(marker.markerType == MarkerType.contractMarker
? Offset(...)
: Offset(epochToX(marker.epoch), quoteToY(marker.quote)));Issue: This code recalculates the center offset for markers not in the Recommendation: Extract the offset calculation into a helper function or ensure markers are always in the map before iteration. 🟡 Medium Priority Issues4. Incomplete DocumentationLocation: /// The [hasReducedOpacity] determines whether the marker should be rendered with reduced (0.5) opacity.Issue: Documentation mentions 0.5 opacity, but the actual implementation uses Recommendation: Update documentation to clarify: "reduces the marker's opacity by 50% (multiplies by 0.5)". 5. Magic NumbersLocation: Multiple files Examples:
Issue: Hardcoded values make it difficult to maintain consistency and adjust styling. Recommendation: Extract magic numbers to named constants or design tokens, especially values that appear in multiple places. 6. Code Duplication in Marker Line PaintersLocation: Issue: All three files have nearly identical logic for determining marker color: final Color markerColor = marker.color ??
(marker.direction == MarkerDirection.up
? theme.markerStyle.upColorProminent
: theme.markerStyle.downColorProminent);Recommendation: Extract to a shared utility function: 7. Inconsistent Text Rendering ApproachLocation: Issue: Recommendation: static void paintBottomText(Canvas canvas, Size size, double x, String text, double zoom, Color color) {
final TextPainter textPainter = TextPainter(
text: TextSpan(
text: text,
style: TextStyle(color: color, fontSize: 14 * zoom, fontWeight: FontWeight.w400),
),
textDirection: TextDirection.ltr,
)..layout();
paintWithTextPainter(
canvas,
painter: textPainter,
anchor: Offset(x, size.height - 24 * zoom / 2),
anchorAlignment: Alignment.center,
);
}🟢 Low Priority / Style Issues8. Unused ParameterLocation: /// The [marker] is the chart marker (kept for signature parity with other line painters).Issue: The Recommendation: Update documentation to reflect actual usage. 9. Export Missing from Main LibraryLocation: Issue: The new Recommendation: Verify if this should be exported for external use, or document that it's internal-only. 10. PR Description MissingLocation: PR description Issue: The PR body is almost entirely empty. No description of what checkpoint markers are, how they work, or what problem they solve. Recommendation: Add:
11. Formatting ConsistencyLocation: Issue: The Recommendation: Run ✅ Positive Observations
RecommendationsBefore Merging:
Post-Merge (Technical Debt):
Security & Performance✅ Security: No security concerns identified Let me know if you'd like me to implement any of these fixes! |
Reviewer's GuideImplements checkpoint-based trade type markers for multi-stage contracts by introducing new marker types and painters, updating tick/digit marker painters to support opacity and checkpoint rendering, extending time marker utilities for bottom text labels, and wiring an example showcasing checkpoint lines and exit spots. Sequence diagram for rendering checkpoint line markerssequenceDiagram
actor User
participant FullscreenChartState
participant Chart as DerivChart
participant TickMarkerIconPainter
participant paintCheckpointLine
participant TimeMarkerPainters
User->>FullscreenChartState: selectRiseFallTradeType()
FullscreenChartState->>FullscreenChartState: create ChartMarker entrySpot
FullscreenChartState->>FullscreenChartState: create ChartMarker checkpointLine(text=1)
FullscreenChartState->>FullscreenChartState: create ChartMarker checkpointLineCollapsed
FullscreenChartState->>FullscreenChartState: create ChartMarker exitSpot
FullscreenChartState->>FullscreenChartState: create ChartMarker exitTime(text=2)
FullscreenChartState->>Chart: updateMarkers(List<ChartMarker>)
Chart->>TickMarkerIconPainter: paintMarkerGroup(canvas,size,theme,markerGroup,...)
TickMarkerIconPainter->>TickMarkerIconPainter: computeOpacity(marker.hasReducedOpacity)
TickMarkerIconPainter->>TickMarkerIconPainter: _drawMarker(...,markerType=checkpointLine,...)
TickMarkerIconPainter->>paintCheckpointLine: paintCheckpointLine(canvas,size,marker,anchor,style,theme,zoom,opacity,props)
paintCheckpointLine->>TimeMarkerPainters: paintVerticalTimeLine(canvas,size,x,color,dashed=true,...)
TimeMarkerPainters-->>paintCheckpointLine: vertical dashed line drawn
alt marker.text not empty
paintCheckpointLine->>TimeMarkerPainters: paintBottomText(canvas,size,x,text,zoom,color)
TimeMarkerPainters-->>paintCheckpointLine: bottom text drawn
end
paintCheckpointLine-->>TickMarkerIconPainter: checkpoint line rendered
TickMarkerIconPainter-->>Chart: marker group rendered
Chart-->>User: chart displays checkpoint-based markers
Class diagram for checkpoint-based trade type markersclassDiagram
class Marker {
<<abstract>>
int epoch
double quote
MarkerDirection direction
void onTap()
}
class ChartMarker {
int epoch
double quote
MarkerDirection direction
MarkerType markerType
String text
Color color
bool hasReducedOpacity
}
Marker <|-- ChartMarker
class MarkerType {
<<enumeration>>
startTime
start
entry
entrySpot
exitSpot
exit
end
exitTime
startTimeCollapsed
exitTimeCollapsed
profitAndLossLabel
profitAndLossLabelFixed
checkpointLine
checkpointLineCollapsed
}
class MarkerGroupIconPainter {
<<abstract>>
+void paintMarkerGroup(Canvas canvas,Size size,ChartTheme theme,MarkerGroup markerGroup,double Function(int epoch) epochToX,double Function(double quote) quoteToY,MarkerPainterProps painterProps,AnimationInfo animationInfo)
}
class TickMarkerIconPainter {
+void paintMarkerGroup(Canvas canvas,Size size,ChartTheme theme,MarkerGroup markerGroup,double Function(int epoch) epochToX,double Function(double quote) quoteToY,MarkerPainterProps painterProps,AnimationInfo animationInfo)
-void _drawMarker(Canvas canvas,Size size,ChartTheme theme,ChartMarker marker,Offset anchor,MarkerStyle style,double zoom,int granularity,double opacity,Paint paint,AnimationInfo animationInfo,String markerGroupId,MarkerGroup markerGroup)
-void _drawEntrySpot(Canvas canvas,ChartMarker marker,Offset anchor,MarkerStyle style,ChartTheme theme,double zoom,double opacity)
-void _drawExitSpot(Canvas canvas,ChartMarker marker,Offset anchor,MarkerStyle style,ChartTheme theme,double zoom,double opacity)
}
class DigitMarkerIconPainter {
+int pipSize
+DigitMarkerIconPainter(int pipSize)
+void paintMarkerGroup(Canvas canvas,Size size,ChartTheme theme,MarkerGroup markerGroup,double Function(int epoch) epochToX,double Function(double quote) quoteToY,MarkerPainterProps painterProps,AnimationInfo animationInfo)
-void _drawMarker(Canvas canvas,Size size,ChartTheme theme,Marker marker,Offset anchor,MarkerStyle style,double zoom,double opacity,MarkerProps props)
}
MarkerGroupIconPainter <|-- TickMarkerIconPainter
MarkerGroupIconPainter <|-- DigitMarkerIconPainter
class TimeMarkerPainters {
<<static>>
+void paintVerticalTimeLine(Canvas canvas,Size size,double x,Color color,bool dashed,double strokeWidth,double dashWidth,double dashSpace)
+void paintBottomIcon(Canvas canvas,Size size,double x,IconData icon,double zoom,Color color)
+void paintBottomText(Canvas canvas,Size size,double x,String text,double zoom,Color color)
}
class paintStartLine {
+void paintStartLine(Canvas canvas,Size size,ChartMarker marker,Offset anchor,MarkerStyle style,ChartTheme theme,double zoom,double opacity,MarkerProps props)
}
class paintEndLine {
+void paintEndLine(Canvas canvas,Size size,ChartMarker marker,Offset anchor,MarkerStyle style,ChartTheme theme,double zoom,double opacity,MarkerProps props)
}
class paintCheckpointLine {
+void paintCheckpointLine(Canvas canvas,Size size,ChartMarker marker,Offset anchor,MarkerStyle style,ChartTheme theme,double zoom,double opacity,MarkerProps props)
}
TickMarkerIconPainter ..> ChartMarker
TickMarkerIconPainter ..> MarkerType
TickMarkerIconPainter ..> paintStartLine
TickMarkerIconPainter ..> paintEndLine
TickMarkerIconPainter ..> paintCheckpointLine
DigitMarkerIconPainter ..> paintStartLine
paintStartLine ..> TimeMarkerPainters
paintEndLine ..> TimeMarkerPainters
paintCheckpointLine ..> TimeMarkerPainters
class FullscreenChartState {
-TradeType _currentTradeType
-int _markerDurationMs
-void _addCheckpointExampleMarkers()
}
FullscreenChartState ..> ChartMarker
FullscreenChartState ..> MarkerType
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
TickMarkerIconPainter.paintMarkerGroupyou now pass a hard-coded1.2zoom into_drawMarkerinstead ofpainterProps.zoom; consider either using the real zoom value or explaining why markers should be scaled independently so marker sizing stays consistent with chart zoom. - The new
ChartMarker.hasReducedOpacityfield is documented as applying to vertical line markers, but the opacity reduction is applied to all markers inTickMarkerIconPainter.paintMarkerGroup; consider either restricting its use to the intended marker types or updating the documentation to reflect the actual behavior. - In
paintEndLineyou now always passdashed: truetopaintVerticalTimeLineand no longer useprops.isRunning, which changes finished contracts from solid to dashed end lines; if that visual distinction is still desired, consider keeping the dashed/solid decision based onprops.isRunning.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `TickMarkerIconPainter.paintMarkerGroup` you now pass a hard-coded `1.2` zoom into `_drawMarker` instead of `painterProps.zoom`; consider either using the real zoom value or explaining why markers should be scaled independently so marker sizing stays consistent with chart zoom.
- The new `ChartMarker.hasReducedOpacity` field is documented as applying to vertical line markers, but the opacity reduction is applied to all markers in `TickMarkerIconPainter.paintMarkerGroup`; consider either restricting its use to the intended marker types or updating the documentation to reflect the actual behavior.
- In `paintEndLine` you now always pass `dashed: true` to `paintVerticalTimeLine` and no longer use `props.isRunning`, which changes finished contracts from solid to dashed end lines; if that visual distinction is still desired, consider keeping the dashed/solid decision based on `props.isRunning`.
## Individual Comments
### Comment 1
<location> `lib/src/deriv_chart/chart/data_visualization/markers/chart_marker.dart:217-223` </location>
<code_context>
/// If null, uses [style.upColor] for [MarkerDirection.up] or [style.downColor] for [MarkerDirection.down].
final Color? color;
+
+ /// Whether vertical line markers should be rendered with reduced (0.5) opacity.
+ ///
+ /// When set to true, marker will be rendered at half opacity. This is useful for indicating markers that represent
+ /// future or unreached checkpoints in multi-stage contracts.
+ ///
+ /// Defaults to false (full opacity).
+ final bool hasReducedOpacity;
}
</code_context>
<issue_to_address>
**suggestion:** The hasReducedOpacity field is documented as vertical-line-specific but is applied to all markers.
`hasReducedOpacity` currently adjusts `markerOpacity` for all marker types in `paintMarkerGroup`, but its docstring describes it as vertical-line-specific. Please either generalize the wording to match the current behavior, or constrain its usage so only vertical line markers use the reduced opacity, so the flag’s intent is unambiguous.
```suggestion
/// Whether this marker should be rendered with reduced (0.5) opacity.
///
/// When set to true, the marker will be rendered at half opacity. This is useful for indicating markers that represent
/// future or unreached checkpoints in multi-stage contracts, but can be used for any marker type.
///
/// Defaults to false (full opacity).
final bool hasReducedOpacity;
```
</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/chart/data_visualization/markers/chart_marker.dart
Outdated
Show resolved
Hide resolved
|
Claude finished @behnam-deriv's task —— View job Code Review: Checkpoint-Based Trade Type MarkersI've completed a comprehensive review of this PR. The implementation adds checkpoint marker support for multi-stage contracts with solid architecture, but there are several critical issues that must be addressed before merging. SummaryThis PR introduces checkpoint-based markers for multi-stage contracts (e.g., Double Rise/Fall), adding two new marker types ( Overall Assessment: 🔴 Critical Issues (Must Fix)1. Null Pointer Exception Risk in
|
|
@claude Null Pointer Exception is not applicable to changes on this PR. |
1 participant
Clickup link:
Fixes issue: #
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
Introduce checkpoint line markers and improved time marker rendering for multi-stage contracts, along with minor demo configuration updates.
New Features:
Enhancements: