Prevent creation of invalid range in XBounds (fixes #4184 and #4049)

[berendkleinhaneveld]

Issue Links 🔗

The issues #4184 and #4049 mention crashes which are caused by an invalid range in the XBounds. This fix might not really solve all underlying problems, but at least it will make it stop crashing. Might need some further investigation.

Goals ⚽

This PR should prevent crashes due to invalid ranges in BarLineScatterCandleBubbleRenderer.XBounds (max > min). An negative range will cause a crash when the iterator is called upon.
I traced the problem to the set(chart, dataset, animator) function in BarLineScatterCandleBubbleRenderer.XBounds. If entryFrom is not nil but entryTo is, and there are entries outside/below the visible range (or when the entries are not sorted), then the entryIndex(entry:) function will/can return an index larger than zero. This results in min becoming larger than zero while max will default to 0. This creates an invalid range.

Implementation Details 🚧

I've made sure that when the entryFrom value is nil while the entryTo values is non-nil, that the min value will be the max value (instead of the default 0). And when entryFrom is non-nil but entryTo is nil, then the max value will be set to the min value to prevent a negative/invalid range.

Testing Details 🔍

The test uses a ScatterChartView with a custom axisMinimum and axisMaximum and a simple dataset where two values are below/outside the range. The last value lies exactly on the lower range. This results in a nil value for entryTo and a non-nil value for entryFrom. And this would result in an invalid range with the previous code.

The call stack is as follows: entryForXValue(_:closestToY:rounding:) => entryIndex(x:closestToY:rounding:) => entryIndex(x:closestToY:rounding:) => Collection.partitioningIndex.
The partitioningIndex function relies on the premise that the collection is already partitioned/sorted(!). Would be good to add a notion somewhere in the documentation that it is a good idea to keep datasets sorted, so if anybody has a good idea of where I could put that, I'd be happy to add it in the PR. I don't have a lot of experience with the code base so I don't know exactly the ramifications of the proposed change, but at least it prevents some nasty crashes and I haven't seen any side effects yet in my (limited) testing.

[kscheff]

This may also be a fix for #4046

[jjatie]

Closing in favour of #4596. @berendkleinhaneveld I would appreciate any help in testing the updated implementation. You can find the behaviour (that always worked as far as we know) here.

I'm not convinced we should use StrideThrough.Iterator, but it guarantees the last known working behaviour.

[berendkleinhaneveld]

Cool, I'll check it out!