Deprecate Link.toLocator() in favor of Publication.locatorFromLink() (#83)

mickael-menu · web-flow · commit 251d7cafbd69 · 2022-01-26T11:53:59.000+01:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -43,6 +43,8 @@ All notable changes to this project will be documented in this file. Take a look
 
 * `Publication.type` is now deprecated in favor of the new `Publication.conformsTo()` API which is more accurate.
     * For example, replace `publication.type == Publication.TYPE.EPUB` with `publication.conformsTo(Publication.Profile.EPUB)` before opening a publication with the `EpubNavigatorFragment`.
+* `Link.toLocator()` is deprecated as it may create an incorrect `Locator` if the link `type` is missing.
+    * Use `publication.locatorFromLink()` instead.
 
 ### Fixed
 
diff --git a/readium/navigator-media2/src/main/java/org/readium/navigator/media2/MediaNavigator.kt b/readium/navigator-media2/src/main/java/org/readium/navigator/media2/MediaNavigator.kt
@@ -82,7 +82,8 @@ class MediaNavigator private constructor(
             if (itemStartPosition == null) null
             else totalDuration?.let { (itemStartPosition + position) / it }
 
-        return link.toLocator().copyWithLocations(
+        val locator = requireNotNull(publication.locatorFromLink(link))
+        return locator.copyWithLocations(
             fragments = listOf("t=${position.inWholeSeconds}"),
             progression = item.duration?.let { position / it },
             totalProgression = totalProgression
@@ -164,7 +165,7 @@ class MediaNavigator private constructor(
      */
     suspend fun go(locator: Locator): Try<Unit, Exception> {
         val itemIndex = publication.readingOrder.indexOfFirstWithHref(locator.href)
-            ?: return Try.failure(Exception.IllegalArgument("Invalid href ${locator.href}."))
+            ?: return Try.failure(Exception.InvalidArgument("Invalid href ${locator.href}."))
         val position = locator.locations.time ?: Duration.ZERO
         Timber.v("Go to locator $locator")
         return seek(itemIndex, position)
@@ -173,8 +174,11 @@ class MediaNavigator private constructor(
     /**
      * Seeks to the beginning of the given link.
      */
-    suspend fun go(link: Link) =
-        go(link.toLocator())
+    suspend fun go(link: Link): Try<Unit, Exception> {
+        val locator = publication.locatorFromLink(link)
+            ?: return Try.failure(Exception.InvalidArgument("Resource not found at ${link.href}"))
+        return go(locator)
+    }
 
     /**
      * Skips to a little amount of time later.
@@ -274,16 +278,13 @@ class MediaNavigator private constructor(
         Ongoing
     }
 
-    sealed class Exception : kotlin.Exception() {
+    sealed class Exception(override val message: String) : kotlin.Exception(message) {
 
         class SessionPlayer internal constructor(
-            internal val error: SessionPlayerError,
-            override val message: String = "${error.name} error occurred in SessionPlayer."
-        ) : Exception()
+            internal val error: SessionPlayerError
+        ) : Exception("${error.name} error occurred in SessionPlayer.")
 
-        class IllegalArgument internal constructor(
-            override val message: String
-        ): Exception()
+        class InvalidArgument(message: String): Exception(message)
     }
 
     /*
diff --git a/readium/navigator/src/main/java/org/readium/r2/navigator/audiobook/R2AudiobookActivity.kt b/readium/navigator/src/main/java/org/readium/r2/navigator/audiobook/R2AudiobookActivity.kt
@@ -78,7 +78,8 @@ open class R2AudiobookActivity : AppCompatActivity(), CoroutineScope, IR2Activit
     }
 
     override fun go(link: Link, animated: Boolean, completion: () -> Unit): Boolean {
-        TODO("not implemented") //To change body of created functions use File | Settings | File Templates.
+        val locator = publication.locatorFromLink(link) ?: return false
+        return go(locator)
     }
 
     override fun goForward(animated: Boolean, completion: () -> Unit): Boolean {
@@ -156,7 +157,7 @@ open class R2AudiobookActivity : AppCompatActivity(), CoroutineScope, IR2Activit
         if (this.lifecycle.currentState.isAtLeast(Lifecycle.State.RESUMED)) {
 
             if (!loadedInitialLocator) {
-                go(publication.readingOrder.first().toLocator())
+                go(publication.readingOrder.first())
             }
 
             binding.seekBar.setOnSeekBarChangeListener(object : SeekBar.OnSeekBarChangeListener {
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
@@ -327,7 +327,8 @@ class EpubNavigatorFragment private constructor(
     }
 
     override fun go(link: Link, animated: Boolean, completion: () -> Unit): Boolean {
-        return go(link.toLocator(), animated, completion)
+        val locator = publication.locatorFromLink(link) ?: return false
+        return go(locator, animated, completion)
     }
 
     private fun run(commands: List<RunScriptCommand>) {
@@ -638,7 +639,9 @@ class EpubNavigatorFragment private constructor(
         get() = publication.metadata.effectiveReadingProgression
 
     override val currentLocator: StateFlow<Locator> get() = _currentLocator
-    private val _currentLocator = MutableStateFlow(initialLocator ?: publication.readingOrder.first().toLocator())
+    private val _currentLocator = MutableStateFlow(initialLocator
+        ?: requireNotNull(publication.locatorFromLink(publication.readingOrder.first()))
+    )
 
     /**
      * While scrolling we receive a lot of new current locations, so we use a coroutine job to
diff --git a/readium/navigator/src/main/java/org/readium/r2/navigator/image/ImageNavigatorFragment.kt b/readium/navigator/src/main/java/org/readium/r2/navigator/image/ImageNavigatorFragment.kt
@@ -60,7 +60,9 @@ class ImageNavigatorFragment private constructor(
     private lateinit var currentActivity: FragmentActivity
 
     override val currentLocator: StateFlow<Locator> get() = _currentLocator
-    private val _currentLocator = MutableStateFlow(initialLocator ?: publication.readingOrder.first().toLocator())
+    private val _currentLocator = MutableStateFlow(initialLocator
+        ?: requireNotNull(publication.locatorFromLink(publication.readingOrder.first()))
+    )
 
     internal var currentPagerPosition: Int = 0
     internal var resources: List<String> = emptyList()
@@ -164,8 +166,10 @@ class ImageNavigatorFragment private constructor(
         return true
     }
 
-    override fun go(link: Link, animated: Boolean, completion: () -> Unit): Boolean =
-        go(link.toLocator(), animated, completion)
+    override fun go(link: Link, animated: Boolean, completion: () -> Unit): Boolean {
+        val locator = publication.locatorFromLink(link) ?: return false
+        return go(locator, animated, completion)
+    }
 
     override fun goForward(animated: Boolean, completion: () -> Unit): Boolean {
         val current = resourcePager.currentItem
diff --git a/readium/navigator/src/main/java/org/readium/r2/navigator/media/MediaService.kt b/readium/navigator/src/main/java/org/readium/r2/navigator/media/MediaService.kt
@@ -139,7 +139,9 @@ open class MediaService : MediaBrowserServiceCompat(), CoroutineScope by MainSco
             }
 
             val locator = (extras?.getParcelable(EXTRA_LOCATOR) as? Locator)
-                ?: href?.let { navigator.publication.linkWithHref(it)?.toLocator() }
+                ?: href
+                    ?.let { navigator.publication.linkWithHref(it) }
+                    ?.let { navigator.publication.locatorFromLink(it) }
 
             if (locator != null && href != null && locator.href != href) {
                 Timber.e("Ambiguous playback location provided. HREF `$href` doesn't match locator $locator.")
@@ -310,7 +312,12 @@ open class MediaService : MediaBrowserServiceCompat(), CoroutineScope by MainSco
             val navigator = MediaSessionNavigator(publication, publicationId, getMediaSession(context, serviceClass).controller)
             pendingNavigator.trySend(PendingNavigator(
                 navigator = navigator,
-                media = PendingMedia(publication, publicationId, locator = initialLocator ?: publication.readingOrder.first().toLocator())
+                media = PendingMedia(
+                    publication = publication,
+                    publicationId = publicationId,
+                    locator = initialLocator
+                        ?: requireNotNull(publication.locatorFromLink(publication.readingOrder.first()))
+                )
             ))
 
             return navigator
diff --git a/readium/navigator/src/main/java/org/readium/r2/navigator/media/MediaSessionNavigator.kt b/readium/navigator/src/main/java/org/readium/r2/navigator/media/MediaSessionNavigator.kt
@@ -158,7 +158,7 @@ class MediaSessionNavigator(
     private suspend fun createLocator(position: Duration?, metadata: MediaMetadataCompat?): Locator? {
         val href = metadata?.resourceHref ?: return null
         val index = publication.readingOrder.indexOfFirstWithHref(href) ?: return null
-        var locator = publication.readingOrder[index].toLocator()
+        var locator = publication.locatorFromLink(publication.readingOrder[index]) ?: return null
 
         if (position != null) {
             val startPosition = durations.slice(0 until index).sum()
@@ -186,8 +186,10 @@ class MediaSessionNavigator(
         return true
     }
 
-    override fun go(link: Link, animated: Boolean, completion: () -> Unit): Boolean =
-        go(link.toLocator(), animated, completion)
+    override fun go(link: Link, animated: Boolean, completion: () -> Unit): Boolean {
+        val locator = publication.locatorFromLink(link) ?: return false
+        return go(locator, animated, completion)
+    }
 
     override fun goForward(animated: Boolean, completion: () -> Unit): Boolean {
         if (!isActive) return false
diff --git a/readium/navigator/src/main/java/org/readium/r2/navigator/pdf/PdfNavigatorFragment.kt b/readium/navigator/src/main/java/org/readium/r2/navigator/pdf/PdfNavigatorFragment.kt
@@ -180,7 +180,9 @@ class PdfNavigatorFragment internal constructor(
     // Navigator
 
     override val currentLocator: StateFlow<Locator> get() = _currentLocator
-    private val _currentLocator = MutableStateFlow(initialLocator ?: publication.readingOrder.first().toLocator())
+    private val _currentLocator = MutableStateFlow(initialLocator
+        ?: requireNotNull(publication.locatorFromLink(publication.readingOrder.first()))
+    )
 
     override fun go(locator: Locator, animated: Boolean, completion: () -> Unit): Boolean {
         listener?.onJumpToLocator(locator)
@@ -189,8 +191,10 @@ class PdfNavigatorFragment internal constructor(
         return goToHref(locator.href, pageNumberToIndex(pageNumber), animated, completion)
     }
 
-    override fun go(link: Link, animated: Boolean, completion: () -> Unit): Boolean =
-        go(link.toLocator(), animated = animated, completion = completion)
+    override fun go(link: Link, animated: Boolean, completion: () -> Unit): Boolean {
+        val locator = publication.locatorFromLink(link) ?: return false
+        return go(locator, animated, completion)
+    }
 
     override fun goForward(animated: Boolean, completion: () -> Unit): Boolean {
         val page = pageIndexToNumber(pdfView.currentPage)
diff --git a/readium/shared/src/main/java/org/readium/r2/shared/publication/Locator.kt b/readium/shared/src/main/java/org/readium/r2/shared/publication/Locator.kt
@@ -200,6 +200,7 @@ data class Locator(
 /**
  * Creates a [Locator] from a reading order [Link].
  */
+@Deprecated("This may create an incorrect `Locator` if the link `type` is missing. Use `publication.locatorFromLink()` instead.")
 fun Link.toLocator(): Locator {
     val components = href.split("#", limit = 2)
     return Locator(
diff --git a/readium/shared/src/main/java/org/readium/r2/shared/publication/Manifest.kt b/readium/shared/src/main/java/org/readium/r2/shared/publication/Manifest.kt
@@ -64,6 +64,38 @@ data class Manifest(
         }
     }
 
+    /**
+     * Finds the first [Link] with the given HREF in the manifest's links.
+     *
+     * Searches through (in order) [readingOrder], [resources] and [links] recursively following
+     * alternate and children links.
+     *
+     * If there's no match, try again after removing any query parameter and anchor from the
+     * given [href].
+     */
+    fun linkWithHref(href: String): Link? {
+        fun List<Link>.deepLinkWithHref(href: String): Link? {
+            for (l in this) {
+                if (l.href == href)
+                    return l
+                else {
+                    l.alternates.deepLinkWithHref(href)?.let { return it }
+                    l.children.deepLinkWithHref(href)?.let { return it }
+                }
+            }
+            return null
+        }
+
+        fun find(href: String): Link? {
+            return readingOrder.deepLinkWithHref(href)
+                ?: resources.deepLinkWithHref(href)
+                ?: links.deepLinkWithHref(href)
+        }
+
+        return find(href)
+            ?: find(href.takeWhile { it !in "#?" })
+    }
+
     /**
      * Finds the first [Link] with the given relation in the manifest's links.
      */
@@ -78,6 +110,29 @@ data class Manifest(
     fun linksWithRel(rel: String): List<Link> =
         (readingOrder + resources + links).filterByRel(rel)
 
+    /**
+     * Creates a new [Locator] object from a [Link] to a resource of this manifest.
+     *
+     * Returns null if the resource is not found in this manifest.
+     */
+    fun locatorFromLink(link: Link): Locator? {
+        val components = link.href.split("#", limit = 2)
+        val href = components.firstOrNull() ?: link.href
+        val resourceLink = linkWithHref(href) ?: return null
+        val type = resourceLink.type ?: return null
+        val fragment = components.getOrNull(1)
+
+        return Locator(
+            href = href,
+            type = type,
+            title = resourceLink.title ?: link.title,
+            locations = Locator.Locations(
+                fragments = listOfNotNull(fragment),
+                progression = if (fragment == null) 0.0 else null
+            )
+        )
+    }
+
     /**
      * Serializes a [Publication] to its RWPM JSON representation.
      */
diff --git a/readium/shared/src/main/java/org/readium/r2/shared/publication/Publication.kt b/readium/shared/src/main/java/org/readium/r2/shared/publication/Publication.kt
@@ -61,7 +61,7 @@ typealias PublicationId = String
  * @param positionsFactory Factory used to build lazily the [positions].
  */
 class Publication(
-    private val manifest: Manifest,
+    manifest: Manifest,
     private val fetcher: Fetcher = EmptyFetcher(),
     private val servicesBuilder: ServicesBuilder = ServicesBuilder(),
 
@@ -135,7 +135,7 @@ class Publication(
      * Returns whether this publication conforms to the given Readium Web Publication Profile.
      */
     fun conformsTo(profile: Profile): Boolean =
-        manifest.conformsTo(profile)
+        _manifest.conformsTo(profile)
 
     /**
      * Finds the first [Link] with the given HREF in the publication's links.
@@ -146,16 +146,7 @@ class Publication(
      * If there's no match, try again after removing any query parameter and anchor from the
      * given [href].
      */
-    fun linkWithHref(href: String): Link? {
-        fun find(href: String): Link? {
-            return readingOrder.deepLinkWithHref(href)
-                ?: resources.deepLinkWithHref(href)
-                ?: links.deepLinkWithHref(href)
-        }
-
-        return find(href)
-            ?: find(href.takeWhile { it !in "#?" })
-    }
+    fun linkWithHref(href: String): Link? = _manifest.linkWithHref(href)
 
     /**
      * Finds the first [Link] having the given [rel] in the publications's links.
@@ -167,6 +158,13 @@ class Publication(
      */
     fun linksWithRel(rel: String): List<Link> = _manifest.linksWithRel(rel)
 
+    /**
+     * Creates a new [Locator] object from a [Link] to a resource of this publication.
+     *
+     * Returns null if the resource is not found in this publication.
+     */
+    fun locatorFromLink(link: Link): Locator? = _manifest.locatorFromLink(link)
+
     /**
      * Returns the resource targeted by the given non-templated [link].
      */
@@ -246,18 +244,6 @@ class Publication(
     internal fun linksWithRole(role: String): List<Link> =
         subcollections[role]?.firstOrNull()?.links ?: emptyList()
 
-    private fun List<Link>.deepLinkWithHref(href: String): Link? {
-        for (l in this) {
-            if (l.href == href)
-                return l
-            else {
-                l.alternates.deepLinkWithHref(href)?.let { return it }
-                l.children.deepLinkWithHref(href)?.let { return it }
-            }
-        }
-        return null
-    }
-
     companion object {
 
         /**
@@ -541,10 +527,7 @@ class Publication(
      * Finds the first resource [Link] (asset or [readingOrder] item) at the given relative path.
      */
     @Deprecated("Use [linkWithHref] instead.", ReplaceWith("linkWithHref(href)"))
-    fun resourceWithHref(href: String): Link? {
-        return readingOrder.deepLinkWithHref(href)
-            ?: resources.deepLinkWithHref(href)
-    }
+    fun resourceWithHref(href: String): Link? = linkWithHref(href)
 
     /**
      * Creates a [Publication]'s [positions].
diff --git a/readium/shared/src/main/java/org/readium/r2/shared/publication/services/search/StringSearchService.kt b/readium/shared/src/main/java/org/readium/r2/shared/publication/services/search/StringSearchService.kt
@@ -129,9 +129,8 @@ class StringSearchService(
                 return emptyList()
 
             val resourceTitle = publication.tableOfContents.titleMatching(link.href)
-            val resourceLocator = link.toLocator().copy(
-                title = resourceTitle ?: link.title
-            )
+            var resourceLocator = publication.locatorFromLink(link) ?: return emptyList()
+            resourceLocator = resourceLocator.copy(title = resourceTitle ?: resourceLocator.title)
             val locators = mutableListOf<Locator>()
 
             withContext(Dispatchers.IO) {
diff --git a/readium/shared/src/test/java/org/readium/r2/shared/publication/LocatorTest.kt b/readium/shared/src/test/java/org/readium/r2/shared/publication/LocatorTest.kt
@@ -60,29 +60,6 @@ class LocatorTest {
         assertNull(Locator.fromJSON(JSONObject("{ 'invalid': 'object' }")))
     }
 
-    @Test fun `create {Locator} from minimal {Link}`() {
-        assertEquals(
-            Locator(href = "http://locator", type = ""),
-            Link(href = "http://locator").toLocator()
-        )
-    }
-
-    @Test fun `create {Locator} from full {Link} with fragment`() {
-        assertEquals(
-            Locator(
-                href = "http://locator",
-                type = "text/html",
-                title = "My Link",
-                locations = Locator.Locations(fragments = listOf("page=42"))
-            ),
-            Link(
-                href = "http://locator#page=42",
-                type = "text/html",
-                title = "My Link"
-            ).toLocator()
-        )
-    }
-
     @Test fun `get {Locator} minimal JSON`() {
         assertJSONEquals(
             JSONObject("""{
diff --git a/readium/shared/src/test/java/org/readium/r2/shared/publication/ManifestTest.kt b/readium/shared/src/test/java/org/readium/r2/shared/publication/ManifestTest.kt
diff --git a/readium/shared/src/test/java/org/readium/r2/shared/publication/PublicationTest.kt b/readium/shared/src/test/java/org/readium/r2/shared/publication/PublicationTest.kt
diff --git a/test-app/src/main/java/org/readium/r2/testapp/outline/NavigationFragment.kt b/test-app/src/main/java/org/readium/r2/testapp/outline/NavigationFragment.kt
diff --git a/test-app/src/main/java/org/readium/r2/testapp/tts/ScreenReaderEngine.kt b/test-app/src/main/java/org/readium/r2/testapp/tts/ScreenReaderEngine.kt

Original file line number	Diff line number	Diff line change
`@@ -78,7 +78,8 @@ open class R2AudiobookActivity : AppCompatActivity(), CoroutineScope, IR2Activit`
`78`	`78`	`}`
`79`	`79`
`80`	`80`	`override fun go(link: Link, animated: Boolean, completion: () -> Unit): Boolean {`
`81`		`- TODO("not implemented") //To change body of created functions use File \| Settings \| File Templates.`
	`81`	`+ val locator = publication.locatorFromLink(link) ?: return false`
	`82`	`+ return go(locator)`
`82`	`83`	`}`
`83`	`84`
`84`	`85`	`override fun goForward(animated: Boolean, completion: () -> Unit): Boolean {`
`@@ -156,7 +157,7 @@ open class R2AudiobookActivity : AppCompatActivity(), CoroutineScope, IR2Activit`
`156`	`157`	`if (this.lifecycle.currentState.isAtLeast(Lifecycle.State.RESUMED)) {`
`157`	`158`
`158`	`159`	`if (!loadedInitialLocator) {`
`159`		`- go(publication.readingOrder.first().toLocator())`
	`160`	`+ go(publication.readingOrder.first())`
`160`	`161`	`}`
`161`	`162`
`162`	`163`	`binding.seekBar.setOnSeekBarChangeListener(object : SeekBar.OnSeekBarChangeListener {`