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

feat(preview): introducing new class ImaginaryPDF #46508

Merged
merged 1 commit into from
Jul 25, 2024

Conversation

ernolf
Copy link
Contributor

@ernolf ernolf commented Jul 13, 2024

Summary

Document previews often appear as minimal, almost blank "miniatures" that can inadvertently reveal content, which may not always be desired.
Many users prefer having document icons instead of these previews.
The classic preview provider settings have considered this preference.

feat: Add separate provider for Imaginary PDF handling

  • Refactor Imaginary to handle only image mimetypes.
  • Introduce ImaginaryPDF class to handle PDF mimetypes.
  • Register ImaginaryPDF in PreviewManager.

This allows for more granular control over which mimetypes are processed by Imaginary.

The approach from #46447 was too cumbersome and deviated too much from the uniform handling of preview providers. This approach has now fully implemented the suggestions and ideas from there.

TODO

  • ...

Checklist

@ChristophWurst
Copy link
Member

The rebase went bad. It seems unrelated commits have been pulled into this branch

@ernolf
Copy link
Contributor Author

ernolf commented Jul 15, 2024

The rebase went bad

Yes. Fixed. Sorry for the noise.

@ernolf ernolf force-pushed the ernolf/enh/imaginary-pdf-handling branch 3 times, most recently from b90c6e8 to a41ed80 Compare July 16, 2024 13:48
lib/private/Preview/ImaginaryPDF.php Outdated Show resolved Hide resolved
@kesselb
Copy link
Contributor

kesselb commented Jul 17, 2024

Please revert all changes to the autoloaders in apps/

@ernolf ernolf requested a review from kesselb July 17, 2024 18:20
Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

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

an illustrator file is basically also only a pdf file, so should probably be included the imaginarypdf as well I guess

@@ -40,7 +40,7 @@ public function getMimeType(): string {
}

public static function supportedMimeTypes(): string {
return '/(image\/(bmp|x-bitmap|png|jpeg|gif|heic|heif|svg\+xml|tiff|webp)|application\/(pdf|illustrator))/';
return '/(image\/(bmp|x-bitmap|png|jpeg|gif|heic|heif|svg\+xml|tiff|webp)|application\/illustrator)/';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return '/(image\/(bmp|x-bitmap|png|jpeg|gif|heic|heif|svg\+xml|tiff|webp)|application\/illustrator)/';
return '/(image\/(bmp|x-bitmap|png|jpeg|gif|heic|heif|svg\+xml|tiff|webp)/';


class ImaginaryPDF extends Imaginary {
public static function supportedMimeTypes(): string {
return '/application\/pdf/';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return '/application\/pdf/';
return '/application\/(illustrator|pdf)/';

@ernolf
Copy link
Contributor Author

ernolf commented Jul 25, 2024

an illustrator file is basically also only a pdf file, so should probably be included the imaginarypdf as well I guess

Did you read #46447 (comment)

I wouldn't classify Illustrator files as PDF files, they are more like (non text-)postscript files, which are of course all very closely related. In the narrowest sense, they are vector-based graphics. What convinced me not to include them in PDF is that they have the file extension .ai and not .pdf and therefore are not assigned a unique icon in Nextcloud like is the case with PDF.
I'm happy to be convinced otherwise, but I personally know .ai files as a graphic element that is used (among other) in PDF files.
It would be possible to add an additional class ImaginaryIllustrator instead, but then the same question would arise as with all other MIME types.

My aim was to clearly devide pure PDF documents - they have a good Icon which is way better than any preview thumb - from other, not (pure PDF) documents.

@szaimen
Copy link
Contributor

szaimen commented Jul 25, 2024

Did you read #46447 (comment)

No I havent.

I wouldn't classify Illustrator files as PDF files, they are more like (non text-)postscript files, which are of course all very closely related. In the narrowest sense, they are vector-based graphics. What convinced me not to include them in PDF is that they have the file extension .ai and not .pdf and therefore are not assigned a unique icon in Nextcloud like is the case with PDF. I'm happy to be convinced otherwise, but I personally know .ai files as a graphic element that is used (among other) in PDF files. It would be possible to add an additional class ImaginaryIllustrator instead, but then the same question would arise as with all other MIME types.

My aim was to clearly devide pure PDF documents - they have a good Icon which is way better than any preview thumb - from other, not (pure PDF) documents.

Fine by me then 👍

Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

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

LGTM but didnt test

Signed-off-by: ernolf <raphael.gradenwitz@googlemail.com>

Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
@kesselb kesselb force-pushed the ernolf/enh/imaginary-pdf-handling branch from 9f48093 to a8bdd4f Compare July 25, 2024 15:01
@kesselb
Copy link
Contributor

kesselb commented Jul 25, 2024

Thank you!

I rebased your pr, updated the autoloaders again and squashed the commits.

@kesselb kesselb merged commit 813264c into master Jul 25, 2024
167 checks passed
Copy link

welcome bot commented Jul 25, 2024

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

@kesselb kesselb deleted the ernolf/enh/imaginary-pdf-handling branch July 25, 2024 21:18
@kesselb kesselb added the pending documentation This pull request needs an associated documentation update label Jul 25, 2024
@kesselb
Copy link
Contributor

kesselb commented Jul 25, 2024

Docs: nextcloud/documentation#12061

@kesselb kesselb removed the pending documentation This pull request needs an associated documentation update label Jul 25, 2024
Copy link
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

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

Successfully merging this pull request may close these issues.

Add Option to Disable Imaginary PDF Previews
5 participants