Improve loading of EPUB reflowable resources (#10)

mickael-menu · web-flow · commit 3670f7cf2588 · 2021-10-15T14:34:51.000+02:00
diff --git a/.gitignore b/.gitignore
@@ -52,6 +52,9 @@ captures/
 .idea/modules.xml
 # Comment next line if keeping position of elements in Navigation Editor is relevant for you
 .idea/navEditor.xml
+# Additional IntelliJ files
+.idea/misc.xml
+.idea/deploymentTargetDropDown.xml
 
 # Keystore files
 *.jks
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,9 +6,20 @@ All notable changes to this project will be documented in this file. Take a look
 
 ## [Unreleased]
 
-### Navigator
+### Changed
+
+#### Navigator
+
+* Improve loading of EPUB reflowable resources.
+    * Resources are hidden until fully loaded and positioned.
+    * Intermediary locators are not broadcasted as `currentLocator` anymore while loading a resource.
+    * Improved accuracy when jumping to the middle of a large resource.
+    * `EpubNavigatorFragment.PaginationListener.onPageLoaded()` is now called only a single time, for the currently visible page.
+    * `VisualNavigator.Listener.onTap()` is called even when a resource is not fully loaded.
 
-#### Fixed
+### Fixed
+
+#### Navigator
 
 * `EpubNavigatorFragment`'s `goForward()` and `goBackward()` are now jumping to the previous or next pages instead of resources.
 
diff --git a/readium/navigator/src/main/java/org/readium/r2/navigator/R2BasicWebView.kt b/readium/navigator/src/main/java/org/readium/r2/navigator/R2BasicWebView.kt
@@ -206,7 +206,6 @@ open class R2BasicWebView(context: Context, attrs: AttributeSet) : WebView(conte
         }
     }
 
-
     /**
      * Called from the JS code when a tap is detected.
      * If the JS indicates the tap is being handled within the web view, don't take action,
@@ -416,10 +415,7 @@ open class R2BasicWebView(context: Context, attrs: AttributeSet) : WebView(conte
     }
 
     fun setProperty(key: String, value: String) {
-        runJavaScript("readium.setProperty(\"$key\", \"$value\");") {
-            // Used to redraw highlights when user settings changed.
-            listener.onPageLoaded()
-        }
+        runJavaScript("readium.setProperty(\"$key\", \"$value\");")
     }
 
     fun removeProperty(key: String) {
diff --git a/readium/navigator/src/main/java/org/readium/r2/navigator/epub/EpubNavigatorFragment.kt b/readium/navigator/src/main/java/org/readium/r2/navigator/epub/EpubNavigatorFragment.kt
@@ -47,6 +47,7 @@ import org.readium.r2.shared.publication.epub.EpubLayout
 import org.readium.r2.shared.publication.presentation.presentation
 import org.readium.r2.shared.publication.services.isRestricted
 import org.readium.r2.shared.publication.services.positions
+import org.readium.r2.shared.publication.services.positionsByReadingOrder
 import kotlin.math.ceil
 import kotlin.reflect.KClass
 
@@ -96,6 +97,7 @@ class EpubNavigatorFragment private constructor(
         EpubNavigatorViewModel.createFactory(config.decorationTemplates.copy())
     }
 
+    internal lateinit var positionsByReadingOrder: List<List<Locator>>
     internal lateinit var positions: List<Locator>
     lateinit var resourcePager: R2ViewPager
 
@@ -126,17 +128,19 @@ class EpubNavigatorFragment private constructor(
         _binding = ActivityR2ViewpagerBinding.inflate(inflater, container, false)
         val view = binding.root
 
-        positions = runBlocking { publication.positions() }
+        positionsByReadingOrder = runBlocking { publication.positionsByReadingOrder() }
+        positions = positionsByReadingOrder.flatten()
         publicationIdentifier = publication.metadata.identifier ?: publication.metadata.title
 
         resourcePager = binding.resourcePager
         resourcePager.type = Publication.TYPE.EPUB
 
         if (publication.metadata.presentation.layout == EpubLayout.REFLOWABLE) {
-            resourcesSingle = publication.readingOrder.map { link ->
+            resourcesSingle = publication.readingOrder.mapIndexed { index, link ->
                 PageResource.EpubReflowable(
                     link = link,
-                    url = link.withBaseUrl(baseUrl).href
+                    url = link.withBaseUrl(baseUrl).href,
+                    positionCount = positionsByReadingOrder.getOrNull(index)?.size ?: 0
                 )
             }
 
@@ -258,7 +262,6 @@ class EpubNavigatorFragment private constructor(
     internal var pendingLocator: Locator? = null
 
     override fun go(locator: Locator, animated: Boolean, completion: () -> Unit): Boolean {
-        pendingLocator = locator
 
         val href = locator.href
             // Remove anchor
@@ -288,6 +291,7 @@ class EpubNavigatorFragment private constructor(
         resourcePager.adapter = adapter
 
         if (publication.metadata.presentation.layout == EpubLayout.REFLOWABLE) {
+            pendingLocator = locator
             setCurrent(resourcesSingle)
         } else {
 
@@ -394,6 +398,7 @@ class EpubNavigatorFragment private constructor(
         override fun onPageLoaded() {
             r2Activity?.onPageLoaded()
             paginationListener?.onPageLoaded()
+            notifyCurrentLocation()
         }
 
         override fun onPageChanged(pageIndex: Int, totalPages: Int, url: String) {
@@ -618,6 +623,10 @@ class EpubNavigatorFragment private constructor(
         debounceLocationNotificationJob = launch {
             delay(100L)
 
+            if (pendingLocator != null) {
+                return@launch
+            }
+
             // The transition has stabilized, so we can ask the web view to refresh its current
             // item to reflect the current scroll position.
             currentFragment?.webView?.updateCurrentItem()
diff --git a/readium/navigator/src/main/java/org/readium/r2/navigator/pager/R2EpubPageFragment.kt b/readium/navigator/src/main/java/org/readium/r2/navigator/pager/R2EpubPageFragment.kt
@@ -12,6 +12,7 @@ package org.readium.r2.navigator.pager
 import android.annotation.SuppressLint
 import android.content.Context
 import android.content.SharedPreferences
+import android.graphics.PointF
 import android.os.Bundle
 import android.util.Base64
 import android.util.DisplayMetrics
@@ -23,10 +24,8 @@ import androidx.core.view.ViewCompat
 import androidx.fragment.app.Fragment
 import androidx.lifecycle.lifecycleScope
 import androidx.webkit.WebViewClientCompat
-import kotlinx.coroutines.delay
 import kotlinx.coroutines.flow.collectLatest
 import kotlinx.coroutines.launch
-import org.readium.r2.navigator.Navigator
 import org.readium.r2.navigator.R
 import org.readium.r2.navigator.R2BasicWebView
 import org.readium.r2.navigator.R2WebView
@@ -49,6 +48,9 @@ class R2EpubPageFragment : Fragment() {
     internal val link: Link?
         get() = requireArguments().getParcelable("link")
 
+    private val positionCount: Long
+        get() = requireArguments().getLong("positionCount")
+
     var webView: R2WebView? = null
         private set
 
@@ -58,6 +60,8 @@ class R2EpubPageFragment : Fragment() {
     private var _binding: ViewpagerFragmentEpubBinding? = null
     private val binding get() = _binding!!
 
+    private var isLoading: Boolean = false
+
     @SuppressLint("SetJavaScriptEnabled")
     override fun onCreateView(inflater: LayoutInflater, container: ViewGroup?, savedInstanceState: Bundle?): View? {
         val navigatorFragment = parentFragment as EpubNavigatorFragment
@@ -68,6 +72,7 @@ class R2EpubPageFragment : Fragment() {
         val webView = binding.webView
         this.webView = webView
 
+        webView.visibility = View.INVISIBLE
         webView.navigator = navigatorFragment
         webView.listener = navigatorFragment.webViewListener
         webView.preferences = preferences
@@ -127,71 +132,13 @@ class R2EpubPageFragment : Fragment() {
 
                 webView.listener.onResourceLoaded(link, webView, url)
 
-                val epubNavigator = (webView.navigator as? EpubNavigatorFragment)
-                val currentFragment: R2EpubPageFragment? =
-                    (epubNavigator?.resourcePager?.adapter as? R2PagerAdapter)?.getCurrentFragment() as? R2EpubPageFragment
-
-                if (currentFragment != null && this@R2EpubPageFragment.tag == currentFragment.tag) {
-                    val locator = epubNavigator.pendingLocator
-                    epubNavigator.pendingLocator = null
-                    var locations = locator?.locations
-
-                    // TODO this seems to be needed, will need to test more
-                    if (url != null && url.indexOf("#") > 0) {
-                        val id = url.substringAfterLast("#")
-                        locations = Locator.Locations(fragments = listOf(id))
+                // To make sure the page is properly laid out before jumping to the target locator,
+                // we execute a dummy JavaScript and wait for the callback result.
+                webView.evaluateJavascript("true") {
+                    viewLifecycleOwner.lifecycleScope.launchWhenCreated {
+                        onLoadPage()
                     }
-
-                    val currentWebView = currentFragment.webView
-                    if (currentWebView != null && locations != null) {
-
-                        lifecycleScope.launchWhenStarted {
-                            // FIXME: We need a better way to wait, because if the value is too low it fails
-                            delay(200)
-
-                            val text = locator?.text
-                            if (text?.highlight != null) {
-                                if (currentWebView.scrollToText(text)) {
-                                    return@launchWhenStarted
-                                }
-
-                                // The delay is necessary before falling back on the other
-                                // locations, because scrollToText() is moving the scroll position
-                                // while looking for the text snippet.
-                                delay(100)
-                            }
-
-                            val htmlId = locations.htmlId
-                            if (htmlId != null && currentWebView.scrollToId(htmlId)) {
-                                return@launchWhenStarted
-                            }
-
-                            var progression = locations.progression
-                            if (progression != null) {
-                                // We need to reverse the progression with RTL because the Web View
-                                // always scrolls from left to right, no matter the reading direction.
-                                progression =
-                                    if (webView.scrollMode || navigatorFragment.readingProgression == ReadingProgression.LTR) progression
-                                    else 1 - progression
-
-                                if (webView.scrollMode) {
-                                    currentWebView.scrollToPosition(progression)
-
-                                } else {
-                                    // Figure out the target web view "page" from the requested
-                                    // progression.
-                                    var item = (progression * currentWebView.numPages).roundToInt()
-                                    if (navigatorFragment.readingProgression == ReadingProgression.RTL && item > 0) {
-                                        item -= 1
-                                    }
-                                    currentWebView.setCurrentItem(item, false)
-                                }
-                            }
-                        }
-                    }
-
                 }
-                webView.listener.onPageLoaded()
             }
 
             // prevent favicon.ico to be loaded, this was causing NullPointerException in NanoHttp
@@ -237,10 +184,19 @@ class R2EpubPageFragment : Fragment() {
             false
         }
 
-        resourceUrl?.let { webView.loadUrl(it) }
+        resourceUrl?.let {
+            isLoading = true
+            webView.loadUrl(it)
+        }
 
         setupPadding()
 
+        // Forward a tap event when the web view is not ready to propagate the taps. This allows
+        // to toggle a navigation UI while a page is loading, for example.
+        binding.root.setOnClickListenerWithPoint { _, point ->
+            webView.listener.onTap(point)
+        }
+
         return containerView
     }
 
@@ -312,15 +268,96 @@ class R2EpubPageFragment : Fragment() {
     internal val paddingTop: Int get() = containerView.paddingTop
     internal val paddingBottom: Int get() = containerView.paddingBottom
 
+    private val isCurrentResource: Boolean get() {
+        val epubNavigator = webView?.navigator as? EpubNavigatorFragment ?: return false
+        val currentFragment = (epubNavigator.resourcePager.adapter as? R2PagerAdapter)?.getCurrentFragment() as? R2EpubPageFragment ?: return false
+        return tag == currentFragment.tag
+    }
+
+    private suspend fun onLoadPage() {
+        if (!isLoading) return
+        isLoading = false
+
+        val webView = requireNotNull(webView)
+        webView.visibility = View.VISIBLE
+
+        if (isCurrentResource) {
+            val epubNavigator = requireNotNull(webView.navigator as? EpubNavigatorFragment)
+            val locator = epubNavigator.pendingLocator
+            epubNavigator.pendingLocator = null
+            if (locator != null) {
+                loadLocator(locator)
+            }
+
+            webView.listener.onPageLoaded()
+        }
+    }
+
+    private suspend fun loadLocator(locator: Locator) {
+        val webView = requireNotNull(webView)
+        val epubNavigator = requireNotNull(webView.navigator as? EpubNavigatorFragment)
+
+        val text = locator.text
+        if (text.highlight != null) {
+            if (webView.scrollToText(text)) {
+                return
+            }
+        }
+
+        val htmlId = locator.locations.htmlId
+        if (htmlId != null && webView.scrollToId(htmlId)) {
+            return
+        }
+
+        var progression = locator.locations.progression
+        if (progression != null) {
+            // We need to reverse the progression with RTL because the Web View
+            // always scrolls from left to right, no matter the reading direction.
+            progression =
+                if (webView.scrollMode || epubNavigator.readingProgression == ReadingProgression.LTR) progression
+                else 1 - progression
+
+            if (webView.scrollMode) {
+                webView.scrollToPosition(progression)
+
+            } else {
+                // Figure out the target web view "page" from the requested
+                // progression.
+                var item = (progression * webView.numPages).roundToInt()
+                if (epubNavigator.readingProgression == ReadingProgression.RTL && item > 0) {
+                    item -= 1
+                }
+                webView.setCurrentItem(item, false)
+            }
+        }
+    }
+
     companion object {
-        fun newInstance(url: String, link: Link? = null): R2EpubPageFragment =
+        fun newInstance(url: String, link: Link? = null, positionCount: Int = 0): R2EpubPageFragment =
             R2EpubPageFragment().apply {
                 arguments = Bundle().apply {
                     putString("url", url)
                     putParcelable("link", link)
+                    putLong("positionCount", positionCount.toLong())
                 }
             }
     }
 }
 
+/**
+ * Same as setOnClickListener, but will also report the tap point in the view.
+ */
+private fun View.setOnClickListenerWithPoint(action: (View, PointF) -> Unit) {
+    var point = PointF()
 
+    setOnTouchListener { v, event ->
+        if (event.action == MotionEvent.ACTION_DOWN) {
+            point = PointF(event.x, event.y)
+        }
+        false
+    }
+
+    setOnClickListener {
+        action(it, point)
+    }
+}
diff --git a/readium/navigator/src/main/java/org/readium/r2/navigator/pager/R2PagerAdapter.kt b/readium/navigator/src/main/java/org/readium/r2/navigator/pager/R2PagerAdapter.kt
@@ -19,7 +19,7 @@ import org.readium.r2.shared.publication.Link
 class R2PagerAdapter internal constructor(val fm: FragmentManager, private val resources: List<PageResource>) : R2FragmentPagerAdapter(fm) {
 
     internal sealed class PageResource {
-        data class EpubReflowable(val link: Link, val url: String) : PageResource()
+        data class EpubReflowable(val link: Link, val url: String, val positionCount: Int) : PageResource()
         data class EpubFxl(val url1: String, val url2: String? = null) : PageResource()
         data class Cbz(val link: Link) : PageResource()
     }
@@ -52,7 +52,7 @@ class R2PagerAdapter internal constructor(val fm: FragmentManager, private val r
     override fun getItem(position: Int): Fragment =
         when (val resource = resources[position]) {
             is PageResource.EpubReflowable -> {
-                R2EpubPageFragment.newInstance(resource.url, resource.link)
+                R2EpubPageFragment.newInstance(resource.url, resource.link, positionCount = resource.positionCount)
             }
             is PageResource.EpubFxl -> {
                 R2FXLPageFragment.newInstance(resource.url1, resource.url2)

Original file line number	Diff line number	Diff line change
`@@ -206,7 +206,6 @@ open class R2BasicWebView(context: Context, attrs: AttributeSet) : WebView(conte`
`206`	`206`	`}`
`207`	`207`	`}`
`208`	`208`
`209`		`-`
`210`	`209`	`/**`
`211`	`210`	`* Called from the JS code when a tap is detected.`
`212`	`211`	`* If the JS indicates the tap is being handled within the web view, don't take action,`
`@@ -416,10 +415,7 @@ open class R2BasicWebView(context: Context, attrs: AttributeSet) : WebView(conte`
`416`	`415`	`}`
`417`	`416`
`418`	`417`	`fun setProperty(key: String, value: String) {`
`419`		`- runJavaScript("readium.setProperty(\"$key\", \"$value\");") {`
`420`		`- // Used to redraw highlights when user settings changed.`
`421`		`- listener.onPageLoaded()`
`422`		`- }`
	`418`	`+ runJavaScript("readium.setProperty(\"$key\", \"$value\");")`
`423`	`419`	`}`
`424`	`420`
`425`	`421`	`fun removeProperty(key: String) {`