[emojisearch] Preserve scroll position for Android view #1599

jingwei99 · 2023-10-16T19:55:48Z

Preserve scroll position works end to end with this PR

scrollposition.mov

jingwei99 · 2023-10-16T20:00:32Z

...azylayout-compose/src/commonMain/kotlin/app/cash/redwood/lazylayout/compose/LazyListState.kt

-      initialFirstVisibleItemIndex,
-    )
+public fun rememberLazyListState(): LazyListState {
+  return rememberSaveable(saver = LazyListState.Saver) {


this change absorbs #1494 (🙏 to @-veyndan)

jingwei99 · 2023-10-16T20:05:41Z

...rch/presenter/src/commonMain/kotlin/com/example/redwood/emojisearch/presenter/EmojiSearch.kt

@@ -108,7 +114,9 @@ private fun LazyColumn(
      val labelToUrl = Json.decodeFromString<Map<String, String>>(emojisJson)

      allEmojis.clear()
-      allEmojis.addAll(labelToUrl.map { (key, value) -> EmojiImage(key, value) })
+      var index = 0
+      allEmojis.addAll(labelToUrl.map { (key, value) -> EmojiImage("${index++}. $key", value) })


print out index to make debugging/seeing what's going on with the list easier

jingwei99 · 2023-10-16T20:37:49Z

test is failing, I'll get to it

swankjesse · 2023-10-16T21:01:14Z

...ood-lazylayout-compose/src/commonMain/kotlin/app/cash/redwood/lazylayout/compose/LazyList.kt

@@ -46,9 +46,8 @@ internal fun LazyList(
 ) {
  val itemProvider = rememberLazyListItemProvider(content)
  var lastVisibleItemIndex by remember { mutableStateOf(0) }
-  val itemsBefore = remember(state.firstVisibleItemIndex) { (state.firstVisibleItemIndex - OffscreenItemsBufferCount / 2).coerceAtLeast(0) }
+  val itemsBefore = remember(state.scrollItemIndex) { (state.scrollItemIndex - OffscreenItemsBufferCount / 2).coerceAtLeast(0) }


ooooh I think we might have broken this.

This should probably be a different mutableState for proper on-demand loading

As-is this only updates once, and I think it needs to update with every scroll.

Whereas the automated scrolling does not need to do that.

swankjesse · 2023-10-16T21:03:59Z

...ood-lazylayout-compose/src/commonMain/kotlin/app/cash/redwood/lazylayout/compose/LazyList.kt

  var placeholderPoolSize by remember { mutableStateOf(20) }
  RefreshableLazyList(
    isVertical,
    itemsBefore = itemsBefore,
    itemsAfter = itemsAfter,
    onViewportChanged = { localFirstVisibleItemIndex, localLastVisibleItemIndex ->
+      // Update LazyListState.


Suggested change

// Update LazyListState.

swankjesse

Scrolling behavior looks great.

Does this break lazy loading?

jingwei99 · 2023-10-16T22:37:53Z

Does this break lazy loading?

fixed!

JakeWharton · 2023-10-17T01:04:30Z

redwood-lazylayout-compose/build.gradle

@@ -13,6 +13,7 @@ kotlin {
  sourceSets {
    commonMain {
      dependencies {
+        api libs.jetbrains.compose.runtime.saveable


This can be implementation if you change the Saver to be private.

will make a follow up PR

here it is: #1600

JakeWharton · 2023-10-17T01:10:12Z

redwood-lazylayout-view/src/main/kotlin/app/cash/redwood/lazylayout/view/ViewLazyList.kt

@@ -101,9 +104,11 @@ internal open class ViewLazyList private constructor(
      addOnScrollListener(
        object : RecyclerView.OnScrollListener() {
          override fun onScrolled(recyclerView: RecyclerView, dx: Int, dy: Int) {
+            val viewsOnScreen = recyclerView.children.map { recyclerView.getChildViewHolder(it).bindingAdapterPosition }


This is a decently expensive operation to perform on every frame of scrolling. It involves allocation of the list and boxing of the position integers.

It is.

There was an alternative of passing in a mutable of viewHolder and positions to adapter to keep track of positions of visible views (by adding/removing keys when onViewDetachedFromWindow and onBindViewHolder), it's somewhat ugly though.

There might be other alternatives.

Previous calls : linearLayoutManager.findFirstVisibleItemPosition() and linearLayoutManager.findLastVisibleItemPosition() are not giving the accurate indices.

Previous calls : linearLayoutManager.findFirstVisibleItemPosition() and linearLayoutManager.findLastVisibleItemPosition() are not giving the accurate indices.

I'd be curious to investigate this more, because it's kinda surprising that a core API would fail like this. Did you find any bug reports on it?

Did you find any bug reports on it?

from my time-boxed googling, there wasn't report with similar symptom

JakeWharton · 2023-10-17T01:11:10Z

redwood-lazylayout-view/src/main/kotlin/app/cash/redwood/lazylayout/view/ViewLazyList.kt

@@ -101,9 +104,11 @@ internal open class ViewLazyList private constructor(
      addOnScrollListener(
        object : RecyclerView.OnScrollListener() {
          override fun onScrolled(recyclerView: RecyclerView, dx: Int, dy: Int) {
+            val viewsOnScreen = recyclerView.children.map { recyclerView.getChildViewHolder(it).bindingAdapterPosition }
+            userHasScrolled = true // Prevent guest code from hijacking the scrollbar.


Does this mean it's impossible to programmatically change the scroll position once the user has interacted with the list a single time?

that's a good point ... yes, it is at the moment

veyndan · 2023-10-17T11:12:03Z

I haven't properly reviewed the PR yet, but I'm noticing a crash in Emoji Search after these changes:

Run Emoji Search using Android Views
Type "medal" into the search box
Click on one of the medal emojis
Press the system back button
The app crashes with the exception app.cash.zipline.ZiplineException: IllegalArgumentException: Failed requirement.

veyndan · 2023-10-17T16:28:10Z

...azylayout-compose/src/commonMain/kotlin/app/cash/redwood/lazylayout/compose/LazyListState.kt

 import androidx.compose.runtime.setValue

 @Composable
-public fun rememberLazyListState(
-  initialFirstVisibleItemIndex: Int = 0,


Why did we remove the default value here? It's kinda nice to have it as it aligns with what Compose UI does with their LazyListState implementation.

the init for initialFirstVisibleItemIndex now is at down below (to 0)

veyndan · 2023-10-17T16:30:55Z

redwood-lazylayout-view/src/main/kotlin/app/cash/redwood/lazylayout/view/ViewLazyList.kt

@@ -101,9 +104,11 @@ internal open class ViewLazyList private constructor(
      addOnScrollListener(
        object : RecyclerView.OnScrollListener() {
          override fun onScrolled(recyclerView: RecyclerView, dx: Int, dy: Int) {
+            val viewsOnScreen = recyclerView.children.map { recyclerView.getChildViewHolder(it).bindingAdapterPosition }


Previous calls : linearLayoutManager.findFirstVisibleItemPosition() and linearLayoutManager.findLastVisibleItemPosition() are not giving the accurate indices.

I'd be curious to investigate this more, because it's kinda surprising that a core API would fail like this. Did you find any bug reports on it?

veyndan · 2023-10-17T16:32:31Z

...azylayout-compose/src/commonMain/kotlin/app/cash/redwood/lazylayout/compose/LazyListState.kt

+  public var firstVisibleItemIndex: Int = 0
+    private set
+
+  public fun restoreIndex(index: Int) {


Could we change this back to scrollToItem? https://developer.android.com/reference/kotlin/androidx/compose/foundation/lazy/LazyListState#scrollToItem(kotlin.Int,kotlin.Int)

this function is not really a scroll to item anymore, it has it's own logic, and what it does is "restore when there's criteria are met"

jingwei99 commented Oct 16, 2023

View reviewed changes

[emojisearch] Preserve scroll position for Android view

431a693

jingwei99 force-pushed the jingwei.preserve_scroll_position branch from 04f469f to 431a693 Compare October 16, 2023 20:04

jingwei99 commented Oct 16, 2023

View reviewed changes

jingwei99 marked this pull request as ready for review October 16, 2023 20:11

jingwei99 requested review from swankjesse, veyndan and JakeWharton October 16, 2023 20:11

swankjesse reviewed Oct 16, 2023

View reviewed changes

swankjesse approved these changes Oct 16, 2023

View reviewed changes

comments

4186212

underscoretang self-requested a review October 16, 2023 22:31

jingwei99 enabled auto-merge (squash) October 16, 2023 22:46

jingwei99 merged commit db2d039 into trunk Oct 16, 2023
8 checks passed

jingwei99 deleted the jingwei.preserve_scroll_position branch October 16, 2023 22:54

JakeWharton reviewed Oct 17, 2023

View reviewed changes

JakeWharton mentioned this pull request Oct 17, 2023

Crash in emoji-search after scroll position preservation commit #1604

Closed

veyndan reviewed Oct 17, 2023

View reviewed changes

veyndan mentioned this pull request Nov 13, 2023

Preserve scroll position on Android Activity restoration #1494

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[emojisearch] Preserve scroll position for Android view #1599

[emojisearch] Preserve scroll position for Android view #1599

jingwei99 commented Oct 16, 2023 •

edited

Loading

jingwei99 Oct 16, 2023 •

edited

Loading

jingwei99 Oct 16, 2023

jingwei99 commented Oct 16, 2023

swankjesse Oct 16, 2023

swankjesse Oct 16, 2023

swankjesse Oct 16, 2023

jingwei99 Oct 16, 2023

swankjesse Oct 16, 2023

jingwei99 Oct 16, 2023

swankjesse left a comment

jingwei99 commented Oct 16, 2023

JakeWharton Oct 17, 2023

jingwei99 Oct 17, 2023

jingwei99 Oct 17, 2023

JakeWharton Oct 17, 2023

jingwei99 Oct 17, 2023

veyndan Oct 17, 2023

jingwei99 Oct 17, 2023

JakeWharton Oct 17, 2023

jingwei99 Oct 17, 2023

veyndan commented Oct 17, 2023

veyndan Oct 17, 2023

jingwei99 Oct 17, 2023

veyndan Oct 17, 2023

veyndan Oct 17, 2023

jingwei99 Oct 17, 2023

[emojisearch] Preserve scroll position for Android view #1599

[emojisearch] Preserve scroll position for Android view #1599

Conversation

jingwei99 commented Oct 16, 2023 • edited Loading

jingwei99 Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jingwei99 commented Oct 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swankjesse left a comment

Choose a reason for hiding this comment

jingwei99 commented Oct 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veyndan commented Oct 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jingwei99 commented Oct 16, 2023 •

edited

Loading

jingwei99 Oct 16, 2023 •

edited

Loading