Fix ChartValuesManager-related concurrency issues (WIP)

Co-authored-by: Patryk Goworowski <[email protected]>

vico/compose/src/main/java/com/patrykandpatrick/vico/compose/chart/Charts.kt

@@ -28,6 +28,7 @@ import androidx.compose.foundation.layout.height
 import androidx.compose.runtime.Composable
 import androidx.compose.runtime.MutableFloatState
 import androidx.compose.runtime.MutableState
+import androidx.compose.runtime.getValue
 import androidx.compose.runtime.mutableFloatStateOf
 import androidx.compose.runtime.mutableStateOf
 import androidx.compose.runtime.remember
@@ -49,6 +50,9 @@ import com.patrykandpatrick.vico.compose.chart.scroll.rememberChartScrollState
 import com.patrykandpatrick.vico.compose.extension.chartTouchEvent
 import com.patrykandpatrick.vico.compose.gesture.OnZoom
 import com.patrykandpatrick.vico.compose.layout.getMeasureContext
+import com.patrykandpatrick.vico.compose.state.component1
+import com.patrykandpatrick.vico.compose.state.component2
+import com.patrykandpatrick.vico.compose.state.component3
 import com.patrykandpatrick.vico.compose.style.currentChartStyle
 import com.patrykandpatrick.vico.core.DEF_MAX_ZOOM
 import com.patrykandpatrick.vico.core.DEF_MIN_ZOOM
@@ -65,6 +69,8 @@ import com.patrykandpatrick.vico.core.chart.edges.FadingEdges
 import com.patrykandpatrick.vico.core.chart.layout.HorizontalLayout
 import com.patrykandpatrick.vico.core.chart.scale.AutoScaleUp
 import com.patrykandpatrick.vico.core.chart.values.ChartValuesManager
+import com.patrykandpatrick.vico.core.chart.values.ChartValuesProvider
+import com.patrykandpatrick.vico.core.chart.values.toChartValuesProvider
 import com.patrykandpatrick.vico.core.entry.ChartEntryModel
 import com.patrykandpatrick.vico.core.entry.ChartModelProducer
 import com.patrykandpatrick.vico.core.extension.set
@@ -130,16 +136,15 @@ public fun <Model : ChartEntryModel> Chart(
     getXStep: ((Model) -> Float)? = null,
 ) {
     val chartValuesManager = remember(chart) { ChartValuesManager() }
-    val (model, oldModel) = chartModelProducer
+    val chartEntryModelWrapper by chartModelProducer
         .collectAsState(chart, chartModelProducer, diffAnimationSpec, runInitialAnimation, chartValuesManager, getXStep)
-        .value
 
     ChartBox(modifier = modifier) {
-        if (model != null) {
+        chartEntryModelWrapper?.let { (chartEntryModel, previousChartEntryModel, chartValuesProvider) ->
             ChartImpl(
                 chart = chart,
-                model = model,
-                oldModel = oldModel,
+                model = chartEntryModel,
+                oldModel = previousChartEntryModel,
                 startAxis = startAxis,
                 topAxis = topAxis,
                 endAxis = endAxis,
@@ -153,7 +158,7 @@ public fun <Model : ChartEntryModel> Chart(
                 autoScaleUp = autoScaleUp,
                 chartScrollState = chartScrollState,
                 horizontalLayout = horizontalLayout,
-                chartValuesManager = chartValuesManager,
+                chartValuesProvider = chartValuesProvider,
             )
         }
     }
@@ -233,7 +238,7 @@ public fun <Model : ChartEntryModel> Chart(
             autoScaleUp = autoScaleUp,
             chartScrollState = chartScrollState,
             horizontalLayout = horizontalLayout,
-            chartValuesManager = chartValuesManager,
+            chartValuesProvider = chartValuesManager.toChartValuesProvider(),
         )
     }
 }
@@ -257,7 +262,7 @@ internal fun <Model : ChartEntryModel> ChartImpl(
     autoScaleUp: AutoScaleUp,
     chartScrollState: ChartScrollState = rememberChartScrollState(),
     horizontalLayout: HorizontalLayout,
-    chartValuesManager: ChartValuesManager,
+    chartValuesProvider: ChartValuesProvider,
 ) {
     val axisManager = remember { AxisManager() }
     val bounds = remember { RectF() }
@@ -269,7 +274,7 @@ internal fun <Model : ChartEntryModel> ChartImpl(
         bounds,
         horizontalLayout,
         with(LocalContext.current) { ::spToPx },
-        chartValuesManager,
+        chartValuesProvider,
     )
     val scrollListener = rememberScrollListener(markerTouchPoint)
     val lastMarkerEntryModels = remember { mutableStateOf(emptyList<Marker.EntryModel>()) }

vico/compose/src/main/java/com/patrykandpatrick/vico/compose/chart/entry/ChartEntryModelExtensions.kt

@@ -25,10 +25,13 @@ import androidx.compose.runtime.State
 import androidx.compose.runtime.remember
 import androidx.compose.runtime.rememberCoroutineScope
 import androidx.compose.ui.platform.LocalInspectionMode
-import com.patrykandpatrick.vico.compose.state.ChartEntryModelState
+import com.patrykandpatrick.vico.compose.state.ChartEntryModelWrapper
+import com.patrykandpatrick.vico.compose.state.ChartEntryModelWrapperState
 import com.patrykandpatrick.vico.core.Animation
 import com.patrykandpatrick.vico.core.chart.Chart
 import com.patrykandpatrick.vico.core.chart.values.ChartValuesManager
+import com.patrykandpatrick.vico.core.chart.values.ChartValuesProvider
+import com.patrykandpatrick.vico.core.chart.values.toChartValuesProvider
 import com.patrykandpatrick.vico.core.entry.ChartEntryModel
 import com.patrykandpatrick.vico.core.entry.ChartModelProducer
 import com.patrykandpatrick.vico.core.entry.diff.MutableDrawingModelStore
@@ -60,23 +63,22 @@ public fun <Model : ChartEntryModel> ChartModelProducer<Model>.collectAsState(
     chartValuesManager: ChartValuesManager,
     getXStep: ((Model) -> Float)?,
     dispatcher: CoroutineDispatcher = Dispatchers.Default,
-): State<Pair<Model?, Model?>> {
-    val chartEntryModelState = remember(chart, producerKey) { ChartEntryModelState<Model>() }
-
+): State<ChartEntryModelWrapper<Model>?> {
+    val chartEntryModelWrapperState = remember(chart, producerKey) { ChartEntryModelWrapperState<Model>() }
     val modelTransformerProvider = remember(chart) { chart.modelTransformerProvider }
     val drawingModelStore = remember(chart) { MutableDrawingModelStore() }
-
     val scope = rememberCoroutineScope()
     val isInPreview = LocalInspectionMode.current
-    var mainAnimationJob: Job? = null
-    var animationFrameJob: Job? = null
-    var finalAnimationFrameJob: Job? = null
-    var isAnimationRunning: Boolean
-    var isAnimationFrameGenerationRunning = false
     DisposableEffect(chart, producerKey, runInitialAnimation, isInPreview) {
+        var mainAnimationJob: Job? = null
+        var animationFrameJob: Job? = null
+        var finalAnimationFrameJob: Job? = null
+        var isAnimationRunning: Boolean
+        var isAnimationFrameGenerationRunning = false
+        var chartValuesProvider: ChartValuesProvider = ChartValuesProvider.Empty
         val afterUpdate: (progressModel: suspend (chartKey: Any, progress: Float) -> Unit) -> Unit = { progressModel ->
             if (animationSpec != null && !isInPreview &&
-                (chartEntryModelState.value.first != null || runInitialAnimation)
+                (chartEntryModelWrapperState.value != null || runInitialAnimation)
             ) {
                 isAnimationRunning = true
                 mainAnimationJob = scope.launch(dispatcher) {
@@ -120,18 +122,24 @@ public fun <Model : ChartEntryModel> ChartModelProducer<Model>.collectAsState(
                     isAnimationRunning = false
                 },
                 startAnimation = afterUpdate,
-                getOldModel = { chartEntryModelState.value.first },
+                getOldModel = { chartEntryModelWrapperState.value?.chartEntryModel },
                 modelTransformerProvider = modelTransformerProvider,
                 drawingModelStore = drawingModelStore,
                 updateChartValues = { model ->
                     chartValuesManager.resetChartValues()
                     chart.updateChartValues(chartValuesManager, model, getXStep?.invoke(model))
-                    chartValuesManager
+                    chartValuesManager.toChartValuesProvider().also { provider -> chartValuesProvider = provider }
                 },
-                onModelCreated = chartEntryModelState::set,
-            )
+            ) { chartEntryModel ->
+                chartEntryModelWrapperState.set(chartEntryModel, chartValuesProvider)
+            }
+        }
+        onDispose {
+            mainAnimationJob?.cancel()
+            animationFrameJob?.cancel()
+            finalAnimationFrameJob?.cancel()
+            unregisterFromUpdates(chart)
         }
-        onDispose { unregisterFromUpdates(chart) }
     }
-    return chartEntryModelState
+    return chartEntryModelWrapperState
 }

vico/compose/src/main/java/com/patrykandpatrick/vico/compose/layout/MeasureContextExtensions.kt

@@ -24,7 +24,7 @@ import androidx.compose.ui.platform.LocalLayoutDirection
 import androidx.compose.ui.unit.LayoutDirection
 import com.patrykandpatrick.vico.core.chart.layout.HorizontalLayout
 import com.patrykandpatrick.vico.core.chart.values.ChartValues
-import com.patrykandpatrick.vico.core.chart.values.ChartValuesManager
+import com.patrykandpatrick.vico.core.chart.values.ChartValuesProvider
 import com.patrykandpatrick.vico.core.context.MeasureContext
 import com.patrykandpatrick.vico.core.context.MutableMeasureContext
 
@@ -35,27 +35,28 @@ import com.patrykandpatrick.vico.core.context.MutableMeasureContext
  * @param canvasBounds the bounds of the canvas that will be used to draw the chart and its components.
  * @param horizontalLayout defines how the chart’s content is positioned horizontally.
  * @param spToPx converts dimensions from sp to px.
- * @param chartValuesManager manages the chart’s [ChartValues].
+ * @param chartValuesProvider provides the chart’s [ChartValues] instances.
  */
 @Composable
 public fun getMeasureContext(
     isHorizontalScrollEnabled: Boolean,
     canvasBounds: RectF,
     horizontalLayout: HorizontalLayout,
     spToPx: (Float) -> Float,
-    chartValuesManager: ChartValuesManager,
+    chartValuesProvider: ChartValuesProvider,
 ): MutableMeasureContext = remember {
     MutableMeasureContext(
         canvasBounds = canvasBounds,
         density = 0f,
         isLtr = true,
-        isHorizontalScrollEnabled = isHorizontalScrollEnabled,
-        horizontalLayout = horizontalLayout,
         spToPx = spToPx,
-        chartValuesManager = chartValuesManager,
+        chartValuesProvider = chartValuesProvider,
     )
 }.apply {
     this.density = LocalDensity.current.density
     this.isLtr = LocalLayoutDirection.current == LayoutDirection.Ltr
     this.isHorizontalScrollEnabled = isHorizontalScrollEnabled
+    this.horizontalLayout = horizontalLayout
+    this.spToPx = spToPx
+    this.chartValuesProvider = chartValuesProvider
 }

vico/compose/src/main/java/com/patrykandpatrick/vico/compose/state/ChartEntryModelWrapper.kt

@@ -0,0 +1,65 @@
+/*
+ * Copyright 2023 by Patryk Goworowski and Patrick Michalik.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.patrykandpatrick.vico.compose.state
+
+import androidx.compose.runtime.Immutable
+import androidx.compose.runtime.State
+import androidx.compose.runtime.getValue
+import androidx.compose.runtime.mutableStateOf
+import androidx.compose.runtime.setValue
+import com.patrykandpatrick.vico.core.chart.values.ChartValuesProvider
+import com.patrykandpatrick.vico.core.entry.ChartEntryModel
+
+/**
+ * Holds a chart’s current [ChartEntryModel] ([chartEntryModel]), previous [ChartEntryModel]
+ * ([previousChartEntryModel]), and [ChartValuesProvider] ([chartValuesProvider]).
+ */
+@Immutable
+public class ChartEntryModelWrapper<T : ChartEntryModel>(
+    public val chartEntryModel: T,
+    public val previousChartEntryModel: T?,
+    public val chartValuesProvider: ChartValuesProvider,
+)
+
+/**
+ * Returns [ChartEntryModelWrapper.chartEntryModel].
+ */
+public operator fun <T : ChartEntryModel> ChartEntryModelWrapper<T>.component1(): T = chartEntryModel
+
+/**
+ * Returns [ChartEntryModelWrapper.previousChartEntryModel].
+ */
+public operator fun <T : ChartEntryModel> ChartEntryModelWrapper<T>.component2(): T? = previousChartEntryModel
+
+/**
+ * Returns [ChartEntryModelWrapper.chartValuesProvider].
+ */
+public operator fun <T : ChartEntryModel> ChartEntryModelWrapper<T>.component3(): ChartValuesProvider =
+    chartValuesProvider
+
+internal class ChartEntryModelWrapperState<T : ChartEntryModel> : State<ChartEntryModelWrapper<T>?> {
+    private var previousChartEntryModel: T? = null
+
+    override var value by mutableStateOf<ChartEntryModelWrapper<T>?>(null)
+        private set
+
+    fun set(chartEntryModel: T, chartValuesProvider: ChartValuesProvider) {
+        val currentChartEntryModel = value?.chartEntryModel
+        if (chartEntryModel.id != currentChartEntryModel?.id) previousChartEntryModel = currentChartEntryModel
+        value = ChartEntryModelWrapper(chartEntryModel, previousChartEntryModel, chartValuesProvider)
+    }
+}

vico/core/src/main/java/com/patrykandpatrick/vico/core/axis/horizontal/DefaultHorizontalAxisItemPlacer.kt

@@ -38,7 +38,7 @@ internal class DefaultHorizontalAxisItemPlacer(
         visibleXRange: ClosedFloatingPointRange<Float>,
         fullXRange: ClosedFloatingPointRange<Float>,
     ): List<Float> {
-        val chartValues = context.chartValuesManager.getChartValues()
+        val chartValues = context.chartValuesProvider.getChartValues()
         val remainder = ((visibleXRange.start - chartValues.minX) / chartValues.xStep - offset) % spacing
         val firstValue = visibleXRange.start + (spacing - remainder) % spacing * chartValues.xStep
         val minXOffset = chartValues.minX % chartValues.xStep
@@ -61,7 +61,7 @@ internal class DefaultHorizontalAxisItemPlacer(
         horizontalDimensions: HorizontalDimensions,
         fullXRange: ClosedFloatingPointRange<Float>,
     ): List<Float> {
-        val chartValues = context.chartValuesManager.getChartValues()
+        val chartValues = context.chartValuesProvider.getChartValues()
         return listOf(chartValues.minX, (chartValues.minX + chartValues.maxX).half, chartValues.maxX)
     }
 
@@ -71,7 +71,7 @@ internal class DefaultHorizontalAxisItemPlacer(
         visibleXRange: ClosedFloatingPointRange<Float>,
         fullXRange: ClosedFloatingPointRange<Float>,
     ): List<Float>? {
-        val chartValues = context.chartValuesManager.getChartValues()
+        val chartValues = context.chartValuesProvider.getChartValues()
         return when (context.horizontalLayout) {
             is HorizontalLayout.Segmented -> {
                 val remainder = (visibleXRange.start - fullXRange.start) % chartValues.xStep

vico/core/src/main/java/com/patrykandpatrick/vico/core/axis/horizontal/HorizontalAxis.kt

@@ -89,7 +89,7 @@ public class HorizontalAxis<Position : AxisPosition.Horizontal>(
         val clipRestoreCount = canvas.save()
         val tickMarkTop = if (position.isBottom) bounds.top else bounds.bottom - axisThickness - tickLength
         val tickMarkBottom = tickMarkTop + axisThickness + tickLength
-        val chartValues = chartValuesManager.getChartValues()
+        val chartValues = chartValuesProvider.getChartValues()
 
         canvas.clipRect(
             bounds.left - itemPlacer.getStartHorizontalAxisInset(this, horizontalDimensions, tickThickness),
@@ -186,7 +186,7 @@ public class HorizontalAxis<Position : AxisPosition.Horizontal>(
         val clipRestoreCount = canvas.save()
         canvas.clipRect(chartBounds)
 
-        val chartValues = chartValuesManager.getChartValues()
+        val chartValues = chartValuesProvider.getChartValues()
 
         if (lineValues == null) {
             labelValues.forEach { x ->
@@ -238,7 +238,7 @@ public class HorizontalAxis<Position : AxisPosition.Horizontal>(
     private fun MeasureContext.getFullXRange(
         horizontalDimensions: HorizontalDimensions,
     ): ClosedFloatingPointRange<Float> = with(horizontalDimensions) {
-        val chartValues = chartValuesManager.getChartValues()
+        val chartValues = chartValuesProvider.getChartValues()
         val start = chartValues.minX - startPadding / xSpacing * chartValues.xStep
         val end = chartValues.maxX + endPadding / xSpacing * chartValues.xStep
         start..end
@@ -248,7 +248,7 @@ public class HorizontalAxis<Position : AxisPosition.Horizontal>(
         context: MeasureContext,
         horizontalDimensions: HorizontalDimensions,
     ): Float = with(context) {
-        val chartValues = chartValuesManager.getChartValues()
+        val chartValues = chartValuesProvider.getChartValues()
         val fullXRange = getFullXRange(horizontalDimensions)
 
         when (val constraint = sizeConstraint) {

vico/core/src/main/java/com/patrykandpatrick/vico/core/axis/vertical/DefaultVerticalAxisItemPlacer.kt

@@ -49,7 +49,7 @@ internal class DefaultVerticalAxisItemPlacer(
         position: AxisPosition.Vertical,
     ): List<Float> {
         if (maxItemCount == 0) return emptyList()
-        val chartValues = context.chartValuesManager.getChartValues(position)
+        val chartValues = context.chartValuesProvider.getChartValues(position)
         return if (chartValues.minY * chartValues.maxY >= 0) {
             getSimpleLabelValues(axisHeight, maxLabelHeight, chartValues)
         } else {
@@ -61,7 +61,7 @@ internal class DefaultVerticalAxisItemPlacer(
         context: MeasureContext,
         position: AxisPosition.Vertical,
     ): List<Float> {
-        val chartValues = context.chartValuesManager.getChartValues(position)
+        val chartValues = context.chartValuesProvider.getChartValues(position)
         return listOf(chartValues.minY, (chartValues.minY + chartValues.maxY).half, chartValues.maxY)
     }