-
-
Notifications
You must be signed in to change notification settings - Fork 6k
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
notifyDataChanged() will crash for dataset without entries #1945
Comments
Well ah it used to check the count before rendering I remember, but I don't know its has been removed the check. |
added PR #1948 There are checks for returned entry type throughout Charts, so returning nil seemed best here. |
Thanks! Guess I need @danielgindi review it since I am not sure if I would miss anything. BTW, he's missing recently, so it might take a long time.. |
Same problem here, specially in real-time graph. With the following example I will show up other bugs also.
I try to explain my scenario without more code because I can't copy all my code here, it is too long and not useful for your debug. The crash happens when I switch off all the toggles, so no dataset in the chart.data exist, and I switch on one again (then adding again a dataset). The errors I get are the following:
The above code works correcly until I remove all dataset and re-add at least one dataset. I tried to set chart.data = nill and then re-add data with/without dataset, it doesn't work. I didn't read the source library code, but, in my opinion, the error is always the same, if you fix it, you will fix a lot of correlated bug. Thanks! |
@cescobaz have you tried this PR, does it fix your crashes? as much as I wanted to merge this, it will potentially impact many things, so I still need @danielgindi to review this not to missing anything :( |
@liuxuan30 I just try this PR and it doesn't fix the bug. The app logs many times the following error:
Then, after a a few seconds of the above error appears multiple times in console, the app crash with:
The last call I can see before crash is on line 125 of file ../Renderers/AxisRendererBase.swift |
Ok, I found a solution. For some reason _xAxis._axisMinimum and _xAxis._axisMaximum are NaN when you set chart.data = nil or all the datasets are empty or there are no dataset in chart.data.
|
More specifically
Setting CandleChartData with a dataset containing 0 entries and then calling data?.notifyDataChanged() will cause a crash.
I think this happens because it will try to reference entries using subscript index, so probably tries to get entries[0] (because count = 0) and that obviously won't work because that entries doesn't exist.
A workaround is to check if your dataset contains any entries and set it data to nil otherwise.
However, I think Charts could improve here by either using safe subscript referencing (so you can check entries[0] safely without crashing) or just checking if there are actual entries before trying to subscript access them.
The text was updated successfully, but these errors were encountered: