patrykandpatrick / vico

A light and extensible chart library for Android.
https://patrykandpatrick.com/vico/guide
Apache License 2.0
2.18k stars 131 forks source link

Initial scroll in ChartScrollState not handled properly #257

Closed thubalek closed 1 year ago

thubalek commented 1 year ago

Describe the bug It looks like InitialScroll setting is handled incorrectly.

Module

To reproduce create lineChart, use ChartScrollSpec with initialScroll = InitialScroll.End

Expected behavior Chart is scrolled to the end.

Android versions Not version specific bug

Comment I believe there is something wrong with com.patrykandpatrick.vico.compose.chart.scroll.ChartScrollState#handleInitialScroll. Property initialScrollHandled is not reset when maxValue is updated. Initially maxValue is 0 (handleIntialScroll is performed), and when maxValue is updated to something non-zero then update of value is skipped.

There is a chance that I'm doing something wrong on my side (as this part is not well documented and I'm beginner regarding JetPack Compose).

patrickmichalik commented 1 year ago

Hello! I’ve followed the steps listed under “To reproduce,” but no unexpected behavior can be observed. Would you be able to provide a minimal reproducible example?

thubalek commented 1 year ago

This is quick and dirty example. I just found that the issue is caused by values coming from mutable state from ViewModel, so it is most likely some kind of misunderstanding of JetPack Compose on my side.

See fragment

// this causes the issue
val values by viewModel.measurements

// this works
// val values = sampleValues

in this example

import androidx.compose.animation.core.spring
import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.layout.height
import androidx.compose.material3.MaterialTheme
import androidx.compose.runtime.Composable
import androidx.compose.runtime.MutableState
import androidx.compose.runtime.getValue
import androidx.compose.runtime.mutableStateOf
import androidx.compose.ui.Modifier
import androidx.compose.ui.tooling.preview.Preview
import androidx.compose.ui.unit.dp
import androidx.lifecycle.ViewModel
import androidx.lifecycle.viewModelScope
import androidx.lifecycle.viewmodel.compose.viewModel
import com.patrykandpatrick.vico.compose.axis.horizontal.bottomAxis
import com.patrykandpatrick.vico.compose.axis.vertical.startAxis
import com.patrykandpatrick.vico.compose.chart.Chart
import com.patrykandpatrick.vico.compose.chart.line.lineChart
import com.patrykandpatrick.vico.compose.chart.scroll.rememberChartScrollSpec
import com.patrykandpatrick.vico.compose.chart.scroll.rememberChartScrollState
import com.patrykandpatrick.vico.core.chart.values.AxisValuesOverrider
import com.patrykandpatrick.vico.core.entry.ChartEntryModel
import com.patrykandpatrick.vico.core.entry.FloatEntry
import com.patrykandpatrick.vico.core.entry.entryModelOf
import com.patrykandpatrick.vico.core.scroll.AutoScrollCondition
import com.patrykandpatrick.vico.core.scroll.InitialScroll
import kotlinx.coroutines.delay
import kotlinx.coroutines.launch

class FixedStepXModelDecorator(
    private val constantStepX: Float,
    decoratedModel: ChartEntryModel,
) :
    ChartEntryModel by decoratedModel {
    override val xStep: Float
        get() = constantStepX
}

private val sampleValues = (0..100).map {
    FloatEntry(it.toFloat(), kotlin.math.sin(it.toDouble()).toFloat() * 100)
}

class ChartTestViewModel : ViewModel() {
    val measurements: MutableState<List<FloatEntry>> = mutableStateOf(emptyList())
    init {
        viewModelScope.launch {
            delay(1000L)
            measurements.value = sampleValues
        }
    }
}

@Preview
@Composable
fun ChartTest(viewModel: ChartTestViewModel = viewModel()) {

    // this causes the issue
    val values by viewModel.measurements

    // this works
    // val values = sampleValues

    val chartEntryModel: ChartEntryModel = FixedStepXModelDecorator(5f, entryModelOf(
        values
    ))

    val chartScrollSpec = rememberChartScrollSpec(
        isScrollEnabled = true,
        initialScroll = InitialScroll.End,
        autoScrollCondition = AutoScrollCondition.OnModelSizeIncreased,
        autoScrollAnimationSpec = spring()
    )

    val chartScrollState = rememberChartScrollState()

    Column {
        val lineColor = MaterialTheme.colorScheme.primary
        Chart(
            chart = lineChart(
                axisValuesOverrider = AxisValuesOverrider.fixed(
                    minY = 0f, maxY = 100f
                ),
            ),
            model = chartEntryModel,
            startAxis = startAxis(
                maxLabelCount = 5,
                valueFormatter = { value, _ ->
                    "${value.toInt()}%"
                }
            ),
            bottomAxis = bottomAxis(
                valueFormatter = { value, _ ->
                    value.toString()
                }
            ),
            modifier = Modifier.height(200.dp),
            chartScrollSpec = chartScrollSpec,
            chartScrollState = chartScrollState,
            isZoomEnabled = true,
        )
    }
}
patrickmichalik commented 1 year ago

Thank you! The chart is empty at first. On the chart’s creation, ChartScrollState#value is correctly assigned the value of ChartScrollState#maxValue, which is zero. When, after one second, the chart is populated, it’s no longer in its initial state—the requested initial scroll value has already been applied. If you’d like ChartScrollState#value to be assigned the value of ChartScrollState#maxValue whenever the width of the chart content increases, you can use automatic scrolling. You’ve correctly passed AutoScrollCondition.OnModelSizeIncreased to rememberChartScrollSpec, but your custom ChartEntryModel-related solutions are incompatible with automatic scrolling. Specifically, FixedStepXModelDecorator doesn’t override ChartEntryModel#id so that the identifier changes along with the data, and the Chart composable never receives the previous ChartEntryModel. ChartEntryModelProducer and the Chart variant that accepts a ChartEntryModelProducer handle these matters out of the box, but for your custom approach, you’ll have to add support for automatic scrolling yourself.

thubalek commented 1 year ago

Thank you very much for your response. FixedStepXModelDecorator is kind of hack as I didn't know how to specify step size correctly. What is suggested approach?

BTW: I thought that when I use delegate in FixedStepXModelDecorator then id is inherited from decorated model. And decorated model is created via entryModelOf(values) so id is assigned automatically. Am I wrong?

BTW2: I see that for the ID you use Any.hashCode(). AFAIK, hash code is not unique, it is perfectly possible that two different objects return the same hash code (https://kotlinlang.org/api/latest/jvm/stdlib/kotlin/-any/hash-code.html) . May it also cause problem?

patrickmichalik commented 1 year ago

At the moment, the x step is calculated automatically, but VerticalAxis lets you specify the label spacing—please see here. In most cases, this is sufficient, but we’re currently working on an update to the API that will, among other things, enable you to override the x step, which can be particularly useful in the case of line charts.

Regarding FixedStepXModelDecorator, the issue here is that static ChartEntryModels created via entryModelOf have a common ID of 0. Generally, for dynamic data, ChartEntryModelProducer should be used—all model IDs are then handled automatically. However, you can update FixedStepXModelDecorator to override ChartEntryModel#id so that the identifier changes along with the data. The default entries.hashCode() should be sufficient.

To answer your question about hashCode, uniqueness can’t be guaranteed with hashes of finite length, but hash collisions aren’t what’s causing automatic scrolling not to work in your chart. The probability of a such a collision occurring is extremely small, and so it’s generally ignored. Passwords authentication relies on comparing hashes, for instance.

github-actions[bot] commented 1 year ago

There hasn’t been any activity here for four weeks. If no activity occurs over the next week, this issue will be closed as stale.