Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[emojisearch] Preserve scroll position for Android view #1599

Merged
merged 2 commits into from
Oct 16, 2023
Merged

[emojisearch] Preserve scroll position for Android view #1599

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
431a693
[emojisearch] Preserve scroll position for Android view
jingwei99 Oct 16, 2023
4186212
comments
jingwei99 Oct 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions redwood-lazylayout-compose/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ kotlin {
sourceSets {
commonMain {
dependencies {
api libs.jetbrains.compose.runtime.saveable
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will make a follow up PR

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here it is: #1600

api projects.redwoodLazylayoutWidget
}
}
Expand Down
12 changes: 6 additions & 6 deletions ...-lazylayout-compose/src/commonMain/kotlin/app/cash/redwood/lazylayout/compose/LazyList.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ internal fun LazyList(
var lastVisibleItemIndex by remember { mutableStateOf(0) }
val itemsBefore = remember(state.firstVisibleItemIndex) { (state.firstVisibleItemIndex - OffscreenItemsBufferCount / 2).coerceAtLeast(0) }
val itemsAfter = remember(lastVisibleItemIndex, itemProvider.itemCount) { (itemProvider.itemCount - (lastVisibleItemIndex + OffscreenItemsBufferCount / 2).coerceAtMost(itemProvider.itemCount)).coerceAtLeast(0) }
val scrollItemIndex = remember(state.scrollToItemTriggeredId) { ScrollItemIndex(state.scrollToItemTriggeredId, state.firstVisibleItemIndex) }
// TODO(jwilson): drop this down to 20 once this is fixed:
// https://github.com/cashapp/redwood/issues/1551
var placeholderPoolSize by remember { mutableStateOf(30) }
Expand All @@ -57,21 +56,22 @@ internal fun LazyList(
itemsBefore = itemsBefore,
itemsAfter = itemsAfter,
onViewportChanged = { localFirstVisibleItemIndex, localLastVisibleItemIndex ->
state.onScrolled(localFirstVisibleItemIndex)

val visibleItemCount = localLastVisibleItemIndex - localFirstVisibleItemIndex
val proposedPlaceholderPoolSize = visibleItemCount + visibleItemCount / 2
// We only ever want to increase the pool size.
if (placeholderPoolSize < proposedPlaceholderPoolSize) {
placeholderPoolSize = proposedPlaceholderPoolSize
}
state.firstVisibleItemIndex = localFirstVisibleItemIndex
lastVisibleItemIndex = localLastVisibleItemIndex
},
width = width,
height = height,
margin = margin,
crossAxisAlignment = crossAxisAlignment,
modifier = modifier,
scrollItemIndex = scrollItemIndex,
scrollItemIndex = ScrollItemIndex(0, state.scrollItemIndex),
placeholder = { repeat(placeholderPoolSize) { placeholder() } },
items = {
for (index in itemsBefore until itemProvider.itemCount - itemsAfter) {
Expand Down Expand Up @@ -102,20 +102,20 @@ internal fun RefreshableLazyList(
var lastVisibleItemIndex by remember { mutableStateOf(0) }
val itemsBefore = remember(state.firstVisibleItemIndex) { (state.firstVisibleItemIndex - OffscreenItemsBufferCount / 2).coerceAtLeast(0) }
val itemsAfter = remember(lastVisibleItemIndex, itemProvider.itemCount) { (itemProvider.itemCount - (lastVisibleItemIndex + OffscreenItemsBufferCount / 2).coerceAtMost(itemProvider.itemCount)).coerceAtLeast(0) }
val scrollItemIndex = remember(state.scrollToItemTriggeredId) { ScrollItemIndex(state.scrollToItemTriggeredId, state.firstVisibleItemIndex) }
var placeholderPoolSize by remember { mutableStateOf(20) }
RefreshableLazyList(
isVertical,
itemsBefore = itemsBefore,
itemsAfter = itemsAfter,
onViewportChanged = { localFirstVisibleItemIndex, localLastVisibleItemIndex ->
state.onScrolled(localFirstVisibleItemIndex)

val visibleItemCount = localLastVisibleItemIndex - localFirstVisibleItemIndex
val proposedPlaceholderPoolSize = visibleItemCount + visibleItemCount / 2
// We only ever want to increase the pool size.
if (placeholderPoolSize < proposedPlaceholderPoolSize) {
placeholderPoolSize = proposedPlaceholderPoolSize
}
state.firstVisibleItemIndex = localFirstVisibleItemIndex
lastVisibleItemIndex = localLastVisibleItemIndex
},
refreshing = refreshing,
Expand All @@ -125,7 +125,7 @@ internal fun RefreshableLazyList(
margin = margin,
crossAxisAlignment = crossAxisAlignment,
modifier = modifier,
scrollItemIndex = scrollItemIndex,
scrollItemIndex = ScrollItemIndex(0, state.scrollItemIndex),
placeholder = { repeat(placeholderPoolSize) { placeholder() } },
pullRefreshContentColor = pullRefreshContentColor,
items = {
Expand Down
75 changes: 57 additions & 18 deletions ...layout-compose/src/commonMain/kotlin/app/cash/redwood/lazylayout/compose/LazyListState.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,32 +18,71 @@ package app.cash.redwood.lazylayout.compose
import androidx.compose.runtime.Composable
import androidx.compose.runtime.getValue
import androidx.compose.runtime.mutableStateOf
import androidx.compose.runtime.remember
import androidx.compose.runtime.saveable.Saver
import androidx.compose.runtime.saveable.rememberSaveable
import androidx.compose.runtime.setValue

@Composable
public fun rememberLazyListState(
initialFirstVisibleItemIndex: Int = 0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

): LazyListState {
return remember {
LazyListState(
initialFirstVisibleItemIndex,
)
public fun rememberLazyListState(): LazyListState {
return rememberSaveable(saver = LazyListState.Saver) {
Copy link
Author

@jingwei99 jingwei99 Oct 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

LazyListState()
}
}

public class LazyListState(
firstVisibleItemIndex: Int = 0,
) {
public var firstVisibleItemIndex: Int by mutableStateOf(firstVisibleItemIndex)
public class LazyListState {
/** We only restore the scroll position once. */
private var hasRestoredScrollPosition = false

/** The scroll position to restore. */
private var restoredIndex: Int = -1

/**
* The value published to the host platform. This starts as 0 and changes exactly once to
* trigger exactly one scroll.
*/
public var scrollItemIndex: Int by mutableStateOf(0)
internal set

internal var scrollToItemTriggeredId by mutableStateOf(0)
public var firstVisibleItemIndex: Int = 0
private set

public fun restoreIndex(index: Int) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

require(index >= 0)

if (this.restoredIndex != -1) return
this.restoredIndex = index

// Scroll to the target item.
if (hasRestoredScrollPosition) {
scrollItemIndex = restoredIndex
}
}

public fun scrollToItem(
index: Int,
) {
firstVisibleItemIndex = index
scrollToItemTriggeredId++
public fun maybeRestoreScrollPosition() {
if (this.hasRestoredScrollPosition) return
this.hasRestoredScrollPosition = true

// Scroll to the target item.
if (restoredIndex != -1) {
scrollItemIndex = restoredIndex
}
}

public fun onScrolled(firstVisibleItemIndex: Int) {
this.firstVisibleItemIndex = firstVisibleItemIndex
}

public companion object {
/**
* The default [Saver] implementation for [LazyListState].
*/
public val Saver: Saver<LazyListState, *> = Saver(
save = { it.firstVisibleItemIndex },
restore = {
LazyListState().apply {
restoreIndex(it)
}
},
)
}
}
10 changes: 8 additions & 2 deletions redwood-lazylayout-view/src/main/kotlin/app/cash/redwood/lazylayout/view/ViewLazyList.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import android.view.ViewGroup.LayoutParams.MATCH_PARENT
import android.view.ViewGroup.LayoutParams.WRAP_CONTENT
import android.widget.FrameLayout
import androidx.annotation.ColorInt
import androidx.core.view.children
import androidx.core.view.doOnDetach
import androidx.core.view.updateLayoutParams
import androidx.core.view.updatePaddingRelative
Expand Down Expand Up @@ -58,6 +59,8 @@ internal open class ViewLazyList private constructor(

private var crossAxisAlignment = CrossAxisAlignment.Start

private var userHasScrolled = false

private val density = Density(recyclerView.context.resources)
private val linearLayoutManager = object : LinearLayoutManager(recyclerView.context) {
override fun generateDefaultLayoutParams(): RecyclerView.LayoutParams? = when (orientation) {
Expand Down Expand Up @@ -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 }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you find any bug reports on it?

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

userHasScrolled = true // Prevent guest code from hijacking the scrollbar.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

onViewportChanged?.invoke(
linearLayoutManager.findFirstVisibleItemPosition(),
linearLayoutManager.findLastVisibleItemPosition(),
viewsOnScreen.min(),
viewsOnScreen.max(),
)
}
},
Expand Down Expand Up @@ -154,6 +159,7 @@ internal open class ViewLazyList private constructor(
}

override fun scrollItemIndex(scrollItemIndex: ScrollItemIndex) {
if (userHasScrolled) return
recyclerView.scrollToPosition(scrollItemIndex.index)
}

Expand Down
16 changes: 9 additions & 7 deletions .../presenter/src/commonMain/kotlin/com/example/redwood/emojisearch/presenter/EmojiSearch.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,12 @@ private fun LazyColumn(

var searchTerm by rememberSaveable(stateSaver = searchTermSaver) { mutableStateOf(TextFieldState("")) }

val lazyListState = rememberLazyListState()

LaunchedEffect(searchTerm) {
lazyListState.restoreIndex(0)
}

LaunchedEffect(refreshSignal) {
try {
refreshing = true
Expand All @@ -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) })
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

lazyListState.maybeRestoreScrollPosition()
} finally {
refreshing = false
}
Expand All @@ -121,12 +129,6 @@ private fun LazyColumn(
}
}

val lazyListState = rememberLazyListState()

LaunchedEffect(searchTerm) {
lazyListState.scrollToItem(0)
}

Column(
width = Constraint.Fill,
height = Constraint.Fill,
Expand Down