search

google / accompanist

A collection of extension libraries for Jetpack Compose
https://google.github.io/accompanist
Apache License 2.0
7.45k stars 599 forks source link

[Pager] targetPage is deprecated #1199

Closed K1rakishou closed 2 years ago

K1rakishou commented 2 years ago

If you still think that you need targetPage, not currentPage please file a bug as we are planning to remove this property in future.

I'm creating this issue because I need the targetPage field. I'm using it for a toolbar transition. There are two toolbars overlapping each other with alpha + translation animations.

https://github.com/K1rakishou/KurobaExLite/blob/c6fcf9ba70b44a8500e607e17b0ac739c3328010/app/src/main/java/com/github/k1rakishou/kurobaexlite/features/home/HomeScreenToolbarContainer.kt#L55

currentPage and targetPage are used as indexes to get the toolbars from a list of toolbars and draw them together and also to figure out which toolbar should be on top of the other (to make it clickable).

andkulikov commented 2 years ago

It is a bit hard to visualise how this component looks like and works from reading a code. Maybe you can share some screenshots of it? Also how do you expect it to work when user swipes from current page to current page + 1? Maybe you can model the needed logic with just currentPage and currentPageOffset? currentPageOffset is a value from -0.5 to 0.5. so If currentPage is 0 and currentPageOffset is > 0 then we are in between currentPage and currentPage + 1 but didn't yet scrolled half of the currentPage. realistically targetPage currently is not that smarter in such conditions. it just returns currentPage+1 if currentPageOffset is > 0 as far as I remember

K1rakishou commented 2 years ago

Currently, I have Pager's (and PagerState) code copied into my project so I can just remove the deprecation and use targetPage as is. However, if in the future you move the Pager into the compose itself then I'm going to have a problem because targetPage is currently using 2 internal fields (animationTargetPage and flingAnimationTarget) to do some calculations (one of them is using the snapper library) so I won't be able to write my own extension function (for example). So that's the problem. If not for those two fields then it would be fine with targetPage getting removed them since I would simply need to copy the targetPage code into my own extension function.

andkulikov commented 2 years ago

Sure, I understand that you can't copy paste this logic as it uses some internals. My question is are you sure you really need those animationTargetPage and flingAnimationTarget for your logic to work good enough? My hypothesis is that it was a mistake by us to start using them and we should not have them. Could you please try to see if there are any use cases when if you do not use flingAnimationTarget it behaves unacceptable so we can discuss possible workarounds for the exact use cases. Thanks

andkulikov commented 2 years ago

There were no such API in ViewPager or ViewPager2 from what I remember. How would you solve that use cases there?

K1rakishou commented 2 years ago

So I tried removing animationTargetPage and flingAnimationTarget from targetPage and it seems to work fine. I guess this should work.

andkulikov commented 2 years ago

Thanks for confirming. It is what I expected