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

Document why a custom PhantomJS build is necessary #36

Closed
mojavelinux opened this issue Feb 23, 2016 · 5 comments
Closed

Document why a custom PhantomJS build is necessary #36

mojavelinux opened this issue Feb 23, 2016 · 5 comments

Comments

@mojavelinux
Copy link
Contributor

Document the reason(s) why decktape relies on a custom build of PhantomJS. The documentation will serve two purposes:

  1. Help users understand why these steps are necessary; justify the extra overhead
  2. Track the upstream change request (where we are in aligning with upstream)

My understanding is that the patch to PhantomJS is to provide a printer module that can combine multiple PDF pages into one. By default, PhantomJS can only capture and write one PDF page to file at a time. Since there is no way to merge PDF files using JavaScript, there's no other easy solution besides patching PhantomJS to provide this functionality.

Where are we with the upstream request? Are they considering adding this new module?

@astefanutti
Copy link
Owner

Here is the list of items to be contributed to PhantomJS upstream so that we don't need to maintain a custom build for it, which is the ultimate goal, and relieve the end-users from the extra overhead:

Item Reference PR Status
Printer module API astefanutti/phantomjs@d8bc4b0 ❗ todo
Add support for capturing viewport when rendering images (required to capture snapshots properly) ariya/phantomjs#13422 🕙 review
Enable outline annotations to be rendered outside printing context (required for clickable hyperlinks with the printer module) astefanutti/qtwebkit@b83bf9342b819dff7721092675f25bc5eb3fa1dc ❕ todo
PDF font embedding fails on Mac 64-bit due to unimplemented methods in QCoreTextFontEngine ariya/phantomjs#13243 ✅ merged
Render anchors as clickable links in PDF documents vitallium/qtwebkit@ef91a25 ✅ merged
Add support for drawing a hyperlink in QPdfEngine vitallium/qtbase@d50c481 ✅ merged
PhantomJS default configuration file support (optional) ariya/phantomjs/issues/13300 🚫 declined

@mojavelinux
Copy link
Contributor Author

This is fantastic information! Can I add it to the README? (Also, could we switch the README to AsciiDoc while we're at it?)

Add support for capturing viewport when rendering images (required to capture snapshots properly)

Now I understand why I was having trouble with that when I was using phantomjs directly. I found a workaround by setting the width and height to the computed width and height (which is odd that it works), but having support out of the box would be so much better.

@astefanutti
Copy link
Owner

@mojavelinux you have my blessing (both for the README update and the AsciiDoc migration 🎉 ), making it clear the ultimate goal is to avoid having to deal with that astefanutti/phantomjs fork.

@mojavelinux
Copy link
Contributor Author

Excellent! I'll send a PR as soon as I get a chance.

@mojavelinux
Copy link
Contributor Author

PR sent!

astefanutti added a commit that referenced this issue Mar 4, 2016
resolves #36 document why DeckTape uses a PhantomJS fork
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants