EventFahrplan / EventFahrplan

An Android app to enjoy event schedules powered by Frab, Pretalx or Wafer.
https://eventfahrplan.eu
Apache License 2.0
212 stars 99 forks source link

List of favorites scrolls to the top when returning from details screen after delay #432

Open johnjohndoe opened 2 years ago

johnjohndoe commented 2 years ago

Environment

How to reproduce

  1. Set the system date and time to match the first day of the conference.
  2. Visit the favorites screen, scroll to the bottom of the list.
  3. Tap a session at the bottom of the list which opens the details screen.
  4. Wait for a few seconds, then press BACK to return to the favorites screen.

Observed behavior

Screen recording

https://user-images.githubusercontent.com/144518/147690155-3ee950fe-c4b1-45cd-a321-1c7b8b31d879.mp4

Expected behavior

Where to start

Related

fabianbieler commented 8 months ago

I did some debugging and found that the collector of StarredListParameter.observe is called on resume of StarredListFragment even if the parameters did not change. This calls ListView::setAdapter which in turn calls ListView::resetList which resets, among other things, the scroll position.

I tried stopping the invocation of the collector with Flow::distinctUntilChanged but this did not work. I'm no export on Kotlin Flows but I suspect this is due to the Flow being cold. However, I wasn't able to confirm this because I couldn't get Flow::shareIn to work.

Anyhow, adjusting Flow::observe in FlowExtension.kt like so did solve the problem for me but I'm not sure if this is the right approach for a fix:

fun <T> Flow<T>.observe(lifecycleOwner: LifecycleOwner, collector: FlowCollector<T>) {
    lifecycleOwner.lifecycleScope.launch {
        var previousValue: T? = null
        lifecycleOwner.repeatOnLifecycle(Lifecycle.State.STARTED) {
            collect { value ->
                if (value == previousValue) return@collect;
                previousValue = value
                collector.emit(value)
            }
        }
    }
}

Note: This bug also appear on ChangeList

johnjohndoe commented 8 months ago

@fabianbieler Thank you for looking into the issue. I noticed as well that the scroll position was buggy during 37C3. I did not have time to analyze it further, though. Did you also try out "one time events" with Channel<T> which are used in the ViewModels?

fabianbieler commented 8 months ago

I have now. Thanks for the hint. It does indeed fix the bug, as well.

Full disclosure: I'm not sure how this works, though.

Please have a look at https://github.com/EventFahrplan/EventFahrplan/pull/598 at your leisure. In particular, please verify that observeStarredListParameter is called from the correct method in StaredListFragment.kt. An alternative would be to launch the coroutine that collects starredSessions inside an init-block. Not sure if that's better.