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

Fix chart only drawing first entry #4829

Merged
merged 3 commits into from
Aug 29, 2022
Merged

Fix chart only drawing first entry #4829

merged 3 commits into from
Aug 29, 2022

Conversation

FelixHerrmann
Copy link
Contributor

Issue Link πŸ”—

#4819 #4792 #4753 #4739 #4722 #4699 (and possibly a lot more)

Goals ⚽

Fixing line chart only drawing first entry when spaceMax on xAxis is not 0.
This will fix the candle stick chart only drawing first value as well!

Implementation Details 🚧

With the addition of the swift-algorithms package in v4.0.0 many search algorithms have changed, including the one in ChartDataSet.entryIndex(x:closestToY:rounding:).

It turns out that partitioningIndex works different in one situation: when the given value is greater than the greatest value in the array.
Screen Shot 2022-05-26 at 23 39 32

Because of that, we need some kind of check that ensures we don't get an index out of bounds.
#4577 #4721 and the initial implementation #4497 do those checks, but wrong. The initial implementation is the best, the index is just one too high as shown in the screenshot above. We simply need to return the last possible index if a higher value is requested. Returning -1 in this situation will result in only drawing the first value!

In addition to that, I replaced all the raw index manipulation with the standard lib APIs and fixed Package.resolved to reflect the versions from the Package.swift!

Testing Details πŸ”

I tested in the demo projects and my personal one.

Before Fix After Fix
Simulator Screen Shot - iPhone 13 Pro - 2022-05-26 at 22 24 17 Simulator Screen Shot - iPhone 13 Pro - 2022-05-26 at 22 25 51
Simulator Screen Shot - iPhone 13 Pro - 2022-05-26 at 22 24 52 Simulator Screen Shot - iPhone 13 Pro - 2022-05-26 at 22 25 32

@TKZwo
Copy link

TKZwo commented Jun 21, 2022

I've tested our apps' graphs with the results from this PR and I can confirm this solves our problem. Good job! I'll pull the updated version in our app as soon as it get available in a release.

@artalejo
Copy link

artalejo commented Jun 27, 2022

It would be highly appreciated if this fix can make it to the next release.

@bzeeman
Copy link

bzeeman commented Aug 1, 2022

Any thoughts on when this fix might be merged?

@FelixHerrmann
Copy link
Contributor Author

Maybe @pmairoldi can tell us when we can expect this fix to be merged?

@morluna
Copy link

morluna commented Aug 25, 2022

Any word on when this can move forward @pmairoldi ? We ran into this same issue and can confirm this change fixes it.

@cbalsalobre
Copy link

Hi @FelixHerrmann this totally solves my problem. In my case I had a lineChart combined with two other lineCharts configured to not show the lines, but just relevant points (mins a maxs detected with my algorithm)
IMG_2190
IMG_2189

So yes that would be great to have this merged with master. Thanks!

@FelixHerrmann
Copy link
Contributor Author

Awesome @cbalsalobre, thanks for confirming!

@FelixHerrmann
Copy link
Contributor Author

A note for the Collaborators: #4839 and #4844 try to fix the same problem but they don't resolve the root cause of the issue

@417-72KI
Copy link
Contributor

We are waiting for merging this for a long time.

Copy link

@TKZwo TKZwo left a comment

Choose a reason for hiding this comment

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

This fix should be in before anything else.

@TKZwo
Copy link

TKZwo commented Aug 29, 2022

@fumoboy007 I think if you edit your comment to 'approve' the 'open discussion' will be closed and this PR will be pulled in.

@pmairoldi pmairoldi merged commit 2c77b2b into ChartsOrg:master Aug 29, 2022
@pmairoldi
Copy link
Collaborator

Thanks for the contribution. Will be out with the next version for Xcode 14

@santieduardo
Copy link

Hello @pmairoldi

Do you know when the next release of this lib will be available?

Thanks

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.

10 participants