Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Additional Scroll/Spread mode clean-up (PR 9832 follow-up) #9858

Merged
merged 8 commits into from
Jun 30, 2018

Conversation

Snuffleupagus
Copy link
Collaborator

Please refer to the individual commit messages for additional details.

Since I had some free time yesterday, this is the additional clean-up mentioned in PR #9832.

This property isn't accessed anywhere in the `web/app.js` file, and is also not being reset in `PDFViewerApplication.close`. Hence it seems that it can simply be removed, especially since the fingerprint is already synchronously available through `PDFViewerApplication.pdfDocument.fingerprint` (provided that a document is loaded).
For some reason, these weren't added to `AppOptions` despite actually being set and read from `web/app.js`. Not adding them creates inconsistencies, since all other options *are* present in `web/app_options.js`.
…de ViewHistory settings

Note how the other "...OnLoad" preferences will allow a *non-default* value to always override a history entry. To improve overall consistency for the viewer options, and to reduce possibly confusing behaviour, this patch changes the `scrollModeOnLoad`/`spreadModeOnLoad` preferences to behave as all the other ones.
Since all the other viewer methods use the getter/setter pattern, e.g. for setting page/scale/rotation, the way that the Scroll/Spread modes are set thus stands out. For consistency, this really ought to use the same pattern as the rest of the `BaseViewer`. (To avoid breaking third-party implementations, the old methods are kept around as aliases.)
…`BaseViewer` with overrides (as necessary) in the extending classes

This structure probably makes somewhat more sense, given that `PDFSinglePageViewer` is somewhat of a special case.
… Scroll/Spread modes in the `BaseViewer` constructor

Since other viewer state, such as the current page/scale/rotation[1], are not available as `BaseViewer` constructor options, this makes the Scroll/Spread modes stand out quite a bit. Hence it probably makes sense to remove/deprecate this, to avoid inconsistent and possibly confusing state in this code.

---

[1] These properties are *purposely* not available in the constructor, since attempting to set them before a document is loaded has number of issues; please refer to mozilla#8539 (comment) for additional details.
…document is closed

Since the Scroll/Spread modes are now document specific, as all other properties such as page/scale/rotation, ensure that the toolbar is always correctly reset.
@timvandermeij
Copy link
Contributor

/botio-linux preview

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://54.67.70.0:8877/f7f3366925e04d0/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/f7f3366925e04d0/output.txt

Total script time: 7.82 mins

Published

@timvandermeij timvandermeij merged commit 872c6e4 into mozilla:master Jun 30, 2018
@timvandermeij
Copy link
Contributor

Good changes; thanks!

@Snuffleupagus Snuffleupagus deleted the scrollMode-refactor branch June 30, 2018 21:28
@Snuffleupagus
Copy link
Collaborator Author

Snuffleupagus commented Jul 1, 2018

@timvandermeij As always, thanks for reviewing/merging this!

Given that the current pre-release needs to be replaced anyway (given the regressions fixed by PR #9832), would it make sense to include these changes here as well?

Rather than cherry-picking certain change sets, it seems to me that it's perhaps easiest to simply mark e.g. the current version (2.0.659 as of this writing) as the new pre-release. Especially considering that a number of fixes for corrupt/broken PDF files landed recently, and these could be good to include as well.

@timvandermeij
Copy link
Contributor

Yes, I think when we make a new (pre)release, it should include all changes up to that date.

@Snuffleupagus
Copy link
Collaborator Author

For future reference: Once this PR has been included in at least one official release, remove the deprecated code as outlined in the diff below.

diff --git a/web/base_viewer.js b/web/base_viewer.js
index c904a448..1d9ac59f 100644
--- a/web/base_viewer.js
+++ b/web/base_viewer.js
@@ -171,18 +171,6 @@ class BaseViewer {
     if (this.removePageBorders) {
       this.viewer.classList.add('removePageBorders');
     }
-
-    if ((typeof PDFJSDev === 'undefined' || PDFJSDev.test('GENERIC')) &&
-        ('scrollMode' in options || 'spreadMode' in options)) {
-      console.error(`The ${this._name} constructor options ` +
-        '`scrollMode`/`spreadMode` are deprecated, use the setters instead.');
-      if (options.scrollMode !== undefined) {
-        this.scrollMode = options.scrollMode;
-      }
-      if (options.spreadMode !== undefined) {
-        this.spreadMode = options.spreadMode;
-      }
-    }
   }
 
   get pagesCount() {
@@ -1078,14 +1066,6 @@ class BaseViewer {
     this.update();
   }
 
-  setScrollMode(mode) {
-    if (typeof PDFJSDev === 'undefined' || PDFJSDev.test('GENERIC')) {
-      console.error(`${this._name}.setScrollMode() is deprecated, ` +
-                    `use the ${this._name}.scrollMode setter instead.`);
-      this.scrollMode = mode;
-    }
-  }
-
   /**
    * @return {number} One of the values in {SpreadMode}.
    */
@@ -1145,14 +1125,6 @@ class BaseViewer {
     this.scrollPageIntoView({ pageNumber, });
     this.update();
   }
-
-  setSpreadMode(mode) {
-    if (typeof PDFJSDev === 'undefined' || PDFJSDev.test('GENERIC')) {
-      console.error(`${this._name}.setSpreadMode() is deprecated, ` +
-                    `use the ${this._name}.spreadMode setter instead.`);
-      this.spreadMode = mode;
-    }
-  }
 }
 
 export {

@timvandermeij
Copy link
Contributor

timvandermeij commented Jul 2, 2018

Yes, it's good that they all have the word "deprecated" in the text so it easily pops up when using grep.

movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018
Additional Scroll/Spread mode clean-up (PR 9832 follow-up)
Snuffleupagus added a commit to Snuffleupagus/pdf.js that referenced this pull request Aug 20, 2018
…read modes (PR 9858 follow-up)

Considering that a number of `[api-minor]` changes have landed since PR 9858, removing this code ought to be OK now (the less time these methods remain exposed, the better); implements mozilla#9858 (comment).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants