Deprecate Locator(link: Link) in favor of Publication.locate(Link) (#47)

Author: mickael-menu

.gitignore

@@ -7,7 +7,7 @@
 Package.resolved
 
 # Carthage
-Carthage/
+./Carthage/
 Cartfile.resolved
 
 # Xcode

CHANGELOG.md

@@ -6,6 +6,13 @@ All notable changes to this project will be documented in this file. Take a look
 
 ## [Unreleased]
 
+### Deprecated
+
+#### Shared
+
+* `Locator(link: Link)` is deprecated as it may create an incorrect `Locator` if the link `type` is missing.
+    * Use `publication.locate(Link)` instead.
+
 ### Fixed
 
 #### Navigator

Makefile

@@ -16,3 +16,9 @@ scripts:
 	yarn --cwd "$(SCRIPTS_PATH)" run format
 	yarn --cwd "$(SCRIPTS_PATH)" run lint
 	yarn --cwd "$(SCRIPTS_PATH)" run bundle
+
+.PHONY: test
+test:
+	# To limit to a particular test suite: -only-testing:R2SharedTests
+	xcodebuild test -scheme "Readium-Package" -destination "platform=iOS Simulator,name=iPhone 12" | xcbeautify -q
+

Sources/Navigator/Audiobook/AudioNavigator.swift

@@ -27,7 +27,7 @@ open class _AudioNavigator: _MediaNavigator, _AudioSessionUser, Loggable {
     public init(publication: Publication, initialLocation: Locator? = nil) {
         self.publication = publication
         self.initialLocation = initialLocation
-            ?? publication.readingOrder.first.map { Locator(link: $0) }
+            ?? publication.readingOrder.first.flatMap { publication.locate($0) }
 
         let durations = publication.readingOrder.map { $0.duration ?? 0 }
         self.durations = durations
@@ -255,7 +255,10 @@ open class _AudioNavigator: _MediaNavigator, _AudioSessionUser, Loggable {
 
     @discardableResult
     public func go(to link: Link, animated: Bool = false, completion: @escaping () -> Void = {}) -> Bool {
-        return go(to: Locator(link: link), animated: animated, completion: completion)
+        guard let locator = publication.locate(link) else {
+            return false
+        }
+        return go(to: locator, animated: animated, completion: completion)
     }
 
     @discardableResult

Sources/Navigator/CBZ/CBZNavigatorViewController.swift

@@ -232,7 +232,7 @@ extension CBZNavigatorViewController {
     public convenience init(for publication: Publication, initialIndex: Int = 0) {
         var location: Locator? = nil
         if publication.readingOrder.indices.contains(initialIndex) {
-            location = Locator(link: publication.readingOrder[initialIndex])
+            location = publication.locate(publication.readingOrder[initialIndex])
         }
         self.init(publication: publication, initialLocation: location)
     }

Sources/Navigator/EPUB/EPUBNavigatorViewController.swift

@@ -520,7 +520,7 @@ open class EPUBNavigatorViewController: UIViewController, VisualNavigator, Selec
                 locations: { $0.progression = progression }
             )
         } else {
-            return Locator(link: link).copy(
+            return publication.locate(link)?.copy(
                 locations: { $0.progression = progression }
             )
         }
@@ -564,7 +564,10 @@ open class EPUBNavigatorViewController: UIViewController, VisualNavigator, Selec
     }
 
     public func go(to link: Link, animated: Bool, completion: @escaping () -> Void) -> Bool {
-        return go(to: Locator(link: link), animated: animated, completion: completion)
+        guard let locator = publication.locate(link) else {
+            return false
+        }
+        return go(to: locator, animated: animated, completion: completion)
     }
 
     public func goForward(animated: Bool, completion: @escaping () -> Void) -> Bool {

Sources/Shared/Publication/Locator.swift

@@ -70,7 +70,8 @@ public struct Locator: Hashable, CustomStringConvertible, Loggable {
 
         try self.init(json: json, warnings: warnings)
     }
-    
+
+    @available(*, deprecated, message: "This may create an incorrect `Locator` if the link `type` is missing. Use `publication.locate(Link)` instead.")
     public init(link: Link) {
         let components = link.href.split(separator: "#", maxSplits: 1).map(String.init)
         let fragments = (components.count > 1) ? [String(components[1])] : []

Sources/Shared/Publication/Manifest.swift

@@ -126,7 +126,36 @@ public struct Manifest: JSONEquatable, Hashable {
 
         return metadata.conformsTo.contains(profile)
     }
-    
+
+    /// Finds the first Link having the given `href` in the manifest's links.
+    public func link(withHREF href: String) -> Link? {
+        func deepFind(in linkLists: [Link]...) -> Link? {
+            for links in linkLists {
+                for link in links {
+                    if link.href == href {
+                        return link
+                    } else if let child = deepFind(in: link.alternates, link.children) {
+                        return child
+                    }
+                }
+            }
+
+            return nil
+        }
+
+        var link = deepFind(in: readingOrder, resources, links)
+        if
+            link == nil,
+            let shortHREF = href.components(separatedBy: .init(charactersIn: "#?")).first,
+            shortHREF != href
+        {
+            // Tries again, but without the anchor and query parameters.
+            link = self.link(withHREF: shortHREF)
+        }
+
+        return link
+    }
+
     /// Finds the first link with the given relation in the manifest's links.
     public func link(withRel rel: LinkRelation) -> Link? {
         return readingOrder.first(withRel: rel)

Sources/Shared/Publication/Publication.swift

@@ -84,7 +84,7 @@ public class Publication: Loggable {
 
     /// Returns whether this publication conforms to the given Readium Web Publication Profile.
     public func conforms(to profile: Profile) -> Bool {
-        return manifest.conforms(to: profile)
+        manifest.conforms(to: profile)
     }
 
     /// The URL where this publication is served, computed from the `Link` with `self` relation.
@@ -97,41 +97,17 @@ public class Publication: Loggable {
 
     /// Finds the first Link having the given `href` in the publication's links.
     public func link(withHREF href: String) -> Link? {
-        func deepFind(in linkLists: [Link]...) -> Link? {
-            for links in linkLists {
-                for link in links {
-                    if link.href == href {
-                        return link
-                    } else if let child = deepFind(in: link.alternates, link.children) {
-                        return child
-                    }
-                }
-            }
-            
-            return nil
-        }
-        
-        var link = deepFind(in: readingOrder, resources, links)
-        if
-            link == nil,
-            let shortHREF = href.components(separatedBy: .init(charactersIn: "#?")).first,
-            shortHREF != href
-        {
-            // Tries again, but without the anchor and query parameters.
-            link = self.link(withHREF: shortHREF)
-        }
-        
-        return link
+        manifest.link(withHREF: href)
     }
 
     /// Finds the first link with the given relation in the publication's links.
     public func link(withRel rel: LinkRelation) -> Link? {
-        return manifest.link(withRel: rel)
+        manifest.link(withRel: rel)
     }
 
     /// Finds all the links with the given relation in the publication's links.
     public func links(withRel rel: LinkRelation) -> [Link] {
-        return manifest.links(withRel: rel)
+        manifest.links(withRel: rel)
     }
 
     /// Returns the resource targeted by the given `link`.
@@ -161,12 +137,12 @@ public class Publication: Loggable {
     ///
     /// e.g. `findService(PositionsService.self)`
     public func findService<T>(_ serviceType: T.Type) -> T? {
-        return services.first { $0 is T } as? T
+        services.first { $0 is T } as? T
     }
 
     /// Finds all the services implementing the given service type.
     public func findServices<T>(_ serviceType: T.Type) -> [T] {
-        return services.filter { $0 is T } as! [T]
+        services.filter { $0 is T } as! [T]
     }
 
     /// Sets the URL where this `Publication`'s RWPM manifest is served.

Sources/Shared/Publication/Services/Locator/DefaultLocatorService.swift

@@ -8,35 +8,70 @@ import Foundation
 
 /// A default implementation of the `LocatorService` using the `PositionsService` to locate its inputs.
 open class DefaultLocatorService: LocatorService, Loggable {
-    
-    let readingOrder: [Link]
-    let positionsByReadingOrder: () -> [[Locator]]
-    
-    public init(readingOrder: [Link], positionsByReadingOrder: @escaping () -> [[Locator]]) {
-        self.readingOrder = readingOrder
-        self.positionsByReadingOrder = positionsByReadingOrder
-    }
 
-    public convenience init(readingOrder: [Link], publication: Weak<Publication>) {
-        self.init(readingOrder: readingOrder, positionsByReadingOrder: { publication()?.positionsByReadingOrder ?? [] })
+    public let publication: Weak<Publication>
+
+    public init(publication: Weak<Publication>) {
+        self.publication = publication
     }
 
+    /// Locates the target of the given `locator`.
+    ///
+    /// If `locator.href` can be found in the links, `locator` will be returned directly.
+    /// Otherwise, will attempt to find the closest match using `totalProgression`, `position`,
+    /// `fragments`, etc.
     open func locate(_ locator: Locator) -> Locator? {
-        guard readingOrder.firstIndex(withHREF: locator.href) != nil else {
+        guard let publication = publication() else {
             return nil
         }
-        
-        return locator
+
+        if publication.link(withHREF: locator.href) != nil {
+            return locator
+        }
+
+        if let totalProgression = locator.locations.totalProgression, let target = locate(progression: totalProgression) {
+            return target.copy(
+                title: locator.title,
+                text: { $0 = locator.text }
+            )
+        }
+
+        return nil
     }
-    
+
+    open func locate(_ link: Link) -> Locator? {
+        let components = link.href.split(separator: "#", maxSplits: 1).map(String.init)
+        let href = components.first ?? link.href
+        let fragment = components.getOrNil(1)
+
+        guard
+            let resourceLink = publication()?.link(withHREF: href),
+            let type = resourceLink.type
+        else {
+            return nil
+        }
+
+        return Locator(
+            href: href,
+            type: type,
+            title: resourceLink.title ?? link.title,
+            locations: Locator.Locations(
+                fragments: Array(ofNotNil: fragment),
+                progression: (fragment == nil) ? 0.0 : nil
+            )
+        )
+    }
+
     open func locate(progression totalProgression: Double) -> Locator? {
         guard 0.0...1.0 ~= totalProgression else {
             log(.error, "Progression must be between 0.0 and 1.0, received \(totalProgression)")
             return nil
         }
 
-        let positions = positionsByReadingOrder()
-        guard let (readingOrderIndex, position) = findClosest(to: totalProgression, in: positions) else {
+        guard
+            let positions = publication()?.positionsByReadingOrder,
+            let (readingOrderIndex, position) = findClosest(to: totalProgression, in: positions)
+        else {
             return nil
         }