From 5565a6f8bf612f29f36f4bc53744628818fbbfed Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sat, 19 Aug 2017 14:23:40 +0200 Subject: [PATCH 1/3] Slightly refactor the pages rotation handling code in the viewer This changes both `PDFViewer` and `PDFThumbnailViewer` to return early in the `pagesRotation` setters if the rotation doesn't change. It also fixes an existing issue, in `PDFViewer`, that would cause errors if the rotation changes *before* the scale has been set to a non-default value. Finally, in preparation for subsequent patches, it also refactors the rotation code in `web/app.js` to update the thumbnails and trigger rendering with the new `rotationchanging` event. --- test/unit/ui_utils_spec.js | 27 +++++++++++++++++++++++++-- web/app.js | 24 ++++++++++++++---------- web/pdf_thumbnail_viewer.js | 7 +++++-- web/pdf_viewer.js | 22 ++++++++++++++++++---- web/ui_utils.js | 5 +++++ 5 files changed, 67 insertions(+), 18 deletions(-) diff --git a/test/unit/ui_utils_spec.js b/test/unit/ui_utils_spec.js index 58f97e9d1..bae36b51d 100644 --- a/test/unit/ui_utils_spec.js +++ b/test/unit/ui_utils_spec.js @@ -14,8 +14,8 @@ */ import { - binarySearchFirstItem, EventBus, getPDFFileNameFromURL, waitOnEventOrTimeout, - WaitOnType + binarySearchFirstItem, EventBus, getPDFFileNameFromURL, isValidRotation, + waitOnEventOrTimeout, WaitOnType } from '../../web/ui_utils'; import { createObjectURL, isNodeJS } from '../../src/shared/util'; @@ -261,6 +261,29 @@ describe('ui_utils', function() { }); }); + describe('isValidRotation', function() { + it('should reject non-integer angles', function() { + expect(isValidRotation()).toEqual(false); + expect(isValidRotation(null)).toEqual(false); + expect(isValidRotation(NaN)).toEqual(false); + expect(isValidRotation([90])).toEqual(false); + expect(isValidRotation('90')).toEqual(false); + expect(isValidRotation(90.5)).toEqual(false); + }); + + it('should reject non-multiple of 90 degree angles', function() { + expect(isValidRotation(45)).toEqual(false); + expect(isValidRotation(-123)).toEqual(false); + }); + + it('should accept valid angles', function() { + expect(isValidRotation(0)).toEqual(true); + expect(isValidRotation(90)).toEqual(true); + expect(isValidRotation(-270)).toEqual(true); + expect(isValidRotation(540)).toEqual(true); + }); + }); + describe('waitOnEventOrTimeout', function() { let eventBus; diff --git a/web/app.js b/web/app.js index caeeb37da..1dc37c0ac 100644 --- a/web/app.js +++ b/web/app.js @@ -1232,16 +1232,10 @@ let PDFViewerApplication = { if (!this.pdfDocument) { return; } - let { pdfViewer, pdfThumbnailViewer, } = this; - let pageNumber = pdfViewer.currentPageNumber; - let newRotation = (pdfViewer.pagesRotation + 360 + delta) % 360; - - pdfViewer.pagesRotation = newRotation; - pdfThumbnailViewer.pagesRotation = newRotation; - - this.forceRendering(); - // Ensure that the active page doesn't change during rotation. - pdfViewer.currentPageNumber = pageNumber; + let newRotation = (this.pdfViewer.pagesRotation + 360 + delta) % 360; + this.pdfViewer.pagesRotation = newRotation; + // Note that the thumbnail viewer is updated, and rendering is triggered, + // in the 'rotationchanging' event handler. }, requestPresentationMode() { @@ -1266,6 +1260,7 @@ let PDFViewerApplication = { eventBus.on('updateviewarea', webViewerUpdateViewarea); eventBus.on('pagechanging', webViewerPageChanging); eventBus.on('scalechanging', webViewerScaleChanging); + eventBus.on('rotationchanging', webViewerRotationChanging); eventBus.on('sidebarviewchanged', webViewerSidebarViewChanged); eventBus.on('pagemode', webViewerPageMode); eventBus.on('namedaction', webViewerNamedAction); @@ -1343,6 +1338,7 @@ let PDFViewerApplication = { eventBus.off('updateviewarea', webViewerUpdateViewarea); eventBus.off('pagechanging', webViewerPageChanging); eventBus.off('scalechanging', webViewerScaleChanging); + eventBus.off('rotationchanging', webViewerRotationChanging); eventBus.off('sidebarviewchanged', webViewerSidebarViewChanged); eventBus.off('pagemode', webViewerPageMode); eventBus.off('namedaction', webViewerNamedAction); @@ -1926,6 +1922,14 @@ function webViewerScaleChanging(evt) { PDFViewerApplication.pdfViewer.update(); } +function webViewerRotationChanging(evt) { + PDFViewerApplication.pdfThumbnailViewer.pagesRotation = evt.pagesRotation; + + PDFViewerApplication.forceRendering(); + // Ensure that the active page doesn't change during rotation. + PDFViewerApplication.pdfViewer.currentPageNumber = evt.pageNumber; +} + function webViewerPageChanging(evt) { let page = evt.pageNumber; diff --git a/web/pdf_thumbnail_viewer.js b/web/pdf_thumbnail_viewer.js index d0ecb469a..ce23d6556 100644 --- a/web/pdf_thumbnail_viewer.js +++ b/web/pdf_thumbnail_viewer.js @@ -14,7 +14,7 @@ */ import { - getVisibleElements, NullL10n, scrollIntoView, watchScroll + getVisibleElements, isValidRotation, NullL10n, scrollIntoView, watchScroll } from './ui_utils'; import { PDFThumbnailView } from './pdf_thumbnail_view'; @@ -95,12 +95,15 @@ class PDFThumbnailViewer { } set pagesRotation(rotation) { - if (!(typeof rotation === 'number' && rotation % 90 === 0)) { + if (!isValidRotation(rotation)) { throw new Error('Invalid thumbnails rotation angle.'); } if (!this.pdfDocument) { return; } + if (this._pagesRotation === rotation) { + return; // The rotation didn't change. + } this._pagesRotation = rotation; for (let i = 0, ii = this._thumbnails.length; i < ii; i++) { diff --git a/web/pdf_viewer.js b/web/pdf_viewer.js index f58652726..7e65818c1 100644 --- a/web/pdf_viewer.js +++ b/web/pdf_viewer.js @@ -16,8 +16,8 @@ import { createPromiseCapability, PDFJS } from 'pdfjs-lib'; import { CSS_UNITS, DEFAULT_SCALE, DEFAULT_SCALE_VALUE, getVisibleElements, - MAX_AUTO_SCALE, NullL10n, RendererType, SCROLLBAR_PADDING, scrollIntoView, - UNKNOWN_SCALE, VERTICAL_PADDING, watchScroll + isValidRotation, MAX_AUTO_SCALE, NullL10n, RendererType, SCROLLBAR_PADDING, + scrollIntoView, UNKNOWN_SCALE, VERTICAL_PADDING, watchScroll } from './ui_utils'; import { PDFRenderingQueue, RenderingStates } from './pdf_rendering_queue'; import { AnnotationLayerBuilder } from './annotation_layer_builder'; @@ -271,20 +271,34 @@ class PDFViewer { * @param {number} rotation - The rotation of the pages (0, 90, 180, 270). */ set pagesRotation(rotation) { - if (!(typeof rotation === 'number' && rotation % 90 === 0)) { + if (!isValidRotation(rotation)) { throw new Error('Invalid pages rotation angle.'); } if (!this.pdfDocument) { return; } + if (this._pagesRotation === rotation) { + return; // The rotation didn't change. + } this._pagesRotation = rotation; + let pageNumber = this._currentPageNumber; + for (let i = 0, ii = this._pages.length; i < ii; i++) { let pageView = this._pages[i]; pageView.update(pageView.scale, rotation); } + // Prevent errors in case the rotation changes *before* the scale has been + // set to a non-default value. + if (this._currentScaleValue) { + this._setScale(this._currentScaleValue, true); + } - this._setScale(this._currentScaleValue, true); + this.eventBus.dispatch('rotationchanging', { + source: this, + pagesRotation: rotation, + pageNumber, + }); if (this.defaultRenderingQueue) { this.update(); diff --git a/web/ui_utils.js b/web/ui_utils.js index a670d6702..aec8a5793 100644 --- a/web/ui_utils.js +++ b/web/ui_utils.js @@ -443,6 +443,10 @@ function normalizeWheelEventDelta(evt) { return delta; } +function isValidRotation(angle) { + return Number.isInteger(angle) && angle % 90 === 0; +} + function cloneObj(obj) { let result = Object.create(null); for (let i in obj) { @@ -655,6 +659,7 @@ export { MAX_AUTO_SCALE, SCROLLBAR_PADDING, VERTICAL_PADDING, + isValidRotation, cloneObj, RendererType, mozL10n, From 44d5138d0f418f38797f18ef0c7dff00028a4e57 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Mon, 21 Aug 2017 11:22:07 +0200 Subject: [PATCH 2/3] Store the rotation in the `ViewHistory` (issue 5927) --- web/app.js | 24 ++++++++++++++++++------ web/pdf_viewer.js | 1 + 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/web/app.js b/web/app.js index 1dc37c0ac..1b7879687 100644 --- a/web/app.js +++ b/web/app.js @@ -15,9 +15,9 @@ /* globals PDFBug, Stats */ import { - animationStarted, DEFAULT_SCALE_VALUE, getPDFFileNameFromURL, MAX_SCALE, - MIN_SCALE, noContextMenuHandler, normalizeWheelEventDelta, parseQueryString, - ProgressBar, RendererType + animationStarted, DEFAULT_SCALE_VALUE, getPDFFileNameFromURL, isValidRotation, + MAX_SCALE, MIN_SCALE, noContextMenuHandler, normalizeWheelEventDelta, + parseQueryString, ProgressBar, RendererType } from './ui_utils'; import { build, createBlob, getDocument, getFilenameFromUrl, InvalidPDFException, @@ -948,6 +948,7 @@ let PDFViewerApplication = { zoom: DEFAULT_SCALE_VALUE, scrollLeft: '0', scrollTop: '0', + rotation: null, sidebarView: SidebarView.NONE, }).catch(() => { /* Unable to read from storage; ignoring errors. */ }); @@ -956,12 +957,14 @@ let PDFViewerApplication = { // Initialize the default values, from user preferences. let hash = this.viewerPrefs['defaultZoomValue'] ? ('zoom=' + this.viewerPrefs['defaultZoomValue']) : null; + let rotation = null; let sidebarView = this.viewerPrefs['sidebarViewOnLoad']; if (values.exists && this.viewerPrefs['showPreviousViewOnLoad']) { hash = 'page=' + values.page + '&zoom=' + (this.viewerPrefs['defaultZoomValue'] || values.zoom) + ',' + values.scrollLeft + ',' + values.scrollTop; + rotation = parseInt(values.rotation, 10); sidebarView = sidebarView || (values.sidebarView | 0); } if (pageMode && !this.viewerPrefs['disablePageMode']) { @@ -970,13 +973,14 @@ let PDFViewerApplication = { } return { hash, + rotation, sidebarView, }; - }).then(({ hash, sidebarView, }) => { + }).then(({ hash, rotation, sidebarView, }) => { initialParams.bookmark = this.initialBookmark; initialParams.hash = hash; - this.setInitialView(hash, { sidebarView, }); + this.setInitialView(hash, { rotation, sidebarView, }); // Make all navigation keys work on document load, // unless the viewer is embedded in a web page. @@ -1131,7 +1135,12 @@ let PDFViewerApplication = { }); }, - setInitialView(storedHash, { sidebarView, } = {}) { + setInitialView(storedHash, { rotation, sidebarView, } = {}) { + let setRotation = (angle) => { + if (isValidRotation(angle)) { + this.pdfViewer.pagesRotation = angle; + } + }; this.isInitialViewSet = true; this.pdfSidebar.setInitialView(sidebarView); @@ -1139,6 +1148,8 @@ let PDFViewerApplication = { this.pdfLinkService.setHash(this.initialBookmark); this.initialBookmark = null; } else if (storedHash) { + setRotation(rotation); + this.pdfLinkService.setHash(storedHash); } @@ -1765,6 +1776,7 @@ function webViewerUpdateViewarea(evt) { 'zoom': location.scale, 'scrollLeft': location.left, 'scrollTop': location.top, + 'rotation': location.rotation, }).catch(function() { /* unable to write to storage */ }); } let href = diff --git a/web/pdf_viewer.js b/web/pdf_viewer.js index 7e65818c1..c352441b9 100644 --- a/web/pdf_viewer.js +++ b/web/pdf_viewer.js @@ -732,6 +732,7 @@ class PDFViewer { scale: normalizedScaleValue, top: intTop, left: intLeft, + rotation: this._pagesRotation, pdfOpenParams, }; } From e135c03123870c3ab4919d6ee74a662b504ea579 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Mon, 21 Aug 2017 11:56:49 +0200 Subject: [PATCH 3/3] Store the rotation in the `PDFHistory` --- web/app.js | 5 +++++ web/interfaces.js | 10 ++++++++++ web/pdf_history.js | 28 ++++++++++++++++++++++------ web/pdf_link_service.js | 24 ++++++++++++++++++++++++ 4 files changed, 61 insertions(+), 6 deletions(-) diff --git a/web/app.js b/web/app.js index 1b7879687..6e63bc05c 100644 --- a/web/app.js +++ b/web/app.js @@ -935,6 +935,8 @@ let PDFViewerApplication = { if (this.pdfHistory.initialBookmark) { this.initialBookmark = this.pdfHistory.initialBookmark; + + this.initialRotation = this.pdfHistory.initialRotation; } } @@ -1145,6 +1147,9 @@ let PDFViewerApplication = { this.pdfSidebar.setInitialView(sidebarView); if (this.initialBookmark) { + setRotation(this.initialRotation); + delete this.initialRotation; + this.pdfLinkService.setHash(this.initialBookmark); this.initialBookmark = null; } else if (storedHash) { diff --git a/web/interfaces.js b/web/interfaces.js index 04c6d97ed..f694a9086 100644 --- a/web/interfaces.js +++ b/web/interfaces.js @@ -30,6 +30,16 @@ class IPDFLinkService { */ set page(value) {} + /** + * @returns {number} + */ + get rotation() {} + + /** + * @param {number} value + */ + set rotation(value) {} + /** * @param dest - The PDF destination object. */ diff --git a/web/pdf_history.js b/web/pdf_history.js index 522af3e6a..cca49c476 100644 --- a/web/pdf_history.js +++ b/web/pdf_history.js @@ -13,7 +13,9 @@ * limitations under the License. */ -import { cloneObj, parseQueryString, waitOnEventOrTimeout } from './ui_utils'; +import { + cloneObj, isValidRotation, parseQueryString, waitOnEventOrTimeout +} from './ui_utils'; import { getGlobalEventBus } from './dom_events'; // Heuristic value used when force-resetting `this._blockHashChange`. @@ -49,7 +51,7 @@ function parseCurrentHash(linkService) { if (!(Number.isInteger(page) && page > 0 && page <= linkService.pagesCount)) { page = null; } - return { hash, page, }; + return { hash, page, rotation: linkService.rotation, }; } class PDFHistory { @@ -62,6 +64,7 @@ class PDFHistory { this.initialized = false; this.initialBookmark = null; + this.initialRotation = null; this._boundEvents = Object.create(null); this._isViewerInPresentationMode = false; @@ -99,6 +102,7 @@ class PDFHistory { this.initialized = true; this.initialBookmark = null; + this.initialRotation = null; this._popStateInProgress = false; this._blockHashChange = 0; @@ -110,7 +114,7 @@ class PDFHistory { this._position = null; if (!this._isValidState(state) || resetHistory) { - let { hash, page, } = parseCurrentHash(this.linkService); + let { hash, page, rotation, } = parseCurrentHash(this.linkService); if (!hash || reInitialized || resetHistory) { // Ensure that the browser history is reset on PDF document load. @@ -119,7 +123,8 @@ class PDFHistory { } // Ensure that the browser history is initialized correctly when // the document hash is present on PDF document load. - this._pushOrReplaceState({ hash, page, }, /* forceReplace = */ true); + this._pushOrReplaceState({ hash, page, rotation, }, + /* forceReplace = */ true); return; } @@ -128,6 +133,10 @@ class PDFHistory { let destination = state.destination; this._updateInternalState(destination, state.uid, /* removeTemporary = */ true); + + if (destination.rotation !== undefined) { + this.initialRotation = destination.rotation; + } if (destination.dest) { this.initialBookmark = JSON.stringify(destination.dest); @@ -188,6 +197,7 @@ class PDFHistory { dest: explicitDest, hash, page: pageNumber, + rotation: this.linkService.rotation, }, forceReplace); if (!this._popStateInProgress) { @@ -402,6 +412,7 @@ class PDFHistory { `page=${location.pageNumber}` : location.pdfOpenParams.substring(1), page: this.linkService.page, first: location.pageNumber, + rotation: location.rotation, }; if (this._popStateInProgress) { @@ -459,8 +470,9 @@ class PDFHistory { // This case corresponds to the user changing the hash of the document. this._currentUid = this._uid; - let { hash, page, } = parseCurrentHash(this.linkService); - this._pushOrReplaceState({ hash, page, }, /* forceReplace */ true); + let { hash, page, rotation, } = parseCurrentHash(this.linkService); + this._pushOrReplaceState({ hash, page, rotation, }, + /* forceReplace = */ true); return; } if (!this._isValidState(state)) { @@ -497,6 +509,10 @@ class PDFHistory { let destination = state.destination; this._updateInternalState(destination, state.uid, /* removeTemporary = */ true); + + if (isValidRotation(destination.rotation)) { + this.linkService.rotation = destination.rotation; + } if (destination.dest) { this.linkService.navigateTo(destination.dest); } else if (destination.hash) { diff --git a/web/pdf_link_service.js b/web/pdf_link_service.js index e024f6992..e4583b327 100644 --- a/web/pdf_link_service.js +++ b/web/pdf_link_service.js @@ -75,6 +75,20 @@ class PDFLinkService { this.pdfViewer.currentPageNumber = value; } + /** + * @returns {number} + */ + get rotation() { + return this.pdfViewer.pagesRotation; + } + + /** + * @param {number} value + */ + set rotation(value) { + this.pdfViewer.pagesRotation = value; + } + /** * @param {string|Array} dest - The named, or explicit, PDF destination. */ @@ -414,6 +428,16 @@ class SimpleLinkService { * @param {number} value */ set page(value) {} + /** + * @returns {number} + */ + get rotation() { + return 0; + } + /** + * @param {number} value + */ + set rotation(value) {} /** * @param dest - The PDF destination object. */