[api-minor] Remove the manual passing of an AnnotationStorage-instance when calling various API-method
#13207
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…nce when calling various API-method Note how we purposely don't expose the `AnnotationStorage`-class directly in the official API (see `src/pdf.js`), since trying to use *multiple* ones simultaneously doesn't really make sense (e.g. in the viewer). Instead we lazily initialize, and cache, just *one* instance via `PDFDocumentProxy.annotationStorage` which should thus be available internally in the API itself without having to be manually passed to various methods. To support these changes, the `AnnotationStorage`-instance initialization is moved into the `WorkerTransport`-class to allow both `PDFDocumentProxy` and `PDFPageProxy` to access it. This patch implements the following simplifications: - Remove the `annotationStorage`-parameter from `PDFDocumentProxy.saveDocument`, since it's already available internally. Furthermore, while it's currently possible to call that method without an `AnnotationStorage`-instance, that really does *not* make any sense at all. In this case you're effectively reducing `PDFDocumentProxy.saveDocument` to a "regular" `PDFDocumentProxy.getData` call, but with *a lot* more overhead, which was obviously not the intention of the `PDFDocumentProxy.saveDocument`-method. - Try to discourage third-party users from calling `PDFDocumentProxy.saveDocument` unconditionally, as a replacement for `PDFDocumentProxy.getData` (note the previous point). - Replace the `annotationStorage`-parameter, in `PDFPageProxy.render`, with a boolean `includeAnnotationStorage`-parameter which simply indicates if the (internally available) `AnnotationStorage`-instance should be used during rendering (e.g. for printing). - By removing the need to *manually* provide `annotationStorage`-parameters to various API-methods, using the API should become simpler (e.g. for third-parties) since you no longer need to worry about manually fetching and passing around this data.
…parameters These changes are done separately, to make it easier to remove them in the future.
|
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/1049e38337a39c4/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://3.101.106.178:8877/0f28b2034cc2ed0/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/1049e38337a39c4/output.txt Total script time: 25.00 mins
Image differences available at: http://54.67.70.0:8877/1049e38337a39c4/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://3.101.106.178:8877/0f28b2034cc2ed0/output.txt Total script time: 28.49 mins
Image differences available at: http://3.101.106.178:8877/0f28b2034cc2ed0/reftest-analyzer.html#web=eq.log |
timvandermeij
approved these changes
Apr 9, 2021
|
I agree that this is a simpler API overall. Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Note how we purposely don't expose the
AnnotationStorage-class directly in the official API (seesrc/pdf.js), since trying to use multiple ones simultaneously doesn't really make sense (e.g. in the viewer).Instead we lazily initialize, and cache, just one instance via
PDFDocumentProxy.annotationStoragewhich should thus be available internally in the API itself without having to be manually passed to various methods.To support these changes, the
AnnotationStorage-instance initialization is moved into theWorkerTransport-class to allow bothPDFDocumentProxyandPDFPageProxyto access it.This patch implements the following simplifications:
Remove the
annotationStorage-parameter fromPDFDocumentProxy.saveDocument, since it's already available internally.Furthermore, while it's currently possible to call that method without an
AnnotationStorage-instance, that really does not make any sense at all. In this case you're effectively reducingPDFDocumentProxy.saveDocumentto a "regular"PDFDocumentProxy.getDatacall, but with a lot more overhead, which was obviously not the intention of thePDFDocumentProxy.saveDocument-method.Try to discourage third-party users from calling
PDFDocumentProxy.saveDocumentunconditionally, as a replacement forPDFDocumentProxy.getData(note the previous point).Replace the
annotationStorage-parameter, inPDFPageProxy.render, with a booleanincludeAnnotationStorage-parameter which simply indicates if the (internally available)AnnotationStorage-instance should be used during rendering (e.g. for printing).By removing the need to manually provide
annotationStorage-parameters to various API-methods, using the API should become simpler (e.g. for third-parties) since you no longer need to worry about manually fetching and passing around this data.