Added more preferences for the available hash parameters. #4851

Eyesonly88 · 2014-05-27T22:06:48Z

No description provided.

Snuffleupagus · 2014-05-27T22:15:57Z

web/viewer.js

@@ -1727,7 +1741,7 @@ function webViewerInitialized() {
  }

  if ('useOnlyCssZoom' in hashParams) {
-    USE_ONLY_CSS_ZOOM = (hashParams['useOnlyCssZoom'] === 'true');
+    PDFJS.useCssZoom = (hashParams['useOnlyCssZoom'] === 'true');


Given that the hash parameter is called useOnlyCssZoom, it seems a bit strange, and inconsistent, that the preference is then called useCssZoom.
If I understand correctly, the reason for the name useOnlyCssZoom would be that we use CSS zooming before re-rendering. But with the parameter set, we don't re-render and instead use only CSS zooming, hence the name.

I named it this way in #4792 :)

I see; but I don't really agree with that name ;-)

Also, do we actually need to introduce this property on PDFJS (given that it's only used in the viewer)?
If we keep this change, this needs to be changed as well: https://github.com/mozilla/pdf.js/blob/master/web/viewer.js#L46, otherwise we break the B2G viewer.

@Snuffleupagus do we need to rename useOnlyCssZoom ?

Ok yep. I wasn't sure what B2G was. I will do the change.
I agree with you @Snuffleupagus that it is inconsistent. I have also noticed that enabledWebgl is also inconsistent because the hash param is actually just webgl ...

yurydelendik · 2014-05-27T22:23:22Z

FIxes #4792

yurydelendik · 2014-05-27T22:33:21Z

Could you add useCssZoom at https://github.com/mozilla/pdf.js/blob/master/src/display/api.js ? Also change line viewer.js at L45.

Eyesonly88 · 2014-05-27T22:46:23Z

Yep, ill do the above changes, just confirming if I should rename useCssZoom to useOnlyCssZoom or not?

yurydelendik · 2014-05-27T22:54:48Z

I don't think it matters much, but let's rename hash parameter to useCssZoom.

Eyesonly88 · 2014-05-27T23:02:31Z

I think from @Snuffleupagus comment above it actually makes sense to me to call useOnlyCssZoom rather than useCssZoom. For a beginner like myself, I was able to know what css zooming did when i enabled it in my testing. I noticed that it stayed blurry after zooming in and realized that the code was not re-rendering. So I think the name helped me a bit to understand what's going on.

yurydelendik · 2014-05-27T23:11:33Z

web/viewer.js

@@ -228,6 +227,21 @@ var PDFView = {
      }, function rejected(reason) {}),
      Preferences.get('sidebarViewOnLoad').then(function resolved(value) {
        self.preferenceSidebarViewOnLoad = value;
+      }, function rejected(reason) {}),


Let's move all .then(function resolved(value) { self.preferenceSidebarViewOnLoad = value; }, function rejected(reason) {}) after Promise.all(...)

You mean just move the sidebarViewOnLoad to be after the Promise.all call. Something like this?

var initializedPromise = Promise.all([ Preferences.get('enableWebGL').then(function resolved(value) { PDFJS.disableWebGL = !value; }, function rejected(reason) {}), ...... ...... ..... // TODO move more preferences and other async stuff here ]).Preferences.get('sidebarViewOnLoad').then(function resolved(value) { self.preferenceSidebarViewOnLoad = value; }, function rejected(reason) {});

I was also wondering if there is a way to specify the rejected fn for all the promises inside the Promise.all([...]) call rather than specifying it for each promise?

sorry, I meant only reject part (too many then's):

var initializedPromise = Promise.all([ Preferences.get('enableWebGL').then(function resolved(value) { PDFJS.disableWebGL = !value; }), Preferences.get('sidebarViewOnLoad').then(function resolved(value) { self.preferenceSidebarViewOnLoad = value; }), .... ]).catch(function (reason) { });

Yep that's what i was wondering about, thanks! Will do that change now.

yurydelendik · 2014-05-27T23:13:21Z

okay, let's use useOnlyCssZoom

Snuffleupagus · 2014-05-28T08:43:44Z

src/display/api.js

+ * @var {boolean}
+ */
+PDFJS.useOnlyCssZoom = (PDFJS.useOnlyCssZoom === undefined ?
+                      false : PDFJS.useOnlyCssZoom);


Nit: add two more spaces to the indentation.

Snuffleupagus · 2014-05-28T09:09:35Z

Unfortunately this will still not work in B2G, since the preference values will override the values set here: https://github.com/orionhealth/pdf.js/blob/4792-addprefhashparam/web/viewer.js#L44-L47.
The best way to fix this would be to add preprocessor tags to default_preferences.js, e.g. like this:

  var DEFAULT_PREFERENCES = {
    showPreviousViewOnLoad: true,
    defaultZoomValue: '',
    sidebarViewOnLoad: 0,
    enableHandToolOnLoad: false,
    enableWebGL: false,
    disableRange: false,
    disableAutoFetch: false,
    disableFontFace: false,
//#if B2G
//  disableTextLayer: true,
//  useOnlyCssZoom: true
//#else
    disableTextLayer: false
    useOnlyCssZoom: false
//#endif
  };

Edit: Once you have addressed the comments, please squash the commits. See https://github.com/mozilla/pdf.js/wiki/Squashing-Commits.

Snuffleupagus · 2014-05-28T09:16:27Z

web/viewer.js

      // TODO move more preferences and other async stuff here
-    ]);
+    ], function (reason) {});


According to #4851 (comment), this should be:

]).catch(function (reason) {});

Yep, i will do the changes in the above comments and squash the commits.

yurydelendik · 2014-05-29T01:02:08Z

/botio-windows preview

pdfjsbot · 2014-05-29T01:02:09Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/c0f700a3afeed34/output.txt

pdfjsbot · 2014-05-29T01:03:02Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/c0f700a3afeed34/output.txt

Total script time: 0.88 mins

Published

Viewer: http://107.22.172.223:8877/c0f700a3afeed34/web/viewer.html
B2G Viewer: http://107.22.172.223:8877/c0f700a3afeed34/extensions/b2g/content/web/viewer.html
Extension: http://107.22.172.223:8877/c0f700a3afeed34/extensions/firefox/pdf.js.xpi
Extension (AMO): http://107.22.172.223:8877/c0f700a3afeed34/extensions/firefox/pdf.js.amo.xpi

yurydelendik · 2014-05-29T01:16:07Z

web/default_preferences.js

+  disableAutoFetch: false,
+  disableFontFace: false,
+//#if B2G
+//  disableTextLayer: true,


nit: unindent by two space -- preprocessor replaces '//' with spaces. the same below

@yurydelendik Ok yep, will do that. To me it read better having them indented but ye I don't mind.
So this is how you reckon it should be?

//#if B2G //disableTextLayer: true, //useOnlyCssZoom: true //#else disableTextLayer: false, useOnlyCssZoom: false //#endif

see line 318 at the generated code at http://107.22.172.223:8877/c0f700a3afeed34/web/viewer.js

yurydelendik · 2014-05-29T01:17:07Z

Looks good with comment above addressed.

timvandermeij · 2014-05-29T09:48:12Z

@Eyesonly88 Could you squash the commits again?

Eyesonly88 · 2014-05-29T22:32:40Z

yep ok, will do that now.

timvandermeij · 2014-05-30T10:17:42Z

@yurydelendik Could you check/merge this?

yurydelendik · 2014-06-02T17:50:40Z

Looks good. Thanks you for the patch

Added more preferences for the available hash parameters.

Snuffleupagus reviewed May 27, 2014
View reviewed changes

yurydelendik reviewed May 27, 2014
View reviewed changes

Snuffleupagus reviewed May 28, 2014
View reviewed changes

yurydelendik reviewed May 29, 2014
View reviewed changes

Added more preference variables from the available hash parameters

e654664

yurydelendik added a commit that referenced this pull request Jun 2, 2014

Merge pull request #4851 from orionhealth/4792-addprefhashparam

c7c1639

Added more preferences for the available hash parameters.

yurydelendik merged commit c7c1639 into mozilla:master Jun 2, 2014

timvandermeij mentioned this pull request Jun 2, 2014

Add preferences for hash parameters #4792

Closed

karthikeyann mentioned this pull request Jul 17, 2014

PDF JS fails to load PDF (without #disableRange=true) #5052

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added more preferences for the available hash parameters. #4851

Added more preferences for the available hash parameters. #4851

Eyesonly88 commented May 27, 2014

Snuffleupagus May 27, 2014

yurydelendik May 27, 2014

Snuffleupagus May 27, 2014

Snuffleupagus May 27, 2014

yurydelendik May 27, 2014

Eyesonly88 May 27, 2014

yurydelendik commented May 27, 2014

yurydelendik commented May 27, 2014

Eyesonly88 commented May 27, 2014

yurydelendik commented May 27, 2014

Eyesonly88 commented May 27, 2014

yurydelendik May 27, 2014

Eyesonly88 May 28, 2014

yurydelendik May 28, 2014

Eyesonly88 May 28, 2014

yurydelendik commented May 27, 2014

Snuffleupagus May 28, 2014

Snuffleupagus commented May 28, 2014

Snuffleupagus May 28, 2014

Eyesonly88 May 28, 2014

yurydelendik commented May 29, 2014

pdfjsbot commented May 29, 2014

pdfjsbot commented May 29, 2014

yurydelendik May 29, 2014

Eyesonly88 May 29, 2014

yurydelendik May 29, 2014

yurydelendik May 29, 2014

yurydelendik commented May 29, 2014

timvandermeij commented May 29, 2014

Eyesonly88 commented May 29, 2014

timvandermeij commented May 30, 2014

yurydelendik commented Jun 2, 2014

Added more preferences for the available hash parameters. #4851

Added more preferences for the available hash parameters. #4851

Conversation

Eyesonly88 commented May 27, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurydelendik commented May 27, 2014

yurydelendik commented May 27, 2014

Eyesonly88 commented May 27, 2014

yurydelendik commented May 27, 2014

Eyesonly88 commented May 27, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurydelendik commented May 27, 2014

Choose a reason for hiding this comment

Snuffleupagus commented May 28, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurydelendik commented May 29, 2014

pdfjsbot commented May 29, 2014

From: Bot.io (Windows)

Received

pdfjsbot commented May 29, 2014

From: Bot.io (Windows)

Success

Published

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurydelendik commented May 29, 2014

timvandermeij commented May 29, 2014

Eyesonly88 commented May 29, 2014

timvandermeij commented May 30, 2014

yurydelendik commented Jun 2, 2014