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

Add PDF support in attachment preview dialog #11656

Draft
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

w15eacre
Copy link
Contributor

@w15eacre w15eacre commented Jan 13, 2025

Add PDF support in attachment preview dialog

To preview the first page of a PDF file you need poppler >= 23.1.0.
Add an optimization to resize images.

Screenshots

plugin

Before the optimization
before1

After
after1

Testing strategy

  • Added tests for a new type of attachments. Unit tests were implemented to verify the functionality of the new attachment type.
  • Manually. Performed manual testing to ensure the correct behavior of the new attachment feature in various scenarios.

Type of change

  • ✅ New feature (change that adds functionality)

@w15eacre
Copy link
Contributor Author

@droidmonkey Could you explain how to add these dependencies for the build?

@droidmonkey
Copy link
Member

droidmonkey commented Jan 13, 2025

Will need to add it to the vcpkg.json and CMakeLists.txt, probably as an optional dependency

On the CI boxes we will install it

@w15eacre
Copy link
Contributor Author

Great!
I figured out how to add a dependency and will do that this week. And I'll try adding UI tests because the dialog has become more complex.

@w15eacre
Copy link
Contributor Author

@droidmonkey I couldn’t find qt5-image-formats-plugin-pdf in the package manager. Can I use Poppler instead?

@droidmonkey
Copy link
Member

Poppler seems well regarded and supported, i don't have an issue with it.

@w15eacre
Copy link
Contributor Author

@droidmonkey I need your help. The CI/CD environment doesn’t have the required library (Poppler). Do I need to add a new CMake option to enable or disable this feature?

@w15eacre w15eacre marked this pull request as ready for review January 18, 2025 23:58
@droidmonkey
Copy link
Member

Don't worry about the CI env, I'll get it all set once your work is ready for review.

@w15eacre
Copy link
Contributor Author

I have finished it. I think previewing the first page is enough, but there’s no problem with adding all pages to the preview.

@w15eacre
Copy link
Contributor Author

@droidmonkey Do you need some more information to update CI?

@droidmonkey
Copy link
Member

No i just haven't had time

@xboxones1
Copy link
Contributor

@w15eacre, @droidmonkey
Why can I create a text document with the ability to edit it, but then why can't I modify a text attachment? I thought it would be a simple text editor since this was closed #3383

@droidmonkey
Copy link
Member

That's actually a good point

@w15eacre
Copy link
Contributor Author

@w15eacre, @droidmonkey

Why can I create a text document with the ability to edit it, but then why can't I modify a text attachment? I thought it would be a simple text editor since this was closed #3383

We can only edit text attachments. How do you see this implemented if there are many files we cannot edit? Should we just enable or disable the edit button in such cases?

@xboxones1
Copy link
Contributor

xboxones1 commented Jan 25, 2025

Only allow editing of supported text formats. I thought that a simple text editor would be implemented, that's why I suggested to see how it is implemented in keepass.

keep

@w15eacre
Copy link
Contributor Author

I will implement this in the next PR because I think it will be a big change

@droidmonkey
Copy link
Member

How do you see this implemented if there are many files we cannot edit?

Introduce a function for "editable Mime-Type" detection. If it's editable you set the preview as not read-only and when you close the preview you ask if you want to save the changes

@droidmonkey droidmonkey added the pr: new feature Pull request that adds a new feature label Jan 25, 2025
@droidmonkey droidmonkey self-requested a review January 25, 2025 21:09
@droidmonkey droidmonkey added this to the v2.7.10 milestone Jan 25, 2025
@droidmonkey
Copy link
Member

droidmonkey commented Jan 25, 2025

@w15eacre I am getting very poor results with all PDF renders (this is on Windows):

image

I am going to reserve this for 2.8.0 since it also introduces a dependency.

@droidmonkey droidmonkey modified the milestones: v2.7.10, v2.8.0 Jan 25, 2025
@droidmonkey droidmonkey force-pushed the feature/support_pdf_preview branch from 6c26118 to c34d1d6 Compare January 25, 2025 22:20
@w15eacre
Copy link
Contributor Author

w15eacre commented Jan 25, 2025

@droidmonkey
That's interesting because I don't have any issues with PDF render quality(Linux). I need to check it on Windows.

image

@w15eacre w15eacre marked this pull request as draft January 26, 2025 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: new feature Pull request that adds a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants