Add option to include y distance for nearest touched spot

[joeldomke]

Instead of only considering the distance on the x axis when checking for the closest spot, this feature allows to choose the closest spot based on Euclidean distance

TODO:

• Write tests

Fixes #716, #261

[imaNNeo]

#892

[imaNNeo]

I will check it soon.

[joeldomke]

Just to be clear, this PR and #892 to are independent of each other, although it might make sense to use them together in many cases.

[imaNNeo]

@joeldomke I think it will fix #892 too.
But it doesn't solve #716. We need to allow users to touch the line itself to fix that (at the moment we only allow them to touch the spots of a line, not the line itself).

Let me explain my approach:
We don't need to add anything in TouchData about handling the y-axis. (You added includeYDistance).
Instead, It's better to consider the y-axis by default, and in the LineTouchResponse add another LineBarSpot property to hold the only one point that we touched based on Euclidean distance and threshold.

I mean instead of having

LineTouchResponse(List<LineBarSpot> lineBarSpots)

Have something like this:

LineTouchResponse(List<LineBarSpot> allSpotsInY, LineBarSpot nearestTouchedSpot)

Now you can do whatever you want with these two values.
You can access allSpotsInY which contains all spots in the same X value (just like before)
And also you can access nearestTouchedSpot which is the closest point you touched and it's only one point (if you touched two points in a threshold, the closest one will be selected)

Let me know what do you think.

[joeldomke]

Initially I liked your approach since it fixes #892 at the same time. But it actually misses the case mentioned in #892 where two points of two lines have the same x and y coordinate, (which is handled by option 2 and 3 in this ticket).

Adding the distance property mentioned in option 2 and 3 in #892 might also be useful for the case where we only consider the x distance.
Let's consider the following case where we have one line with a data point at x = 5 and another line with a data point at x = 6. Now the user taps on x = 5.2. Both data points would be returned in the LineTouchResponse which will lead to the following selection. I don't think there currently is a way to prevent this.

If you don't want to add anything to TouchData we could do the following:

LineTouchResponse(List<LineBarSpot> lineBarSpots, List<LineBarSpot> lineBarSpotsEuclidian)

were lineBarSpots has the same behavior as before. 'lineBarSpotsEuclidianis the same aslineBarSpotsusing the euclidian distance. And then we also add the distance to theLineBarSpot`.

[imaNNeo]

There is a touchSpotThreshold property in the LineTouchData class.
The default value of touchSpotThreshold is 10, which causes this problem. You can try a lower value like 3 to fix the mentioned problem.

[joeldomke]

Lowering the threshold does not really fix the problem. After lowering it (which I maybe don't want because I want certain user interaction) I could just move the two points close together and would again face the same issue.

[joeldomke]

I switched to a new approach where the user can set a distance function. Also I added the distance to LineBarSpot. I still have to clean up and format the code. (It seems like I have messed something up with the formatting. I don't have the project set up properly on my laptop, and will fix it once I'm home.)

[imaNNeo]

Let me know when you did finish your coding. I will review it all at once.

[imaNNeo]

Any news on this?

[joeldomke]

Should be finished now

[codecov]

Codecov Report

Merging #891 (6c69dd5) into dev (10867bf) will increase coverage by 0.21%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev     #891      +/-   ##
==========================================
+ Coverage   80.65%   80.87%   +0.21%     
==========================================
  Files          32       32              
  Lines        2973     3007      +34     
==========================================
+ Hits         2398     2432      +34     
  Misses        575      575              

Impacted Files	Coverage Δ
lib/src/chart/line_chart/line_chart_painter.dart	80.23% <100.00%> (+0.12%)	⬆️
...src/chart/scatter_chart/scatter_chart_painter.dart	95.90% <0.00%> (+0.44%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 10867bf...6c69dd5. Read the comment docs.

[imaNNeo]

I wrote my last comments. Also, you must rebase this branch with our dev branch. (It seems there are some minor conflicts)

[joeldomke]

I merged dev into my branch, did this solve the conflicts?

[imaNNeo]

No, it doesn't solve the problem.
You need to rebase with the dev branch.
I think you can reset your commits and just make one commit containing your needed changes. (Now it's 11 commits)

[joeldomke]

I squashed them into one and rebased with the dev branch

[imaNNeo]

I know it took a long time.
But there were 3 more minor comments to solve.

Thanks for being patient.

[imaNNeo]

And about the CHANGELOG.md, thank you for adding BREAKING change.
But also we have an IMPROVEMENT.
You made the touch logic customizable!
We need to let users know how they can customize the touch logic.

[imaNNeo]

Any update?
We need to release a version, and I'm waiting for this branch.
Do you have any estimation on this?

[joeldomke]

Yes, it's hopefully done now. And also thank your for your patience. This is my first PR and there were a lot of things I missed at first, but I enjoyed it.

[imaNNeo]

Thank you @joeldomke
Don't worry, that was good!
It has just merged into the dev branch.
I'm going to bump up a version :)

It will be great if you modify (or add) a chart to showcase this feature in your next PR.
I hope to see more from you :))