feat: add counter text type for tick contract markers#472
feat: add counter text type for tick contract markers#472behnam-deriv merged 2 commits intomasterfrom
Conversation
Introduce `MarkerTextType` enum (`plain` / `counter`) on `ChartMarker`
to control how the `text` field is rendered on the dashed connector line.
- Add `makeDelimitedTextPainter` helper that splits text on a delimiter
and applies distinct styles to each part (e.g. "2" vs "/10")
- Render text inline on the dashed line in `TickMarkerIconPainter`,
splitting the line around the label when text is present
- Add `markerTextColor` to `MarkerStyle` and both default themes
Reviewer's GuideImplements inline text rendering (including a new counter style) on the tick contract connector line by extending marker metadata, adding a delimited text painter helper, and wiring theme colors for marker text; existing markers without text remain visually unchanged. Sequence diagram for painting tick connector line with optional counter textsequenceDiagram
participant TickMarkerIconPainter
participant MarkerGroup
participant ChartMarker
participant ChartTheme
participant PaintTextHelpers
participant Canvas
TickMarkerIconPainter->>MarkerGroup: markers
TickMarkerIconPainter->>MarkerGroup: firstWhereOrNull(markerType == contractMarker)
MarkerGroup-->>TickMarkerIconPainter: ChartMarker contractMarker
TickMarkerIconPainter->>ChartMarker: get text, textType
TickMarkerIconPainter->>ChartTheme: get markerStyle.markerTextColor
alt contractMarker has non empty text
TickMarkerIconPainter->>TickMarkerIconPainter: _paintDashedLineWithText(canvas, offsets, color, text, textType, theme, opacity)
alt textType == counter
TickMarkerIconPainter->>PaintTextHelpers: makeDelimitedTextPainter(text, '/', primaryStyle, secondaryStyle)
PaintTextHelpers-->>TickMarkerIconPainter: TextPainter
else textType == plain
TickMarkerIconPainter->>PaintTextHelpers: makeTextPainter(text, style)
PaintTextHelpers-->>TickMarkerIconPainter: TextPainter
end
TickMarkerIconPainter->>Canvas: paintHorizontalDashedLine(left segment)
TickMarkerIconPainter->>Canvas: paintHorizontalDashedLine(right segment)
TickMarkerIconPainter->>PaintTextHelpers: paintWithTextPainter(canvas, textPainter, anchor)
else no text
TickMarkerIconPainter->>Canvas: paintHorizontalDashedLine(full line)
end
Class diagram for updated marker text styling and themingclassDiagram
class MarkerType {
}
class MarkerTextType {
<<enumeration>>
plain
counter
}
class Marker {
}
class ChartMarker {
String~nullable~ text
MarkerType~nullable~ markerType
MarkerTextType textType
Color~nullable~ color
bool hasReducedOpacity
Offset displayOffset
}
class MarkerStyle {
Color lineProfitColor
Color lineLossColor
Color lineDefaultColor
Color markerTextColor
double radius
TextStyle activeMarkerText
}
class LightThemeColors {
Color upColorProminent
Color downColorProminent
Color markerTextColor
}
class DarkThemeColors {
Color upColorProminent
Color downColorProminent
Color markerTextColor
}
class ChartDefaultTheme {
}
class ChartDefaultLightTheme {
}
class ChartDefaultDarkTheme {
}
Marker <|-- ChartMarker
ChartMarker --> MarkerType
ChartMarker --> MarkerTextType
ChartDefaultTheme <|-- ChartDefaultLightTheme
ChartDefaultTheme <|-- ChartDefaultDarkTheme
ChartDefaultLightTheme --> MarkerStyle : creates
ChartDefaultDarkTheme --> MarkerStyle : creates
LightThemeColors --> MarkerStyle : provides markerTextColor
DarkThemeColors --> MarkerStyle : provides markerTextColor
MarkerStyle --> Marker : styling for markers
MarkerStyle --> ChartMarker : styling for chart markers
MarkerTextType ..> MarkerStyle : controls text styling
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. OpenSSF Scorecard
Scanned Manifest Files |
![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
_paintDashedLineWithText, the logic assumeslineStart.dx < lineEnd.dxand specific left-to-right geometry; it may be worth explicitly constraining or asserting this (or handling the reversed case) to avoid subtle layout issues if marker positions ever invert. - The plain/counter text styles in
_paintDashedLineWithTextare hard-coded (font sizes 20/12, weights, etc.); consider moving these into theme/MarkerStyle so they can be tuned consistently with the rest of the chart typography. - The non-text path now calls
paintHorizontalDashedLinewith the start and end X coordinates reversed from the previous implementation; double-check whetherpaintHorizontalDashedLineexpects a specific ordering and, if so, keep the call sites consistent to avoid unexpected dash rendering.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_paintDashedLineWithText`, the logic assumes `lineStart.dx < lineEnd.dx` and specific left-to-right geometry; it may be worth explicitly constraining or asserting this (or handling the reversed case) to avoid subtle layout issues if marker positions ever invert.
- The plain/counter text styles in `_paintDashedLineWithText` are hard-coded (font sizes 20/12, weights, etc.); consider moving these into theme/MarkerStyle so they can be tuned consistently with the rest of the chart typography.
- The non-text path now calls `paintHorizontalDashedLine` with the start and end X coordinates reversed from the previous implementation; double-check whether `paintHorizontalDashedLine` expects a specific ordering and, if so, keep the call sites consistent to avoid unexpected dash rendering.
## Individual Comments
### Comment 1
<location path="lib/src/deriv_chart/chart/data_visualization/markers/marker_icon_painters/tick_marker_icon_painter.dart" line_range="338-349" />
<code_context>
+ ),
+ );
+
+ final double textRight = lineEnd.dx - _textPaddingFromStartLine;
+ final double textLeft = textRight - textPainter.width;
+
+ if (lineStart.dx < textLeft - _textGap) {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Text layout does not guard against insufficient horizontal space between markers.
With narrow space between `lineStart` and `lineEnd`, `textPainter.width` can exceed the available width so that `textLeft` < `lineStart.dx`, causing the text to overlap the marker or bleed outside the segment. The current guards (`lineStart.dx < textLeft - _textGap` / `lineStart.dx < lineEnd.dx`) don’t prevent this. Consider checking whether `textPainter.width + 2 * (_textGap + _textPaddingFromStartLine)` exceeds `lineEnd.dx - lineStart.dx` and then either skip the text and draw only the dashed line, reduce the font size, or clamp the text so it never crosses `lineStart.dx`.
```suggestion
: makeTextPainter(
text,
TextStyle(
color: textColor,
fontSize: 12,
fontWeight: FontWeight.w700,
height: 1,
),
);
// Guard against insufficient horizontal space between markers for the label.
final double availableWidth = lineEnd.dx - lineStart.dx;
final double requiredWidth =
textPainter.width + 2 * (_textGap + _textPaddingFromStartLine);
if (requiredWidth > availableWidth && lineStart.dx < lineEnd.dx) {
// Not enough space to render the text without overlapping markers
// or bleeding outside the segment. Draw only the dashed line.
paintHorizontalDashedLine(
canvas,
lineEnd.dx,
lineStart.dx,
lineStart.dy,
lineColor,
1,
dashWidth: 2,
dashSpace: 2,
);
return;
}
final double textRight = lineEnd.dx - _textPaddingFromStartLine;
final double textLeft = textRight - textPainter.width;
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
...iv_chart/chart/data_visualization/markers/marker_icon_painters/tick_marker_icon_painter.dart
Show resolved
Hide resolved
|
Claude finished @behnam-deriv's task —— View job Code Review CompleteI've completed my review of PR #472. Here's my comprehensive feedback: Priority IssuesHIGH Priority: Text Layout Overflow Guard Missing 🔴Location: Issue: The current implementation doesn't guard against insufficient horizontal space between the contract marker and start time marker. When the distance is too small for the text width plus padding/gaps, the text can:
Current Logic: final double textRight = lineEnd.dx - _textPaddingFromStartLine;
final double textLeft = textRight - textPainter.width;
if (lineStart.dx < textLeft - _textGap) {
// Draw left line segment
}The check Recommended Fix: // Add guard at the beginning of _paintDashedLineWithText
final double availableWidth = lineEnd.dx - lineStart.dx;
final double requiredWidth = textPainter.width + 2 * (_textGap + _textPaddingFromStartLine);
if (requiredWidth > availableWidth) {
// Not enough space - fall back to plain dashed line
paintHorizontalDashedLine(
canvas,
lineEnd.dx,
lineStart.dx,
lineStart.dy,
lineColor,
1,
dashWidth: 2,
dashSpace: 2,
);
return;
}This matches the suggestion from Sourcery's review. MEDIUM Priority: Hard-Coded Text Styles 🟡Location: Issue: The text styles for both
Problems:
Recommended Fix: /// Text style for counter primary (current value) portion
final TextStyle markerCounterPrimaryTextStyle;
/// Text style for counter secondary (total value) portion
final TextStyle markerCounterSecondaryTextStyle;
/// Text style for plain marker text
final TextStyle markerPlainTextStyle;Then use these in the painter instead of inline TextStyle definitions. MEDIUM Priority: Coordinate Ordering Assumption 🟡Location: Issue: The method assumes Potential Problem:
Recommended Fix: void _paintDashedLineWithText(...) {
assert(lineStart.dx < lineEnd.dx,
'Contract marker must be positioned left of start time marker');
// ... rest of implementation
}Or handle both cases if this inversion is valid. Code Quality IssuesLOW Priority: Missing Documentation for Constants 🟢Location: The constants const double _textPaddingFromStartLine = 16;
const double _textGap = 4;Recommendation: /// Distance (in pixels) from the start time marker to the right edge of the text label.
const double _textPaddingFromStartLine = 16;
/// Gap (in pixels) between the text label and adjacent dashed line segments.
const double _textGap = 4;LOW Priority: Enhanced Enum Documentation 🟢Location: The Current: /// The text must contain a `/` separator.Suggested: /// The text must contain a `/` separator. If the delimiter is not found
/// or appears at index 0, rendering falls back to `plain` style.
/// Example: "2/10" renders as "**2**/10" (bold/large + regular/small).This matches the actual fallback behavior in Test CoverageMEDIUM Priority: No Tests for New Functionality 🟡Finding: Grep search for Missing Test Coverage:
Recommendation: Add test file Positive Observations ✅
Summary
Overall Assessment: The implementation is functionally sound but has important edge case handling gaps that should be addressed before merging. The missing overflow guard is the most critical issue as it can cause visual artifacts in production. The hard-coded text styles violate the established theming architecture and should be refactored for consistency. Recommendation: Request changes to address the HIGH priority overflow guard and consider addressing MEDIUM priority issues for better code quality and maintainability. |
|
The overlaping of contractMarker with marker text is actually the expected behaviour , hence the high priority issue comment from claude is not a valid in this case. |
|
Claude finished @behnam-deriv's task —— View job Todo
Updated Review Based on ClarificationThank you for the clarification! I understand now that the text overlapping with the contract marker is intentional design behavior for this feature. I've updated my review accordingly. Revised Priority IssuesHIGH Priority: Hard-Coded Text Styles Should Use Theme 🔴Location: Issue: The implementation references theme properties that don't exist yet:
Looking at the actual code in
Impact: This code will fail at runtime with "The getter 'markerCounterPrimaryTextStyle' isn't defined for the type 'MarkerStyle'". Recommended Fix: /// Text style for counter primary (current value) portion - e.g., "2" in "2/10"
final TextStyle markerCounterPrimaryTextStyle;
/// Text style for counter secondary (total value) portion - e.g., "/10" in "2/10"
final TextStyle markerCounterSecondaryTextStyle;
/// Text style for plain marker text
final TextStyle markerPlainTextStyle;And initialize them in both // In MarkerStyle constructor in both theme files:
markerCounterPrimaryTextStyle: const TextStyle(
fontSize: 20,
fontWeight: FontWeight.w700,
height: 1,
),
markerCounterSecondaryTextStyle: const TextStyle(
fontSize: 12,
fontWeight: FontWeight.w400,
height: 1,
),
markerPlainTextStyle: const TextStyle(
fontSize: 12,
fontWeight: FontWeight.w700,
height: 1,
),MEDIUM Priority: Missing Test Coverage 🟡Finding: No tests exist for the new functionality. Missing Test Coverage:
Recommendation: Add test file LOW Priority: Coordinate Ordering Assumption 🟢Location: Issue: The method assumes Current Logic:
Recommendation: Add assertion for clarity: void _paintDashedLineWithText(...) {
assert(lineStart.dx < lineEnd.dx,
'Contract marker must be positioned left of start time marker');
// ... rest of implementation
}This makes the coordinate ordering assumption explicit and helps catch unexpected usage during development. Code Quality Observations✅ Positive: Documentation ImprovementsThe constants now have excellent documentation (lines 317-321): // Distance (in pixels) from the start time marker to the right edge of the text label.
const double _textPaddingFromStartLine = 16;
// Gap (in pixels) between the text label and adjacent dashed line segments.
const double _textGap = 4;✅ Positive: Clean ArchitectureThe separation between model ( ✅ Positive: Backward CompatibilityThe default ✅ Positive: Theme IntegrationThe addition of Revised Summary
Overall Assessment: The implementation has a critical issue - the code references theme properties that don't exist in Recommendation:
Note: I've removed the previous "overflow guard" issue from my review since you've confirmed that text overlapping the contract marker is the intended visual design. The implementation correctly positions the text near the contract marker as designed. |
1 participant
Summary
This PR introduces a
MarkerTextTypeenum and the supporting rendering logic to display styled text inline on the dashed connector line of a contract marker; specifically to show a live tick counter (e.g. 2/10) for tick-duration contracts.Changes
ChartMarker/MarkerTypeAdded
MarkerTextTypeenum with two variants:plain— renders the text in a uniform style (default, backward-compatible)counter— splits the text on/and renders the left part prominently (larger, bold) and the right part (including/) smaller, to convey a "current / total" progressAdded
textTypefield toChartMarker(defaults toMarkerTextType.plain)TickMarkerIconPaintercontractMarkerhas non-emptytext, the dashed connector line is now split around an inline text label instead of drawing a plain line_paintDashedLineWithTextmethod handles layout: it positions the text near the start-time marker, then draws the dashed line in two segments on either sidepaint_text.dartmakeDelimitedTextPainter. a helper that splits a string on a given delimiter and builds aTextPainterwith aTextSpanapplying different styles to each partTheme
markerTextColortoMarkerStyleand wired it up in bothChartDefaultDarkThemeandChartDefaultLightThemeusing the appropriate design tokensHow it works
The painter splits
"2/10"at/, renders"2"at 20 px / bold and"/10"at 12 px / regular, then draws the dashed line on both sides of the label along the connector.For markers without text (or with
textType: plain) the existing rendering path is unchanged.Summary by Sourcery
Add support for styled inline text on contract marker connector lines, enabling counter-style tick labels and theming for marker text.
New Features:
Enhancements: