Skip to content

Fix chart only drawing first entry#4829

Merged
pmairoldi merged 3 commits intoChartsOrg:masterfrom
FelixHerrmann:master
Aug 29, 2022
Merged

Fix chart only drawing first entry#4829
pmairoldi merged 3 commits intoChartsOrg:masterfrom
FelixHerrmann:master

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