From e0ae1575827f2d86f9fe6186f82b74ec34a197cc Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sat, 17 Mar 2018 17:10:37 +0100 Subject: [PATCH 1/4] [api-minor] Fix various issues related to the pageSize information The `getPageSizeInches` method was implemented on `PDFDocumentProxy`, which seems conceptually wrong since the size property isn't global to the document but rather specific to each page. Hence the method is moved into `PDFPageProxy`, as `get pageSizeInches` instead to address this. Despite the fact that new API functionality was implemented, no unit-tests were added. To prevent issues later on, we should *always* ensure that new functionality has at least some test-coverage; something that this patch also takes care of. The new `PDFDocumentProperties._parsePageSize` method seemed unnecessary convoluted. Furthermore, in the "no data provided"-case it even returned incorrect data (an array, rather than the expected object). Finally, the fallback strings didn't actually agree with the `en-US` locale. This inconsistency doesn't look too great, and it's thus addressed here as well. --- src/display/api.js | 31 ++++++++++------------ test/unit/api_spec.js | 8 ++++++ web/pdf_document_properties.js | 47 +++++++++++++++++----------------- 3 files changed, 45 insertions(+), 41 deletions(-) diff --git a/src/display/api.js b/src/display/api.js index 0d2e316fe..203dbe10f 100644 --- a/src/display/api.js +++ b/src/display/api.js @@ -692,23 +692,6 @@ var PDFDocumentProxy = (function PDFDocumentProxyClosure() { getMetadata: function PDFDocumentProxy_getMetadata() { return this.transport.getMetadata(); }, - /** - * @param {number} pageNumber The page number to get the page size from. - * The first page is 1, which is also the default page used. - * @return {Promise} A promise that is resolved with an dict containing the - * width and height in inches. - */ - getPageSizeInches(pageNumber) { - pageNumber = pageNumber || 1; - return this.getPage(pageNumber).then((page) => { - const [x1, y1, x2, y2] = page.view; - // convert values from user units to inches - return { - width: (x2 - x1) / 72 * page.userUnit, - height: (y2 - y1) / 72 * page.userUnit, - }; - }); - }, /** * @return {Promise} A promise that is resolved with a TypedArray that has * the raw data from the PDF. @@ -890,6 +873,20 @@ var PDFPageProxy = (function PDFPageProxyClosure() { get view() { return this.pageInfo.view; }, + + /** + * The size of the current page, converted from PDF units to inches. + * @return {Object} An Object containing the properties: {number} `width` + * and {number} `height`, given in inches. + */ + get pageSizeInches() { + const [x1, y1, x2, y2] = this.view, userUnit = this.userUnit; + return { + width: (x2 - x1) / 72 * userUnit, + height: (y2 - y1) / 72 * userUnit, + }; + }, + /** * @param {number} scale The desired scale of the viewport. * @param {number} rotate Degrees to rotate the viewport. If omitted this diff --git a/test/unit/api_spec.js b/test/unit/api_spec.js index 35e729283..ed4a809ea 100644 --- a/test/unit/api_spec.js +++ b/test/unit/api_spec.js @@ -964,6 +964,14 @@ describe('api', function() { it('gets view', function () { expect(page.view).toEqual([0, 0, 595.28, 841.89]); }); + + it('gets page size (in inches)', function() { + const { width, height, } = page.pageSizeInches; + + expect(+width.toPrecision(3)).toEqual(8.27); + expect(+height.toPrecision(4)).toEqual(11.69); + }); + it('gets viewport', function () { var viewport = page.getViewport(1.5, 90); expect(viewport.viewBox).toEqual(page.view); diff --git a/web/pdf_document_properties.js b/web/pdf_document_properties.js index 7f351592b..b4987b376 100644 --- a/web/pdf_document_properties.js +++ b/web/pdf_document_properties.js @@ -80,13 +80,12 @@ class PDFDocumentProperties { this._parseFileSize(this.maybeFileSize), this._parseDate(info.CreationDate), this._parseDate(info.ModDate), - this.pdfDocument.getPageSizeInches().then((pageSizeInches) => { - return this._parsePageSize(pageSizeInches); + this.pdfDocument.getPage(1).then((pdfPage) => { + return this._parsePageSize(pdfPage.pageSizeInches); }), - ]); - }).then(([info, metadata, fileName, fileSize, - creationDate, modDate, pageSize]) => { + }).then(([info, metadata, fileName, fileSize, creationDate, modDate, + pageSizes]) => { freezeFieldData({ 'fileName': fileName, 'fileSize': fileSize, @@ -100,8 +99,8 @@ class PDFDocumentProperties { 'producer': info.Producer, 'version': info.PDFFormatVersion, 'pageCount': this.pdfDocument.numPages, - 'pageSizeInch': pageSize.inch, - 'pageSizeMM': pageSize.mm, + 'pageSizeInch': pageSizes.inch, + 'pageSizeMM': pageSizes.mm, }); this._updateUI(); @@ -224,25 +223,25 @@ class PDFDocumentProperties { */ _parsePageSize(pageSizeInches) { if (!pageSizeInches) { - return Promise.resolve([undefined, undefined]); + return Promise.resolve({ inch: undefined, mm: undefined, }); } - const sizes_two_units = { - 'width_in': Math.round(pageSizeInches.width * 100) / 100, - 'height_in': Math.round(pageSizeInches.height * 100) / 100, - // 1in = 25.4mm; no need to round to 2 decimals for mm - 'width_mm': Math.round(pageSizeInches.width * 25.4 * 10) / 10, - 'height_mm': Math.round(pageSizeInches.height * 25.4 * 10) / 10, - }; + const { width, height, } = pageSizeInches; + return Promise.all([ - this.l10n.get('document_properties_page_size_in', - sizes_two_units, '{{width_in}} in × {{height_in}} in'), - this.l10n.get('document_properties_page_size_mm', - sizes_two_units, '{{width_mm}} mm × {{height_mm}} mm'), - ]).then(([parsedPageSizeInches, parsedPageSizeMM]) => { - return Promise.resolve({ - inch: parsedPageSizeInches, - mm: parsedPageSizeMM, - }); + this.l10n.get('document_properties_page_size_in', { + width_in: Math.round(width * 100) / 100, + height_in: Math.round(height * 100) / 100, + }, '{{width_in}}in × {{height_in}}in'), + // 1in = 25.4mm; no need to round to 2 decimals for millimeters. + this.l10n.get('document_properties_page_size_mm', { + width_mm: Math.round(width * 25.4 * 10) / 10, + height_mm: Math.round(height * 25.4 * 10) / 10, + }, '{{width_mm}}mm × {{height_mm}}mm'), + ]).then((sizes) => { + return { + inch: sizes[0], + mm: sizes[1], + }; }); } From 1730447ca19cd16e3ef4d4696f6761a0e918618e Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sat, 17 Mar 2018 17:21:15 +0100 Subject: [PATCH 2/4] Tweak the pageSize l10n strings for the document properties dialog The units are currently repeated after each dimension, which seems unnecessary and is also not done in other PDF viewers (such as e.g. Adobe Reader). Furthermore, the name of the l10n arguments can be simplified slightly, since the name of the strings themselves should be enough information. Finally, the `width`/`height` should be formatted according to the current locale, as is already done for other strings in the document properties dialog. --- l10n/en-US/viewer.properties | 12 ++++++------ l10n/sv-SE/viewer.properties | 7 +++++++ web/pdf_document_properties.js | 16 ++++++++-------- 3 files changed, 21 insertions(+), 14 deletions(-) diff --git a/l10n/en-US/viewer.properties b/l10n/en-US/viewer.properties index 7a795712b..4533de643 100644 --- a/l10n/en-US/viewer.properties +++ b/l10n/en-US/viewer.properties @@ -90,12 +90,12 @@ document_properties_producer=PDF Producer: document_properties_version=PDF Version: document_properties_page_count=Page Count: document_properties_page_size=Page Size: -# LOCALIZATION NOTE (document_properties_page_size_in): "{{width_in}}" and "{{height_in}}" -# will be replaced by the size of the first page of the PDF file in inches. -document_properties_page_size_in={{width_in}}in × {{height_in}}in -# LOCALIZATION NOTE (document_properties_page_size_mm): "{{width_mm}}" and "{{height_mm}}" -# will be replaced by the size of the first page of the PDF file in millimeters. -document_properties_page_size_mm={{width_mm}}mm × {{height_mm}}mm +# LOCALIZATION NOTE (document_properties_page_size_in_2): "{{width}}" and "{{height}}" +# will be replaced by the size of the (current) page, in inches. +document_properties_page_size_in_2={{width}} × {{height}} in +# LOCALIZATION NOTE (document_properties_page_size_mm_2): "{{width}}" and "{{height}}" +# will be replaced by the size of the (current) page, in millimeters. +document_properties_page_size_mm_2={{width}} × {{height}} mm document_properties_close=Close print_progress_message=Preparing document for printing… diff --git a/l10n/sv-SE/viewer.properties b/l10n/sv-SE/viewer.properties index 412b76520..8b015fa50 100644 --- a/l10n/sv-SE/viewer.properties +++ b/l10n/sv-SE/viewer.properties @@ -89,6 +89,13 @@ document_properties_creator=Skapare: document_properties_producer=PDF-producent: document_properties_version=PDF-version: document_properties_page_count=Sidantal: +document_properties_page_size=Sidstorlek: +# LOCALIZATION NOTE (document_properties_page_size_in_2): "{{width}}" and "{{height}}" +# will be replaced by the size of the (current) page, in inches. +document_properties_page_size_in_2={{width}} × {{height}} tum +# LOCALIZATION NOTE (document_properties_page_size_mm_2): "{{width}}" and "{{height}}" +# will be replaced by the size of the (current) page, in millimeters. +document_properties_page_size_mm_2={{width}} × {{height}} mm document_properties_close=Stäng print_progress_message=Förbereder sidor för utskrift… diff --git a/web/pdf_document_properties.js b/web/pdf_document_properties.js index b4987b376..13a294ae0 100644 --- a/web/pdf_document_properties.js +++ b/web/pdf_document_properties.js @@ -228,15 +228,15 @@ class PDFDocumentProperties { const { width, height, } = pageSizeInches; return Promise.all([ - this.l10n.get('document_properties_page_size_in', { - width_in: Math.round(width * 100) / 100, - height_in: Math.round(height * 100) / 100, - }, '{{width_in}}in × {{height_in}}in'), + this.l10n.get('document_properties_page_size_in_2', { + width: (Math.round(width * 100) / 100).toLocaleString(), + height: (Math.round(height * 100) / 100).toLocaleString(), + }, '{{width}} × {{height}} in'), // 1in = 25.4mm; no need to round to 2 decimals for millimeters. - this.l10n.get('document_properties_page_size_mm', { - width_mm: Math.round(width * 25.4 * 10) / 10, - height_mm: Math.round(height * 25.4 * 10) / 10, - }, '{{width_mm}}mm × {{height_mm}}mm'), + this.l10n.get('document_properties_page_size_mm_2', { + width: (Math.round(width * 25.4 * 10) / 10).toLocaleString(), + height: (Math.round(height * 25.4 * 10) / 10).toLocaleString(), + }, '{{width}} × {{height}} mm'), ]).then((sizes) => { return { inch: sizes[0], From adeaefedae3aec791ff8bf79c74fb546e7adb229 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sat, 17 Mar 2018 17:53:13 +0100 Subject: [PATCH 3/4] Don't unnecessarily update the fileSize, in the document properties dialog, when it's already been correctly set --- web/pdf_document_properties.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/web/pdf_document_properties.js b/web/pdf_document_properties.js index 13a294ae0..1db3c2626 100644 --- a/web/pdf_document_properties.js +++ b/web/pdf_document_properties.js @@ -108,8 +108,12 @@ class PDFDocumentProperties { // `this.setFileSize` wasn't called) or may be incorrectly set. return this.pdfDocument.getDownloadInfo(); }).then(({ length, }) => { + this.maybeFileSize = length; return this._parseFileSize(length); }).then((fileSize) => { + if (fileSize === this.fieldData['fileSize']) { + return; // The fileSize has already been correctly set. + } let data = cloneObj(this.fieldData); data['fileSize'] = fileSize; @@ -157,7 +161,7 @@ class PDFDocumentProperties { * @param {number} fileSize - The file size of the PDF document. */ setFileSize(fileSize) { - if (typeof fileSize === 'number' && fileSize > 0) { + if (Number.isInteger(fileSize) && fileSize > 0) { this.maybeFileSize = fileSize; } } From 51ddcd6380673ea02b81250d4e4f9b3d7b227ef9 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sat, 17 Mar 2018 18:50:34 +0100 Subject: [PATCH 4/4] Display the pageSize of the *currently* active page in the document properties dialog --- web/app.js | 2 +- web/pdf_document_properties.js | 19 ++++++++++++++++--- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/web/app.js b/web/app.js index 015723807..252d6fa21 100644 --- a/web/app.js +++ b/web/app.js @@ -429,7 +429,7 @@ let PDFViewerApplication = { this.pdfDocumentProperties = new PDFDocumentProperties(appConfig.documentProperties, - this.overlayManager, this.l10n); + this.overlayManager, eventBus, this.l10n); this.pdfCursorTools = new PDFCursorTools({ container, diff --git a/web/pdf_document_properties.js b/web/pdf_document_properties.js index 1db3c2626..880c0a1b8 100644 --- a/web/pdf_document_properties.js +++ b/web/pdf_document_properties.js @@ -30,10 +30,11 @@ class PDFDocumentProperties { /** * @param {PDFDocumentPropertiesOptions} options * @param {OverlayManager} overlayManager - Manager for the viewer overlays. + * @param {EventBus} eventBus - The application event bus. * @param {IL10n} l10n - Localization service. */ constructor({ overlayName, fields, container, closeButton, }, - overlayManager, l10n = NullL10n) { + overlayManager, eventBus, l10n = NullL10n) { this.overlayName = overlayName; this.fields = fields; this.container = container; @@ -47,6 +48,12 @@ class PDFDocumentProperties { } this.overlayManager.register(this.overlayName, this.container, this.close.bind(this)); + + if (eventBus) { + eventBus.on('pagechanging', (evt) => { + this._currentPageNumber = evt.pageNumber; + }); + } } /** @@ -64,12 +71,16 @@ class PDFDocumentProperties { Promise.all([this.overlayManager.open(this.overlayName), this._dataAvailableCapability.promise]).then(() => { + const currentPageNumber = this._currentPageNumber; + // If the document properties were previously fetched (for this PDF file), // just update the dialog immediately to avoid redundant lookups. - if (this.fieldData) { + if (this.fieldData && + currentPageNumber === this.fieldData['_currentPageNumber']) { this._updateUI(); return; } + // Get the document properties. this.pdfDocument.getMetadata().then( ({ info, metadata, contentDispositionFilename, }) => { @@ -80,7 +91,7 @@ class PDFDocumentProperties { this._parseFileSize(this.maybeFileSize), this._parseDate(info.CreationDate), this._parseDate(info.ModDate), - this.pdfDocument.getPage(1).then((pdfPage) => { + this.pdfDocument.getPage(currentPageNumber).then((pdfPage) => { return this._parsePageSize(pdfPage.pageSizeInches); }), ]); @@ -101,6 +112,7 @@ class PDFDocumentProperties { 'pageCount': this.pdfDocument.numPages, 'pageSizeInch': pageSizes.inch, 'pageSizeMM': pageSizes.mm, + '_currentPageNumber': currentPageNumber, }); this._updateUI(); @@ -176,6 +188,7 @@ class PDFDocumentProperties { this.maybeFileSize = 0; delete this.fieldData; this._dataAvailableCapability = createPromiseCapability(); + this._currentPageNumber = 1; } /**