Update Protocols for a PDFRenderer to conform to #1
Conversation
| @@ -9,67 +9,92 @@ | |||
| import Foundation | |||
| import UIKit | |||
|
|
|||
| // the items that are marked required are needed to | |||
There was a problem hiding this comment.
This will be a theme in a couple places, but basically: the protocols here are expected to represent any pdf renderer.
If we define things here that only PSPDFKit uses, everything would have to be optional and a developer would not know what is needed to construct the class when they try and contribute another renderer.
Example, these properties would be in a class like PSPDFKitMinitexAnnotation : MinitexPDFAnnotation inside of "PDF-iOS" instead of in the original protocol. One can always add properties to a class that conforms to this protocol later.
| } | ||
| /// Complex annotations may be fully described by something like JSON from a particular | ||
| /// renderer. This allows easy access by the host to a version it can store or sync. | ||
| var serializedRepresentation: Data { get } |
There was a problem hiding this comment.
In asking the question of "what is the bare minimum" of what an annotation should be, the answer would be "a page that marks where it is" and "some serializable data" that represents it. It could be a drawing, bounding box, highlight, note, etc.. So it's very high level right now.
Every renderer is very likely to have different representations and features for annotations. That is solved by creating a new class within that PDFRenderer implementation that adheres to this protocol. The host is only concerned with this limited set of properties and functions.
| @objc public protocol MinitexPDFViewController: class { | ||
| // we have to pass in a dictionary because @objc protocol function |
There was a problem hiding this comment.
Addressing just the comment here: There must have been an unrelated build error (it does happen often) preventing this from working. Updating this, along with the factory init, to use typed parameters is working okay.
| @objc public protocol MinitexPDFViewController: class { | ||
| // we have to pass in a dictionary because @objc protocol function | ||
| // cannot accept multiple parameters | ||
| @objc init(dictionary: [String: Any]) |
There was a problem hiding this comment.
In SimplyE, some classes are reconstructed from JSON representations, which take in "dictionaries" to initialize. In the general case, though, knowing exactly what parameters are required, to init a class, is standard for consuming an API, and it helps to describe to the host what should be done in advance of initialization, nil checks, etc.
Relying on a dictionary, where one does not know its contents, forces a developer to research the class internally. Requirements can be changed without any build errors, which hides bugs further down the road.
| } | ||
|
|
||
| /// A page in a PDF document. | ||
| @objc public protocol MinitexPDFPage: class { |
There was a problem hiding this comment.
A type for a "page" is created. It self-describes in other places in the code (example: let property: MinitexPDFPage). We define a minimum baseline of needing a UInt to represent a page, but other renderers may need other interal representations, so as long as SomeUnknownRenderer provides a getter for a UInt, it can use its own internal representation of a "page."
|
|
||
| @objc public protocol MinitexPDFViewControllerDelegate: class { | ||
| @objc func userDidNavigate(page: Int) | ||
| @objc func saveBookmarks(pageNumbers: [UInt]) | ||
| @objc func saveAnnotations(annotations: [MinitexPDFAnnotation]) |
There was a problem hiding this comment.
Small updates to the delegate methods to align with standard convention as well as letting the host decide ultimately what to do:
It's up to the API consumer / host to decide if it wants to "save" bookmarks or not. The first method is correct by convention to describe something that is being done: "userDidNavigate", so all the methods names are updated to this format for consistency.
As long as a delegate receives every bookmark created, it can keep its own array of them saved, if it wants to.
Documentation has also been added to all the functions.
| /// - bookmarks: any saved bookmarks by the user | ||
| /// - annotations: any additional generic annotations created by the user | ||
| /// - Returns: An instance of MinitexPDFViewController, or nil if something failed. | ||
| public static func create(fileUrl: URL, |
There was a problem hiding this comment.
The factory init is made to match the protocol init for the same view controller.
| as? MinitexPDFViewController.Type else { | ||
| return nil | ||
| } | ||
| /// The API host must create a View Controller for the PDF from a static factory method. |
There was a problem hiding this comment.
This style of documentation will also show up in the Xcode UI if someone command-clicks on the method.
| if let pdfViewControllerClass = NSClassFromString("PDF.PDFViewController") as? MinitexPDFViewController.Type { | ||
| return pdfViewControllerClass.init(file: fileUrl, openToPage: page, bookmarks: bookmarks, annotations: annotations) | ||
| } else { | ||
| // PDF renderer using Apple's PDFKit. |
There was a problem hiding this comment.
Just like in Dean's audiobook frameworks, there will be a generic implementation of PDF support provided if no other renderer is present.
Add types where necessary. Create protocols for common types like PDF pages and annotations. Create intentional init methods with these types. Add header documentation. These protocols should provide a common starting point to create and manage PDFs, no matter which underlying PDF renderer is used.
gioneill
force-pushed
the
oa-support
branch
2 times, most recently
from
7a1bb8c
to
1a59fe1
Compare
| public let JSONData: Data? | ||
| /// A generic annotation in a PDF document. | ||
| @objc public protocol MinitexPDFAnnotation: class { | ||
| /// An annotation must belong to one page of the PDF document. |
There was a problem hiding this comment.
Based on Paul's note, this comment should be updated to denote either that the annotation is wholly contained to this page OR BEGINS on this page. Example, a highlight that may stretch between multiple pages.
|
@gioneill I'm doing some cleanup on this repo, was there a reason to leave this PR open? I see this is merging into Also, this repo has many branches but they all appear merged to master, I was going to remove them all unless they were left for a particular reason. |
The following Pull Request is being made to create discussion on specific updates to the "PDFRendererProvider" set of Protocols.
At a high level:
Demo builds of "PDF-iOS", imported into SimplyE forks, represent a successful ability to launch and navigate PDF files using the third party PSPDFKit SDK. "PDF-iOS" represents one of potentially many "PDFRenderer's", which, by this set of protocols would be provided to a host like SimplyE. This is successful work implemented by Vui N. and Paul S. of the Minitex partnership to NYPL and Library Simplified.
I recommend viewing these comments in the split-view "files changed" view.