MIgrate to ViewPager2 #157

stevenzeck · 2020-08-12T23:52:19Z

Closes readium/kotlin-toolkit#148. This is a draft PR. It should also be merged at the same time as readium/r2-testapp-kotlin#349.

stevenzeck · 2020-08-13T04:35:30Z

r2-navigator/src/main/java/org/readium/r2/navigator/pager/R2PagerAdapter.kt

 import org.readium.r2.shared.publication.Link
 import org.readium.r2.shared.publication.Publication


-class R2PagerAdapter(val fm: FragmentManager, private val resources: List<Any>, private val title: String, private val type: Publication.TYPE, private val publicationPath: String = "") : R2FragmentPagerAdapter(fm) {
+class R2PagerAdapter(val fm: FragmentManager, val l: Lifecycle, private val resources: List<Any>, private val title: String, private val type: Publication.TYPE, private val publicationPath: String = "") : FragmentStateAdapter(fm, l) {

    private var currentFragment: Fragment? = null


Really want to centralize this. There are instances of currentFragment in several files, usually with quite a bit of casting to retrieve it. Just declare the currentFragment property and access the getter and setter to manage it.

stevenzeck · 2020-08-19T00:35:10Z

@mickael-menu @aferditamuriqi I've tracked down the RTL issues to two things.

Between R2BasicWebView and EpubNavigatorFragment, the scrollLeft/scrollRight and goForward/goBackward functions only really support LTR since scrollLeft calls goBackward, which always sets the ViewPager's currentItem -1. For RTL it should be doing +1
currentFragment?.activity?.layoutDirectionIsRTL() is returning false for RTL content. Not sure exactly what this is doing behind the scenes: ViewCompat.getLayoutDirection(findViewById(android.R.id.content))

I want to try and keep the function names sensible while making the necessary fixes. So I'm thinking when scrollLeft is called, it will check if it's RTL vs LTR and call goForward and goBackward respectively. For the latter two functions, remove the currentFragment?.activity?.layoutDirectionIsRTL() check and rely on publication.contentLayout.readingProgression == ReadingProgression.RTL instead (or even resourcePager.layoutDirection). And do the opposite when scrollRight is called.

mickael-menu · 2020-08-19T10:42:22Z

I want to try and keep the function names sensible while making the necessary fixes. So I'm thinking when scrollLeft is called, it will check if it's RTL vs LTR and call goForward and goBackward respectively. For the latter two functions, remove the currentFragment?.activity?.layoutDirectionIsRTL() check and rely on publication.contentLayout.readingProgression == ReadingProgression.RTL instead (or even resourcePager.layoutDirection). And do the opposite when scrollRight is called.

Sounds about right. 👍

You could also expose goLeft() and goRight() in R2BasicWebView.Listener:

fun goLeft(animated: Boolean = false, completion: () -> Unit = {}): Boolean
fun goRight(animated: Boolean = false, completion: () -> Unit = {}): Boolean

There's a default implementation in VisualNavigator, that EpubNavigatorFragment extends, which already perform the logic:

r2-navigator-kotlin/r2-navigator/src/main/java/org/readium/r2/navigator/IR2Activity.kt

Lines 89 to 107 in 71c4937

    
           fun VisualNavigator.goLeft(animated: Boolean = false, completion: () -> Unit = {}): Boolean { 
        
               return when (readingProgression) { 
        
                   ReadingProgression.LTR, ReadingProgression.TTB, ReadingProgression.AUTO -> 
        
                       goBackward(animated = animated, completion = completion) 
        
                   ReadingProgression.RTL, ReadingProgression.BTT -> 
        
                       goForward(animated = animated, completion = completion) 
        
               } 
        
           } 
        
           fun VisualNavigator.goRight(animated: Boolean = false, completion: () -> Unit = {}): Boolean { 
        
               return when (readingProgression) { 
        
                   ReadingProgression.LTR, ReadingProgression.TTB, ReadingProgression.AUTO -> 
        
                       goForward(animated = animated, completion = completion) 
        
                   ReadingProgression.RTL, ReadingProgression.BTT -> 
        
                       goBackward(animated = animated, completion = completion) 
        
               } 
        
           }

…the getCurrentFragment calls

stevenzeck · 2020-08-20T02:51:56Z

@mickael-menu I changed the navigator in R2BasicWebView to use VisualNavigator instead of Navigator and did what you suggested, works good. Finding three issues right now:

Fixed format books aren't paging (other than ToC). That's a big problem, but probably a stupid mistake. Guessing it's related to isUserInputEnabled
Scroll mode isn't being applied to webviews other than the current fragment. I haven't dug a whole lot into how the CSS is updated from the native code, but --USER__scroll: readium-scroll- isn't being set to on/off properly in other fragments' webviews other than the one currently open. I don't want to switch branches now, but I'm guessing the current viewPager only manages the previousFragment, currentFragment and nextFragment? I don't think ViewPager2 works that way, which is probably part of the problem.
A very minor one. In RTL content, when paging left between resources it quickly jumps to the beginning of the current resource just before it goes to the next one. The opposite happens when paging right (it goes to the end of the current resource before it goes to the previous one). I don't know why but it's bothering me.

… to first page of resource in RTL layouts

mickael-menu · 2020-09-17T06:35:50Z

r2-navigator/src/main/java/org/readium/r2/navigator/R2BasicWebView.kt

+                // This is a temporary fix to prevent the webview from snapping to the beginning
+                // of the resource when paging between them in RTL layouts
+                if (navigator.readingProgression == ReadingProgression.LTR) {
+                    this@R2BasicWebView.evaluateJavascript("scrollLeft();", null)
                }


@stevenzeck What did you mean by temporary? Do you have any idea on how to fix this in a more permanent way?

It was temporary in that I was hoping there was a different problem, but I couldn't find one. I'll remove that part of the comment.

mickael-menu · 2020-09-17T06:38:06Z

r2-navigator/src/main/java/org/readium/r2/navigator/epub/EpubNavigatorFragment.kt

@@ -117,33 +117,33 @@ class EpubNavigatorFragment(


        if (publication.metadata.presentation.layout == EpubLayout.REFLOWABLE) {
-            adapter = R2PagerAdapter(supportFragmentManager, resourcesSingle, publication.metadata.title, Publication.TYPE.EPUB)
-            resourcePager.type = Publication.TYPE.EPUB
+            resourcePager.isUserInputEnabled = false


@stevenzeck Why did you need to switch off isUserInputEnabled?

This is the equivalent of onInterceptTouchEvent/onTouchEvent from the original ViewPager. We have to prevent the ViewPager from paging via user input and do it ourselves, otherwise it will go from resource to resource instead of page to page.

Okay, I guess this is the reason FXL EPUB can't be interacted with. Since they are not paginated, maybe this can be done selectively only for reflowable. However I wonder why links are working in reflowable EPUBs despite isUserInputEnabled?

mickael-menu · 2020-09-17T06:44:19Z

r2-navigator/src/main/java/org/readium/r2/navigator/epub/EpubNavigatorFragment.kt

+//        resourcePager.direction = publication.contentLayout.readingProgression
+        resourcePager.layoutDirection = View.LAYOUT_DIRECTION_LTR

        if (publication.cssStyle == ReadingProgression.RTL.value) {
-            resourcePager.direction = ReadingProgression.RTL
+            resourcePager.layoutDirection = View.LAYOUT_DIRECTION_RTL
        }


publication.cssStyle will soon be deprecated, so I think we can rely exclusively on publication.contentLayout.readingProgression for now. Something along the line:

resourcePager.layoutDirection = when (publication.contentLayout.readingProgression) { ReadingProgression.RTL -> View.Layout_DIRECTION_RTL else -> View.Layout_DIRECTION_LTR }

I'm not yet sure if TTB and BTT will need to be handled differently with the ViewPager2. But it's not used yet anyway.

Yeah, I was wondering what TTB and BTT are exactly and how they would factor into this. Will make the change you suggested.

It's for vertical reading progression: top-to-bottom and bottom-to-top.

I wonder if we could not handle them with https://developer.android.com/reference/kotlin/androidx/viewpager2/widget/ViewPager2#setorientation And then combining it with Layout_DIRECTION_RTL, maybe it would be equivalent to BTT.

mickael-menu · 2020-09-17T06:55:52Z

r2-navigator/src/main/java/org/readium/r2/navigator/pager/R2PagerAdapter.kt

 import org.readium.r2.shared.publication.Link
 import org.readium.r2.shared.publication.Publication


-class R2PagerAdapter(val fm: FragmentManager, private val resources: List<Any>, private val title: String, private val type: Publication.TYPE, private val publicationPath: String = "") : R2FragmentPagerAdapter(fm) {


@stevenzeck We can remove R2FragmentPagerAdapter.kt since it's not used anymore.

mickael-menu · 2020-09-17T06:56:25Z

r2-navigator/src/main/java/org/readium/r2/navigator/pager/R2PagerAdapter.kt

+//    private var currentFragment: Fragment? = null
+//    private var previousFragment: Fragment? = null
+//    private var nextFragment: Fragment? = null
+//
+//    fun getCurrentFragment(): Fragment? {
+//        return currentFragment
+//    }
+//
+//    fun getPreviousFragment(): Fragment? {
+//        return previousFragment
+//    }
+//
+//    fun getNextFragment(): Fragment? {
+//        return nextFragment
+//    }


@stevenzeck Could you remove these as well?

mickael-menu · 2020-09-17T07:55:11Z

A few issues I noticed during my tests:

If the current resource is not yet loaded, we can swipe quickly through several resources. I'm not sure it's caused by this PR but I never noticed it before. Generally I think we should prevent user swiping while the current resource is loading.
FXL EPUB can't be interacted with (text selection, or links). There are some links in BellaOriginal3.epub.
CBZ can't be zoomed in. Probably related to the aforementioned FXL issue.
There's a sliding animation when restoring the last location in a CBZ. Maybe there's a way to disable animation with ViewPager2 when jumping to the initial locator?
The last read position in the middle of a reflowable resource is not restored after reopening the book. Sometimes it's acting weirdly by suddenly jumping to the location after I touch the page. But most of the time it stays on the first page.
The app is crashing when rotating the screen with this stacktrace:

Caused by: android.view.InflateException: Binary XML file line readium/r2-navigator-kotlin#19 in org.readium.r2reader:layout/activity_r2_epub: Binary XML file line readium/r2-navigator-kotlin#19 in org.readium.r2reader:layout/activity_r2_epub: Error inflating class fragment
     Caused by: android.view.InflateException: Binary XML file line readium/r2-navigator-kotlin#19 in org.readium.r2reader:layout/activity_r2_epub: Error inflating class fragment
     Caused by: kotlin.UninitializedPropertyAccessException: lateinit property webView has not been initialized
        at org.readium.r2.navigator.pager.R2EpubPageFragment.getWebView(R2EpubPageFragment.kt:44)
        at org.readium.r2.navigator.epub.EpubNavigatorFragment$onCreateView$2.onPageSelected(EpubNavigatorFragment.kt:163)
        at androidx.viewpager2.widget.CompositeOnPageChangeCallback.onPageSelected(CompositeOnPageChangeCallback.java:73)
        at androidx.viewpager2.widget.CompositeOnPageChangeCallback.onPageSelected(CompositeOnPageChangeCallback.java:73)
        at androidx.viewpager2.widget.ScrollEventAdapter.dispatchSelected(ScrollEventAdapter.java:432)
        at androidx.viewpager2.widget.ScrollEventAdapter.notifyProgrammaticScroll(ScrollEventAdapter.java:320)
        at androidx.viewpager2.widget.ViewPager2.setCurrentItemInternal(ViewPager2.java:652)
        at androidx.viewpager2.widget.ViewPager2.setCurrentItem(ViewPager2.java:607)
        at androidx.viewpager2.widget.ViewPager2.setCurrentItem(ViewPager2.java:591)
        at org.readium.r2.navigator.epub.EpubNavigatorFragment$go$1.invoke(EpubNavigatorFragment.kt:234)
        at org.readium.r2.navigator.epub.EpubNavigatorFragment.go(EpubNavigatorFragment.kt:251)
        at org.readium.r2.navigator.Navigator$DefaultImpls.go$default(IR2Activity.kt:59)
        at org.readium.r2.navigator.epub.EpubNavigatorFragment.onCreateView(EpubNavigatorFragment.kt:179)

Apart from that, the paging across resources itself seems to be working pretty well. Thanks again @stevenzeck 👍

I'll do more exhaustive testing once we fix any pending issue and raised question.

mickael-menu · 2020-09-17T08:28:32Z

r2-navigator/src/main/java/org/readium/r2/navigator/epub/EpubNavigatorFragment.kt

    private val r2PagerAdapter: R2PagerAdapter
        get() = resourcePager.adapter as R2PagerAdapter

-    private val currentFragment: R2EpubPageFragment? get() =
-        r2PagerAdapter.mFragments.get(r2PagerAdapter.getItemId(resourcePager.currentItem)) as? R2EpubPageFragment
+    val currentFragment: R2EpubPageFragment? get() =


@stevenzeck Could you set currentFragment as internal to keep the internals of EpubNavigatorFragment private?

stevenzeck · 2020-09-19T17:17:08Z

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.

stevenzeck · 2020-09-19T17:51:09Z

If the current resource is not yet loaded, we can swipe quickly through several resources. I'm not sure it's caused by this PR but I never noticed it before. Generally I think we should prevent user swiping while the current resource is loading.

We think this is due to a newly imported book not being cached yet.

FXL EPUB can't be interacted with (text selection, or links). There are some links in BellaOriginal3.epub.

I can recreate this on the alpha branch as well. I can look into.

CBZ can't be zoomed in. Probably related to the aforementioned FXL issue.

I'm able to zoom in and out of the CBZ in the testapp (Cory Doctorow's). What content and device does it not work in?

There's a sliding animation when restoring the last location in a CBZ. Maybe there's a way to disable animation with ViewPager2 when jumping to the initial locator?

I can recreate this. Will look into. It's also jumping back an old position if you rotate the screen.

The last read position in the middle of a reflowable resource is not restored after reopening the book. Sometimes it's acting weirdly by suddenly jumping to the location after I touch the page. But most of the time it stays on the first page.

A couple of things:

Looks like when the book is opened, the fragment where the LRP is located is not in the resourcePager FragmentManager. So the progression in the notifyCurrentLocation function in EpubNavigatorFragment is always 0.0.
The fragments themselves are not in the resourcePager when go is called with the initialLocator in EpubNavigatorFragment. Trying to figure out when and where they are loaded (thinking the streamer has something to do with it), and I suspect there is old code that still tries to reference a "currentFragment" indirectly, or assumes that there is a current fragment set somewhere.

For this to work properly, I think it needs to get away from all of that logic. Let the ViewPager and adapter handle all of that.

The app is crashing when rotating the screen

I'm getting that stack trace for reflowables as well. The activity is recreated when orientation changes, and for some reason the webview isn't being initialized somewhere during that transition. I'm also sporadically seeing an error related to FragmentManager is already executing transactions.

mickael-menu · 2020-09-23T10:15:08Z

I'm able to zoom in and out of the CBZ in the testapp (Cory Doctorow's). What content and device does it not work in?

I'm using the same book, on a Pixel 3a. After further tests, actually I noticed that I can zoom in/out after double taping on the page, but it's not very smooth. It looks like there's a conflict between the swipe gesture and the pinch one.

Looks like when the book is opened, the fragment where the LRP is located is not in the resourcePager FragmentManager. So the progression in the notifyCurrentLocation function in EpubNavigatorFragment is always 0.0.

We have similar issue on iOS and I thought about preventing the initial currentLocator notification until the resource is properly loaded.

stevenzeck · 2020-09-23T21:52:44Z

I'm able to do pinch and double tab for zooming in and out using my Pixel C. I'll try it on my Pixel 3 too.

For the locator issue, there's a lot of code in the navigator where things are done out of order and done several times. Ultimately we need to:

Load the necessary fragments (or views if we decide to do that instead down the line) based on the current location. I say based on the current location because the FragmentManager won't load all fragments, just the ones it thinks it needs. So if it is loading chapter 10, it will only pull fragments 8, 9, 10, 11 and 12 (I'm making these numbers up, but gives you the idea).
For each fragment or view, load the resource in the webview
Go to the current location. Tell the ViewPager to open the correct fragment/view and tell the webview to scroll to the correct position.

mickael-menu · 2020-09-24T06:57:01Z

Sounds like a good plan. I don't know how it is implemented currently in the EPUB navigator, how does it differ? And do you think it would be possible to implement it with the current R2ViewPager, and then merge it into the ViewPager2 PR?

It could be useful also to reify a state machine to have a better control/view on the state of the navigator.

mickael-menu · 2021-08-11T16:02:45Z

Closing after the group decision in today's Agenda. The PR is quite obsolete now and we figured using ViewPager2 would be too limiting. For example, we can't implement the "continuous scrolling" feature with it since it's always working as a paginated view.

As an alternative, we should be able to use a RecyclerView with a PagerSnapHelper.

Start of migration to ViewPager2

a260170

stevenzeck mentioned this pull request Aug 12, 2020

Migrate to ViewPager2 readium/r2-testapp-kotlin#349

Closed

stevenzeck marked this pull request as draft August 12, 2020 23:54

stevenzeck commented Aug 13, 2020

View reviewed changes

Fix RTL paging. Remove commented out code. Some condensing of all of …

bd5524c

…the getCurrentFragment calls

stevenzeck added 2 commits August 19, 2020 21:56

Fix fixed format book paging

9141e10

Make suggested changes in webview listener, fix scroll mode, fix snap…

91306cc

… to first page of resource in RTL layouts

stevenzeck marked this pull request as ready for review September 10, 2020 01:25

mickael-menu reviewed Sep 17, 2020

View reviewed changes

Merge branch 'alpha' into viewpager2

237e08f

mickael-menu reviewed Sep 17, 2020

View reviewed changes

Merge branch 'alpha' into viewpager2

29a50b0

mickael-menu changed the base branch from alpha to develop February 24, 2021 08:08

mickael-menu closed this Aug 11, 2021

mickael-menu mentioned this pull request Dec 20, 2022

Full vertical scroll (or less internal components) readium/kotlin-toolkit#313

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MIgrate to ViewPager2 #157

MIgrate to ViewPager2 #157

stevenzeck commented Aug 12, 2020 •

edited

Loading

stevenzeck Aug 13, 2020

stevenzeck commented Aug 19, 2020 •

edited

Loading

mickael-menu commented Aug 19, 2020

stevenzeck commented Aug 20, 2020

mickael-menu Sep 17, 2020

stevenzeck Sep 17, 2020

mickael-menu Sep 17, 2020

stevenzeck Sep 17, 2020

mickael-menu Sep 18, 2020

mickael-menu Sep 17, 2020

stevenzeck Sep 17, 2020

mickael-menu Sep 18, 2020

mickael-menu Sep 17, 2020

stevenzeck Sep 17, 2020

mickael-menu Sep 17, 2020

stevenzeck Sep 17, 2020

mickael-menu commented Sep 17, 2020

mickael-menu Sep 17, 2020

stevenzeck Sep 18, 2020

stevenzeck commented Sep 19, 2020

stevenzeck commented Sep 19, 2020 •

edited

Loading

mickael-menu commented Sep 23, 2020

stevenzeck commented Sep 23, 2020 •

edited

Loading

mickael-menu commented Sep 24, 2020

mickael-menu commented Aug 11, 2021

MIgrate to ViewPager2 #157

MIgrate to ViewPager2 #157

Conversation

stevenzeck commented Aug 12, 2020 • edited Loading

Choose a reason for hiding this comment

stevenzeck commented Aug 19, 2020 • edited Loading

mickael-menu commented Aug 19, 2020

stevenzeck commented Aug 20, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickael-menu commented Sep 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenzeck commented Sep 19, 2020

stevenzeck commented Sep 19, 2020 • edited Loading

mickael-menu commented Sep 23, 2020

stevenzeck commented Sep 23, 2020 • edited Loading

mickael-menu commented Sep 24, 2020

mickael-menu commented Aug 11, 2021

stevenzeck commented Aug 12, 2020 •

edited

Loading

stevenzeck commented Aug 19, 2020 •

edited

Loading

stevenzeck commented Sep 19, 2020 •

edited

Loading

stevenzeck commented Sep 23, 2020 •

edited

Loading