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

Implement PDF Previewer Using Poppler #31906

Closed
wants to merge 1 commit into
base: master
from

Conversation

@hone
Contributor

hone commented Feb 6, 2018

mutool uses AGPL for licensing which has strict distribution rules.
#30667 (comment)

Poppler is licensed under GPL, so this should provide a free
alternative for most Rails apps.

@rails-bot

This comment has been minimized.

rails-bot commented Feb 6, 2018

r? @pixeltrix

(@rails-bot has picked a reviewer for you, use r? to override)

@rails-bot

This comment has been minimized.

rails-bot commented Feb 6, 2018

warning Warning warning

  • Pull requests are usually filed against the master branch for this repo, but this one is against 5-2-stable. Please double check that you specified the right target!
@hone

This comment has been minimized.

Contributor

hone commented Feb 6, 2018

/cc @matthewd @georgeclaghorn @rafaelfranca this is my PR from our discussions before. If this is amenable to people, I'd like to propose potentially having this as the default since mutool would require commercial licenses for any rails app that wants to use it.

@georgeclaghorn

This comment has been minimized.

Member

georgeclaghorn commented Feb 6, 2018

Can you please rebase against master? Whoever merges this will backport it to 5-2-stable.

@hone hone force-pushed the hone:active_storage_poppler branch Feb 6, 2018

@hone hone changed the base branch from 5-2-stable to master Feb 6, 2018

@hone hone force-pushed the hone:active_storage_poppler branch Feb 6, 2018

activestorage/lib/active_storage/previewer/poppler_pdf_previewer.rb Outdated
end
end
end

This comment has been minimized.

@bogdanvlviv

bogdanvlviv Feb 6, 2018

Contributor

✂️ empty line.

activestorage/test/previewer/poppler_pdf_previewer_test.rb Outdated
end
end
end

This comment has been minimized.

@bogdanvlviv

bogdanvlviv Feb 6, 2018

Contributor

✂️ empty line

@hone hone force-pushed the hone:active_storage_poppler branch Feb 6, 2018

@hone

This comment has been minimized.

Contributor

hone commented Feb 6, 2018

@georgeclaghorn

This comment has been minimized.

Member

georgeclaghorn commented Feb 7, 2018

There’s a failing test that needs fixing.

Have you thought about how to document this alternative?

@hone hone force-pushed the hone:active_storage_poppler branch Feb 7, 2018

@rafaelfranca rafaelfranca added this to the 5.2.0 milestone Feb 7, 2018

@hone hone force-pushed the hone:active_storage_poppler branch 2 times, most recently Feb 14, 2018

@hone

This comment has been minimized.

Contributor

hone commented Feb 14, 2018

@matthewd is this along the lines of what you were thinking of?

# without processing a PDF. This check ensures
# the binary exists since command not found is
# exit code 127.
def self.mutool_exists?

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 27, 2018

Member

Let's memoize the return of this to avoid spawning a new process every time we want to generate a preview.

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 27, 2018

Member

Also we need to avoid the commands write to stdout/stderr.

activestorage/lib/active_storage/previewer/poppler_pdf_previewer.rb Outdated
class Previewer::PopplerPDFPreviewer < Previewer
def self.accept?(blob)
blob.content_type == "application/pdf" &&
system("#{pdftoppm_path} -v")

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 27, 2018

Member

Also memoize this.

This comment has been minimized.

@matthewd

matthewd Feb 27, 2018

Member

This should use the multi-argument form, in case the path contains a space

Implement PDF Previewer Using Poppler
mutool uses AGPL for licensing which has strict distribution rules.
#30667 (comment)

Poppler is licensed under GPL, so this should provide a free
alternative for most Rails apps.

@hone hone force-pushed the hone:active_storage_poppler branch to 6ec803d Mar 1, 2018

@hone

This comment has been minimized.

Contributor

hone commented Mar 1, 2018

@rafaelfranca should PDFPreviewer class name reflect the binary underneath if we have two PDF Previewers in rails?

@hone

This comment has been minimized.

Contributor

hone commented Mar 1, 2018

Anyone have an idea why this change would break the railties test on 2.5.0? https://travis-ci.org/rails/rails/jobs/347576562

Edit: looks like rebasing and pushing again fixed it?

@georgeclaghorn

This comment has been minimized.

Member

georgeclaghorn commented Mar 5, 2018

should PDFPreviewer class name reflect the binary underneath if we have two PDF Previewers in rails?

Yeah, the old PDFPreviewer should be renamed to MuPDFPreviewer.

@dhh dhh assigned georgeclaghorn and unassigned pixeltrix Mar 6, 2018

@georgeclaghorn

This comment has been minimized.

Member

georgeclaghorn commented Mar 6, 2018

Merged in 0b717c2 with the requested changes and some modest doc updates so we can issue the last release candidate. Thanks!

@hone hone deleted the hone:active_storage_poppler branch Mar 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment