From 1fac29d1849ebc4b2b29a090b587d1ffed9f3adf Mon Sep 17 00:00:00 2001
From: Jonas Jenwald <jonas.jenwald@gmail.com>
Date: Sat, 7 Mar 2020 13:11:51 +0100
Subject: [PATCH 1/2] Slightly improve the `BaseViewer.{firstPagePromise,
onePageRendered, pagesPromise}` functionality
There's a couple of issues with this functionality:
- The respective `PromiseCapability` instances are not being reset, in `BaseViewer._resetView`, when the document is closed which is inconsistent with all other state.
- While the default viewer depends on these promises, and they thus ought to be considered part of e.g. the `PDFViewer` API-surface, they're not really defined in a particularily user-visible way (being that they're attached to the `BaseViewer` instance *inline* in `BaseViewer.setDocument`).
- There's some internal `BaseViewer` state, e.g. `BaseViewer._pageViewsReady`, which is tracked manually and could instead be tracked indirectly via the relevant `PromiseCapability`, thus reducing the need to track state *twice* since that's always best to avoid.
*Please note:* In the existing implementation, these promises are not defined *until* the `BaseViewer.setDocument` method has been called.
While it would've been simple to lift that restriction in this patch, I'm purposely choosing *not* to do so since this ensures that any Promise handlers added inside of `BaseViewer.setDocument` are always invoked *before* any external ones (and keeping that behaviour seems generally reasonable).
---
web/app.js | 4 +---
web/base_viewer.js | 52 +++++++++++++++++++++++++++-------------------
2 files changed, 32 insertions(+), 24 deletions(-)
diff --git a/web/app.js b/web/app.js
index ea154e465..00ba6264a 100644
--- a/web/app.js
+++ b/web/app.js
@@ -1057,9 +1057,7 @@ const PDFViewerApplication = {
const pdfViewer = this.pdfViewer;
pdfViewer.setDocument(pdfDocument);
- const firstPagePromise = pdfViewer.firstPagePromise;
- const pagesPromise = pdfViewer.pagesPromise;
- const onePageRendered = pdfViewer.onePageRendered;
+ const { firstPagePromise, onePageRendered, pagesPromise } = pdfViewer;
const pdfThumbnailViewer = this.pdfThumbnailViewer;
pdfThumbnailViewer.setDocument(pdfDocument);
diff --git a/web/base_viewer.js b/web/base_viewer.js
index 6395fe177..3861acf95 100644
--- a/web/base_viewer.js
+++ b/web/base_viewer.js
@@ -198,13 +198,13 @@ class BaseViewer {
* @type {boolean} - True if all {PDFPageView} objects are initialized.
*/
get pageViewsReady() {
- if (!this._pageViewsReady) {
+ if (!this._pagesCapability.settled) {
return false;
}
// Prevent printing errors when 'disableAutoFetch' is set, by ensuring
// that *all* pages have in fact been completely loaded.
return this._pages.every(function(pageView) {
- return !!(pageView && pageView.pdfPage);
+ return pageView && pageView.pdfPage;
});
}
@@ -376,6 +376,21 @@ class BaseViewer {
}
}
+ get firstPagePromise() {
+ return this.pdfDocument ? this._firstPageCapability.promise : null;
+ }
+
+ get onePageRendered() {
+ return this.pdfDocument ? this._onePageRenderedCapability.promise : null;
+ }
+
+ get pagesPromise() {
+ return this.pdfDocument ? this._pagesCapability.promise : null;
+ }
+
+ /**
+ * @private
+ */
get _setDocumentViewerElement() {
// In most viewers, e.g. `PDFViewer`, this should return `this.viewer`.
throw new Error("Not implemented: _setDocumentViewerElement");
@@ -399,24 +414,15 @@ class BaseViewer {
return;
}
const pagesCount = pdfDocument.numPages;
+ const firstPagePromise = pdfDocument.getPage(1);
- const pagesCapability = createPromiseCapability();
- this.pagesPromise = pagesCapability.promise;
-
- pagesCapability.promise.then(() => {
- this._pageViewsReady = true;
+ this._pagesCapability.promise.then(() => {
this.eventBus.dispatch("pagesloaded", {
source: this,
pagesCount,
});
});
- const onePageRenderedCapability = createPromiseCapability();
- this.onePageRendered = onePageRenderedCapability.promise;
-
- const firstPagePromise = pdfDocument.getPage(1);
- this.firstPagePromise = firstPagePromise;
-
this._onBeforeDraw = evt => {
const pageView = this._pages[evt.pageNumber - 1];
if (!pageView) {
@@ -429,10 +435,10 @@ class BaseViewer {
this.eventBus._on("pagerender", this._onBeforeDraw);
this._onAfterDraw = evt => {
- if (evt.cssTransform || onePageRenderedCapability.settled) {
+ if (evt.cssTransform || this._onePageRenderedCapability.settled) {
return;
}
- onePageRenderedCapability.resolve();
+ this._onePageRenderedCapability.resolve();
this.eventBus._off("pagerendered", this._onAfterDraw);
this._onAfterDraw = null;
@@ -443,6 +449,8 @@ class BaseViewer {
// viewport for all pages
firstPagePromise
.then(firstPdfPage => {
+ this._firstPageCapability.resolve(firstPdfPage);
+
const scale = this.currentScale;
const viewport = firstPdfPage.getViewport({ scale: scale * CSS_UNITS });
for (let pageNum = 1; pageNum <= pagesCount; ++pageNum) {
@@ -485,7 +493,7 @@ class BaseViewer {
// Fetch all the pages since the viewport is needed before printing
// starts to create the correct size canvas. Wait until one page is
// rendered so we don't tie up too many resources early on.
- onePageRenderedCapability.promise.then(() => {
+ this._onePageRenderedCapability.promise.then(() => {
if (this.findController) {
this.findController.setDocument(pdfDocument); // Enable searching.
}
@@ -497,13 +505,13 @@ class BaseViewer {
pagesCount > 7500
) {
// XXX: Printing is semi-broken with auto fetch disabled.
- pagesCapability.resolve();
+ this._pagesCapability.resolve();
return;
}
let getPagesLeft = pagesCount - 1; // The first page was already loaded.
if (getPagesLeft <= 0) {
- pagesCapability.resolve();
+ this._pagesCapability.resolve();
return;
}
for (let pageNum = 2; pageNum <= pagesCount; ++pageNum) {
@@ -515,7 +523,7 @@ class BaseViewer {
}
this.linkService.cachePageRef(pageNum, pdfPage.ref);
if (--getPagesLeft === 0) {
- pagesCapability.resolve();
+ this._pagesCapability.resolve();
}
},
reason => {
@@ -524,7 +532,7 @@ class BaseViewer {
reason
);
if (--getPagesLeft === 0) {
- pagesCapability.resolve();
+ this._pagesCapability.resolve();
}
}
);
@@ -577,7 +585,9 @@ class BaseViewer {
this._location = null;
this._pagesRotation = 0;
this._pagesRequests = new WeakMap();
- this._pageViewsReady = false;
+ this._firstPageCapability = createPromiseCapability();
+ this._onePageRenderedCapability = createPromiseCapability();
+ this._pagesCapability = createPromiseCapability();
this._scrollMode = ScrollMode.VERTICAL;
this._spreadMode = SpreadMode.NONE;
From 3eb4c1940dd94fd11d239bf1b36d3582b6aa3781 Mon Sep 17 00:00:00 2001
From: Jonas Jenwald <jonas.jenwald@gmail.com>
Date: Sat, 7 Mar 2020 22:24:27 +0100
Subject: [PATCH 2/2] Initialize the `textLayerFactory` once in
`BaseViewer.setDocument`, rather than repeating it for every page
For reasons that I don't even pretend to understand, the `textLayerFactory` property is determined for *every single* page in the PDF document.
Given that the `TextLayerMode` should be consistent for *all* pages in a document, we obviously could/should define `textLayerFactory` just once instead.
---
web/base_viewer.js | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/web/base_viewer.js b/web/base_viewer.js
index 3861acf95..4b2fbe734 100644
--- a/web/base_viewer.js
+++ b/web/base_viewer.js
@@ -453,11 +453,10 @@ class BaseViewer {
const scale = this.currentScale;
const viewport = firstPdfPage.getViewport({ scale: scale * CSS_UNITS });
+ const textLayerFactory =
+ this.textLayerMode !== TextLayerMode.DISABLE ? this : null;
+
for (let pageNum = 1; pageNum <= pagesCount; ++pageNum) {
- let textLayerFactory = null;
- if (this.textLayerMode !== TextLayerMode.DISABLE) {
- textLayerFactory = this;
- }
const pageView = new PDFPageView({
container: this._setDocumentViewerElement,
eventBus: this.eventBus,