From 72ef1830853e28aca700ffc9876ed21bcdbf6e51 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sun, 4 Apr 2021 16:49:06 +0200 Subject: [PATCH 1/2] [api-minor] Remove the manual passing of an `AnnotationStorage`-instance 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. --- src/display/api.js | 60 +++++++++++++++++++----------------- test/driver.js | 9 +++--- web/app.js | 4 +-- web/firefox_print_service.js | 2 +- web/pdf_print_service.js | 2 +- 5 files changed, 38 insertions(+), 39 deletions(-) diff --git a/src/display/api.js b/src/display/api.js index d20931cd3..b47718076 100644 --- a/src/display/api.js +++ b/src/display/api.js @@ -675,7 +675,7 @@ class PDFDocumentProxy { * @type {AnnotationStorage} Storage for annotation data in forms. */ get annotationStorage() { - return shadow(this, "annotationStorage", new AnnotationStorage()); + return this._transport.annotationStorage; } /** @@ -954,13 +954,20 @@ class PDFDocumentProxy { } /** - * @param {AnnotationStorage} annotationStorage - Storage for annotation - * data in forms. * @returns {Promise} A promise that is resolved with a * {Uint8Array} containing the full data of the saved document. */ - saveDocument(annotationStorage) { - return this._transport.saveDocument(annotationStorage); + saveDocument() { + if ( + (typeof PDFJSDev === "undefined" || PDFJSDev.test("GENERIC")) && + this._transport.annotationStorage.size <= 0 + ) { + deprecated( + "saveDocument called while `annotationStorage` is empty, " + + "please use the getData-method instead." + ); + } + return this._transport.saveDocument(); } /** @@ -1068,7 +1075,7 @@ class PDFDocumentProxy { * some operations. The default value is `false`. * @property {boolean} [renderInteractiveForms] - Whether or not interactive * form elements are rendered in the display layer. If so, we do not render - * them on the canvas as well. + * them on the canvas as well. The default value is `false`. * @property {Array} [transform] - Additional transform, applied just * before viewport transform. * @property {Object} [imageLayer] - An object that has `beginLayout`, @@ -1080,8 +1087,9 @@ class PDFDocumentProxy { * value, a `CanvasGradient` object (a linear or radial gradient) or * a `CanvasPattern` object (a repetitive image). The default value is * 'rgb(255,255,255)'. - * @property {AnnotationStorage} [annotationStorage] - Storage for annotation - * data in forms. + * @property {boolean} [includeAnnotationStorage] - Render stored interactive + * form element data, from the {@link AnnotationStorage}-instance, onto the + * canvas itself; useful e.g. for printing. The default value is `false`. * @property {Promise} [optionalContentConfigPromise] - * A promise that should resolve with an {@link OptionalContentConfig} * created from `PDFDocumentProxy.getOptionalContentConfig`. If `null`, @@ -1230,7 +1238,7 @@ class PDFPageProxy { imageLayer = null, canvasFactory = null, background = null, - annotationStorage = null, + includeAnnotationStorage = false, optionalContentConfigPromise = null, }) { if (this._stats) { @@ -1264,6 +1272,9 @@ class PDFPageProxy { const webGLContext = new WebGLContext({ enable: enableWebGL, }); + const annotationStorage = includeAnnotationStorage + ? this._transport.annotationStorage.serializable + : null; // If there's no displayReadyCapability yet, then the operatorList // was never requested before. Make the request and create the promise. @@ -1282,7 +1293,7 @@ class PDFPageProxy { pageIndex: this._pageIndex, intent: renderingIntent, renderInteractiveForms: renderInteractiveForms === true, - annotationStorage: annotationStorage?.serializable || null, + annotationStorage, }); } @@ -2192,8 +2203,8 @@ class WorkerTransport { this.setupMessageHandler(); } - get loadingTaskSettled() { - return this.loadingTask._capability.settled; + get annotationStorage() { + return shadow(this, "annotationStorage", new AnnotationStorage()); } destroy() { @@ -2220,21 +2231,14 @@ class WorkerTransport { }); this.pageCache.length = 0; this.pagePromises.length = 0; + // Allow `AnnotationStorage`-related clean-up when destroying the document. + if (this.hasOwnProperty("annotationStorage")) { + this.annotationStorage.resetModified(); + } // We also need to wait for the worker to finish its long running tasks. const terminated = this.messageHandler.sendWithPromise("Terminate", null); waitOn.push(terminated); - // Allow `AnnotationStorage`-related clean-up when destroying the document. - if (this.loadingTaskSettled) { - const annotationStorageResetModified = this.loadingTask.promise - .then(pdfDocument => { - // Avoid initializing the `annotationStorage` if it doesn't exist. - if (pdfDocument.hasOwnProperty("annotationStorage")) { - pdfDocument.annotationStorage.resetModified(); - } - }) - .catch(() => {}); - waitOn.push(annotationStorageResetModified); - } + Promise.all(waitOn).then(() => { this.commonObjs.clear(); this.fontLoader.clear(); @@ -2669,17 +2673,15 @@ class WorkerTransport { }); } - saveDocument(annotationStorage) { + saveDocument() { return this.messageHandler .sendWithPromise("SaveDocument", { numPages: this._numPages, - annotationStorage: annotationStorage?.serializable || null, + annotationStorage: this.annotationStorage.serializable, filename: this._fullReader?.filename ?? null, }) .finally(() => { - if (annotationStorage) { - annotationStorage.resetModified(); - } + this.annotationStorage.resetModified(); }); } diff --git a/test/driver.js b/test/driver.js index a83f64218..d91b5166a 100644 --- a/test/driver.js +++ b/test/driver.js @@ -635,14 +635,13 @@ var Driver = (function DriverClosure() { optionalContentConfigPromise: task.optionalContentConfigPromise, }; if (renderPrint) { - const annotationStorage = task.annotationStorage; - if (annotationStorage) { - const docAnnotationStorage = task.pdfDoc.annotationStorage; - const entries = Object.entries(annotationStorage); + if (task.annotationStorage) { + const entries = Object.entries(task.annotationStorage), + docAnnotationStorage = task.pdfDoc.annotationStorage; for (const [key, value] of entries) { docAnnotationStorage.setValue(key, value); } - renderContext.annotationStorage = docAnnotationStorage; + renderContext.includeAnnotationStorage = true; } renderContext.intent = "print"; } diff --git a/web/app.js b/web/app.js index cec82bddd..27dc2935f 100644 --- a/web/app.js +++ b/web/app.js @@ -998,9 +998,7 @@ const PDFViewerApplication = { try { this._ensureDownloadComplete(); - const data = await this.pdfDocument.saveDocument( - this.pdfDocument.annotationStorage - ); + const data = await this.pdfDocument.saveDocument(); const blob = new Blob([data], { type: "application/pdf" }); await this.downloadManager.download(blob, url, filename, sourceEventType); diff --git a/web/firefox_print_service.js b/web/firefox_print_service.js index b0d6b119f..2008f7dd3 100644 --- a/web/firefox_print_service.js +++ b/web/firefox_print_service.js @@ -66,7 +66,7 @@ function composePage( transform: [PRINT_UNITS, 0, 0, PRINT_UNITS, 0, 0], viewport: pdfPage.getViewport({ scale: 1, rotation: size.rotation }), intent: "print", - annotationStorage: pdfDocument.annotationStorage, + includeAnnotationStorage: true, optionalContentConfigPromise, }; currentRenderTask = thisRenderTask = pdfPage.render(renderContext); diff --git a/web/pdf_print_service.js b/web/pdf_print_service.js index e199903dd..f24b66ab3 100644 --- a/web/pdf_print_service.js +++ b/web/pdf_print_service.js @@ -48,7 +48,7 @@ function renderPage( transform: [PRINT_UNITS, 0, 0, PRINT_UNITS, 0, 0], viewport: pdfPage.getViewport({ scale: 1, rotation: size.rotation }), intent: "print", - annotationStorage: pdfDocument.annotationStorage, + includeAnnotationStorage: true, optionalContentConfigPromise, }; return pdfPage.render(renderContext).promise; From 737a8e846d35e146895e409baadaf7b4243c5b54 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Tue, 6 Apr 2021 13:40:15 +0200 Subject: [PATCH 2/2] Add `deprecated` handling of the now removed `AnnotationStorage` API-parameters These changes are done separately, to make it easier to remove them in the future. --- src/display/api.js | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/display/api.js b/src/display/api.js index b47718076..917459991 100644 --- a/src/display/api.js +++ b/src/display/api.js @@ -958,6 +958,12 @@ class PDFDocumentProxy { * {Uint8Array} containing the full data of the saved document. */ saveDocument() { + if ( + (typeof PDFJSDev === "undefined" || PDFJSDev.test("GENERIC")) && + arguments.length > 0 + ) { + deprecated("saveDocument no longer accepts any options."); + } if ( (typeof PDFJSDev === "undefined" || PDFJSDev.test("GENERIC")) && this._transport.annotationStorage.size <= 0 @@ -1241,6 +1247,16 @@ class PDFPageProxy { includeAnnotationStorage = false, optionalContentConfigPromise = null, }) { + if ( + (typeof PDFJSDev === "undefined" || PDFJSDev.test("GENERIC")) && + arguments[0]?.annotationStorage !== undefined + ) { + deprecated( + "render no longer accepts an `annotationStorage` option, " + + "please use the `includeAnnotationStorage`-boolean instead." + ); + includeAnnotationStorage ||= !!arguments[0].annotationStorage; + } if (this._stats) { this._stats.time("Overall"); }