-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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., Imaginary: disable document processing by default #46447
Conversation
Signed-off-by: ernolf <raphael.gradenwitz@googlemail.com>
Signed-off-by: ernolf <raphael.gradenwitz@googlemail.com>
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.
Thanks for the PR @ernolf!
I'd personally vote for enabling it by default and allowing to disable it
* Defaults to ``false`` | ||
*/ | ||
'imaginary_process_documents' => false, |
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.
* Defaults to ``false`` | |
*/ | |
'imaginary_process_documents' => false, | |
* Defaults to ``true`` | |
*/ | |
'imaginary_process_documents' => true, |
return '/(image\/(bmp|x-bitmap|png|jpeg|gif|heic|heif|svg\+xml|tiff|webp)|application\/(pdf|illustrator))/'; | ||
$config = \OC::$server->getConfig(); | ||
// Check if processing of documents is enabled | ||
$processDocuments = $config->getSystemValueBool('imaginary_process_documents', false); |
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.
$processDocuments = $config->getSystemValueBool('imaginary_process_documents', false); | |
$processDocuments = $config->getSystemValueBool('imaginary_process_documents', true); |
case 'application/pdf': | ||
case 'application/illustrator': | ||
// Check if processing of documents is enabled | ||
if (!$this->config->getSystemValueBool('imaginary_process_documents', false)) { |
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.
if (!$this->config->getSystemValueBool('imaginary_process_documents', false)) { | |
if (!$this->config->getSystemValueBool('imaginary_process_documents', true)) { |
@@ -1307,6 +1307,17 @@ | |||
*/ | |||
'preview_imaginary_key' => 'secret', | |||
|
|||
/** | |||
* Imaginary can, but does not, process PDF and Illustrator files by default. |
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.
* Imaginary can, but does not, process PDF and Illustrator files by default. | |
* Imaginary processes PDF and Illustrator files by default. |
Thanks for your pull request 👍 Cleaner, then a new configuration option, would be to use subclassing and reuse the existing preview provider configuration.
Index: lib/private/PreviewManager.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/lib/private/PreviewManager.php b/lib/private/PreviewManager.php
--- a/lib/private/PreviewManager.php (revision e6502b1399ae57a1ab3e9097ab5337e51216adb2)
+++ b/lib/private/PreviewManager.php (date 1720727804062)
@@ -346,6 +346,7 @@
$this->registerCoreProvider(Preview\MP3::class, '/audio\/mpeg/');
$this->registerCoreProvider(Preview\OpenDocument::class, '/application\/vnd.oasis.opendocument.*/');
$this->registerCoreProvider(Preview\Imaginary::class, Preview\Imaginary::supportedMimeTypes());
+ $this->registerCoreProvider(Preview\ImaginaryDocuments::class, Preview\ImaginaryDocuments::supportedMimeTypes());
// SVG and Bitmap require imagick
if ($this->imagickSupport->hasExtension()) {
Index: lib/private/Preview/Imaginary.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/lib/private/Preview/Imaginary.php b/lib/private/Preview/Imaginary.php
--- a/lib/private/Preview/Imaginary.php (revision e6502b1399ae57a1ab3e9097ab5337e51216adb2)
+++ b/lib/private/Preview/Imaginary.php (date 1720727664657)
@@ -40,7 +40,7 @@
}
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))/';
}
public function getCroppedThumbnail(File $file, int $maxX, int $maxY, bool $crop): ?IImage {
Index: lib/private/Preview/ImaginaryDocuments.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/lib/private/Preview/ImaginaryDocuments.php b/lib/private/Preview/ImaginaryDocuments.php
new file mode 100644
--- /dev/null (date 1720728758772)
+++ b/lib/private/Preview/ImaginaryDocuments.php (date 1720728758772)
@@ -0,0 +1,15 @@
+<?php
+
+declare(strict_types=1);
+/**
+ * SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+namespace OC\Preview;
+
+class ImaginaryDocuments extends Imaginary {
+ public static function supportedMimeTypes(): string {
+ return '/application\/(pdf|illustrator)/';
+ }
+} |
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.
Conceptually, this seems like a good idea.
However, I prefer kesselb's approach for its simplicity as this is mostly about changing supported mime types.
I also believe @kesselb's suggestion is beneficial as it moves us towards standardizing preview handling. Some thoughts:
Maintaining one provider per MIME type (as with legacy providers) or further dividing images into lossy, lossless, and vector-based categories might confuse existing Imaginary users. Therefore, I propose simplifying this into two categories: images processed by "Imaginary" and PDF document previews processed by "ImaginaryPDF". Since only PDF documents can be handled and Illustrator actually belongs among the images, introducing a provider called "ImaginaryDocuments" would also be rather confusing. I've submitted a new pull request #46508 (this time as a member) that expands on kesselb's approach and this thoughts. So, I kindly request your support there and would like to close this pull request if that would be OK for you. |
Hello there, 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.) |
Since #46508 is merged, this can be closed |
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.
This feature reintroduces this choice for Imaginary.
Given that it is easier to generate previews than to remove existing ones, the default value is set to 'false'.
ernolf
TODO
Checklist