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.

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

[Pager] Re-write Pager on top of LazyRow|Column #678

Merged
merged 72 commits into from Sep 14, 2021
Merged

Conversation

chrisbanes
Copy link
Contributor

@chrisbanes chrisbanes commented Aug 24, 2021

Added features

The migration has allowed us to add some features.

Removed features

Unfortunately, the migration has resulted in some features being removed:

  • The infiniteLooping parameter has been removed. Instead a sample has been created showing you how to implement this.
  • offscreenLimit. We no longer have any control of what items are laid out 'off screen'.

API changes

  • The pageCount parameter has moved from PagerState to HorizontalPager and VerticalPager
  • Had to remove the animation spec parameters on animateScrollToPage()

Other things

Tasks

  • Re-enable dragEnabled
  • Add back some offset parameters
  • Fix animateScrollToPage(). Currently it only animates in one direction.
  • Update docs and guide

Future tasks

  • Move SnappingFlingBehavior to new module + docs + tests.
  • Add ability to fling more than 1 page.

@google-cla google-cla bot added the cla: yes label Aug 24, 2021
@danielesegato
Copy link

Theoretically it is possible to have the behaviour similar to infinite looping with Lazy lists.

Would that get messy with key though? @andkulikov

it makes it a bit strange, agree. but the hard limitation of keys is not to never have items with the same key, but to never have items with the same key composed together at the same time. which is probably true in the same way for any other possible pager implementation with infinite looping. like it will not work correctly if you only have 1 or 2 items

Thanks for this.
This is exactly what I was thinking of doing.

However, I believe the 3 items minimum is only true if each items takes the whole space, otherwise if you are showing part of the items before / after you are gonna need more items to avoid key collisions.

val pageCount = 10

// We start the pager in the middle of the raw number of pages
val startIndex = Int.MAX_VALUE / 2

Choose a reason for hiding this comment

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

I think this should be

val startIndex = (Int.MAX_VALUE / 2).let { it - it.floorMod(pageCount) }

to start from the first index

@danielesegato
Copy link

danielesegato commented Sep 13, 2021

I've grabbed the code for the pager at the current state, it looks like it assume each page will always take all the available space?

I can't have the item before and after peak into vision anymore. Has this feature also been removed?

EDIT1:

I believe the issue is here

                        // Constraint the content to be <= than the size of the pager.
                        .fillParentMaxSize()

it should be a max size, but this is forcing the content of each page to be exactly that size.
LazyLists maxWidth/maxHeight are private, otherwise it could be sizeIn(maxWidth = maxWidth, maxHeight = maxHeight)

And removing this constraint makes the page aligned to the left, before we had the control to have it aligned on the left, right or (default) center.

EDIT2:

if I set contentPadding on the Pager it does use that to show items on the left and right, is this how we are supposed to achieve this functionality?
Before we set a width to the page and if it wasn't the full width it would leave the side space for items before and after.

@andkulikov
Copy link
Collaborator

andkulikov commented Sep 13, 2021

I can't have the item before and after peak into vision anymore. Has this feature also been removed?

I think setting contentPadding is a new way of making items to fill the area smaller then the viewport. This should enable nearest items to be partially visible

@danielesegato
Copy link

I can't have the item before and after peak into vision anymore. Has this feature also been removed?

I think setting contentPadding is a new way of making items to fill the area smaller then the viewport. This should enable nearest items to be partially visible

Thanks, yes, I had just figured it out, I somehow found the previous behavior more intuitive but it does what I need. (I had just edited my previous comment)

This also means each page will need to have the same width (in an horizontal pager), am I right?

@andkulikov
Copy link
Collaborator

This also means each page will need to have the same width (in an horizontal pager), am I right?

Yes. If you have items of different size, or items which are not really filling the screen you probably need not a Pager, but just LazyRow with snapping behavior. Chris is going to make the snapping FlingBehavior for LazyRow public in one of the further commits

@chrisbanes chrisbanes enabled auto-merge (squash) September 13, 2021 15:21
@chrisbanes
Copy link
Contributor Author

Chris is going to make the snapping FlingBehavior for LazyRow public in one of the further commits

Yep, working on it right now.

@ColtonIdle
Copy link

@chrisbanes sorry for the discussion on a closed item, but I'm back from vacation and just got to setup a proof of concept of where removing "offscreenLimit" is detrimental. I don't bring it up to argue, but instead to see if you have any idea on how to handle this sort of flow.

Essentially, my designer gave us a tutorial/wizard when first opening up the app. There is text pinned to the top, and image right below it and a button pinned to the bottom.

As you can see, if I have offscreenLimit set to 1 (default) then I get this result which my designer does not want.

before_1.mp4

but if I take the offscreenLimit and set it to the size of the pager, then everything is laid out, and so the height of the horizontalPager is the height of the tallest item, which is what my designer wants. As you can see here:

after_1.mp4

At this point, do you think the "lazy" horizontalPager that will be coming out in the next release in accompanist will be the wrong tool for the job for me or is there someway that I will be able to replicate this behavior using the lazy horizontalPager?

@andkulikov
Copy link
Collaborator

As you can see, if I have offscreenLimit set to 1 (default) then I get this result which my designer does not want.

There are so many different ways how to solve that. But in general, the design process is the collaboration between the designer and the engineer. They both can discuss all the trade offs and agree on the solution. As I mentioned earlier measuring all the items only to know the height is not a feasible solution for large item counts.
You can assume that you would always have small item count and use the hack I previously shared where you compose more items then visible.
You can say that we can't assume that there will be a small item count and discuss with the designer what other options you have. Again, I already mentioned that what you described is not always a better UX. What if your user never planned to scroll this pager at all? Then they will be surprised to see a huge gap between the Pager and the next component. It is not self-explanatory that this gap will be needed when you scroll and the large item would became visible. I would say that changing the heigh with the animation is probably more preferable solution in my opinion.
Or you can reorganize the screen so they items are not depending on each other size, like the component bellow the pager would always aligned to the bottom.

Do not be afraid to discuss the trade offs with your designers and propose options. If there is a technical limitation which negatively affects the performance sometimes it is better to tweak the design a bit to not make your app slow.

We could explore adding something like offscreenLimit to LazyRow in the future, but even if we do so I would not want our users to solve the use cases like you mentioned with that mechanism.

@Tolriq
Copy link

Tolriq commented Sep 30, 2021

Sorry to also post on a closed issue, but not having offscreenLimit means the non visible pages are not rendered and so things like images are no more preloaded. This cause delays and possible jank during scrolling as the screen is composed during the scrolling.
Is there's some easy workaround to address this ?

@andkulikov
Copy link
Collaborator

Sorry to also post on a closed issue, but not having offscreenLimit means the non visible pages are not rendered and so things like images are no more preloaded. This cause delays and possible jank during scrolling as the screen is composed during the scrolling. Is there's some easy workaround to address this ?

I expect that in most cases one nearest item will anyway be composed by the prefetching logic. What value was you passing to offscreenLimit previously?
You can also experiment with the workaround explained here

@Tolriq
Copy link

Tolriq commented Sep 30, 2021

I was using just 1. And unless I missed a param there does not seems to have any prefetch.

Will try your workaround thanks.

@Tolriq
Copy link

Tolriq commented Sep 30, 2021

So after testing more the adjacent pages are pre loaded but coil can't load the images, I suppose some layout params are wrong or something like that.
The workaround can't really work for me due to #753 as it actually makes the content goes out of screens if I add an aspectRatio.

I'll try to find time to investigate more and open a proper issue.

@ln-12
Copy link
Contributor

ln-12 commented Oct 4, 2021

Is there a workaround for the missing dragEnabled in the meantime?

@andkulikov
Copy link
Collaborator

Is there a workaround for the missing dragEnabled in the meantime?

You can use the same technique as used for LazyRow in general. See https://stackoverflow.com/questions/66502096/how-to-disable-and-enable-scrolling-in-lazycolumn-lazyrow-in-jetpack-compose

@ln-12
Copy link
Contributor

ln-12 commented Oct 4, 2021

You can use the same technique as used for LazyRow in general. See https://stackoverflow.com/questions/66502096/how-to-disable-and-enable-scrolling-in-lazycolumn-lazyrow-in-jetpack-compose

Thanks for the quick reply! I also saw that, but it would require to make the state public (as mentioned above the line). To make it work, I could duplicate the state and pager classes locally in my project, but I wanted to avoid redundant code.

@andkulikov
Copy link
Collaborator

You can use the same technique as used for LazyRow in general. See https://stackoverflow.com/questions/66502096/how-to-disable-and-enable-scrolling-in-lazycolumn-lazyrow-in-jetpack-compose

Thanks for the quick reply! I also saw that, but it would require to make the state public (as mentioned above the line). To make it work, I could duplicate the state and pager classes locally in my project, but I wanted to avoid redundant code.

No, you shouldn't need to do it. There is exactly the same scroll function exposed right on PagerState.

@amenon
Copy link

amenon commented Oct 4, 2021

You can use the same technique as used for LazyRow in general. See https://stackoverflow.com/questions/66502096/how-to-disable-and-enable-scrolling-in-lazycolumn-lazyrow-in-jetpack-compose

Thanks for the quick reply! I also saw that, but it would require to make the state public (as mentioned above the line). To make it work, I could duplicate the state and pager classes locally in my project, but I wanted to avoid redundant code.

No, you shouldn't need to do it. There is exactly the same scroll function exposed right on PagerState.

I am attempting to use this method to disable dragging of the pager. However, it looks like this method blocks all gestures within the content of the pager. I cannot scroll vertically or tap buttons within the content. Here is the extension function I added:

/**
 * Use this to disable scrolling on a pager
 * eg:
 *   val scope = rememberCoroutineScope()
 *   val pagerState = rememberPagerState()
 *   pagerState.disableScrolling(scope)
 */
@OptIn(ExperimentalPagerApi::class)
fun PagerState.disableScrolling(scope: CoroutineScope) {
    scope.launch {
        scroll(scrollPriority = MutatePriority.PreventUserInput) {
            // Await indefinitely, blocking scrolls
            awaitCancellation()
        }
    }
}

Am I missing something here @andkulikov?

@ColtonIdle
Copy link

@andkulikov not that it's a problem that you have to worry about. But I tried using your workaround finally today and it doesn't work with a HorizontalPager as it makes the viewport of every item a huge size in width.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
8 participants