These changes consists mainly of replacing `var` with `let`/`const`, adding a couple of default parameters to function signatures, and finally converting `EventBus`/`ProgressBar` to proper classes.
After PR 8510, we now always lookup the localized `page_scale_percent` string to prevent any possible ordering issues. Since the scaleSelect dropdown is updated asynchronous, there's really no point in having a helper function any more, hence this code can rather be placed inline in `Toolbar._updateUIState`.
*This is an existing issue that I noticed while testing PR 8552.*
When zooming or rotation occurs, we'll try to use the current canvas as a (CSS transformed) preview until the page has been completely re-drawn.
If you manage to change the scale (or rotation) *very* quickly, it's possible that `PDFPageView.update` can be called *before* a previous `render` operation has progressed far enough to remove the `hidden` property from the canvas.
The result is thus that a page may be *entirely* black during zooming or rotation, which doesn't look very good. This effect can be a bit difficult to spot, but it does manifest even in the default viewer.
Part of the rotation handling code, in what's now `web/app.js`, hasn't really changed since before the viewer was split into multiple files/components.
Similar to other properties, such as current page/scale, we should probably avoid tracking state in multiple places. Hence I'm suggesting that we don't store the rotation in `PDFViewerApplication`, and access the value in `PDFViewer` instead.
Since `PDFViewerApplication.pageRotation` has existed for a very long time, a getter was added to avoid outright breaking third-party code that may depend on it.
Since this call occurs *before* the `PDFViewer.setDocument` call, it won't actually cause any scale change.
Furthermore, moving it should not be necessary, since the `scale` is already used as the fallback case in `PDFViewerApplication.setInitialView` (provided it's non-zero, which isn't even the case in the default viewer).
Hence this patch should cause no functional changes at all, since it simply removes a piece of unnecessary code.
This patch adds Streams API support in getTextContent
so that we can stream data in chunks instead of fetching
whole data from worker thread to main thread. This patch
supports Streams API without changing the core functionality
of getTextContent.
Enqueue textContent directly at getTextContent in partialEvaluator.
Adds desiredSize and ready property in streamSink.
Currently, these properties are reset in what appears to be somewhat arbitrary locations (within the `load` and `open` methods respectively). The explanation is probably that both of these properties predates the existence of any centralized clean-up code in the viewer.
Hence I think that it makes sense to move the resetting of these properties to the `close` method, since that improves the overview of what's actually cleaned-up/reset when changing documents in the viewer.
This method is currently called from `PDFViewer._scrollUpdate` on *every* scroll event in the viewer.
However, I cannot see why this code is now necessary (assuming that it once was), since text-selection and searching still works *exactly* the same way with this patch as with the current `master`.
When `PDFPageView.updatePosition` is called, the page can be in either of these states:
1. The page hasn't been rendered, in which case the `textLayer` property doesn't exist yet.
2. The page is currently rendering, meaning that the `textLayer` property exists. Given that the `textContent` won't be fetched until the page has been successfully rendered, `TextLayerBuilder.render` will return immediately and effectively be a no-op (since there's nothing to render yet).
3. The has been been rendered, and the `textLayer` is currently rendering.
4. The page, and its `textLayer`, has been completely rendered. In this case, `TextLayerBuilder.render` will return immediately and effectively be a no-op.
Here, only the *third* case seem to require any further analysis:
When scrolling occurs while the `textLayer` is rendering, `TextLayerBuilder.render` will via a helper method call `TextLayerRenderTask.cancel` (in src/display/text_layer.js) to stop processing.
However, due to the run-to-completion nature of JavaScript, once `TextLayerRenderTask._render` has been invoked `appendText` will always run.[1]
So even though we cancel rendering of pending `textLayer`s during scrolling, via the repeated `TextLayerBuilder.render` calls from within the `PDFPageView.updatePosition` method, that does *not* prevent us from running the code inside of `TextLayerRenderTask._render` over and over for the *same* page; which all seems *very* inefficient to me.[2]
All this will thus have the effect of delaying the *actual* rendering of a `textLayer` ever so slightly while scrolling in the viewer. However, it does so at the expense of potentially hundreds of unnecessary `appendText` calls.[3]
Hence it seems to me that it's less resource intensive overall to simply let rendering of the `textLayer` complete, once it has started. Obviously, we still abort all rendering of a page, and its `textLayer`, when it's being destroyed (e.g. by being evicted from the page cache).
In case that there's any worry that the patch could affect e.g. highlighting of search results, please note that the existing code in `TextLayerBuilder.render` already calls `updateMatches` when the `TextLayerTask` resolves successfully.
*I'm sorry that this became quite long, but to try and summarize:*
`PDFPageView.updatePosition` doesn't actually do anything in *most* cases. In the one case where it matters, it seems that it's actually doing more harm than good; which is why I'm proposing that we just remove it.
---
[1] Although we may be able to skip the `render` call, provided that it happens *after* a `timeout` (as is the case in the default viewer).
[2] With current work being done to support streaming of `TextContent`, we'd also need to add just as many duplicate API calls to `PDFPageView.updatePosition`.
[3] The number of duplicate `appendText` calls is directly proportional not only to the scroll speed, but also to the number of pages in the document.
Since the localization service is now asynchronous, depending on the load the browser is under, there's a small risk that the lookup of the 'page_scale_percent' string could be delayed slightly.
If the scale would change a couple of times in *very* quick succession, there's perhaps a *theoretical* possibility that the Zoom dropdown would display an incorrect value.
Consider the following, somewhat contrived, theoretical example of two zoom commands being executed *right* after one another:
```javascript
PDFViewerApplication.pdfViewer.currentScale = 1.23;
PDFViewerApplication.pdfViewer.currentScaleValue = 'page-width';
```
Only the `currentScale` call will currently trigger a l10n lookup in `selectScaleOption`. However, as far as I understand, there's no *guarantee* that the l10n string is resolved *before* `selectScaleOption` is called again as a result of the `currentScaleValue` call.
This thus has the possibility of putting the Zoom dropdown into an inconsistent state, since it's currently updated synchronously for one code-path and asynchronously for another.
To avoid these issues, I'm proposing that we *always* update the Zoom dropdown asynchronously, such that we can guarantee that the ordering is correct.
After PR 8394, at least in Firefox, the Zoom dropdown now frequently displays an old custom scale instead of the correct one. To see this behaviour, the following STR works for me:
1. Open https://mozilla.github.io/pdf.js/web/viewer.html.
2. Zoom in, by clicking on the corresponding button in the toolbar.
3. Run `PDFViewerApplication.pdfViewer.currentScaleValue` in the console.
4. Compare what's displayed in the Zoom dropdown with what's printed in the console. (If no difference can be observed, try repeating steps 2 through 4 a couple of times.)
I really don't understand why this happens, but it seems that waiting until a custom scale has been set *before* selecting it fixes things in Firefox (and works fine in e.g. Chrome as well).
Note that this patch thus makes this particular piece of the code consistent with the state prior to PR 8394.
The click handler used in Presentation Mode didn't check if the first/last page was already reached, which after PR 7529 now causes an unnecessary console error.
Hence we should simply use the already existing `_goToPreviousPage`/`_goToNextPage` methods instead, since they do the necessary bounds checking.
Fixes 8498.
Moreover, rename `FindStates` to `FindState` since enumeration names are
usually not in plural, for readability and consistency with the ones in
`src/shared/util.js`.
This patch intends to simplify future ES6 refactoring of the thumbnail code, since the current code isn't going to work well together with proper `Class`es.
With the current way that the `HandTool` is implemented, if someone would try to also add a Zoom tool (as issue 1260 asks for) that probably wouldn't work very well given that you'd then have two cursor tools which may not play nice together.
Hence this patch, which attempts to refactor things so that it should be simpler to add e.g. a Zoom tool as well (given that that issue is marked as "good-beginner-bug", and I'm not sure if that really applies considering the current state of the code).
Note that I personally have no interest in implementing a Zoom tool (similar to Adobe Reader) since I wouldn't use it, but I figured that it can't hurt to make this code a bit more future proof.
PR 7341 added special handling for `nameddest`s that look like pageNumbers, to prevent issues since we previously *incorrectly* supported specifying a pageNumber directly in the hash; i.e. `#10` versus the correct `#page=10` format.
Since this behaviour wasn't correct, PR 7757 fixed and deprecated the old format, which means that we no longer need to maintain the `nameddest` hack in multiple files.
This patch replaces a `var self = this;` statement with arrow functions, and `var` with `let` in `PDFLinkService.navigateTo`.
Furthermore, when I started looking at this method, it quickly became clear that its code is somewhat of a mess. Since I'm one of the persons that have touched this code over the years, I figured that it'd be a good idea to try and clean it up a bit.
Also replaces `var` with `let` in code that's touched in the patch. Please note that this should be completely safe, for two separate reasons, since trying to access let in a scope where it's not defined is first of all a runtime error and second of all an ESLint error (thanks to the `no-undef` rule).
Currently this method first uses a loop to build a temporary array to hold Promises, which are then resolved from a recursive helper function once the textContent is fetched for each page.
To me, this is unncessarily complicated, since we can do everything within one loop by simply chaining the asynchronous calls to retrieve the textContent. (Note that this guarantees that the textContent of the pages is still fetched sequentially.)
Also replaces `var` with `let` in the functions/methods that are touched in the patch. Please note that this should be completely safe, for two separate reasons, since trying to access `let` in a scope where it's not defined is first of all a runtime error and second of all an ESLint error (thanks to the `no-undef` rule).
This patch contains the following improvements:
- Only fetch the various document properties *once* per PDF file opened, and cache the result (in a frozen object).
- Always update the *entire* dialog at once, to prevent inconsistent UI state (issue 8371).
- Ensure that the dialog, and all its internal properties, are reset when `PDFViewerApplication.close` is called.
- Inline, and re-factor, the `getProperties` method in `open`, since that's the only call-site.
- Always overwrite the fileSize with the value obtained from `pdfDocument.getDownloadInfo`, to ensure that it's correct.
- ES6-ify the code that's touched in this patch.
Fixes 8371.
Please see http://eslint.org/docs/rules/object-shorthand.
For the most part, these changes are of the search-and-replace kind, and the previously enabled `no-undef` rule should complement the tests in helping ensure that no stupid errors crept into to the patch.
Rather than having to manually use `initializedPromise`, which really ought to be a private property, to ensure that setting/getting values in the `ViewHistory` works as intended, this re-factoring simply changes all of its methods to be asynchronous.
Furthermore, a `getMultiple` method (mirroring the existing `setMultiple` one) is also added to `ViewHistory`.
Finally, this patch also addresses an existing issue, where certain preferences (e.g. the default zoom level) would be ignored when calling `setInitialView` if reading from the `ViewHistory` fails for some reason.
With the exception of just one test-case, all the current `ui_utils` unit-tests can run successfully on Node.js (since most of them doesn't rely on the DOM).
To get this working, I had to first of all add a new `LIB` build flag such that `gulp lib` produces a `web/pdfjs.js` file that is able to load `pdf.js` successfully.
Second of all, since neither `document` nor `navigator` is available in Node.js, `web/ui_utils.js` was adjusted slightly to avoid errors.
This prevents issues with the filename detection being skipped, when trying to download the opened PDF attachment, since `getPDFFileNameFromURL` ignores `data:` URLs for performance reasons.
Jonas Jenwald
Tim van der Meij
Yury Delendik
Mukul Mishra
curiosity26