imaNNeo / fl_chart

FL Chart is a highly customizable Flutter chart library that supports Line Chart, Bar Chart, Pie Chart, Scatter Chart, and Radar Chart.
https://flchart.dev
MIT License
6.77k stars 1.74k forks source link

NullSpot's isNull broken when using copyWith #1607

Closed PvtPuddles closed 5 months ago

PvtPuddles commented 6 months ago

Describe the bug I was experiencing a bug where lines in a FlLineChart would not be drawn if I applied a transformation to the line. Tooltips and touch data were both displaying fine. After a good bit of debugging, we discovered:

When creating a null spot, a user will call FlSpot.nullSpot;.

When applying a transformation to a line (such as scaling an axis), a user would call spots.map((spot) => spot.copyWith(x: spot.x, y: spot.y + 10));. For a nullSpot, this will return an FlSpot(NaN, NaN) that is not equal to FlSpot.nullSpot.

Any line segment that ends in this "fake nullSpot" will not be drawn on the chart (which, in my case, was all of them).

To Reproduce

print(FlSpot.nullSpot == FlSpot.nullSpot.copyWith()); // False
print(FlSpot.nullSpot.copyWith().isNull()); // False

Expected Behavior

Either:

Versions

PvtPuddles commented 6 months ago

For anyone else who encounters this issue, there are two work-arounds:

  1. Before calling .copyWith on a spot, verify the spot is not null.
  2. Transform any data you plan to use before converting into FlSpots.
imaNNeo commented 5 months ago

I didn't understand the scenario that you have, but I fixed the issue of comparing two FlSpot.nullSpot together. It returns true now. I wrote some unit tests for it here: https://github.com/imaNNeo/fl_chart/blob/ef72300af9c81256bd86cc2ede74f552a3036bc3/test/chart/base/axis_chart/axis_chart_data_test.dart#L69-L73

Thanks for reporting it! Please check your scenario again

PvtPuddles commented 5 months ago

Yes, this looks like it solves the issue nicely.

Just to demonstrate what my scenario was, it looked something like this:

double? transformUp(double? yVal) => yVal == null ? null : yVal + 2;

// [(1, 1), (2, 2), nullSpot, (3, 3)]
final trace1 = [FlSpot(1, 1), FlSpot(2, 2), FlSpot.nullSpot, FlSpot(3, 3)];
// [(1, 3), (2, 4), (null, null), (3, 5)]
final trace2 = trace1.map((spot) => spot.copyWith(y: transformUp(spot.y));

I worked around the issue before by checking if the spot is null, though it took a while to figure out what went wrong.

I'm sure this will save someone a headache in the future, so thanks for the fix!