Fix EPUB currentLocator update when positions are missing (#119)

Author: mickael-menu

CHANGELOG.md

@@ -8,6 +8,10 @@ All notable changes to this project will be documented in this file. Take a look
 
 ### Added
 
+#### Shared
+
+* Add a new `Locator.Builder` object to progressively construct an immutable `Locator` object.
+
 #### Navigator
 
 * Improved Javascript support in the EPUB navigator:

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

@@ -54,6 +54,7 @@ import org.readium.r2.shared.publication.presentation.presentation
 import org.readium.r2.shared.publication.services.isRestricted
 import org.readium.r2.shared.publication.services.positionsByReadingOrder
 import org.readium.r2.shared.util.launchWebBrowser
+import org.readium.r2.shared.util.mediatype.MediaType
 import kotlin.math.ceil
 import kotlin.reflect.KClass
 
@@ -342,7 +343,7 @@ class EpubNavigatorFragment private constructor(
 
         resourcePager.adapter = adapter
 
-        if (publication.metadata.presentation.layout == EpubLayout.REFLOWABLE) {
+        if (publication.metadata.presentation.layout != EpubLayout.FIXED) {
             pendingLocator = locator
             setCurrent(resourcesSingle)
         } else {
@@ -732,19 +733,26 @@ class EpubNavigatorFragment private constructor(
             webView.updateCurrentItem()
 
             val resource = publication.readingOrder[resourcePager.currentItem]
-            val progression = webView.progression.coerceIn(0.0, 1.0)
-            val positions = publication.positionsByResource[resource.href]?.takeIf { it.isNotEmpty() }
-                    ?: return@launch
+            val builder = Locator.Builder(
+                href = resource.href,
+                type = resource.type ?: MediaType.XHTML.toString(),
+            )
 
-            val positionIndex = ceil(progression * (positions.size - 1)).toInt()
-            if (!positions.indices.contains(positionIndex)) {
-                return@launch
+            val progression = webView.progression.coerceIn(0.0, 1.0)
+            builder.locations.progression = progression
+
+            val positions = publication.positionsByResource[resource.href]
+                ?.takeIf { it.isNotEmpty() }
+            if (positions != null) {
+                val index = ceil(progression * (positions.size - 1)).toInt()
+                positions.getOrNull(index)
+                    ?.let { builder.merge(it) }
             }
 
-            val locator = positions[positionIndex]
-                    .copy(title = tableOfContentsTitleByHref[resource.href])
-                    .copyWithLocations(progression = progression)
+            tableOfContentsTitleByHref[resource.href]
+                ?.let { builder.title = it }
 
+            val locator = builder.build()
             _currentLocator.value = locator
 
             // Deprecated notifications

readium/shared/src/main/java/org/readium/r2/shared/publication/Locator.kt

@@ -21,7 +21,7 @@ import org.readium.r2.shared.util.logging.WarningLogger
 import org.readium.r2.shared.util.logging.log
 
 /**
- * Provides a precise location in a publication in a format that can be stored and shared.
+ * Represents a precise location in a publication in a format that can be stored and shared.
  *
  * There are many different use cases for locators:
  *  - getting back to the last position in a publication
@@ -30,7 +30,7 @@ import org.readium.r2.shared.util.logging.log
  *  - search results
  *  - human-readable (and shareable) reference in a publication
  *
- * https://github.com/readium/architecture/tree/master/locators
+ * https://readium.org/architecture/models/locators/
  */
 @Parcelize
 data class Locator(
@@ -41,6 +41,45 @@ data class Locator(
     val text: Text = Text()
 ) : JSONable, Parcelable {
 
+    /**
+     * Builder for a [Locator] object.
+     */
+    data class Builder(
+        var href: String,
+        var type: String,
+        var title: String? = null,
+        var locations: Locations.Builder = Locations.Builder(),
+        var text: Text.Builder = Text.Builder()
+    ) {
+        constructor(locator: Locator): this(
+            href = locator.href,
+            type = locator.type,
+            title = locator.title,
+            locations = Locations.Builder(locator.locations),
+            text = Text.Builder(locator.text),
+        )
+
+        /**
+         * Merges the given [locator], replacing any non null properties.
+         */
+        fun merge(locator: Locator): Builder {
+            href = locator.href
+            type = locator.type
+            locator.title?.let { title = it }
+            locations.merge(locator.locations)
+            text.merge(locator.text)
+            return this
+        }
+
+        fun build(): Locator = Locator(
+            href = href,
+            type = type,
+            title = title,
+            locations = locations.build(),
+            text = text.build()
+        )
+    }
+
     /**
      * One or more alternative expressions of the location.
      * https://github.com/readium/architecture/tree/master/models/locators#the-location-object
@@ -61,6 +100,51 @@ data class Locator(
         val otherLocations: @WriteWith<JSONParceler> Map<String, Any> = emptyMap()
     ) : JSONable, Parcelable {
 
+        /**
+         * Builder for a [Locations] object.
+         */
+        data class Builder(
+            var fragments: MutableList<String> = mutableListOf(),
+            var progression: Double? = null,
+            var position: Int? = null,
+            var totalProgression: Double? = null,
+            var otherLocations: MutableMap<String, Any> = mutableMapOf()
+        ) {
+            constructor(locations: Locations) : this(
+                fragments = locations.fragments.toMutableList(),
+                progression = locations.progression,
+                position = locations.position,
+                totalProgression = locations.totalProgression,
+                otherLocations = locations.otherLocations.toMutableMap()
+            )
+
+            fun merge(locations: Locations): Builder {
+                locations.progression?.let { progression = it }
+                locations.position?.let { position = it }
+                locations.totalProgression?.let { totalProgression = it }
+
+                if (locations.fragments.isNotEmpty()) {
+                    fragments = (locations.fragments + fragments)
+                        .distinct()
+                        .toMutableList()
+                }
+
+                for ((k, v) in locations.otherLocations) {
+                    otherLocations[k] = v
+                }
+
+                return this
+            }
+
+            fun build(): Locations = Locations(
+                fragments = fragments.toList(),
+                progression = progression,
+                position = position,
+                totalProgression = totalProgression,
+                otherLocations = otherLocations.toMap()
+            )
+        }
+
         override fun toJSON() = JSONObject(otherLocations).apply {
             putIfNotEmpty("fragments", fragments)
             put("progression", progression)
@@ -123,6 +207,38 @@ data class Locator(
         val after: String? = null
     ) : JSONable, Parcelable {
 
+        /**
+         * Builder for a [Text] object.
+         */
+        data class Builder(
+            var before: String? = null,
+            var highlight: String? = null,
+            var after: String? = null
+        ) {
+            constructor(text: Text) : this(
+                before = text.before,
+                highlight = text.highlight,
+                after = text.after,
+            )
+
+            fun merge(text: Text): Builder {
+                // It doesn't make sense to merge partially a [Text] object, as the [before],
+                // [highlight] and [after] properties are related to each other.
+                if (text.before != null || text.highlight != null || text.after != null) {
+                    before = text.before
+                    highlight = text.highlight
+                    after = text.after
+                }
+                return this
+            }
+
+            fun build(): Text = Text(
+                before = before,
+                highlight = highlight,
+                after = after
+            )
+        }
+
         override fun toJSON() = JSONObject().apply {
             put("before", before)
             put("highlight", highlight)
@@ -136,11 +252,21 @@ data class Locator(
                 highlight = json?.optNullableString("highlight"),
                 after = json?.optNullableString("after")
             )
-
         }
-
     }
 
+    /**
+     * Makes a copy of the `Locator` after modifying its properties using a `Builder`.
+     *
+     * ```
+     * locator.copy {
+     *     locations.progression = 0.5
+     * }
+     * ```
+     */
+    fun copy(build: Builder.() -> Unit): Locator =
+        Builder(this).apply(build).build()
+
     /**
      * Shortcut to get a copy of the [Locator] with different [Locations] sub-properties.
      */