-
Notifications
You must be signed in to change notification settings - Fork 67
[WIP] add full size preview if previews are disabled #512
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
Conversation
js/galleryfileaction.js
Outdated
| $.getJSON(url).then(function (config) { | ||
| window.galleryFileAction.buildFeaturesList(config.features); | ||
|
|
||
| if (config.mediatypes.length === 0) { |
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.
I think that's risky, because this could happen because of some issue and might lead to misleading bug reports.
You should be able to get the value of enable_previews via OC javascript methods.
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.
I only know OC.AppConfig. What issues could occur?
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.
I did a quick search and couldn't find anything. Maybe worth asking on IRC if there is a way to get these values in JS.
The alternative would be to add a variable to the response we send to indicate that the preview system has been disabled.
Not getting any media types can be a symptom of a bad config (php, oC, etc.) or a bug in Gallery, so if users see the thumbnails and gets the large previews, they might think that everything is working as expected.
I think it's best to detect the exact condition we want to work with.
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.
I think it is sufficient to detect it this way, because no thumbnails are generated. My pr only affects the file viewer, on the gallery page it still says "No pictures found".
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.
Let me give you an example.
Afaik, the slideshow doesn't work on public shares using the new shorter URLs on 9.0 because the scripts are not loaded any more, so the user sees thumbnails, but clicking on them does nothing downloads them.
Let's say scripts are still loaded, but there is a defect which prevents the proper list of supported media types to be sent.
With your patch, the user would have no idea that something is broken, because he would see the thumbnails and get some kind of preview, but only for a subset of the supported media types.
So it just makes debugging a bit harder and I think this could be avoided by properly letting the frontend know that previews have been disabled.
Doing it in the config controller also has the advantage of sending the same message to the Gallery side, making it easy to print a different message or a notification to the user.
|
Let's get some feedback by more people. |
|
I vote for |
Since you're using Gallery's endpoints, SVG won't work and the file will be downloaded instead of shown in the browser. |
|
closing due to lack of feedback |
fix #488
To test this you have to add
'enable_previews' => falseto your oc config. Afterwards you should be able to view all your images in the file app as usual.Any advice how to clean this up?
@oparoz @XANi