Resolve caching and concurrency issues affecting `ChartEntryModelProd…

…ucer` and `ComposedChartEntryModelProducer` (WIP) Co-authored-by: Patryk Goworowski <[email protected]>
patrykandpatrick · Sep 28, 2023 · 2c4647d · 2c4647d
1 parent 4988676
commit 2c4647d
Show file tree

Hide file tree

Showing 4 changed files with 208 additions and 171 deletions.
diff --git a/.../src/main/java/com/patrykandpatrick/vico/compose/chart/entry/ChartEntryModelExtensions.kt b/.../src/main/java/com/patrykandpatrick/vico/compose/chart/entry/ChartEntryModelExtensions.kt
@@ -32,6 +32,7 @@ import com.patrykandpatrick.vico.core.chart.values.ChartValuesManager
 import com.patrykandpatrick.vico.core.entry.ChartEntryModel
 import com.patrykandpatrick.vico.core.entry.ChartModelProducer
 import com.patrykandpatrick.vico.core.entry.diff.MutableDrawingModelStore
+import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.Job
 import kotlinx.coroutines.cancelAndJoin
 import kotlinx.coroutines.launch
@@ -65,52 +66,54 @@ public fun <Model : ChartEntryModel> ChartModelProducer<Model>.collectAsState(
 
     val scope = rememberCoroutineScope()
     val isInPreview = LocalInspectionMode.current
-    var animationJob: Job? = null
+    var outerAnimationJob: Job? = null
+    var innerAnimationJob: Job? = null
     var isAnimationRunning: Boolean?
     DisposableEffect(chart, producerKey, runInitialAnimation, isInPreview) {
-        val afterUpdate = { progressModel: (chartKey: Any, progress: Float) -> Unit ->
+        val afterUpdate: (progressModel: (chartKey: Any, progress: Float) -> Unit) -> Unit = { progressModel ->
             if (animationSpec != null && !isInPreview &&
                 (chartEntryModelState.value.first != null || runInitialAnimation)
             ) {
-                isAnimationRunning = false
-                animationJob = scope.launch {
+                isAnimationRunning = true
+                outerAnimationJob = scope.launch {
                     animate(
                         initialValue = Animation.range.start,
                         targetValue = Animation.range.endInclusive,
                         animationSpec = animationSpec,
                     ) { value, _ ->
-                        if (isAnimationRunning == false) {
-                            progressModel(chart, value)
+                        if (isAnimationRunning == true) {
+                            innerAnimationJob = scope.launch(Dispatchers.Default) { progressModel(chart, value) }
                         }
                     }
                 }
             } else {
-                progressModel(chart, Animation.range.endInclusive)
+                scope.launch { progressModel(chart, Animation.range.endInclusive) }
             }
         }
-        registerForUpdates(
-            key = chart,
-            cancelAnimation = {
-                runBlocking { animationJob?.cancelAndJoin() }
-                isAnimationRunning = true
-            },
-            startAnimation = afterUpdate,
-            getOldModel = { chartEntryModelState.value.first },
-            modelTransformerProvider = modelTransformerProvider,
-            drawingModelStore = drawingModelStore,
-            updateChartValues = { model ->
-                chartValuesManager.resetChartValues()
-                chart.updateChartValues(chartValuesManager, model, getXStep?.invoke(model))
-                chartValuesManager
-            },
-        ) { updatedModel ->
-            chartEntryModelState.set(updatedModel)
-        }
-        onDispose {
-            unregisterFromUpdates(chart)
-            animationJob = null
-            isAnimationRunning = null
+        scope.launch {
+            registerForUpdates(
+                key = chart,
+                cancelAnimation = {
+                    runBlocking {
+                        outerAnimationJob?.cancelAndJoin()
+                        innerAnimationJob?.cancelAndJoin()
+                    }
+                    isAnimationRunning = false
+                },
+                startAnimation = afterUpdate,
+                getOldModel = { chartEntryModelState.value.first },
+                modelTransformerProvider = modelTransformerProvider,
+                drawingModelStore = drawingModelStore,
+                updateChartValues = { model ->
+                    chartValuesManager.resetChartValues()
+                    chart.updateChartValues(chartValuesManager, model, getXStep?.invoke(model))
+                    chartValuesManager
+                },
+            ) { updatedModel ->
+                chartEntryModelState.set(updatedModel)
+            }
         }
+        onDispose { unregisterFromUpdates(chart) }
     }
     return chartEntryModelState
 }
diff --git a/vico/core/src/main/java/com/patrykandpatrick/vico/core/entry/ChartEntryModelProducer.kt b/vico/core/src/main/java/com/patrykandpatrick/vico/core/entry/ChartEntryModelProducer.kt
@@ -16,42 +16,34 @@
 
 package com.patrykandpatrick.vico.core.entry
 
+import androidx.annotation.WorkerThread
 import com.patrykandpatrick.vico.core.DEF_THREAD_POOL_SIZE
 import com.patrykandpatrick.vico.core.chart.Chart
 import com.patrykandpatrick.vico.core.chart.values.ChartValuesManager
 import com.patrykandpatrick.vico.core.entry.diff.DrawingModelStore
 import com.patrykandpatrick.vico.core.entry.diff.MutableDrawingModelStore
-import com.patrykandpatrick.vico.core.extension.copy
-import com.patrykandpatrick.vico.core.extension.setToAllChildren
 import java.util.concurrent.Executor
 import java.util.concurrent.Executors
 
 /**
  * A [ChartModelProducer] implementation that generates [ChartEntryModel] instances.
  *
- * @param entryCollections a two-dimensional list of [ChartEntry] instances used to generate the [ChartEntryModel].
- * @param backgroundExecutor an [Executor] used to generate instances of the [ChartEntryModel] off the main thread.
+ * @param entryCollections the initial list of series (where each series is a list of [ChartEntry] instances).
+ * @param backgroundExecutor used to handle data updates. For synchronous behavior, use [Runnable.run]
+ * (`Runnable::run`).
  *
  * @see ChartModelProducer
  */
 public class ChartEntryModelProducer(
     entryCollections: List<List<ChartEntry>>,
-    backgroundExecutor: Executor = Executors.newFixedThreadPool(DEF_THREAD_POOL_SIZE),
+    private val backgroundExecutor: Executor = Executors.newFixedThreadPool(DEF_THREAD_POOL_SIZE),
 ) : ChartModelProducer<ChartEntryModel> {
 
+    private var series = emptyList<List<ChartEntry>>()
     private var cachedInternalModel: InternalModel? = null
-
-    private var entriesHashCode: Int? = null
-
+    private var runningUpdateCount = 0
     private val updateReceivers: HashMap<Any, UpdateReceiver> = HashMap()
 
-    private val executor: Executor = backgroundExecutor
-
-    /**
-     * A mutable two-dimensional list of the [ChartEntry] instances used to generate the [ChartEntryModel].
-     */
-    private val entries: ArrayList<ArrayList<ChartEntry>> = ArrayList()
-
     public constructor(
         vararg entryCollections: List<ChartEntry>,
         backgroundExecutor: Executor = Executors.newFixedThreadPool(DEF_THREAD_POOL_SIZE),
@@ -62,17 +54,19 @@ public class ChartEntryModelProducer(
     }
 
     /**
-     * Updates the two-dimensional list of [ChartEntry] instances and notifies listeners about the update.
+     * Requests a data update. If the update is accepted, `true` is returned. If the update is rejected, which occurs
+     * when there’s already an update in progress, `false` is returned.
      *
      * @see entries
      * @see registerForUpdates
      */
-    public fun setEntries(entries: List<List<ChartEntry>>) {
-        this.entries.setToAllChildren(entries)
-        entriesHashCode = entries.hashCode()
+    public fun setEntries(entries: List<List<ChartEntry>>): Boolean {
+        if (runningUpdateCount++ != 0) return false
+        series = entries.toList()
         cachedInternalModel = null
         updateReceivers.values.forEach { updateReceiver ->
-            executor.execute {
+            runningUpdateCount++
+            backgroundExecutor.execute {
                 updateReceiver.cancelAnimation()
                 updateReceiver.modelTransformer?.prepareForTransformation(
                     oldModel = updateReceiver.getOldModel(),
@@ -81,50 +75,53 @@ public class ChartEntryModelProducer(
                     chartValuesManager = updateReceiver.updateChartValues(getModel()),
                 )
                 updateReceiver.startAnimation(::progressModel)
+                runningUpdateCount--
             }
         }
+        runningUpdateCount--
+        return true
     }
 
     /**
-     * Updates the two-dimensional list of [ChartEntry] instances and notifies listeners about the update.
+     * Requests a data update. If the update is accepted, `true` is returned. If the update is rejected, which occurs
+     * when there’s already an update in progress, `false` is returned.
      *
      * @see entries
      * @see registerForUpdates
      */
-    public fun setEntries(vararg entries: List<ChartEntry>) {
-        setEntries(entries.toList())
-    }
-
-    private fun getInternalModel(drawingModelStore: DrawingModelStore = DrawingModelStore.empty): InternalModel {
-        cachedInternalModel.let { if (it != null) return it }
-        val xRange = entries.xRange
-        val yRange = entries.yRange
-        val aggregateYRange = entries.calculateStackedYRange()
-        return InternalModel(
-            entries = entries.copy(),
-            minX = xRange.start,
-            maxX = xRange.endInclusive,
-            minY = yRange.start,
-            maxY = yRange.endInclusive,
-            stackedPositiveY = aggregateYRange.endInclusive,
-            stackedNegativeY = aggregateYRange.start,
-            xGcd = entries.calculateXGcd(),
-            id = entriesHashCode ?: entries.hashCode().also { entriesHashCode = it },
-            drawingModelStore = drawingModelStore,
-        )
-    }
+    public fun setEntries(vararg entries: List<ChartEntry>): Boolean = setEntries(entries.toList())
+
+    private fun getInternalModel(drawingModelStore: DrawingModelStore = DrawingModelStore.empty) =
+        cachedInternalModel?.copy(drawingModelStore = drawingModelStore)
+            ?: run {
+                val xRange = series.xRange
+                val yRange = series.yRange
+                val aggregateYRange = series.calculateStackedYRange()
+                InternalModel(
+                    entries = series,
+                    minX = xRange.start,
+                    maxX = xRange.endInclusive,
+                    minY = yRange.start,
+                    maxY = yRange.endInclusive,
+                    stackedPositiveY = aggregateYRange.endInclusive,
+                    stackedNegativeY = aggregateYRange.start,
+                    xGcd = series.calculateXGcd(),
+                    id = series.hashCode(),
+                    drawingModelStore = drawingModelStore,
+                ).also { cachedInternalModel = it }
+            }
 
     override fun getModel(): ChartEntryModel = getInternalModel()
 
+    @WorkerThread
     override fun progressModel(key: Any, progress: Float) {
         with(updateReceivers[key] ?: return) {
-            executor.execute {
-                modelTransformer?.transform(drawingModelStore, progress)
-                onModelCreated(getInternalModel(drawingModelStore.copy()))
-            }
+            modelTransformer?.transform(drawingModelStore, progress)
+            onModelCreated(getInternalModel(drawingModelStore.copy()))
         }
     }
 
+    @WorkerThread
     override fun registerForUpdates(
         key: Any,
         cancelAnimation: () -> Unit,
@@ -145,16 +142,14 @@ public class ChartEntryModelProducer(
             getOldModel,
             updateChartValues,
         )
-        executor.execute {
-            cancelAnimation()
-            modelTransformer?.prepareForTransformation(
-                oldModel = getOldModel(),
-                newModel = getModel(),
-                drawingModelStore = drawingModelStore,
-                chartValuesManager = updateChartValues(getModel()),
-            )
-            startAnimation(::progressModel)
-        }
+        cancelAnimation()
+        modelTransformer?.prepareForTransformation(
+            oldModel = getOldModel(),
+            newModel = getModel(),
+            drawingModelStore = drawingModelStore,
+            chartValuesManager = updateChartValues(getModel()),
+        )
+        startAnimation(::progressModel)
     }
 
     override fun unregisterFromUpdates(key: Any) {