From ca4b2867207271fe4753eb4ea43dcaa7fa5b69dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20Menu?= Date: Wed, 14 Feb 2024 14:41:01 +0100 Subject: [PATCH 1/2] Fix audiobook regression --- .../Audiobook/PublicationMediaLoader.swift | 22 +++---- .../EPUB/EPUBNavigatorViewController.swift | 2 +- .../HTMLResourceContentIterator.swift | 63 ++++++++++--------- .../Audio/PublicationMediaLoaderTests.swift | 30 +++++++++ 4 files changed, 73 insertions(+), 44 deletions(-) create mode 100644 Tests/NavigatorTests/Audio/PublicationMediaLoaderTests.swift diff --git a/Sources/Navigator/Audiobook/PublicationMediaLoader.swift b/Sources/Navigator/Audiobook/PublicationMediaLoader.swift index 714b5df4e..bba1fdb41 100644 --- a/Sources/Navigator/Audiobook/PublicationMediaLoader.swift +++ b/Sources/Navigator/Audiobook/PublicationMediaLoader.swift @@ -84,7 +84,7 @@ final class PublicationMediaLoader: NSObject, AVAssetResourceLoaderDelegate { /// Terminates and removes the given loading request, cancelling it if necessary. private func finishRequest(_ request: AVAssetResourceLoadingRequest) { guard - let href = request.href, + let href = request.request.url?.audioHREF, var reqs = requests[href], let index = reqs.firstIndex(where: { req, _ in req == request }) else { @@ -106,7 +106,7 @@ final class PublicationMediaLoader: NSObject, AVAssetResourceLoaderDelegate { // MARK: - AVAssetResourceLoaderDelegate func resourceLoader(_ resourceLoader: AVAssetResourceLoader, shouldWaitForLoadingOfRequestedResource loadingRequest: AVAssetResourceLoadingRequest) -> Bool { - guard let href = loadingRequest.href else { + guard let href = loadingRequest.request.url?.audioHREF else { return false } @@ -163,22 +163,16 @@ final class PublicationMediaLoader: NSObject, AVAssetResourceLoaderDelegate { private let schemePrefix = "r2" -private extension AVAssetResourceLoadingRequest { - var href: String? { - guard let url = request.url?.absoluteURL, url.scheme.rawValue.hasPrefix(schemePrefix) == true else { +extension URL { + var audioHREF: String? { + guard let url = absoluteURL, url.scheme.rawValue.hasPrefix(schemePrefix) == true else { return nil } // The URL can be either: - // * r2file://directory/local-file.mp3 + // * r2:relative/file.mp3 + // * r2file:///directory/local-file.mp3 // * r2http(s)://domain.com/external-file.mp3 - switch url.scheme.rawValue { - case "r2file", "r2": - return url.path - case "r2http", "r2https": - return url.string.removingPrefix(schemePrefix) - default: - return nil - } + return url.string.removingPrefix(schemePrefix).removingPrefix(":") } } diff --git a/Sources/Navigator/EPUB/EPUBNavigatorViewController.swift b/Sources/Navigator/EPUB/EPUBNavigatorViewController.swift index 557b8f005..613a4b0fc 100644 --- a/Sources/Navigator/EPUB/EPUBNavigatorViewController.swift +++ b/Sources/Navigator/EPUB/EPUBNavigatorViewController.swift @@ -117,7 +117,7 @@ open class EPUBNavigatorViewController: UIViewController, decorationTemplates: [Decoration.Style.Id: HTMLDecorationTemplate] = HTMLDecorationTemplate.defaultTemplates(), fontFamilyDeclarations: [AnyHTMLFontFamilyDeclaration] = [], readiumCSSRSProperties: CSSRSProperties = CSSRSProperties(), - debugState: Bool = false + debugState: Bool = true ) { self.userSettings = userSettings self.preferences = preferences diff --git a/Sources/Shared/Publication/Services/Content/Iterators/HTMLResourceContentIterator.swift b/Sources/Shared/Publication/Services/Content/Iterators/HTMLResourceContentIterator.swift index 594916f28..5c3c48ac5 100644 --- a/Sources/Shared/Publication/Services/Content/Iterators/HTMLResourceContentIterator.swift +++ b/Sources/Shared/Publication/Services/Content/Iterators/HTMLResourceContentIterator.swift @@ -97,30 +97,12 @@ public class HTMLResourceContentIterator: ContentIterator { .readAsString() .eraseToAnyError() .tryMap { try SwiftSoup.parse($0) } - .tryMap { try parse(document: $0, locator: locator, beforeMaxLength: beforeMaxLength) } + .tryMap { try ContentParser(document: $0, baseLocator: locator, beforeMaxLength: beforeMaxLength).parse() } .map { adjustProgressions(of: $0) } resource.close() return result } - private func parse(document: Document, locator: Locator, beforeMaxLength: Int) throws -> ParsedElements { - let parser = try ContentParser( - baseLocator: locator, - startElement: locator.locations.cssSelector - .flatMap { - // The JS third-party library used to generate the CSS - // Selector sometimes adds `:root >`, which doesn't work - // with SwiftSoup. - try document.select($0.removingPrefix(":root > ")).first() - }, - beforeMaxLength: beforeMaxLength - ) - - try (document.body() ?? document).traverse(parser) - - return parser.result - } - private func adjustProgressions(of elements: ParsedElements) -> ParsedElements { let count = Double(elements.elements.count) guard count > 0 else { @@ -146,34 +128,57 @@ public class HTMLResourceContentIterator: ContentIterator { /// The `startIndex` will be calculated from the element matched by the /// base `locator`, if possible. Defaults to 0. private struct ParsedElements { - var elements: [ContentElement] = [] - var startIndex: Int = 0 + var elements: [ContentElement] + var startIndex: Int } private class ContentParser: NodeVisitor { + private let document: Document private let baseLocator: Locator private let baseHREF: AnyURL? private let startElement: Element? private let beforeMaxLength: Int - init(baseLocator: Locator, startElement: Element?, beforeMaxLength: Int) { + init(document: Document, baseLocator: Locator, beforeMaxLength: Int) { + self.document = document self.baseLocator = baseLocator baseHREF = AnyURL(string: baseLocator.href) - self.startElement = startElement + self.startElement = baseLocator.locations.cssSelector + .flatMap { + // The JS third-party library used to generate the CSS + // Selector sometimes adds `:root >`, which doesn't work + // with SwiftSoup. + try? document.select($0.removingPrefix(":root > ")).first() + } self.beforeMaxLength = beforeMaxLength } - - var result: ParsedElements { - ParsedElements( + + func parse() throws -> ParsedElements { + try (document.body() ?? document).traverse(self) + + if startIndex == nil, let text = baseLocator.text.highlight?.takeUnlessBlank()?.coalescingWhitespaces() { + print("\nSEARCH FOR \(text)\n") + var i = -1 + startIndex = elements.firstIndex { element in + i += 1 + guard let currentText = (element as? TextualContentElement)?.text?.coalescingWhitespaces() else { + return false + } + print("--- \(i) - \(currentText.hasPrefix(text) || text.hasPrefix(currentText))\n\(currentText)\n") + return currentText.hasPrefix(text) || text.hasPrefix(currentText) + } + } + + return ParsedElements( elements: elements, startIndex: (baseLocator.locations.progression == 1.0) ? elements.count - 1 - : startIndex + : (startIndex ?? 0) ) } private var elements: [ContentElement] = [] - private var startIndex = 0 + private var startIndex: Int? = nil /// Segments accumulated for the current element. private var segmentsAcc: [TextContentElement.Segment] = [] @@ -326,7 +331,7 @@ public class HTMLResourceContentIterator: ContentIterator { let parent = breadcrumbs.last - if startIndex == 0, startElement != nil, parent?.element == startElement { + if startIndex == nil, startElement != nil, parent?.element == startElement { startIndex = elements.count } diff --git a/Tests/NavigatorTests/Audio/PublicationMediaLoaderTests.swift b/Tests/NavigatorTests/Audio/PublicationMediaLoaderTests.swift new file mode 100644 index 000000000..addb9a191 --- /dev/null +++ b/Tests/NavigatorTests/Audio/PublicationMediaLoaderTests.swift @@ -0,0 +1,30 @@ +// +// Copyright 2024 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. +// + +@testable import R2Navigator +import XCTest + +class PublicationMediaLoaderTests: XCTestCase { + func testURLToHREF() { + XCTAssertEqual(URL(string: "r2:relative/file.mp3")!.audioHREF, "relative/file.mp3") + XCTAssertEqual(URL(string: "r2:/absolute/file.mp3")!.audioHREF, "/absolute/file.mp3") + XCTAssertEqual(URL(string: "r2file:///directory/file.mp3")!.audioHREF, "file:///directory/file.mp3") + XCTAssertEqual(URL(string: "r2http:///domain.com/file.mp3")!.audioHREF, "http:///domain.com/file.mp3") + XCTAssertEqual(URL(string: "r2https:///domain.com/file.mp3")!.audioHREF, "https:///domain.com/file.mp3") + + // Encoded characters + XCTAssertEqual(URL(string: "r2:relative/a%20file.mp3")!.audioHREF, "relative/a%20file.mp3") + XCTAssertEqual(URL(string: "r2:/absolute/a%20file.mp3")!.audioHREF, "/absolute/a%20file.mp3") + XCTAssertEqual(URL(string: "r2file:///directory/a%20file.mp3")!.audioHREF, "file:///directory/a%20file.mp3") + XCTAssertEqual(URL(string: "r2http:///domain.com/a%20file.mp3")!.audioHREF, "http:///domain.com/a%20file.mp3") + XCTAssertEqual(URL(string: "r2https:///domain.com/a%20file.mp3")!.audioHREF, "https:///domain.com/a%20file.mp3") + + // Ignores if the r2 prefix is missing. + XCTAssertNil(URL(string: "relative/file.mp3")!.audioHREF) + XCTAssertNil(URL(string: "file:///directory/file.mp3")!.audioHREF) + XCTAssertNil(URL(string: "http:///domain.com/file.mp3")!.audioHREF) + } +} From 57b872b455c8a25886e8eebe483dd293fdf9b515 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20Menu?= Date: Wed, 14 Feb 2024 14:41:46 +0100 Subject: [PATCH 2/2] Lint --- .../EPUB/EPUBNavigatorViewController.swift | 2 +- .../HTMLResourceContentIterator.swift | 63 +++++++++---------- .../Audio/PublicationMediaLoaderTests.swift | 4 +- 3 files changed, 32 insertions(+), 37 deletions(-) diff --git a/Sources/Navigator/EPUB/EPUBNavigatorViewController.swift b/Sources/Navigator/EPUB/EPUBNavigatorViewController.swift index 613a4b0fc..557b8f005 100644 --- a/Sources/Navigator/EPUB/EPUBNavigatorViewController.swift +++ b/Sources/Navigator/EPUB/EPUBNavigatorViewController.swift @@ -117,7 +117,7 @@ open class EPUBNavigatorViewController: UIViewController, decorationTemplates: [Decoration.Style.Id: HTMLDecorationTemplate] = HTMLDecorationTemplate.defaultTemplates(), fontFamilyDeclarations: [AnyHTMLFontFamilyDeclaration] = [], readiumCSSRSProperties: CSSRSProperties = CSSRSProperties(), - debugState: Bool = true + debugState: Bool = false ) { self.userSettings = userSettings self.preferences = preferences diff --git a/Sources/Shared/Publication/Services/Content/Iterators/HTMLResourceContentIterator.swift b/Sources/Shared/Publication/Services/Content/Iterators/HTMLResourceContentIterator.swift index 5c3c48ac5..594916f28 100644 --- a/Sources/Shared/Publication/Services/Content/Iterators/HTMLResourceContentIterator.swift +++ b/Sources/Shared/Publication/Services/Content/Iterators/HTMLResourceContentIterator.swift @@ -97,12 +97,30 @@ public class HTMLResourceContentIterator: ContentIterator { .readAsString() .eraseToAnyError() .tryMap { try SwiftSoup.parse($0) } - .tryMap { try ContentParser(document: $0, baseLocator: locator, beforeMaxLength: beforeMaxLength).parse() } + .tryMap { try parse(document: $0, locator: locator, beforeMaxLength: beforeMaxLength) } .map { adjustProgressions(of: $0) } resource.close() return result } + private func parse(document: Document, locator: Locator, beforeMaxLength: Int) throws -> ParsedElements { + let parser = try ContentParser( + baseLocator: locator, + startElement: locator.locations.cssSelector + .flatMap { + // The JS third-party library used to generate the CSS + // Selector sometimes adds `:root >`, which doesn't work + // with SwiftSoup. + try document.select($0.removingPrefix(":root > ")).first() + }, + beforeMaxLength: beforeMaxLength + ) + + try (document.body() ?? document).traverse(parser) + + return parser.result + } + private func adjustProgressions(of elements: ParsedElements) -> ParsedElements { let count = Double(elements.elements.count) guard count > 0 else { @@ -128,57 +146,34 @@ public class HTMLResourceContentIterator: ContentIterator { /// The `startIndex` will be calculated from the element matched by the /// base `locator`, if possible. Defaults to 0. private struct ParsedElements { - var elements: [ContentElement] - var startIndex: Int + var elements: [ContentElement] = [] + var startIndex: Int = 0 } private class ContentParser: NodeVisitor { - private let document: Document private let baseLocator: Locator private let baseHREF: AnyURL? private let startElement: Element? private let beforeMaxLength: Int - init(document: Document, baseLocator: Locator, beforeMaxLength: Int) { - self.document = document + init(baseLocator: Locator, startElement: Element?, beforeMaxLength: Int) { self.baseLocator = baseLocator baseHREF = AnyURL(string: baseLocator.href) - self.startElement = baseLocator.locations.cssSelector - .flatMap { - // The JS third-party library used to generate the CSS - // Selector sometimes adds `:root >`, which doesn't work - // with SwiftSoup. - try? document.select($0.removingPrefix(":root > ")).first() - } + self.startElement = startElement self.beforeMaxLength = beforeMaxLength } - - func parse() throws -> ParsedElements { - try (document.body() ?? document).traverse(self) - - if startIndex == nil, let text = baseLocator.text.highlight?.takeUnlessBlank()?.coalescingWhitespaces() { - print("\nSEARCH FOR \(text)\n") - var i = -1 - startIndex = elements.firstIndex { element in - i += 1 - guard let currentText = (element as? TextualContentElement)?.text?.coalescingWhitespaces() else { - return false - } - print("--- \(i) - \(currentText.hasPrefix(text) || text.hasPrefix(currentText))\n\(currentText)\n") - return currentText.hasPrefix(text) || text.hasPrefix(currentText) - } - } - - return ParsedElements( + + var result: ParsedElements { + ParsedElements( elements: elements, startIndex: (baseLocator.locations.progression == 1.0) ? elements.count - 1 - : (startIndex ?? 0) + : startIndex ) } private var elements: [ContentElement] = [] - private var startIndex: Int? = nil + private var startIndex = 0 /// Segments accumulated for the current element. private var segmentsAcc: [TextContentElement.Segment] = [] @@ -331,7 +326,7 @@ public class HTMLResourceContentIterator: ContentIterator { let parent = breadcrumbs.last - if startIndex == nil, startElement != nil, parent?.element == startElement { + if startIndex == 0, startElement != nil, parent?.element == startElement { startIndex = elements.count } diff --git a/Tests/NavigatorTests/Audio/PublicationMediaLoaderTests.swift b/Tests/NavigatorTests/Audio/PublicationMediaLoaderTests.swift index addb9a191..327c83203 100644 --- a/Tests/NavigatorTests/Audio/PublicationMediaLoaderTests.swift +++ b/Tests/NavigatorTests/Audio/PublicationMediaLoaderTests.swift @@ -14,14 +14,14 @@ class PublicationMediaLoaderTests: XCTestCase { XCTAssertEqual(URL(string: "r2file:///directory/file.mp3")!.audioHREF, "file:///directory/file.mp3") XCTAssertEqual(URL(string: "r2http:///domain.com/file.mp3")!.audioHREF, "http:///domain.com/file.mp3") XCTAssertEqual(URL(string: "r2https:///domain.com/file.mp3")!.audioHREF, "https:///domain.com/file.mp3") - + // Encoded characters XCTAssertEqual(URL(string: "r2:relative/a%20file.mp3")!.audioHREF, "relative/a%20file.mp3") XCTAssertEqual(URL(string: "r2:/absolute/a%20file.mp3")!.audioHREF, "/absolute/a%20file.mp3") XCTAssertEqual(URL(string: "r2file:///directory/a%20file.mp3")!.audioHREF, "file:///directory/a%20file.mp3") XCTAssertEqual(URL(string: "r2http:///domain.com/a%20file.mp3")!.audioHREF, "http:///domain.com/a%20file.mp3") XCTAssertEqual(URL(string: "r2https:///domain.com/a%20file.mp3")!.audioHREF, "https:///domain.com/a%20file.mp3") - + // Ignores if the r2 prefix is missing. XCTAssertNil(URL(string: "relative/file.mp3")!.audioHREF) XCTAssertNil(URL(string: "file:///directory/file.mp3")!.audioHREF)