Skip to content

Add PDF support in attachment preview dialog#11656

Draft
w15eacre wants to merge 5 commits intokeepassxreboot:developfrom
w15eacre:feature/support_pdf_preview
Draft

Add PDF support in attachment preview dialog#11656
w15eacre wants to merge 5 commits intokeepassxreboot:developfrom
w15eacre:feature/support_pdf_preview

Conversation

@w15eacre
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

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

@droidmonkey
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

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

@droidmonkey
Copy link
Copy Markdown
Member

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

@w15eacre
Copy link
Copy Markdown
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
Copy Markdown
Member

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

@w15eacre
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

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

@droidmonkey
Copy link
Copy Markdown
Member

No i just haven't had time

@xboxones1
Copy link
Copy Markdown
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
Copy Markdown
Member

That's actually a good point

@w15eacre
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

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

@droidmonkey
Copy link
Copy Markdown
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 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
Copy Markdown
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
Copy Markdown
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

@calculatorcrazy
Copy link
Copy Markdown

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 keep
Do you know if there is any encrypted text editor for Linux?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: new feature Pull request adds a new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants