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

Do not load previews for unsupported mimes #27558

Merged
merged 1 commit into from
Apr 4, 2017
Merged

Conversation

VicDeo
Copy link
Member

@VicDeo VicDeo commented Apr 3, 2017

Description

- As suggested, set response to 204 instead of 404 if there is no preview

  • Pass supported mimetypes via oc_appconfig.core.enabledPreviewProviders and test mime against this list on js side.

Related Issue

Fixes #24216

Motivation and Context

#24216 (comment)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

@VicDeo VicDeo added this to the 10.0 milestone Apr 3, 2017
@VicDeo
Copy link
Member Author

VicDeo commented Apr 3, 2017

I'd skip loading unsupported previews completely but I haven't found a way to grab all available mimetypes at once yet :/

another option is to extend DAV response with
<oc:previewurl>urlToPreview</oc:previewurl>
and kill that funky construction part in js completely

But Trashbin is still using old non-dav endpoint (what about other apps(?))

cc @PVince81

@PVince81
Copy link
Contributor

PVince81 commented Apr 3, 2017

I think just returning 204 should be enough.

Now I wonder if mobile clients might break with this... @davivel

If yes, then let's add a special header or query param that only the web UI gives to get that 204 response.

@VicDeo
Copy link
Member Author

VicDeo commented Apr 3, 2017

ok, reverting my local miserable attempts to pass supported mimes via oc_appconfig.core

@DeepDiver1975
Copy link
Member

404 is the status if a resource is not found. This is the correct behaviour in this case. 👎

@PVince81
Copy link
Contributor

PVince81 commented Apr 4, 2017

In this case the only solution I can think of is to pass the list of supported previews through config.php or other means (like the mimetype JS thing) and prevent the client to request the preview URLs when it is known to not have previews.

That or send back the icon instead of a preview. But this could break the expectation of mobile client.

@DeepDiver1975
Copy link
Member

In this case the only solution I can think of is to pass the list of supported previews through config.php or other means (like the mimetype JS thing) and prevent the client to request the preview URLs when it is known to not have previews.

👍 this is the way it is already supposed to work

@PVince81
Copy link
Contributor

PVince81 commented Apr 4, 2017

@DeepDiver1975 it doesn't, because the web UI currently has no idea what preview types are supported. There used to be an attribute isPreviewSupported that was returned with the old ajax/list.php endpoint which we replaced by Webdav. Webdav doesn't have such attribute.

Well, we could maybe add that attribute in Webdav and call it "oc:preview-url". If set, the web UI uses it to fetch the preview. If not set, fetch the icon instead.

@VicDeo VicDeo force-pushed the missing-preview-204 branch from f5383ee to 9e9c3e9 Compare April 4, 2017 15:20
@VicDeo
Copy link
Member Author

VicDeo commented Apr 4, 2017

@DeepDiver1975 @PVince81 oook... Finally I've found how to make this lazy-registered stuff less lazy.
Is it better now?

@@ -51,7 +51,7 @@
$info = \OC\Files\Filesystem::getFileInfo($file);

if (!$info instanceof OCP\Files\FileInfo || !$always && !\OC::$server->getPreviewManager()->isAvailable($info)) {
\OC_Response::setStatus(404);
\OC_Response::setStatus(204);
Copy link
Contributor

Choose a reason for hiding this comment

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

please revert

@PVince81
Copy link
Contributor

PVince81 commented Apr 4, 2017

Tested, works.

However you need to revert the error code to 404.

@VicDeo VicDeo force-pushed the missing-preview-204 branch from 9e9c3e9 to c6d6fca Compare April 4, 2017 15:41
@VicDeo
Copy link
Member Author

VicDeo commented Apr 4, 2017

@PVince81 reverted

@PVince81
Copy link
Contributor

PVince81 commented Apr 4, 2017

👍

Let's hope you don't need to massage Jenkins again

@VicDeo VicDeo changed the title Set status to 204 if there is no preview available Do not load previews for unsupported mimes Apr 4, 2017
@VicDeo VicDeo force-pushed the missing-preview-204 branch from c6d6fca to 45aef4d Compare April 4, 2017 16:22
@VicDeo
Copy link
Member Author

VicDeo commented Apr 4, 2017

@PVince81 Tests passed

@PVince81 PVince81 merged commit 0814b3f into master Apr 4, 2017
@PVince81 PVince81 deleted the missing-preview-204 branch April 4, 2017 18:49
@dercorn
Copy link
Contributor

dercorn commented Apr 5, 2017

@VicDeo @PVince81 @DeepDiver1975 Excellent work! 👍

@PVince81
Copy link
Contributor

Regression #27932

@lock
Copy link

lock bot commented Aug 3, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TBD: Don't throw 404 in /core/preview.png?file= if no preview exists
4 participants