-
Notifications
You must be signed in to change notification settings - Fork 20
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
ImageMagickCommandFactory: only extract first layer for PDFs #113
Conversation
Mostly for brainstorming cc @jcoyne |
@@ -51,7 +51,10 @@ def render(args) | |||
def info | |||
@info ||= begin | |||
result = info_service.call(id, file) | |||
ImageInformation.new(width: result[:width], height: result[:height]) | |||
ImageInformation.new( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer we make ImageInformation
have a dimensions attribute and add the format attribute. Is there a reason we can't do that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think I was confused by the weird override of the initialize
method. Updated now though.
Getting a bunch of failures like this:
There must be something buggy with the |
Why do we benchmark the commands, anyway? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Looks great.
Looks like there's a few tests to fix up. |
@jcoyne yeah, that's the CommandRunner stuff that's confusing me. |
If the benchmarking isn't required, we could just remove the CommandRunner entirely. |
The bizarre thing is I set a I thought maybe a different |
Messing around, I tried replacing E.g.:
|
spec/models/riiif/image_spec.rb
Outdated
.and_return('imagedata') | ||
|
||
expect(subject.render('size' => 'full', format: 'jpg')).to eq 'imagedata' | ||
subject.render('size' => 'full', format: 'jpg') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This no longer tests anything. Can you restore the Riiif::CommandRunner
?
spec/models/riiif/image_spec.rb
Outdated
@@ -69,20 +65,13 @@ | |||
end | |||
|
|||
describe '#render' do | |||
before do | |||
allow(Riiif::CommandRunner).to receive(:execute) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the errors are a result of this line being removed.
Should fix black images; see samvera/hydra-derivatives#110 and samvera/hydra-derivatives#120
Thanks, this looks great! |
Should fix black images; see samvera/hydra-derivatives#110 and samvera/hydra-derivatives#120