From 253aae15fcac61910ae02f4af6215026b5ec3d67 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Tue, 26 Jun 2018 14:07:52 +0200 Subject: [PATCH 1/4] Ensure that all entries above the current `cacheSize` is removed when initializing a `ViewHistory` instance Note how, in the current code, only *one* old history entry would ever be removed. That would mean that if e.g. the `cacheSize` is reduced, it would potentially require loading of multiple files before the database would be correctly pruned. Furthermore, in the case where the database was empty on load there's no need to attempt to shrink it, since trying to reduce the size of an *empty* array won't do much :-) --- web/view_history.js | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/web/view_history.js b/web/view_history.js index 79ebf0e03..c50d8064c 100644 --- a/web/view_history.js +++ b/web/view_history.js @@ -33,11 +33,12 @@ class ViewHistory { let database = JSON.parse(databaseStr || '{}'); if (!('files' in database)) { database.files = []; + } else { + while (database.files.length >= this.cacheSize) { + database.files.shift(); + } } - if (database.files.length >= this.cacheSize) { - database.files.shift(); - } - let index; + let index = -1; for (let i = 0, length = database.files.length; i < length; i++) { let branch = database.files[i]; if (branch.fingerprint === this.fingerprint) { @@ -45,7 +46,7 @@ class ViewHistory { break; } } - if (typeof index !== 'number') { + if (index === -1) { index = database.files.push({ fingerprint: this.fingerprint, }) - 1; } this.file = database.files[index]; From 5bbbbf152e91b7bc57f420421bc77529f24f4e03 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Tue, 26 Jun 2018 14:07:56 +0200 Subject: [PATCH 2/4] Stop storing/using the 'exists' property of `ViewHistory` database entries Rather than using a "special" property to check if a `ViewHistory` database entry existed on load, it seems that you could just as well check for the existence of one of the actually needed properties instead (here 'page' is used). This way we can avoid storing what, more of less, amounts to useless state, which will help reduce the size of the `ViewHistory` database. Given that we don't directly, nor need to, validate the `ViewHistory` data but rely on other parts of the code-base to do so, the existance of an 'exists' property doesn't in and of itself really add much utility. Finally, to simplify the implementation the 'exists' property won't be actively removed from the `ViewHistory` data. Instead we'll simply stop adding 'exists' when writing `ViewHistory` data, and rely on the existing pruning of old entries to eventually remove any remnants of 'exists' from storage. --- web/app.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/web/app.js b/web/app.js index f0e33d4f5..f87d52c4f 100644 --- a/web/app.js +++ b/web/app.js @@ -1016,8 +1016,7 @@ let PDFViewerApplication = { hash: null, }; let storePromise = store.getMultiple({ - exists: false, - page: '1', + page: null, zoom: DEFAULT_SCALE_VALUE, scrollLeft: '0', scrollTop: '0', @@ -1037,7 +1036,7 @@ let PDFViewerApplication = { let scrollMode = AppOptions.get('scrollModeOnLoad'); let spreadMode = AppOptions.get('spreadModeOnLoad'); - if (values.exists && AppOptions.get('showPreviousViewOnLoad')) { + if (values.page && AppOptions.get('showPreviousViewOnLoad')) { hash = 'page=' + values.page + '&zoom=' + (AppOptions.get('defaultZoomValue') || values.zoom) + ',' + values.scrollLeft + ',' + values.scrollTop; @@ -1852,7 +1851,6 @@ function webViewerUpdateViewarea(evt) { if (store && PDFViewerApplication.isInitialViewSet) { store.setMultiple({ - 'exists': true, 'page': location.pageNumber, 'zoom': location.scale, 'scrollLeft': location.left, From 3691f9cc78f16e122479b503d7b8bcca1eff554f Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Tue, 26 Jun 2018 14:08:00 +0200 Subject: [PATCH 3/4] Simplify the handling of the `defaultZoomValue` preference in `PDFViewerApplication.load` --- web/app.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/web/app.js b/web/app.js index f87d52c4f..8025431b2 100644 --- a/web/app.js +++ b/web/app.js @@ -1029,17 +1029,18 @@ let PDFViewerApplication = { Promise.all([storePromise, pageModePromise]).then( ([values = {}, pageMode]) => { // Initialize the default values, from user preferences. - let hash = AppOptions.get('defaultZoomValue') ? - ('zoom=' + AppOptions.get('defaultZoomValue')) : null; + const zoom = AppOptions.get('defaultZoomValue'); + let hash = zoom ? `zoom=${zoom}` : null; + let rotation = null; let sidebarView = AppOptions.get('sidebarViewOnLoad'); let scrollMode = AppOptions.get('scrollModeOnLoad'); let spreadMode = AppOptions.get('spreadModeOnLoad'); if (values.page && AppOptions.get('showPreviousViewOnLoad')) { - hash = 'page=' + values.page + - '&zoom=' + (AppOptions.get('defaultZoomValue') || values.zoom) + + hash = 'page=' + values.page + '&zoom=' + (zoom || values.zoom) + ',' + values.scrollLeft + ',' + values.scrollTop; + rotation = parseInt(values.rotation, 10); sidebarView = sidebarView || (values.sidebarView | 0); if (values.scrollMode !== null) { From 95a4fa25b9048c05e87f5906289e4b6920b233e3 Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Tue, 26 Jun 2018 14:08:04 +0200 Subject: [PATCH 4/4] Don't wait arbitrary long for all pages to be resolved before attempting to re-apply the initial position on load (PR 6318 follow-up) With the current code, the location in the viewer could change *well* after the user has started to interact with the viewer (for very large and/or slow loading documents). Attempt to reduce the likelyhood of that happening, by adding an upper bound to the time spent waiting before attempting to re-apply the initial position. --- web/app.js | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/web/app.js b/web/app.js index 8025431b2..e4f01cf15 100644 --- a/web/app.js +++ b/web/app.js @@ -48,7 +48,8 @@ import { Toolbar } from './toolbar'; import { ViewHistory } from './view_history'; const DEFAULT_SCALE_DELTA = 1.1; -const DISABLE_AUTO_FETCH_LOADING_BAR_TIMEOUT = 5000; +const DISABLE_AUTO_FETCH_LOADING_BAR_TIMEOUT = 5000; // ms +const FORCE_PAGES_LOADED_TIMEOUT = 10000; // ms const DefaultExternalServices = { updateFindControlState(data) {}, @@ -1074,10 +1075,20 @@ let PDFViewerApplication = { if (!this.isViewerEmbedded) { pdfViewer.focus(); } - return pagesPromise; + + return Promise.race([ + pagesPromise, + new Promise((resolve) => { + setTimeout(resolve, FORCE_PAGES_LOADED_TIMEOUT); + }), + ]); }).then(() => { // For documents with different page sizes, once all pages are resolved, // ensure that the correct location becomes visible on load. + // To reduce the risk, in very large and/or slow loading documents, + // that the location changes *after* the user has started interacting + // with the viewer, wait for either `pagesPromise` or a timeout above. + if (!initialParams.bookmark && !initialParams.hash) { return; }