feat: Enhanced line chart curve function#2000
feat: Enhanced line chart curve function#2000huanghui1998hhh wants to merge 12 commits intoimaNNeo:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2000 +/- ##
==========================================
+ Coverage 92.55% 92.97% +0.41%
==========================================
Files 50 51 +1
Lines 3760 3884 +124
==========================================
+ Hits 3480 3611 +131
+ Misses 280 273 -7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
It looks good! |
|
@imaNNeo Done! Please review. |
|
This works a lot better than og algo, any chance of merging it? |
|
@imaNNeo Hi~ Any work on it? |
| } | ||
| } | ||
|
|
||
| static LineChartCurve _resovleCurve( |
| preventCurveOvershootingThreshold: preventCurveOvershootingThreshold, | ||
| ); | ||
|
|
||
| static Offset _flag = Offset.zero; |
There was a problem hiding this comment.
Why do we need to keep a mutable static field inside our const LineChartCubicTensionCurve()?
Is it needed? Can we remove that?
| tinyThresholdSquared: tinyThresholdSquared, | ||
| ); | ||
|
|
||
| static Offset? _flag; |
|
|
||
| static const noCurve = LineChartNoCurve(); | ||
|
|
||
| static LineChartCubicTensionCurve cubicTension({ |
There was a problem hiding this comment.
Can we remove these static functions? Because it's not scalable, for every new type we should add a new function here.
(We can keep the noCurve, but let's not add others).
I see that we use them only for testing. So we can use the constructor LineChartCubicTensionCurve() instead.
| preventCurveOvershootingThreshold: preventCurveOvershootingThreshold, | ||
| ); | ||
|
|
||
| static LineChartCubicMonotoneCurve cubicMonotone({ |
| } | ||
|
|
||
| class LineChartCubicTensionCurve | ||
| extends LineChartCurve<LineChartCubicTensionCurve> with EquatableMixin { |
There was a problem hiding this comment.
I think with EquatableMixin is redundant here (we can remove it)
| return b; | ||
| } | ||
|
|
||
| abstract class LineChartCurve<T extends LineChartCurve<T>> with EquatableMixin { |
There was a problem hiding this comment.
I don't understand why we need to have this Generic type here? <T extends LineChartCurve<T>>
We can have the lerp function like this:
LineChartCurve lerp(covariant LineChartCurve other, double t);And it already works well! (without the need to have a generic type)
| lineChartStepData: lineChartStepData ?? this.lineChartStepData, | ||
| ); | ||
|
|
||
| LineChartCurve? _resolveCopyWithCurve( |
There was a problem hiding this comment.
The logic that we have in this method is a bit complicated. And it seems it silently ignores deprecated params when the current curve is not LineChartCubicTensionCurve.
I think we can have it simpler like this:
LineChartBarData copyWith({
...
}) =>
LineChartBarData(
...
curve: curve ?? _resolveCopyWithCurve(
isCurved,
curveSmoothness,
preventCurveOverShooting,
preventCurveOvershootingThreshold,
),
...
);
LineChartCurve _resolveCopyWithCurve(
bool? isCurved,
double? curveSmoothness,
bool? preventCurveOverShooting,
double? preventCurveOvershootingThreshold,
) {
final thisCurve = curve;
if (isCurved == null) return thisCurve;
if (!isCurved) return LineChartCurve.noCurve;
if (thisCurve is LineChartCubicTensionCurve) {
return LineChartCubicTensionCurve(
smoothness: curveSmoothness ?? thisCurve.smoothness,
preventCurveOverShooting:
preventCurveOverShooting ?? thisCurve.preventCurveOverShooting,
preventCurveOvershootingThreshold:
preventCurveOvershootingThreshold ??
thisCurve.preventCurveOvershootingThreshold,
);
}
return LineChartCubicTensionCurve(
smoothness: curveSmoothness ?? 0.35,
preventCurveOverShooting: preventCurveOverShooting ?? false,
preventCurveOvershootingThreshold: preventCurveOvershootingThreshold ?? 10.0,
);
}(I also double checked it with an LLM, and it seems it covers all the edge cases)
There was a problem hiding this comment.
And it would be very nice if you could write some unit tests that cover the backward compatibility for the constructor and copyWith functions. (For example, construct with deprecated params, and use copyWith using the new implementation, or the other way around)
Thanks!
This PR enhances the curve API:
I’m not entirely sure whether these decisions align with your original vision, please give me some advice. I'll complete the follow-up work.
@imaNNeo