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

patched Houdini convert method to add when making call #163

Merged
merged 15 commits into from
Sep 9, 2022

Conversation

dannylamb
Copy link
Contributor

@dannylamb dannylamb commented Sep 9, 2022

GitHub Issue: Islandora/documentation#2165

What does this Pull Request do?

Tells houdini to only use the first page to generate a derivative.

What's new?

A slightly modified imagemagick command if you're pulling images from a PDF

How should this be tested?

Upload a big PDF (50+ MB) and wait a while. Check the size of the thumbnail/service file that gets generated. Apply this PR to Crayfish and do it again. It should be faster and the image should be smaller.

@codecov
Copy link

codecov bot commented Sep 9, 2022

Codecov Report

Base: 76.60% // Head: 74.45% // Decreases project coverage by -2.14% ⚠️

Coverage data is based on head (181d59b) compared to base (277f48a).
Patch has no changes to coverable lines.

Additional details and impacted files
@@             Coverage Diff              @@
##                2.x     #163      +/-   ##
============================================
- Coverage     76.60%   74.45%   -2.15%     
+ Complexity      161      149      -12     
============================================
  Files             6        5       -1     
  Lines           654      599      -55     
============================================
- Hits            501      446      -55     
  Misses          153      153              
Impacted Files Coverage Δ
...d_dir/Houdini/src/Controller/HoudiniController.php

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@dannylamb
Copy link
Contributor Author

Oh codecov, guiltin' me as always. Hang on.

@dannylamb
Copy link
Contributor Author

I don't understand, I added test coverage codecov!!!!

@dannylamb
Copy link
Contributor Author

Anyhow, I did totally add tests around the branch i introduced for checking if it's a pdf. @Islandora/committers this is good to check out.

@dannylamb
Copy link
Contributor Author

FYI you can test with Islandora-Devops/isle-buildkit#216

Copy link
Member

@seth-shaw-asu seth-shaw-asu left a comment

Choose a reason for hiding this comment

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

This is impressively timely. I was just thinking about this earlier this week. Works as advertised! 👍

@seth-shaw-asu seth-shaw-asu merged commit 42278f6 into 2.x Sep 9, 2022
@seth-shaw-asu seth-shaw-asu deleted the pdf_firstpage_only branch September 9, 2022 20:58
@dannylamb
Copy link
Contributor Author

🙇 thanks @seth-shaw-asu !

@seth-shaw-asu
Copy link
Member

My pleasure. We already have this in production which has resolved some of our problematic thumbnails.

whikloj pushed a commit to whikloj/Crayfish that referenced this pull request Sep 16, 2022
@whikloj whikloj mentioned this pull request Sep 16, 2022
seth-shaw-asu pushed a commit that referenced this pull request Sep 19, 2022
* patched Houdini convert method to add  when making call (#163)
* Fix one test and remove some duplicate code
Co-authored-by: dannylamb <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants