Re-factor the code to remove all uses of PDFViewerApplication.downloadComplete
#18463
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
This was only necessary to prevent (unlikely) visual glitches when `disableAutoFetch = true` is being used. However, it turns out that we can move this functionality into the `ProgressBar` class instead by checking if the entire PDF document has loaded. This works since the API is always reporting 100% loading progress regardless of how the document was loaded; see [this code](https://github.com/mozilla/pdf.js/blob/ed83d7c5e16798a56c493d56aaa8200dd280bb17/src/display/api.js#L2735-L2740).
The old implementation in `PDFViewerApplication.download` means that if the `getDocument`-call hasn't yet downloaded the *entire* PDF document it will be re-downloaded. This seems generally undesirable since: - In some (probably rare) cases a URL may not be valid an arbitrary number of times, which means that the download may fail. - It will lead to wasted resources, since we'll end up fetching the same PDF document *twice* in that case (once via the `getDocument`-call and once to allow the user to save it). Hence this patch suggests that we change this very old code to instead always call the `PDFDocumentProxy.getData` method, since that'll trigger immediate downloading of the remaining document via the existing `getDocument`-call. Finally, the patch removes the `PDFViewerApplication.downloadComplete` property since it's now unused.
|
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://54.241.84.105:8877/29025d32dd12097/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/29025d32dd12097/output.txt Total script time: 1.04 mins Published |
timvandermeij
approved these changes
Jul 20, 2024
|
Thank you for simplifying this! |
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.
Stop using
downloadCompleteinPDFViewerApplication.progressThis was only necessary to prevent (unlikely) visual glitches when
disableAutoFetch = trueis being used.However, it turns out that we can move this functionality into the
ProgressBarclass instead by checking if the entire PDF document has loaded. This works since the API is always reporting 100% loading progress regardless of how the document was loaded; see this code.Avoid downloading the document twice in
PDFViewerApplication.downloadThe old implementation in
PDFViewerApplication.downloadmeans that if thegetDocument-call hasn't yet downloaded the entire PDF document it will be re-downloaded. This seems generally undesirable since:getDocument-call and once to allow the user to save it).Hence this patch suggests that we change this very old code to instead always call the
PDFDocumentProxy.getDatamethod, since that'll trigger immediate downloading of the remaining document via the existinggetDocument-call.Finally, the patch removes the
PDFViewerApplication.downloadCompleteproperty since it's now unused.