feat(android, ios, web): implement support for pdf.#118
Conversation
3eeeb5a to
c84dc78
Compare
jspizziri
left a comment
There was a problem hiding this comment.
@rhusted1982 I'm going to focus on reviewing one platform first (iOS) then once we have that down you can port the feedback to the rest. Then I'll review the other platforms as well.
| import ReadiumAdapterGCDWebServer | ||
|
|
||
| /// Provides a shared HTTP server backed by GCDWebServer for PDF resources. | ||
| enum PDFHTTPServer { |
There was a problem hiding this comment.
This seems identical to the EPUBHTTPServer.swift.
Wouldn't it be better to share a single HTTP service across both EPUB and PDF?
There was a problem hiding this comment.
Create HTTPServer in the common directory, use for both PDF and EPUB.
| let urlHash = abs(cleanURLString.hashValue) | ||
| let localURL = cacheDir.appendingPathComponent("\(urlHash).pdf") | ||
| if fileManager.fileExists(atPath: localURL.path) { | ||
| self.runOpenPipeline(url: localURL, bookId: bookId, locator: locator, selectionActions: selectionActions, sender: sender, completion: completion) |
There was a problem hiding this comment.
Can you explain why all of this is needed? Is there some sort of performance problem with streaming PDFs? The swift-toolkit doesn't seem to download streamed files like this.
Additionally, I have concerns about if .hashValue is actually consistent over time, as if I understand correctly, in swift > 4.2 .hashValue has a random seed (across processes)so the resulting hash of the same value will not be stable across them.
There was a problem hiding this comment.
This change, to cache the PDF before rendering it, was needed to avoid a performance problem that exists in readium 3.5.0. In 3.5.0, if it streamed the PDF over the HTTP server, it noticeably slower (as it chunked it 2K at a time) though it would ultimately work. Loading the PDF to the cache directory and opening it from there avoided the HTTP streaming and loaded much faster.
In readium 3.9.0, they did introduce changes that makes this caching moot as it moves away from the HTTP server altogether.
For the .hashValue, it would not be stable, per your comment I added a new hashURL private method to hash the url not leveraging .hashValue so it should now be stable.
|
|
||
| class PDFViewController: ReaderViewController { | ||
|
|
||
| private var isPDFViewConfigured = false |
There was a problem hiding this comment.
Could we simplify this whole configureNativePDFView / findNativePDFView / viewDidLayoutSubviews dance? It looks like the navigator already does most of this for us:
- Doesn't the
PDFPreferenceswe pass inPDFModule(scroll = true,scrollAxis = .vertical) already make the navigator setdisplaysAsBook,.singlePageContinuous, and the scroll view up in its ownapply(settings:)? - For the scale/
autoScalesbits we're actually overriding, can't we use the official hook:PDFNavigatorDelegate.navigator(_:setupPDFView:)? Don't we already conform to that protocol + are set as the delegate. That'd also let us drop the recursivefindNativePDFView, sincepdfNavigator.pdfViewis exposed publicly?
My worry with the current approach is that it reaches into the navigator's view tree and re-applies settings on a layout-timing one-shot, so it quietly fights the navigator's own apply() (e.g. autoScales/scaleFactor) and would break if we ever add a preferences UI or on rotation. The swift-toolkit TestApp's PDFViewController doesn't do any of this — it just sets preferences and the delegate.
Can we try leaning on PDFPreferences for the layout, and moving anything that's left into setupPDFView? If there was a specific reason the navigator defaults looked wrong (fit-to-width?).
Open to being wrong about this.
| self?.log(.error, "Failed to open publication: \(error)") | ||
| } | ||
| }, | ||
| receiveCompletion: { _ in }, |
There was a problem hiding this comment.
why did we remove error logging?
also strong vs weak capture?
| self.url(path: url) | ||
| .flatMap { self.openPublication(at: $0, allowUserInteraction: true, sender: sender ) } | ||
| if let remoteURL = URL(string: url), remoteURL.scheme != nil, remoteURL.scheme != "file", | ||
| remoteURL.pathExtension.lowercased() == "pdf" || url.contains(".pdf") { |
There was a problem hiding this comment.
it seems like we should rework PDF detection a bit. However, this is only relevant if we need to keep this downloading of streamed files workflow.
| bookId: String, | ||
| selectionActions: [SelectionActionData]? | ||
| ) throws -> ReaderViewController { | ||
| var preferences = PDFPreferences() |
There was a problem hiding this comment.
this seems to interact a bit with my other note about what we're doing in the PDFViewController
| }, [goToPage, container]); | ||
|
|
||
| return { pageNumber, pageCount, goForward, goBackward, goToLocator, isReady }; | ||
| }; No newline at end of file |
| id: 'mi-may-newsletter', | ||
| title: 'MI May Newsletter', | ||
| author: 'Militia Immaculatae', | ||
| epubUrl: 'https://militiaoftheimmaculata.com/wp-content/uploads/2020/06/2026-MAY-E-Pub-final-web.pdf', |
There was a problem hiding this comment.
Can we use a more conventional test PDFs?
There was a problem hiding this comment.
Updated from newsletters to Jane Austen's Sense and Sensibility.
| id: 'mi-june-newsletter', | ||
| title: 'MI June Newsletter', | ||
| author: 'Militia Immaculatae', | ||
| epubUrl: 'https://militiaoftheimmaculata.com/wp-content/uploads/2020/06/2026-JUNE-E-Pub-final-pages-web.pdf', |
There was a problem hiding this comment.
epubUrl as a key name no longer makes much sense.
There was a problem hiding this comment.
Added a new key, pdfUrl and a new hook usePdfFile to load those files. If epubPath or epubUrl are set, uses existing useEpubFile, if pdfUrl is set, uses new usePdfFile.
| Decoration, | ||
| SelectionAction, | ||
| PublicationReadyEvent, | ||
| ReadiumFile, |
There was a problem hiding this comment.
in the demo reader of an PDF, there's a large white "gap" at the top of the PDF viewer between the control bar and the first page of the PDF. Where is this coming from?
There was a problem hiding this comment.
Also, we need to carefully think about how "zoom" is supported on PDFs. I know there's at least a double-tap zoom on iOS but pinch to zoom seems like it might be necessary?
b5c74e1 to
71c98b8
Compare
71c98b8 to
393d1c4
Compare
Overview
The intent of this PR is to add PDF support on web and mobile.
Web
Web takes inspiration from the current EPUB functionality and follows that architecture to render the PDF inside the eReader for a custom layout.
Web leverages a custom layout or iframe to render the pdf in the eReader
Web will try to render using the custom layout, if this fails (and it can, CORS being one example), it will fall back on an iframe to render the pdf.
PDFs on web are rendered and scrolled vertically, basically the default.
Reader Settings from EPUB are not shown for PDF as they are not applicable.
Back and Next buttons will not be displayed since vertical scrolling.
Table of Contents is available for the custom page only, iframe cannot support this.
Mobile
Mobile takes inspiration from the current EPUB functionality and follows that architecture to render the PDF inside the eReader.
PDFs do not leverage the forward and back buttons, instead preferring vertical scrolling.
Reader Settings from EPUB are not shown for PDF as they are not applicable.
Back and Next buttons will not be displayed since vertical scrolling.
Table of Contents is supported.
Testing
Testing was done using a single and multiple column PDF.
Testing was done using the Example NextJS web app for web.
Testing was done using the Example Native app for iOS and Android. Testing involved Android Emulator for a mobile device, iOS Simulator for a mobile and tablet device.
Multiple Column PDF
iOS
Android
Web (rendered via iframe)
Single Column PDF
iOS
Android
Web (rendered via iframe)
Table of Contents
iOS
Android
Web (rendered via custom layout)