Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Highlighter fix #4721

Merged
merged 5 commits into from May 25, 2022
Merged

Highlighter fix #4721

merged 5 commits into from May 25, 2022

Conversation

kcome
Copy link
Contributor

@kcome kcome commented Sep 23, 2021

Fixed the use of partitioningIndex(where:) in ChartDataSet.entriesForXValue
Fixed the rounding issue of ChartDataSet.entryIndex()

Issue Link πŸ”—

#4719

Goals ⚽

Having a working highlighter which is broken due to incorrect use of Algorithms partitioningIndex()

Implementation Details 🚧

Partition the entries first, then call partitioningIndex()

Testing Details πŸ”

Using the attached ChartDemo-iOS-Swift project

Open LineChart1 or LineChart2, tap data points, highlight should work

…XValue

Fixed the rounding issue of ChartDataSet.entryIndex()
@krzysztof-grobelny
Copy link

@kcome Hello! Would you mind sharing some explanation on that fix? I've noticed the defect shows up only after building Charts with Xcode 13, no?

@kcome
Copy link
Contributor Author

kcome commented Oct 10, 2021

@krzysztof-grobelny The issue #4719 happens on both Xcode 13 and 12.

There are two fixes together solves #4719 :

1 is in class ChartDataSet, function open override func entryIndex(x xValue: Double, closestToY yValue: Double, rounding: ChartDataSetRounding) -> Int. When using "closest" rounding method, there was no checking present. E.g. in data-set [(1,1), (2,2), (3,3)], when trying to find closest point to xValue 2.1 with closest rounding, previous solution gives (3, 3) but actually the closes point is (2,2).

2 is in open override func entriesForXValue(_ xValue: Double) -> [ChartDataEntry]. According to swift-algorithm document:

The partitioningIndex(where:) method returns the index of the start of the second partition when called on an already partitioned collection.

Previous implementation didn't call .partition(by: match) so the result of .partitioningIndex(where: match) is not what we want.

@mt-bsingh
Copy link

@kcome Thanks for the fix, @krzysztof-grobelny This issue happens on XCode12, In the demo code for ChartDemo-iOS-Swift. Some bars are selectable sometimes but definitely not all. This fix looks right to me. Same time I found out that's the problem.

@kcome
Copy link
Contributor Author

kcome commented Feb 9, 2022

@liuxuan30 could we merge this PR?

ghost pushed a commit to theshiftstudio/Charts that referenced this pull request Mar 7, 2022
The fix is provided by the community and applied based to the following pull request:
ChartsOrg#4721
ghost pushed a commit to theshiftstudio/Charts that referenced this pull request Mar 7, 2022
The fix is provided by the community and applied based on the following pull request:
ChartsOrg#4721
@kcome
Copy link
Contributor Author

kcome commented Mar 20, 2022

Hi @pmairoldi , I resolved a conflict in this PR. Could we start merge? Let me know if there is anything else need to be done.

@dudenamedjune
Copy link

Think I have a proper fix open for this #4817
#4719 (comment)

@417-72KI
Copy link
Contributor

I tried with #4817 and seems to work chartValueSelected(_:entry:highlight:),
but chartValueNothingSelected(_:) still not worked.

This PR works both and seems to be expected specs ☺️

Copy link
Collaborator

@pmairoldi pmairoldi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Seems to work fine.

@@ -197,9 +198,11 @@ open class ChartDataSet: ChartBaseDataSet
open override func entriesForXValue(_ xValue: Double) -> [ChartDataEntry]
{
let match: (ChartDataEntry) -> Bool = { $0.x == xValue }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this line should be:

let match: (ChartDataEntry) -> Bool = { $0.x >= xValue }

Note the >= instead of ==.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually nevermind, sorry for bothering you! Please disregard this and other comments - I'll delete them.

My thinking was that partitioningIndex function requires that all values after the desired index match the expression, but after looking more carefully I see the added partition function before running partitioningIndex - I'm guessing the intention is to make it possible to search in non-ordered data. I'm a little worried about the performance here, but otherwise I think the implementation is correct, except one detail: comparing Doubles directly. This is probably bigger discussion about the library design, but basically due to Double representation in memory, we should never compare Doubles using the equality operator. This will break for example in a simple case: 0.3 != 0.1 + 0.2. But as I said, that's probably a separate discussion.

@wlxo0401
Copy link
Contributor

chartValueNothingSelected not work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet