Skip to content

Conversation

@nickvergessen
Copy link
Contributor

@nickvergessen nickvergessen added this to the 8.1-current milestone Apr 15, 2015
@nickvergessen
Copy link
Contributor Author

@oparoz

@karlitschek
Copy link
Contributor

👍

@oparoz
Copy link
Contributor

oparoz commented Apr 15, 2015

That works for small 36x36 previews
preview
but not for publicly shared links
publicpreview

@oparoz
Copy link
Contributor

oparoz commented Apr 15, 2015

I think there are a few open tickets asking for those large previews to be rendered as text directly, but I don't remember what's holding that back.

@nickvergessen
Copy link
Contributor Author

Well the other idea would be to not use the max-size thumb trick for txt previews. But then we would need to extend the API of the preview providers, so they can specify whether or not they support the max-size trick

@ghost
Copy link

ghost commented Apr 15, 2015

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/11536/
🚀 Test PASSed.🚀
chuck

@scrutinizer-notifier
Copy link

The inspection completed: 1 updated code elements

@oparoz
Copy link
Contributor

oparoz commented Apr 15, 2015

I don't think the max preview is to blame. This has never worked properly, so it would be best to fix the txt provider imo.

When I stumbled upon #11469, I did the same thing you did and ran into the exact same issue. That was on oC7

@nickvergessen
Copy link
Contributor Author

@oparoz well but if we use a font size that looks good on the public page (sharing thing), then you get a not-readable 36*36 preview, because that is just a resized version of the public page preview.

@oparoz
Copy link
Contributor

oparoz commented Apr 15, 2015

Txt seems to be the only exception, so maybe it best to fix the public page than mess with the preview system?
Maybe use 3 instead of 5 for now so that small previews work and large previews are a bit less bad?
$fontSize = ($maxX) ? (int) ((3 / 36) * $maxX) : 5; //5px

@oparoz
Copy link
Contributor

oparoz commented Apr 15, 2015

There are plenty of exceptions already in the public page
https://github.com/owncloud/core/blob/master/apps/files_sharing/js/public.js#L104-L117

@nickvergessen
Copy link
Contributor Author

Closing in favor of #15652

@nickvergessen nickvergessen deleted the issue/15634-empty-txt-previews branch April 16, 2015 09:05
@nickvergessen nickvergessen restored the issue/15634-empty-txt-previews branch April 16, 2015 09:07
@nickvergessen
Copy link
Contributor Author

Okay, not really replaced, since this one fixes the mini previews, while the other one fixes the public page

@nickvergessen nickvergessen reopened this Apr 16, 2015
@ghost
Copy link

ghost commented Apr 16, 2015

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/11563/
🚀 Test PASSed.🚀
chuck

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@oparoz

Txt seems to be the only exception, so maybe it best to fix the public page than mess with the preview system?
Maybe use 3 instead of 5 for now so that small previews work and large previews are a bit less bad?
$fontSize = ($maxX) ? (int) ((3 / 36) * $maxX) : 5; //5px

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@oparoz I used 5 / 36 because this is the original font size that was used.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK

@oparoz
Copy link
Contributor

oparoz commented Apr 16, 2015

👍 from me, if used in combination with #15652 and if that's the sane thing to do.

@nickvergessen
Copy link
Contributor Author

@MorrisJobke another review? ;)

@MorrisJobke
Copy link
Contributor

👍

MorrisJobke added a commit that referenced this pull request Apr 20, 2015
@MorrisJobke MorrisJobke merged commit 3e8f6cd into master Apr 20, 2015
@MorrisJobke MorrisJobke deleted the issue/15634-empty-txt-previews branch April 20, 2015 13:55
@lock lock bot locked as resolved and limited conversation to collaborators Aug 12, 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.

Blank text file previews

6 participants