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

Added thumbnail rendering for PDF-files #378

Closed
wants to merge 49 commits into from

Conversation

Heiholf
Copy link
Contributor

@Heiholf Heiholf commented Aug 23, 2024

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
Factor_1 into this Factor_2p5
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:

  • Some ImageMagick Images
  • PDFs with unreadable metadata
  • PDFs which even Okular can't render

- 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
@CyanVoxel CyanVoxel added Type: Enhancement New feature or request Type: UI/UX User interface and/or user experience Status: Review Needed A review of this is needed labels Aug 23, 2024
Copy link
Collaborator

@eivl eivl left a 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
Copy link
Collaborator

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.

Suggested change
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.

Copy link
Contributor Author

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.

Comment on lines 790 to 794
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()
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Comment on lines 795 to 797
# 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]
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

@CyanVoxel CyanVoxel removed the Status: Review Needed A review of this is needed label Sep 8, 2024
@CyanVoxel CyanVoxel added this to the Alpha v9.5 (Post-SQL) milestone Sep 8, 2024
- put transparent pixel replacement logic into new util file
- made pdf scale less magical
- fixed bug of not closing buffer
@CyanVoxel CyanVoxel changed the base branch from thumbnails to Alpha-v9.4 September 15, 2024 07:27
@CyanVoxel
Copy link
Member

I'm moved this PR from the deprecated thumbnails branch to Alpha-v9.4, which is the branch that thumbnails was eventually merged into. However the v9.4.x releases are now feature frozen and are now solely reserved for bugfixes. This means that after the changes from thumbnails are ported to main that I'll once again be moving the target for this PR over to there as a v9.5 feature.

Sorry for the disruption, and thank you for your patience.

CyanVoxel added a commit that referenced this pull request Oct 14, 2024
* 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]>
@CyanVoxel
Copy link
Member

I've ported this functionality to main via #543, thank you for your contribution with this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement New feature or request Type: UI/UX User interface and/or user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants