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

Allow automatic print rotation via the enablePrintAutoRotate preference #8043

Merged
merged 2 commits into from
Feb 8, 2017

Conversation

Rob--W
Copy link
Member

@Rob--W Rob--W commented Feb 7, 2017

Tests:

  1. gulp server
  2. Download test1.pdf and test2.pdf (courtesy of Support printing page with different rotation #6103) and save them to the pdf.js repo.
  3. Open the viewer, run PDFViewerApplication.preferences.set('enablePrintAutoRotate', true); and refresh.
  4. Press Ctrl + P within the page (or click on the print button).
  5. Verify that the pages have the same orientation.
  6. Repeat step 3 - 5 for test1.pdf
  7. Repeat step 3 with false instead of true, then perform step 4 and confirm that the page rotation did not change.

Fixes #6696, supersedes #6103

Determine the page rotation at the same place as where the page size is
determined. This allows us to implement custom print page rotation logic
in one place, in the future.
@Rob--W
Copy link
Member Author

Rob--W commented Feb 7, 2017

/botio-linux preview

Copy link
Contributor

@timvandermeij timvandermeij left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This solution looks really clean! After addressing the comment, I'll do more testing before approving this.

web/app.js Outdated
@@ -1457,6 +1460,9 @@ function webViewerInitialized() {
if ('useonlycsszoom' in hashParams) {
PDFJS.useOnlyCssZoom = (hashParams['useonlycsszoom'] === 'true');
}
if ('printautorotate' in hashParams) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the hash parameter addition. The hash parameters are only used for debugging purposes, not for toggling features. Moreover, using the preferences for this is preferred and quite a few hash parameters have already been removed before.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would you test the parameter in the web viewer without the hash param? I only added it in order to test it from the test server.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would you test the parameter in the web viewer without the hash param?

E.g. by executing PDFViewerApplication.preferences.set('enablePrintAutoRotate', {true, false}); in the console and then reloading the viewer.

Furthermore, we shouldn't be adding new things to PDFJS especially not in the viewer, since the usage of PDFJS will most likely be removed in future PDF.js versions.
Can we please just pass the preference into PDFViewer in the same way as e.g. the enhanceTextSelection preference is handled instead!?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!
Can you please also add a JSDoc comment for the new PDFViewer parameter at https://github.com/mozilla/pdf.js/blob/master/web/pdf_viewer.js#L69?

@Rob--W Rob--W force-pushed the issue6696-auto-rotate-page branch from 5ea1dfd to f95827c Compare February 8, 2017 10:35
@Rob--W
Copy link
Member Author

Rob--W commented Feb 8, 2017

/botio-linux preview

@Rob--W Rob--W force-pushed the issue6696-auto-rotate-page branch from f95827c to ece44d3 Compare February 8, 2017 12:59
@timvandermeij
Copy link
Contributor

/botio-linux preview

@pdfjsbot
Copy link

pdfjsbot commented Feb 8, 2017

From: Bot.io (Linux)


Received

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

Live output at: http://107.21.233.14:8877/47ff66d588af8e9/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Feb 8, 2017

From: Bot.io (Linux)


Success

Full output at http://107.21.233.14:8877/47ff66d588af8e9/output.txt

Total script time: 2.17 mins

Published

@timvandermeij timvandermeij merged commit 573236e into mozilla:master Feb 8, 2017
@timvandermeij
Copy link
Contributor

Thank you for implementing this!

@Piita
Copy link

Piita commented Jun 13, 2017

I'm not sure if I'm reading all of this correctly. I have my default pdf view set as "Preview in Firefox." When I print from that viewer, the pdf prints in whatever orientation my printer settings are set at. My printer dialog does not allow me to auto detect page orientation for printing, so in order to change the orientation I have to do it on a per document basis. I tried the solution for PDFViewerApplication.preferences.set('enablePrintAutoRotate', true), but when I run about:config, I do not find either portion of that as an option in my list of commands. I am running Windows 10 and trying to dump Chrome because it lags so badly, but this printing issue is a major issue that will keep me from changing. Can you help?

@timvandermeij
Copy link
Contributor

I have [email protected] in about:config, but that is most likely because I have the development add-on installed. I'm not sure how to find it using the regular Firefox build. Maybe others have an idea here? If it works well, we may also consider enabling it by default in a separate issue.

@Piita
Copy link

Piita commented Jun 14, 2017 via email

@yurydelendik
Copy link
Contributor

pdfjs.enablePrintAutoRotate key is used for firefox embedded preference

@yurydelendik
Copy link
Contributor

@timvandermeij
Copy link
Contributor

@Piita It is recommended to just update to Firefox 54. Per the link above, it should have the pdfjs.enablePrintAutoRotate key in about:config then so you can toggle it. There should be no need to install the developer add-on.

@Piita
Copy link

Piita commented Jun 15, 2017 via email

@jscher2000
Copy link

@Piita, auto-rotate relates to how pages 2-n are drawn in relation to page 1. In other words, if page 1 is portrait and page 2 is landscape, page 2 will be rotated rather than shrunk-to-fit. However, pdf.js does not auto-detect the current page setup and rotate the first page in relation to that setting. That's issue #2851.

movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018
Allow automatic print rotation via the enablePrintAutoRotate preference
@mozilla mozilla deleted a comment from JonKim-bot Jul 9, 2021
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.

Automatic page orientation (portrait/landscape)
7 participants