Skip to content

Commit

Permalink
Improve how we disable PDFThumbnailView.setImage for documents with…
Browse files Browse the repository at this point in the history
… Optional Content (PR 12170 follow-up)

Rather than always disable `PDFThumbnailView.setImage` as soon as user has changed the visibility of the Optional Content, we can utilize the new method added in the previous patch to improve thumbnail performance. Note in particular how, in the old code, even *resetting* of the Optional Content to its default state wouldn't enable `PDFThumbnailView.setImage` again.

While slightly unrelated, this patch also removes the `PDFThumbnailViewer._optionalContentConfigPromise`-property since it's completely unused.
  • Loading branch information
Snuffleupagus committed Jul 24, 2022
1 parent 17ab0f0 commit e9120f3
Show file tree
Hide file tree
Showing 4 changed files with 38 additions and 25 deletions.
1 change: 1 addition & 0 deletions web/base_viewer.js
Original file line number Diff line number Diff line change
Expand Up @@ -1824,6 +1824,7 @@ class BaseViewer {
return Promise.resolve(null);
}
if (!this._optionalContentConfigPromise) {
console.error("optionalContentConfigPromise: Not initialized yet.");
// Prevent issues if the getter is accessed *before* the `onePageRendered`
// promise has resolved; won't (normally) happen in the default viewer.
return this.pdfDocument.getOptionalContentConfig();
Expand Down
36 changes: 36 additions & 0 deletions web/pdf_page_view.js
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,8 @@ const MAX_CANVAS_PIXELS = compatibilityParams.maxCanvasPixels || 16777216;
class PDFPageView {
#annotationMode = AnnotationMode.ENABLE_FORMS;

#useThumbnailCanvas = true;

/**
* @param {PDFPageViewOptions} options
*/
Expand Down Expand Up @@ -174,6 +176,22 @@ class PDFPageView {
this.div = div;

container?.append(div);

if (this._isStandalone) {
const { optionalContentConfigPromise } = options;
if (optionalContentConfigPromise) {
// Ensure that the thumbnails always display the *initial* document
// state.
optionalContentConfigPromise.then(optionalContentConfig => {
if (
optionalContentConfigPromise !== this._optionalContentConfigPromise
) {
return;
}
this.#useThumbnailCanvas = optionalContentConfig.hasInitialVisibility;
});
}
}
}

setPdfPage(pdfPage) {
Expand Down Expand Up @@ -376,6 +394,16 @@ class PDFPageView {
}
if (optionalContentConfigPromise instanceof Promise) {
this._optionalContentConfigPromise = optionalContentConfigPromise;

// Ensure that the thumbnails always display the *initial* document state.
optionalContentConfigPromise.then(optionalContentConfig => {
if (
optionalContentConfigPromise !== this._optionalContentConfigPromise
) {
return;
}
this.#useThumbnailCanvas = optionalContentConfig.hasInitialVisibility;
});
}

const totalRotation = (this.rotation + this.pdfPageRotate) % 360;
Expand Down Expand Up @@ -1003,6 +1031,14 @@ class PDFPageView {
this.div.removeAttribute("data-page-label");
}
}

/**
* For use by the `PDFThumbnailView.setImage`-method.
* @ignore
*/
get thumbnailCanvas() {
return this.#useThumbnailCanvas ? this.canvas : null;
}
}

export { PDFPageView };
12 changes: 1 addition & 11 deletions web/pdf_thumbnail_view.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ const THUMBNAIL_WIDTH = 98; // px
* The default value is `null`.
* @property {IPDFLinkService} linkService - The navigation/linking service.
* @property {PDFRenderingQueue} renderingQueue - The rendering queue object.
* @property {function} checkSetImageDisabled
* @property {IL10n} l10n - Localization service.
* @property {Object} [pageColors] - Overwrites background and foreground colors
* with user defined ones in order to improve readability in high contrast
Expand Down Expand Up @@ -88,7 +87,6 @@ class PDFThumbnailView {
optionalContentConfigPromise,
linkService,
renderingQueue,
checkSetImageDisabled,
l10n,
pageColors,
}) {
Expand All @@ -109,11 +107,6 @@ class PDFThumbnailView {
this.renderTask = null;
this.renderingState = RenderingStates.INITIAL;
this.resume = null;
this._checkSetImageDisabled =
checkSetImageDisabled ||
function () {
return false;
};

const pageWidth = this.viewport.width,
pageHeight = this.viewport.height,
Expand Down Expand Up @@ -356,13 +349,10 @@ class PDFThumbnailView {
}

setImage(pageView) {
if (this._checkSetImageDisabled()) {
return;
}
if (this.renderingState !== RenderingStates.INITIAL) {
return;
}
const { canvas, pdfPage } = pageView;
const { thumbnailCanvas: canvas, pdfPage } = pageView;
if (!canvas) {
return;
}
Expand Down
14 changes: 0 additions & 14 deletions web/pdf_thumbnail_viewer.js
Original file line number Diff line number Diff line change
Expand Up @@ -85,12 +85,6 @@ class PDFThumbnailViewer {

this.scroll = watchScroll(this.container, this._scrollUpdated.bind(this));
this._resetView();

eventBus._on("optionalcontentconfigchanged", () => {
// Ensure that the thumbnails always render with the *default* optional
// content configuration.
this._setImageDisabled = true;
});
}

/**
Expand Down Expand Up @@ -195,8 +189,6 @@ class PDFThumbnailViewer {
this._currentPageNumber = 1;
this._pageLabels = null;
this._pagesRotation = 0;
this._optionalContentConfigPromise = null;
this._setImageDisabled = false;

// Remove the thumbnails from the DOM.
this.container.textContent = "";
Expand All @@ -220,13 +212,8 @@ class PDFThumbnailViewer {

firstPagePromise
.then(firstPdfPage => {
this._optionalContentConfigPromise = optionalContentConfigPromise;

const pagesCount = pdfDocument.numPages;
const viewport = firstPdfPage.getViewport({ scale: 1 });
const checkSetImageDisabled = () => {
return this._setImageDisabled;
};

for (let pageNum = 1; pageNum <= pagesCount; ++pageNum) {
const thumbnail = new PDFThumbnailView({
Expand All @@ -236,7 +223,6 @@ class PDFThumbnailViewer {
optionalContentConfigPromise,
linkService: this.linkService,
renderingQueue: this.renderingQueue,
checkSetImageDisabled,
l10n: this.l10n,
pageColors: this.pageColors,
});
Expand Down

0 comments on commit e9120f3

Please sign in to comment.