-
-
Notifications
You must be signed in to change notification settings - Fork 388
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
Added thumbnail rendering for PDF-files #378
Conversation
- Fix RAW images not being loaded correctly in the preview panel - Fix trying to read size data from null images - Refactor `os.stat` to `<Path object>.stat()` - Remove unnecessary upper/lower conversions - Improve encoding compatibility beyond UTF-8 when reading text files - Code cleanup
Add additional default icons for: - Blender - Presentation - Program - Spreadsheet Add/expand additional media types: - PDF - Packages
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 request that changes be made. the implementation is ofc open to you, but ask me for input if you want to and you will get it!
else: | ||
page_size *= size / page_size.width() | ||
# Enlarge image to improve image downscaling (kind of arbitrary) | ||
page_size *= 2.5 |
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.
Use a good variable name instead; magic numbers like this will be confusing in the future.
page_size *= 2.5 | |
ENLARGE_SCALE_FACTOR = 2.5 | |
page_size *= ENLARGE_SCALE_FACTOR |
The suggestion is not here for you to use as a drop-in replacement. You should save this to the class and, possibly in the future (or right now), move it to a constant namespace.
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 have decided not to move it somewhere else because i didn't want to spread the logic to far apart. The variable probably won't be ever used outside the function and IMO the thumbnail renderers should be self-contained.
buffer: QBuffer = QBuffer() | ||
buffer.open(QBuffer.OpenModeFlag.ReadWrite) | ||
rendered_qimage.save(buffer, "PNG") | ||
pil_image: Image.Image = Image.open(BytesIO(buffer.buffer().data())) | ||
buffer.close() |
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.
can you use a context manager instead?
I don't know QBuffer
so I cant make an inline suggestion, I'm open to research this and help you out.
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.
AFAIK Qt for Python is directly generate from the C++ code without adding python-like features. I could add a context manager for the buffer but I do not think that is a good idea and would generate too much unnecessary code.
However, I will definitely put it in a "fake context manager" with try-except to close the buffer even with errors occuring.
# Replace transparent pixels with white (otherwise Background defaults to transparent) | ||
pixel_array = np.asarray(pil_image.convert("RGBA")).copy() | ||
pixel_array[pixel_array[:, :, 3] == 0] = [255, 255, 255, 255] |
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 sounds like something very usefull as its own method. would be useful for others in the future. Can you add this as its own method and just call it here?
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 am considering creating a utils file for images. But I'm not totally sure how the whole project is laid out.
- put transparent pixel replacement logic into new util file - made pdf scale less magical - fixed bug of not closing buffer
I'm moved this PR from the deprecated Sorry for the disruption, and thank you for your patience. |
* feat: add pdf thumbnail support Co-Authored-By: Heiholf <[email protected]> * fix: remove redef * tests: add test comparing pdf to png snapshot Co-Authored-By: yed <[email protected]> * fix: fix info in docstrings * fix: remove sample png generation * fix: change the pdf snapshot to use a black square * chore: fix whitespace --------- Co-authored-by: Heiholf <[email protected]> Co-authored-by: yed <[email protected]>
I've ported this functionality to main via #543, thank you for your contribution with this! |
Adds thumbnail support for PDF files as proposed in #374. Uses Qt's inbuilt PDF renderer.
One thing which is kind of weird is the arbitrary scaling of the image with 2.5 but it turns this
into this 
and makes the preview more readable (I think the 6.25x work increase is worth it).
I tried checking if the PDF is password protected. However, the docs don't have any info for the function
passwordRequired()
and it always returns true.I tested it on these PDF example files. The majority work properly but some don't: