Migrate to ViewPager2 #349
Conversation
|
Does the viewpager2 now support RTL ? When I looked at this topic last it didn't support it and it wasn't as open as viewpager to add RTL. |
|
I have to look into my notes what the issues were. The "standard" RTL is does support, but I do remember there were some issues that couldn't be resolved. RTL and swiping issues come to mind. But I'll update here once I find my notes. |
|
Let me know what you find out. Do you know of any free epubs out there that are RTL to test with? |
|
@aferditamuriqi, @mickael-menu provided me a few sample RTL epubs to test with. I think most of the issues are going to be in the navigator rather than here in the testapp. |
…the getCurrentFragment calls
… to first page of resource in RTL layouts
There was a problem hiding this comment.
Overall I think these changes are breaking too much non-obvious (deprecation warnings) stuff in the test app.
Unfortunately at the moment the test app is too tightly coupled to the EPUB navigator internals. This makes that kind of refactoring painful since it breaks reading apps. Down the line I intend to deprecate this usage and hide the view pager from the reading app, but until then I'd like to keep things backward-compatible to avoid too many breaking migrations.
For this particular PR, there are two things that needs to be fixed to be backward-compatible:
R2PagerAdapterneeds to have the old API back (mFragments,getItemId(), etc.), which can internally adapt the calls to theViewPager2API.R2ViewPagermust still be used, we can handle this in different ways:typealias R2ViewPager = ViewPager2. Certainly the easiest, but it might be breaking the oldR2ViewPagerAPI which could be used by reading apps but not the test app. Also we're exposing the whole ViewPager2 API, which might come back to bite us later.- Since we can't extend
ViewPager2, we could embed it privately in a newR2ViewPagerby composition. This is the best solution IMHO becauseViewPager2stays private and therefore we could always switch to another implementation later. We could even potentially allow switching dynamically between ViewPager 1 and 2.
| Handler().postDelayed({ | ||
| if (publication.metadata.presentation.layout == EpubLayout.REFLOWABLE) { | ||
| val currentFragment = (resourcePager.adapter as R2PagerAdapter).getCurrentFragment() as R2EpubPageFragment | ||
| // val currentFragment = (resourcePager.adapter as R2PagerAdapter).getCurrentFragment() as R2EpubPageFragment |
There was a problem hiding this comment.
@stevenzeck To minimize breaking changes, we can add back a getCurrentFragment() API in R2PagerAdapter which will do fm.findFragmentByTag("f${resourcePager.currentItem}")
| // val currentFragment = ((resourcePager.adapter as R2PagerAdapter).mFragments.get((resourcePager.adapter as R2PagerAdapter).getItemId(resourcePager.currentItem))) as? R2EpubPageFragment | ||
| val currentFragment = (resourcePager.adapter as R2PagerAdapter).fm.findFragmentByTag("f${resourcePager.currentItem}") as? R2EpubPageFragment |
There was a problem hiding this comment.
@stevenzeck Same thing, maybe there's a way to have mFragments and getItemId in the new R2PagerAdapter to avoid breaking this?
| tools:context="org.readium.r2.navigator.epub.R2EpubActivity"> | ||
|
|
||
| <org.readium.r2.navigator.pager.R2ViewPager | ||
| <androidx.viewpager2.widget.ViewPager2 |
There was a problem hiding this comment.
@stevenzeck We should look for a way to have R2ViewPager work as well. I'll put more details in the general review comment.
|
After talking with @mickael-menu in Slack, I think it's better to hold off on this PR until all of the old R2ViewPager code is deprecated (mFragments, getCurrentFragment, etc). I will continue making a few updates based on feedback and fixing the issues mentioned, and will keep the branch up to date as best as I can. |
|
any updates on this @stevenzeck ? |
|
@mrifni to be honest I haven't looked at this for a while. I want to avoid having to support things in the current Readium APIs that aren't easily supported with ViewPager2. I don't remember what those APIs are anymore, but remember talking to Mickael about them. I'm currently rewriting the testapp to use fragments instead of activities, moving off of Anko for the UI, using Room DB, etc. |
|
@mrifni Some relevant discussion: https://github.com/readium/r2-testapp-kotlin/discussions/394 |
|
Closing after the group decision in today's Agenda. The PR is quite obsolete now and we figured using As an alternative, we should be able to use a |
Draft PR. This should be merged at the same time as readium/r2-navigator-kotlin#157.