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

Preview is active on file sidebar even if preview is set false on config.php #6764

Closed
rogerfv1 opened this issue Oct 4, 2017 · 9 comments
Closed

Comments

@rogerfv1
Copy link

rogerfv1 commented Oct 4, 2017

Steps to reproduce

  1. Set 'enable_previews' => false on config.php.
  2. Create a 'new text file' (ex: preview.txt).
  3. Click on 'Details' of that file.

Expected behaviour

The preview should not be shown on details sidebar.

Actual behaviour

The preview is showed on details sidebar.

Server configuration

Operating system: Debian 8

Web server: Apache 2.4

Database: Postgresql 9.4

PHP version: php 5.6

Nextcloud version: 12.0.3

Are you using external storage, if yes which one: no

Are you using encryption: no

Are you using an external user-backend, if yes which one: LDAP

We looked at the source code to see if we found the function that defines the preview load.
So we find a javascript function named loadPreview but it seems that this function is not considering the enable_previews set on config.php.

File: /var/www/nextcloud/apps/files/js/sidebarpreviewmanager.js

"...
loadPreview: function (model, $thumbnailDiv, $thumbnailContainer) {
if (model.get('hasPreview') === false && this.getMimeTypePreviewHandler(model.get('mimetype')) === null) {

...
"

In this case when getMimeTypePreviewHandler = 'native code' the preview is showed even you set enable_previews to false on config.php.

These are the results we got in our tests.

enable_previews (config.php) MimeType hasPreview getMimeTypePreviewHandler Preview is showed?(actual behaviour) Preview is showed? (expected behaviour)
true text/plain true native code yes yes
true image/png true null yes yes
true text/csv false native code yes yes
true application/pdf false native code yes yes
false text/plain false native code yes no
false image/png false null false yes
false text/csv false native code yes no
false application/pdf false native code yes no
@danxuliu
Copy link
Member

danxuliu commented Jan 6, 2018

@rogerfv1 Thanks for the detailed information!

This happens in current master too, and it could be solved by providing the enable_previews value in oc_config in JSConfigHelper.php and then taking oc_config.enable_previews into account in the condition of SidebarPreviewManager.loadPreview.

For consistency it should be taken into account too in the public shared file page.

However... should enable_previews in the system configuration be used for all that, or should its effects be limited to the generation of previews in the server? In that later case it would be good to add a parameter anyway in the administration settings of the WebUI to control whether previews are shown o not. @MorrisJobke @rullzer @blizzz @nickvergessen

@blizzz
Copy link
Member

blizzz commented Jan 15, 2018

or vice versa, loadPreviews should take enable_previews into account? after all it has an else branch, so it can continue there in case.

fgsl pushed a commit to fgsl/server that referenced this issue Jan 26, 2018
…fig.php nextcloud#6764

- Provide the enable_previews value in oc_config in JSConfigHelper.php.
- Take oc_config.enable_previews into account in the condition of SidebarPreviewManager.loadPreview.
- For consistency it take into account too in the public shared file page.
@fgsl
Copy link

fgsl commented Jan 26, 2018

I think that change must be implemented according to proposed by @rogerfv1 and @danxuliu.
From the point of view of sysadmin, I think that enable_previews must be used in the system configuration for all.

@fgsl
Copy link

fgsl commented Jan 29, 2018

@rullzer, according @rogerfv1, there are two reasons to disable image previews:

  1. There is a note in Nextcloud documentation suggesting to disable preview for enforcing environment:
    https://docs.nextcloud.com/server/12/admin_manual/configuration_server/harden_server.html

  2. For load/performance test that @rogerfv1 did (two years ago), response time of file access was better with enable_previews disabled. So, we got to serve more users with same configuration. One observation: we don't have these comparison data between times anymore.

@nextcloud-bot nextcloud-bot added the stale Ticket or PR with no recent activity label Jun 20, 2018
@skjnldsv skjnldsv added the 1. to develop Accepted and waiting to be taken care of label Jun 6, 2019
@stale stale bot removed the stale Ticket or PR with no recent activity label Jun 6, 2019
@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 1. to develop Accepted and waiting to be taken care of labels Jul 12, 2019
@skjnldsv
Copy link
Member

Fixed with #15719

@danxuliu
Copy link
Member

If I am not mistaken the issue is hidden, but not fixed, by #15719; #15719 breaks the SidebarPreviewManager, but as soon as that is fixed I think that this issue will appear again.

@skjnldsv
Copy link
Member

skjnldsv commented Oct 29, 2019

No because we now rely on what dav is telling us (if a file have a preview or not)

$propFind->handle(self::HAS_PREVIEW_PROPERTYNAME, function () use ($node) {
return json_encode($this->previewManager->isAvailable($node->getFileInfo()));
});

public function isAvailable(\OCP\Files\FileInfo $file) {
if (!$this->config->getSystemValue('enable_previews', true)) {
return false;
}

@danxuliu
Copy link
Member

But the SidebarPreviewManager purpose, if I am not mistaken, is to provide client-only previews besides any server preview that the file might or might not have. Therefore the preview from the SidebarPreviewManager should not be based on the information returned by DAV (but it should nevertheless honour the enable_previews setting or something additional as discussed in this issue).

@skjnldsv
Copy link
Member

You can forgtot about the SidebarPreviewManager :)

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

No branches or pull requests

7 participants