From 3ab1381de8277f00e23cc840fe4fa11c644a508b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mickae=CC=88l=20Menu?= Date: Wed, 27 Apr 2022 18:06:27 +0200 Subject: [PATCH 1/5] Refactor the `LocatorService` implementations and add `locate(Link)` --- Makefile | 5 ++ .../Locator/DefaultLocatorService.swift | 67 ++++++++++++++----- .../Services/Locator/LocatorService.swift | 24 ++++--- .../Positions/InMemoryPositionsService.swift | 23 +++++++ .../PerResourcePositionsService.swift | 12 +--- .../Services/PublicationServicesBuilder.swift | 2 +- Sources/Shared/Toolkit/Weak.swift | 24 ++++++- .../Audio/Services/AudioLocatorService.swift | 55 ++++++--------- TestApp/.gitignore | 2 + .../Locator/DefaultLocatorServiceTests.swift | 8 ++- .../Services/AudioLocatorServiceTests.swift | 4 +- 11 files changed, 151 insertions(+), 75 deletions(-) create mode 100644 Sources/Shared/Publication/Services/Positions/InMemoryPositionsService.swift diff --git a/Makefile b/Makefile index 18ef3ac3a..2ae3ae81e 100644 --- a/Makefile +++ b/Makefile @@ -16,3 +16,8 @@ scripts: yarn --cwd "$(SCRIPTS_PATH)" run format yarn --cwd "$(SCRIPTS_PATH)" run lint yarn --cwd "$(SCRIPTS_PATH)" run bundle + +.PHONY: test +test: + xcodebuild test -scheme "Readium-Package" -destination "platform=iOS Simulator,name=iPhone 12" + diff --git a/Sources/Shared/Publication/Services/Locator/DefaultLocatorService.swift b/Sources/Shared/Publication/Services/Locator/DefaultLocatorService.swift index 4f98b21bb..cb87bb3cf 100644 --- a/Sources/Shared/Publication/Services/Locator/DefaultLocatorService.swift +++ b/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) { - self.init(readingOrder: readingOrder, positionsByReadingOrder: { publication()?.positionsByReadingOrder ?? [] }) + public let publication: Weak + + public init(publication: Weak) { + 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 } diff --git a/Sources/Shared/Publication/Services/Locator/LocatorService.swift b/Sources/Shared/Publication/Services/Locator/LocatorService.swift index 4d076a1a5..1cd7bfaaa 100644 --- a/Sources/Shared/Publication/Services/Locator/LocatorService.swift +++ b/Sources/Shared/Publication/Services/Locator/LocatorService.swift @@ -16,13 +16,21 @@ public typealias LocatorServiceFactory = (PublicationServiceContext) -> LocatorS /// - Converting a `Locator` which was created from an alternate manifest with a different reading /// order. For example, when downloading a streamed manifest or offloading a package. public protocol LocatorService: PublicationService { - + /// Locates the target of the given `locator`. func locate(_ locator: Locator) -> Locator? - + + /// Locates the target of the given `link`. + func locate(_ link: Link) -> Locator? + /// Locates the target at the given `progression` relative to the whole publication. func locate(progression: Double) -> Locator? - +} + +public extension LocatorService { + func locate(_ locator: Locator) -> Locator? { nil } + func locate(_ link: Link) -> Locator? { nil } + func locate(progression: Double) -> Locator? { nil } } @@ -31,19 +39,19 @@ public protocol LocatorService: PublicationService { public extension Publication { /// Locates the target of the given `locator`. - /// - /// If `locator.href` can be found in the reading order, `locator` will be returned directly. - /// Otherwise, will attempt to find the closest match using `totalProgression`, `position`, - /// `fragments`, etc. func locate(_ locator: Locator) -> Locator? { findService(LocatorService.self)?.locate(locator) } - + /// Locates the target at the given `progression` relative to the whole publication. func locate(progression: Double) -> Locator? { findService(LocatorService.self)?.locate(progression: progression) } + /// Locates the target of the given `link`. + func locate(_ link: Link) -> Locator? { + findService(LocatorService.self)?.locate(link) + } } diff --git a/Sources/Shared/Publication/Services/Positions/InMemoryPositionsService.swift b/Sources/Shared/Publication/Services/Positions/InMemoryPositionsService.swift new file mode 100644 index 000000000..e99b0e4eb --- /dev/null +++ b/Sources/Shared/Publication/Services/Positions/InMemoryPositionsService.swift @@ -0,0 +1,23 @@ +// +// Copyright 2022 Readium Foundation. All rights reserved. +// Use of this source code is governed by the BSD-style license +// available in the top-level LICENSE file of the project. +// + +import Foundation + +/// A [PositionsService] holding the pre-computed position locators in memory. +public class InMemoryPositionsService : PositionsService { + + public private(set) var positionsByReadingOrder: [[Locator]] + + public init(positionsByReadingOrder: [[Locator]]) { + self.positionsByReadingOrder = positionsByReadingOrder + } + + public static func makeFactory(positionsByReadingOrder: [[Locator]]) -> (PublicationServiceContext) -> InMemoryPositionsService { + { _ in + InMemoryPositionsService(positionsByReadingOrder: positionsByReadingOrder) + } + } +} diff --git a/Sources/Shared/Publication/Services/Positions/PerResourcePositionsService.swift b/Sources/Shared/Publication/Services/Positions/PerResourcePositionsService.swift index 49a40d272..055c39b9d 100644 --- a/Sources/Shared/Publication/Services/Positions/PerResourcePositionsService.swift +++ b/Sources/Shared/Publication/Services/Positions/PerResourcePositionsService.swift @@ -1,12 +1,7 @@ // -// PerResourcePositionsService.swift -// r2-shared-swift -// -// Created by Mickaël Menu on 01/06/2020. -// -// Copyright 2020 Readium Foundation. All rights reserved. -// Use of this source code is governed by a BSD-style license which is detailed -// in the LICENSE file present in the project repository where this source code is maintained. +// Copyright 2022 Readium Foundation. All rights reserved. +// Use of this source code is governed by the BSD-style license +// available in the top-level LICENSE file of the project. // import Foundation @@ -46,6 +41,5 @@ public final class PerResourcePositionsService: PositionsService { PerResourcePositionsService(readingOrder: context.manifest.readingOrder, fallbackMediaType: fallbackMediaType) } } - } diff --git a/Sources/Shared/Publication/Services/PublicationServicesBuilder.swift b/Sources/Shared/Publication/Services/PublicationServicesBuilder.swift index 8e3e01353..2004e4533 100644 --- a/Sources/Shared/Publication/Services/PublicationServicesBuilder.swift +++ b/Sources/Shared/Publication/Services/PublicationServicesBuilder.swift @@ -21,7 +21,7 @@ public struct PublicationServicesBuilder { public init( contentProtection: ContentProtectionServiceFactory? = nil, cover: CoverServiceFactory? = nil, - locator: LocatorServiceFactory? = { DefaultLocatorService(readingOrder: $0.manifest.readingOrder, publication: $0.publication) }, + locator: LocatorServiceFactory? = { DefaultLocatorService(publication: $0.publication) }, positions: PositionsServiceFactory? = nil, search: SearchServiceFactory? = nil, setup: (inout PublicationServicesBuilder) -> Void = { _ in } diff --git a/Sources/Shared/Toolkit/Weak.swift b/Sources/Shared/Toolkit/Weak.swift index e641762b2..362ac82a0 100644 --- a/Sources/Shared/Toolkit/Weak.swift +++ b/Sources/Shared/Toolkit/Weak.swift @@ -11,7 +11,7 @@ import Foundation /// Get the reference by calling `weakVar()`. /// Conveniently, the reference can be reset by setting the `ref` property. @dynamicCallable -public final class Weak { +public class Weak { // Weakly held reference. public weak var ref: T? @@ -23,4 +23,24 @@ public final class Weak { public func dynamicallyCall(withArguments args: [Any]) -> T? { ref } -} \ No newline at end of file +} + +/// Smart pointer passing as a Weak reference but preventing the reference from being lost. +/// Mainly useful for the unit test suite. +public class _Strong: Weak { + + private var strongRef: T? + + public override var ref: T? { + get { super.ref } + set { + super.ref = newValue + strongRef = newValue + } + } + + public override init(_ ref: T? = nil) { + self.strongRef = ref + super.init(ref) + } +} diff --git a/Sources/Streamer/Parser/Audio/Services/AudioLocatorService.swift b/Sources/Streamer/Parser/Audio/Services/AudioLocatorService.swift index 63e1ccb6d..75edb17da 100644 --- a/Sources/Streamer/Parser/Audio/Services/AudioLocatorService.swift +++ b/Sources/Streamer/Parser/Audio/Services/AudioLocatorService.swift @@ -8,39 +8,26 @@ import Foundation import R2Shared /// Locator service for audio publications. -final class AudioLocatorService: LocatorService { - - /// Total duration of the publication. - private let totalDuration: Double? - +final class AudioLocatorService: DefaultLocatorService { + + static func makeFactory() -> (PublicationServiceContext) -> AudioLocatorService { + { context in AudioLocatorService(publication: context.publication) } + } + + private lazy var readingOrder: [Link] = + publication()?.readingOrder ?? [] + /// Duration per reading order index. - private let durations: [Double] - - private let readingOrder: [Link] - - init(readingOrder: [Link]) { - self.durations = readingOrder.map { $0.duration ?? 0 } + private lazy var durations: [Double] = + readingOrder.map { $0.duration ?? 0 } + + /// Total duration of the publication. + private lazy var totalDuration: Double? = { let totalDuration = durations.reduce(0, +) - self.totalDuration = (totalDuration > 0) ? totalDuration : nil - self.readingOrder = readingOrder - } - - func locate(_ locator: Locator) -> Locator? { - if readingOrder.firstIndex(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 - } - - func locate(progression: Double) -> Locator? { + return (totalDuration > 0) ? totalDuration : nil + }() + + override func locate(progression: Double) -> Locator? { guard let totalDuration = totalDuration else { return nil } @@ -68,11 +55,7 @@ final class AudioLocatorService: LocatorService { ) ) } - - static func makeFactory() -> (PublicationServiceContext) -> AudioLocatorService { - { context in AudioLocatorService(readingOrder: context.manifest.readingOrder) } - } - + /// Finds the reading order item containing the time `position` (in seconds), as well as its /// start time. private func readingOrderItemAtPosition(_ position: Double) -> (link: Link, startPosition: Double)? { diff --git a/TestApp/.gitignore b/TestApp/.gitignore index 8225e5fbf..3c74969de 100644 --- a/TestApp/.gitignore +++ b/TestApp/.gitignore @@ -1,6 +1,8 @@ TestApp.xcodeproj TestApp.xcworkspace project.yml +# IntelliJ AppCode +.idea Carthage/ Cartfile diff --git a/Tests/SharedTests/Publication/Services/Locator/DefaultLocatorServiceTests.swift b/Tests/SharedTests/Publication/Services/Locator/DefaultLocatorServiceTests.swift index 9fd681d1f..d880d0633 100644 --- a/Tests/SharedTests/Publication/Services/Locator/DefaultLocatorServiceTests.swift +++ b/Tests/SharedTests/Publication/Services/Locator/DefaultLocatorServiceTests.swift @@ -114,9 +114,13 @@ class DefaultLocatorServiceTests: XCTestCase { } func makeService(readingOrder: [Link] = [], positions: [[Locator]] = []) -> DefaultLocatorService { - DefaultLocatorService(readingOrder: readingOrder, positionsByReadingOrder: { positions }) + DefaultLocatorService(publication: _Strong(Publication( + manifest: Manifest(metadata: Metadata(title: ""), readingOrder: readingOrder), + servicesBuilder: PublicationServicesBuilder( + positions: InMemoryPositionsService.makeFactory(positionsByReadingOrder: positions) + ) + ))) } - } private let positionsFixture: [[Locator]] = [ diff --git a/Tests/StreamerTests/Parser/Audio/Services/AudioLocatorServiceTests.swift b/Tests/StreamerTests/Parser/Audio/Services/AudioLocatorServiceTests.swift index 3f9825973..93dc64dcc 100644 --- a/Tests/StreamerTests/Parser/Audio/Services/AudioLocatorServiceTests.swift +++ b/Tests/StreamerTests/Parser/Audio/Services/AudioLocatorServiceTests.swift @@ -165,7 +165,9 @@ class AudioLocatorServiceTests: XCTestCase { private func makeService(readingOrder: [Link]) -> AudioLocatorService { AudioLocatorService( - readingOrder: readingOrder + publication: _Strong(Publication( + manifest: Manifest(metadata: Metadata(title: ""), readingOrder: readingOrder) + )) ) } From 69640753d77b126a5b9a904d8ea0671caf4d0756 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mickae=CC=88l=20Menu?= Date: Wed, 27 Apr 2022 18:34:52 +0200 Subject: [PATCH 2/5] Add tests and deprecate `Locator(link: Link)` --- Makefile | 3 +- Sources/Shared/Publication/Locator.swift | 3 +- Sources/Shared/Publication/Manifest.swift | 31 +++++++- Sources/Shared/Publication/Publication.swift | 36 ++------- .../Locator/DefaultLocatorService.swift | 2 +- .../Services/Search/StringSearchService.swift | 10 ++- .../Common/Outline/OutlineTableView.swift | 17 ++-- .../Locator/DefaultLocatorServiceTests.swift | 78 ++++++++++++++++++- 8 files changed, 134 insertions(+), 46 deletions(-) diff --git a/Makefile b/Makefile index 2ae3ae81e..cbbe29f95 100644 --- a/Makefile +++ b/Makefile @@ -19,5 +19,6 @@ scripts: .PHONY: test test: - xcodebuild test -scheme "Readium-Package" -destination "platform=iOS Simulator,name=iPhone 12" + # To limit to a particular test suite: -only-testing:R2SharedTests + xcodebuild test -scheme "Readium-Package" -destination "platform=iOS Simulator,name=iPhone 12" | xcbeautify -q diff --git a/Sources/Shared/Publication/Locator.swift b/Sources/Shared/Publication/Locator.swift index b787b0216..4d57543cb 100644 --- a/Sources/Shared/Publication/Locator.swift +++ b/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])] : [] diff --git a/Sources/Shared/Publication/Manifest.swift b/Sources/Shared/Publication/Manifest.swift index afd0c8b15..e3287ba96 100644 --- a/Sources/Shared/Publication/Manifest.swift +++ b/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) diff --git a/Sources/Shared/Publication/Publication.swift b/Sources/Shared/Publication/Publication.swift index dc55f15b6..74f7e60db 100644 --- a/Sources/Shared/Publication/Publication.swift +++ b/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(_ 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(_ 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. diff --git a/Sources/Shared/Publication/Services/Locator/DefaultLocatorService.swift b/Sources/Shared/Publication/Services/Locator/DefaultLocatorService.swift index cb87bb3cf..026812efa 100644 --- a/Sources/Shared/Publication/Services/Locator/DefaultLocatorService.swift +++ b/Sources/Shared/Publication/Services/Locator/DefaultLocatorService.swift @@ -57,7 +57,7 @@ open class DefaultLocatorService: LocatorService, Loggable { title: resourceLink.title ?? link.title, locations: Locator.Locations( fragments: Array(ofNotNil: fragment), - progression: (fragment != nil) ? 0.0 : nil + progression: (fragment == nil) ? 0.0 : nil ) ) } diff --git a/Sources/Shared/Publication/Services/Search/StringSearchService.swift b/Sources/Shared/Publication/Services/Search/StringSearchService.swift index aa36b8a7d..fff15be4b 100644 --- a/Sources/Shared/Publication/Services/Search/StringSearchService.swift +++ b/Sources/Shared/Publication/Services/Search/StringSearchService.swift @@ -154,13 +154,17 @@ public class _StringSearchService: _SearchService { } private func findLocators(in link: Link, resourceIndex: Int, text: String, cancellable: CancellableObject) -> [Locator] { - guard !text.isEmpty else { + guard + !text.isEmpty, + var resourceLocator = publication.locate(link) + else { return [] } let currentLocale = options.language.map { Locale(identifier: $0) } ?? locale - let title = publication.tableOfContents.titleMatchingHREF(link.href) ?? link.title - let resourceLocator = Locator(link: link).copy(title: title) + resourceLocator = resourceLocator.copy( + title: publication.tableOfContents.titleMatchingHREF(link.href) ?? link.title + ) var locators: [Locator] = [] diff --git a/TestApp/Sources/Reader/Common/Outline/OutlineTableView.swift b/TestApp/Sources/Reader/Common/Outline/OutlineTableView.swift index e0c08d1ea..fc5276ac1 100644 --- a/TestApp/Sources/Reader/Common/Outline/OutlineTableView.swift +++ b/TestApp/Sources/Reader/Common/Outline/OutlineTableView.swift @@ -19,27 +19,28 @@ enum OutlineSection: Int { } struct OutlineTableView: View { + private let publication: Publication @ObservedObject private var bookmarksModel: BookmarksViewModel @ObservedObject private var highlightsModel: HighlightsViewModel @State private var selectedSection: OutlineSection = .tableOfContents // Outlines (list of links) to display for each section. private var outlines: [OutlineSection: [(level: Int, link: R2Shared.Link)]] = [:] - + init(publication: Publication, bookId: Book.Id, bookmarkRepository: BookmarkRepository, highlightRepository: HighlightRepository) { - + self.publication = publication + self.bookmarksModel = BookmarksViewModel(bookId: bookId, repository: bookmarkRepository) + self.highlightsModel = HighlightsViewModel(bookId: bookId, repository: highlightRepository) + func flatten(_ links: [R2Shared.Link], level: Int = 0) -> [(level: Int, link: R2Shared.Link)] { return links.flatMap { [(level, $0)] + flatten($0.children, level: level + 1) } } - outlines = [ + self.outlines = [ .tableOfContents: flatten(publication.tableOfContents), .landmarks: flatten(publication.landmarks), .pageList: flatten(publication.pageList) ] - - bookmarksModel = BookmarksViewModel(bookId: bookId, repository: bookmarkRepository) - highlightsModel = HighlightsViewModel(bookId: bookId, repository: highlightRepository) } var body: some View { @@ -55,7 +56,9 @@ struct OutlineTableView: View { .frame(maxWidth: .infinity, alignment: .leading) .contentShape(Rectangle()) .onTapGesture { - locatorSubject.send(Locator(link: item.link)) + if let locator = publication.locate(item.link) { + locatorSubject.send(locator) + } } } } else { diff --git a/Tests/SharedTests/Publication/Services/Locator/DefaultLocatorServiceTests.swift b/Tests/SharedTests/Publication/Services/Locator/DefaultLocatorServiceTests.swift index d880d0633..a0ed65605 100644 --- a/Tests/SharedTests/Publication/Services/Locator/DefaultLocatorServiceTests.swift +++ b/Tests/SharedTests/Publication/Services/Locator/DefaultLocatorServiceTests.swift @@ -113,9 +113,83 @@ class DefaultLocatorServiceTests: XCTestCase { XCTAssertNil(service.locate(progression: 0.5)) } - func makeService(readingOrder: [Link] = [], positions: [[Locator]] = []) -> DefaultLocatorService { + func testFromMinimalLink() { + let service = makeService(readingOrder: [ + Link(href: "/href", type: "text/html", title: "Resource") + ]) + + XCTAssertEqual( + service.locate(Link(href: "/href")), + Locator(href: "/href", type: "text/html", title: "Resource", locations: Locator.Locations(progression: 0.0)) + ) + } + + func testFromLinkInReadingOrderResourcesOrLinks() { + let service = makeService( + links: [Link(href: "/href3", type: "text/html")], + readingOrder: [Link(href: "/href1", type: "text/html")], + resources: [Link(href: "/href2", type: "text/html")] + ) + + XCTAssertEqual( + service.locate(Link(href: "/href1")), + Locator(href: "/href1", type: "text/html", locations: Locator.Locations(progression: 0.0)) + ) + + XCTAssertEqual( + service.locate(Link(href: "/href2")), + Locator(href: "/href2", type: "text/html", locations: Locator.Locations(progression: 0.0)) + ) + + XCTAssertEqual( + service.locate(Link(href: "/href3")), + Locator(href: "/href3", type: "text/html", locations: Locator.Locations(progression: 0.0)) + ) + } + + func testFromLinkWithFragment() { + let service = makeService(readingOrder: [ + Link(href: "/href", type: "text/html", title: "Resource") + ]) + + XCTAssertEqual( + service.locate(Link(href: "/href#page=42", type: "text/xml", title: "My link")), + Locator(href: "/href", type: "text/html", title: "Resource", locations: Locator.Locations(fragments: ["page=42"])) + ) + } + + func testTitleFallbackFromLink() { + let service = makeService(readingOrder: [ + Link(href: "/href", type: "text/html") + ]) + + XCTAssertEqual( + service.locate(Link(href: "/href", title: "My link")), + Locator(href: "/href", type: "text/html", title: "My link", locations: Locator.Locations(progression: 0.0)) + ) + } + + func testFromLinkNotFound() { + let service = makeService(readingOrder: [ + Link(href: "/href", type: "text/html") + ]) + + XCTAssertNil(service.locate(Link(href: "notfound"))) + } + + func makeService( + links: [Link] = [], + readingOrder: [Link] = [], + resources: [Link] = [], + positions: [[Locator]] = [] + ) -> DefaultLocatorService { DefaultLocatorService(publication: _Strong(Publication( - manifest: Manifest(metadata: Metadata(title: ""), readingOrder: readingOrder), + manifest: Manifest( + metadata: Metadata(title: ""), + links: links, + readingOrder: readingOrder, + resources: resources + ), servicesBuilder: PublicationServicesBuilder( positions: InMemoryPositionsService.makeFactory(positionsByReadingOrder: positions) ) From 935a51cea7a63e055e97af02b9e37e39f17b014c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mickae=CC=88l=20Menu?= Date: Wed, 27 Apr 2022 18:36:04 +0200 Subject: [PATCH 3/5] Update changelog --- CHANGELOG.md | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c94d38914..21f3e7e50 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,15 @@ All notable changes to this project will be documented in this file. Take a look **Warning:** Features marked as *alpha* may change or be removed in a future release without notice. Use with caution. - +## [Unreleased] + +### Shared + +#### Deprecated + +* `Locator(link: Link)` is deprecated as it may create an incorrect `Locator` if the link `type` is missing. + * Use `publication.locate(Link)` instead. + ## [2.3.0] From d6d3deab4d8c8e19f454f3b1ed3144ed5cd00483 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mickae=CC=88l=20Menu?= Date: Wed, 27 Apr 2022 18:50:19 +0200 Subject: [PATCH 4/5] Update Carthage project --- .gitignore | 2 +- Support/Carthage/Readium.xcodeproj/project.pbxproj | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index ba719ac08..0c5fcb4e6 100644 --- a/.gitignore +++ b/.gitignore @@ -7,7 +7,7 @@ Package.resolved # Carthage -Carthage/ +./Carthage/ Cartfile.resolved # Xcode diff --git a/Support/Carthage/Readium.xcodeproj/project.pbxproj b/Support/Carthage/Readium.xcodeproj/project.pbxproj index 2458f598c..8b2068588 100644 --- a/Support/Carthage/Readium.xcodeproj/project.pbxproj +++ b/Support/Carthage/Readium.xcodeproj/project.pbxproj @@ -274,6 +274,7 @@ E8CB4E5729E7000FC55FC937 /* CoreServices.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 342D5C0FEE79A2ABEE24A43E /* CoreServices.framework */; }; E9AADF25494C968A44979B66 /* UInt64.swift in Sources */ = {isa = PBXBuildFile; fileRef = 57338C29681D4872D425AB81 /* UInt64.swift */; }; E9EE047B89D0084523F7C888 /* Feed.swift in Sources */ = {isa = PBXBuildFile; fileRef = C5E7CEDF6EA681FE8119791B /* Feed.swift */; }; + EA8C7F894E3BE8D6D954DC47 /* InMemoryPositionsService.swift in Sources */ = {isa = PBXBuildFile; fileRef = 505BF8A630F7C7B96754E333 /* InMemoryPositionsService.swift */; }; EB4D11D2D1A0C64FF0E982C3 /* Optional.swift in Sources */ = {isa = PBXBuildFile; fileRef = CC925E451D875E5F74748EDC /* Optional.swift */; }; EDCA3449EA5683B37D82FEBE /* PDFKit.swift in Sources */ = {isa = PBXBuildFile; fileRef = ABAF1D0444B94E2CDD80087D /* PDFKit.swift */; }; EE951A131E38E316BF7A1129 /* LCPDialogViewController.xib in Resources */ = {isa = PBXBuildFile; fileRef = ED5C6546C24E5E619E4CC9D1 /* LCPDialogViewController.xib */; }; @@ -412,6 +413,7 @@ 4BB5D42EEF0083D833E2A572 /* Publication+OPDS.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "Publication+OPDS.swift"; sourceTree = ""; }; 4E564AE6D5137499C81FEBE2 /* TargetAction.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TargetAction.swift; sourceTree = ""; }; 500E55D9CA753D6D6AA76D10 /* EPUBLicenseContainer.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = EPUBLicenseContainer.swift; sourceTree = ""; }; + 505BF8A630F7C7B96754E333 /* InMemoryPositionsService.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = InMemoryPositionsService.swift; sourceTree = ""; }; 508E0CD4F9F02CC851E6D1E1 /* Publication+EPUB.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "Publication+EPUB.swift"; sourceTree = ""; }; 54699BC0E00F327E67908F6A /* Encryption.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Encryption.swift; sourceTree = ""; }; 55BC4119B8937D17ED80B1AB /* ControlFlow.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ControlFlow.swift; sourceTree = ""; }; @@ -982,6 +984,7 @@ 5BC52D8F4F854FDA56D10A8E /* Positions */ = { isa = PBXGroup; children = ( + 505BF8A630F7C7B96754E333 /* InMemoryPositionsService.swift */, 01CCE64AE9824DCF6D6413BC /* PerResourcePositionsService.swift */, BC45956B8991A9488F957B06 /* PositionsService.swift */, ); @@ -1882,6 +1885,7 @@ 39FC65D3797EF5069A04F34B /* HTTPFetcher.swift in Sources */, 2B57BE89EFAE517F79A17667 /* HTTPProblemDetails.swift in Sources */, FD13DEAC62A3ED6714841B7A /* HTTPRequest.swift in Sources */, + EA8C7F894E3BE8D6D954DC47 /* InMemoryPositionsService.swift in Sources */, 9C6B7AFB6FB0635EF5B7B71C /* JSON.swift in Sources */, 69150D0B00F5665C3DA0000B /* LazyResource.swift in Sources */, 5C9617AE1B5678A95ABFF1AA /* Link.swift in Sources */, From 489a20d057501d84e5d812e7301acfab1ee3d718 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mickae=CC=88l=20Menu?= Date: Tue, 10 May 2022 12:16:34 +0200 Subject: [PATCH 5/5] Fix build warnings --- .../Navigator/Audiobook/AudioNavigator.swift | 7 +++- .../CBZ/CBZNavigatorViewController.swift | 2 +- .../EPUB/EPUBNavigatorViewController.swift | 7 +++- .../Services/Search/StringSearchService.swift | 8 ++-- Sources/Shared/Toolkit/DocumentTypes.swift | 22 ++++++---- .../Library/LibraryViewController.swift | 19 ++++----- .../OPDSCatalogSelectorViewController.swift | 21 ++++++---- .../Publication/LocatorTests.swift | 41 ------------------- 8 files changed, 51 insertions(+), 76 deletions(-) diff --git a/Sources/Navigator/Audiobook/AudioNavigator.swift b/Sources/Navigator/Audiobook/AudioNavigator.swift index b4a741be9..af256f547 100644 --- a/Sources/Navigator/Audiobook/AudioNavigator.swift +++ b/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 diff --git a/Sources/Navigator/CBZ/CBZNavigatorViewController.swift b/Sources/Navigator/CBZ/CBZNavigatorViewController.swift index 979a0b476..76335cbb1 100644 --- a/Sources/Navigator/CBZ/CBZNavigatorViewController.swift +++ b/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) } diff --git a/Sources/Navigator/EPUB/EPUBNavigatorViewController.swift b/Sources/Navigator/EPUB/EPUBNavigatorViewController.swift index 75eaea02f..e296b8ed9 100644 --- a/Sources/Navigator/EPUB/EPUBNavigatorViewController.swift +++ b/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 { diff --git a/Sources/Shared/Publication/Services/Search/StringSearchService.swift b/Sources/Shared/Publication/Services/Search/StringSearchService.swift index fff15be4b..cf7674a21 100644 --- a/Sources/Shared/Publication/Services/Search/StringSearchService.swift +++ b/Sources/Shared/Publication/Services/Search/StringSearchService.swift @@ -161,13 +161,15 @@ public class _StringSearchService: _SearchService { return [] } - let currentLocale = options.language.map { Locale(identifier: $0) } ?? locale + let title = publication.tableOfContents.titleMatchingHREF(link.href) resourceLocator = resourceLocator.copy( - title: publication.tableOfContents.titleMatchingHREF(link.href) ?? link.title + title: Optional(title ?? link.title) ) var locators: [Locator] = [] + let currentLocale = options.language.map { Locale(identifier: $0) } ?? locale + for range in searchAlgorithm.findRanges(of: query, options: options, in: text, locale: currentLocale, cancellable: cancellable) { guard !cancellable.isCancelled else { return locators @@ -307,4 +309,4 @@ fileprivate extension Link { } return children.titleMatchingHREF(targetHREF) } -} \ No newline at end of file +} diff --git a/Sources/Shared/Toolkit/DocumentTypes.swift b/Sources/Shared/Toolkit/DocumentTypes.swift index d44143411..8f366162b 100644 --- a/Sources/Shared/Toolkit/DocumentTypes.swift +++ b/Sources/Shared/Toolkit/DocumentTypes.swift @@ -1,17 +1,17 @@ // -// DocumentTypes.swift -// r2-shared-swift -// -// Created by Mickaël Menu on 26.06.19. -// -// Copyright 2019 Readium Foundation. All rights reserved. -// Use of this source code is governed by a BSD-style license which is detailed -// in the LICENSE file present in the project repository where this source code is maintained. +// Copyright 2022 Readium Foundation. All rights reserved. +// Use of this source code is governed by the BSD-style license +// available in the top-level LICENSE file of the project. // import CoreServices import Foundation +#if canImport(UniformTypeIdentifiers) +import UniformTypeIdentifiers +#endif + + /// Provides a convenient access layer to the Document Types declared in the `Info.plist`, /// under `CFBundleDocumentTypes`. public struct DocumentTypes { @@ -25,6 +25,12 @@ public struct DocumentTypes { /// Supported UTIs. public let supportedUTIs: [String] + /// Supported UTTypes. + @available(iOS 14.0, *) + public var supportedUTTypes: [UTType] { + supportedUTIs.compactMap { UTType($0) } + } + /// Supported document media types. public let supportedMediaTypes: [MediaType] diff --git a/TestApp/Sources/Library/LibraryViewController.swift b/TestApp/Sources/Library/LibraryViewController.swift index 1514c910b..6c56bfa5b 100644 --- a/TestApp/Sources/Library/LibraryViewController.swift +++ b/TestApp/Sources/Library/LibraryViewController.swift @@ -19,7 +19,7 @@ import R2Streamer import R2Navigator import Kingfisher import ReadiumOPDS - +import UniformTypeIdentifiers protocol LibraryViewControllerFactory { func make() -> LibraryViewController @@ -50,11 +50,6 @@ class LibraryViewController: UIViewController, Loggable { @IBOutlet weak var collectionView: UICollectionView! { didSet { - // The contentInset of collectionVIew might be changed by iOS 9/10. - // This property has been set as false on storyboard. - // In case it's changed by mistake somewhere, set it again here. - self.automaticallyAdjustsScrollViewInsets = false - collectionView.backgroundColor = #colorLiteral(red: 1.0, green: 1.0, blue: 1.0, alpha: 1.0) collectionView.contentInset = UIEdgeInsets(top: 15, left: 20, bottom: 20, right: 20) @@ -155,9 +150,12 @@ class LibraryViewController: UIViewController, Loggable { } private func addBookFromDevice() { - var utis = DocumentTypes.main.supportedUTIs - utis.append(String(kUTTypeText)) - let documentPicker = UIDocumentPickerViewController(documentTypes: utis, in: .import) + var types = DocumentTypes.main.supportedUTTypes + if let type = UTType(String(kUTTypeText)) { + types.append(type) + } + + let documentPicker = UIDocumentPickerViewController(forOpeningContentTypes: types) documentPicker.delegate = self present(documentPicker, animated: true, completion: nil) } @@ -236,9 +234,6 @@ extension LibraryViewController { extension LibraryViewController: UIDocumentPickerDelegate { public func documentPicker(_ controller: UIDocumentPickerViewController, didPickDocumentsAt urls: [URL]) { - guard controller.documentPickerMode == .import else { - return - } importFiles(at: urls) } diff --git a/TestApp/Sources/OPDS/OPDSCatalogSelectorViewController.swift b/TestApp/Sources/OPDS/OPDSCatalogSelectorViewController.swift index 1e9e5d468..6f991589a 100644 --- a/TestApp/Sources/OPDS/OPDSCatalogSelectorViewController.swift +++ b/TestApp/Sources/OPDS/OPDSCatalogSelectorViewController.swift @@ -104,23 +104,30 @@ class OPDSCatalogSelectorViewController: UITableViewController { navigationController?.pushViewController(viewController, animated: true) } - override func tableView(_ tableView: UITableView, editActionsForRowAt indexPath: IndexPath) -> [UITableViewRowAction]? { - + override func tableView(_ tableView: UITableView, trailingSwipeActionsConfigurationForRowAt indexPath: IndexPath) -> UISwipeActionsConfiguration? { // action one - let editAction = UITableViewRowAction(style: .default, title: NSLocalizedString("edit_button", comment: "Edit a OPDS feed button"), handler: { (action, indexPath) in + let editAction = UIContextualAction( + style: .normal, + title: NSLocalizedString("edit_button", comment: "Edit a OPDS feed button") + ) { _, _, completion in self.showEditPopup(feedIndex: indexPath.row) - }) + completion(true) + } editAction.backgroundColor = UIColor.gray // action two - let deleteAction = UITableViewRowAction(style: .default, title: NSLocalizedString("remove_button", comment: "Remove an OPDS feed button"), handler: { (action, indexPath) in + let deleteAction = UIContextualAction( + style: .destructive, + title: NSLocalizedString("remove_button", comment: "Remove an OPDS feed button") + ) { _, _, completion in self.catalogData?.remove(at: indexPath.row) UserDefaults.standard.set(self.catalogData, forKey: self.userDefaultsID) self.tableView.reloadData() - }) + completion(true) + } deleteAction.backgroundColor = UIColor.gray - return [editAction, deleteAction] + return UISwipeActionsConfiguration(actions: [editAction, deleteAction]) } @objc func showAddFeedPopup() { diff --git a/Tests/SharedTests/Publication/LocatorTests.swift b/Tests/SharedTests/Publication/LocatorTests.swift index edfb0afca..3e3f1b5ed 100644 --- a/Tests/SharedTests/Publication/LocatorTests.swift +++ b/Tests/SharedTests/Publication/LocatorTests.swift @@ -75,47 +75,6 @@ class LocatorTests: XCTestCase { XCTAssertEqual([Locator](json: nil), []) } - func testMakeFromFullLink() { - XCTAssertEqual( - Locator(link: Link( - href: "http://locator", - type: "text/html", - title: "Link title" - )), - Locator( - href: "http://locator", - type: "text/html", - title: "Link title" - ) - ) - } - - func testMakeFromMinimalLink() { - XCTAssertEqual( - Locator(link: Link( - href: "http://locator" - )), - Locator( - href: "http://locator", - type: "", - title: nil - ) - ) - } - - func testMakeFromLinkWithFragment() { - XCTAssertEqual( - Locator(link: Link( - href: "http://locator#page=42" - )), - Locator( - href: "http://locator", - type: "", - locations: .init(fragments: ["page=42"]) - ) - ) - } - func testGetMinimalJSON() { AssertJSONEqual( Locator(