Show a text preview instead of a bitmap preview of text #15652

oparoz · 2015-04-15T15:40:32Z

A fix for

Add public preview for text files Add public preview for text files files_texteditor#13
Scale up the font on larger previews Scale up the font on larger previews #15635
File preview is in way too small of a font File preview is in way too small of a font #11469
Public page asks for previews of the wrong dimension Public page asks for previews of the wrong dimension #15653

The text can now be seen with a standard font, without magnification or size problems.

Bonus: the preview size is fixed so that this page works on smaller devices.

ghost · 2015-04-15T16:06:48Z

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

oparoz · 2015-04-15T16:10:11Z

@PVince81 Is the JS OK?

oparoz · 2015-04-15T16:12:06Z

That's what it looks like

oparoz · 2015-04-15T17:45:22Z

Assigning to 8.1 because it helps fix an alleged regression #15634

nickvergessen · 2015-04-16T08:29:12Z

apps/files_sharing/js/public.js

@@ -87,9 +87,12 @@ OCA.Sharing.PublicApp = {


 		// dynamically load image previews
+		var bottomMargin = 350;
+		var previewWidth = $(window).width() * window.devicePixelRatio;
+		var previewHeight = $(window).height() - bottomMargin * window.devicePixelRatio;


please add a min height here, otherwise when your screen is pretty small, you dont see anything anymore

Proposal:

var previewHeight = $(window).height() - bottomMargin * window.devicePixelRatio; previewHeight = Math.max(450, previewHeight);

I can confirm that the text disappears if too small. Will set a minimum height of 150.

I think an scroll-container with 150px is not really usable?

I've tested it on both desktop and mobile and it's OK, but maybe 200 is more suitable.

From my pov, the goal is to quickly being able to assess what the preview is about. Even 3000 chars might be tmi.

nickvergessen · 2015-04-16T08:33:14Z

👎 for now

nickvergessen · 2015-04-16T08:38:03Z

apps/files_sharing/js/public.js

+				headers: {Range: "bytes=0-3000"}
+			}).then(function (data) {
+				var textDiv = $('<span/>').addClass('text-preview').height(previewHeight);
+				textDiv.text(data);


I think we should append something like <br><br>... when the data is exactly 3000 characters, to make sure people understand this is only a partly preview

I'm not sure some space is going to be enough, but I don't know how else we can make it work.
Maybe
2 spaces
(...)
?

@jancborchardt any idea?

Or maybe @jancborchardt has a better sane idea

oparoz · 2015-04-16T13:29:33Z

Pushed a new version with:

1000 chars loaded (no need to load 3000 I think)
min height of 200 so that it doesn't disappear
max height of 800, enough to fit the 1000 chars
auto-scroll
ellipsis at the end of the text, if text is deemed to be longer than 1000 chars

nickvergessen · 2015-04-16T14:22:48Z

apps/files_sharing/js/public.js

+		} else if (mimetype.substr(0, mimetype.indexOf('/')) === 'text') {
+			$.ajax({
+				url: $('#downloadURL').val(),
+				headers: {Range: "bytes=0-1000"}


this seems to not work.
For me always the complete file is loaded

Not always. Either the web server or oC is deciding not to send a range past a certain file size.
Could have something to do with compression.

Maybe @LukasReschke knows why this is only done when using x-sendfile

core/lib/private/files.php

Line 191 in 1fbf5d8

if (isset($_SERVER['HTTP_RANGE']) &&

So, apparently, oC doesn't support partial reading via its PHP methods and it relies either on the web server or on WebDAV to magically send the proper ranges.

The problem with WebDAV is that I can't find a URL which works

remote.php/publicwebdav needs files_external or s2s enabled I think

public.php/webdav requires authentication

oparoz · 2015-04-16T17:48:02Z

@nickvergessen - This should now work properly by bypassing the files_sharing download page and using WebDAV directly

ghost · 2015-04-16T18:12:11Z

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

nickvergessen · 2015-04-17T05:21:00Z

apps/files_sharing/js/public.js

@@ -105,6 +110,26 @@ OCA.Sharing.PublicApp = {
 			(maxGifSize === -1 || fileSize <= (maxGifSize * 1024 * 1024))) {
 			img.attr('src', $('#downloadURL').val());
 			img.appendTo('#imgframe');
+		} else if (mimetype.substr(0, mimetype.indexOf('/')) === 'text') {
+			// Undocumented Url to public WebDAV endpoint


🙈 I don't think this is a good idea

What? To add a comment?

To use an undocumented endpoint?

Ah, it's undocumented, but not unsupported. @PVince81 pointed out that WebDAV endpoints should actually replace all the "download" endpoints in 8.2.

oparoz · 2015-04-23T14:57:02Z

@PeterPablo - I did think of alternative ways such as adding a border or even a glass cover effect, but it never seemed right.
I'm just going to go with what was recommended and users can open another issue if they feel we can do better. That will leave plenty of time to explore options such as lazy loading per example.

jancborchardt · 2015-04-23T15:00:32Z

Please no drop shadow or other separation, it’s ok like it is.

Generally it’s good to discuss stuff before implementing, but at some point it’s also too much and we just need to get something out. Then iterate. ;)

PeterPablo · 2015-04-23T15:02:07Z

Fine by me. I had the impression you were still looking for further inspiration.

+ fix the preview height

+ add ellipsis + fix OC.generateUrl calls

scrutinizer-notifier · 2015-04-23T16:41:30Z

The inspection completed: 4 new issues, 1 updated code elements

ghost · 2015-04-23T16:45:54Z

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

jancborchardt · 2015-04-23T17:39:28Z

@oparoz can you squash the commits into one as well?

Still not sure this is something for 8.1 since it’s really more a feature than a bugfix. @DeepDiver1975?

oparoz · 2015-04-23T17:45:41Z

@jancborchardt - I will once this is OKed. I was asked not to do it until the end since it's easier to track changes that way.
And regarding the milestone, well on 8.0-, people can't read the text because it's too small and in master it's too big, so to me it means that there is a bug somewhere with the way text previews are handled, but if the changes seem too risky, then maybe assign it to 8.1-next?

nickvergessen · 2015-04-24T08:34:10Z

@jancborchardt just so you dont miss it, #15652 (comment) is what people see now on the public page

jancborchardt · 2015-04-24T10:37:43Z

Ok, true.

DeepDiver1975 · 2015-05-04T07:52:42Z

I'm fine to keep this for 8.1

DeepDiver1975 · 2015-05-04T07:54:55Z

nice - works for me 👍

LukasReschke · 2015-05-04T09:44:48Z

Let's get this in for now then 👍

Show a text preview instead of a bitmap preview of text

`params` in the `OC.generateUrl` function call only replaces all specified occurences of a key just like the l10n PHP functionality does. This means that to build a query string we have to use `OC.buildQueryString` instead of the params parameters. Fixes #16336 which is a regression introduced with 58a87d0 of #15652. Without this fix downloading single files from a public shared folder is not possible.

Show a text preview instead of a bitmap preview of text

`params` in the `OC.generateUrl` function call only replaces all specified occurences of a key just like the l10n PHP functionality does. This means that to build a query string we have to use `OC.buildQueryString` instead of the params parameters. Fixes owncloud#16336 which is a regression introduced with owncloud@58a87d0 of owncloud#15652. Without this fix downloading single files from a public shared folder is not possible.

This was referenced Apr 15, 2015

Add public preview for text files owncloud/files_texteditor#13

Open

Blank text file previews #15634

Closed

File preview is in way too small of a font #11469

Closed

oparoz added 2 - Developing feature:previews labels Apr 15, 2015

oparoz self-assigned this Apr 15, 2015

oparoz added 3 - To Review and removed 2 - Developing labels Apr 15, 2015

oparoz added this to the 8.1-current milestone Apr 15, 2015

nickvergessen reviewed Apr 16, 2015
View reviewed changes

nickvergessen mentioned this pull request Apr 16, 2015

Scale up the font on larger previews #15635

Merged

oparoz added 2 - Developing and removed 3 - To Review labels Apr 16, 2015

oparoz added 3 - To Review and removed 2 - Developing labels Apr 16, 2015

nickvergessen reviewed Apr 16, 2015
View reviewed changes

oparoz added 2 - Developing 3 - To Review and removed 3 - To Review 2 - Developing labels Apr 16, 2015

nickvergessen reviewed Apr 17, 2015
View reviewed changes

oparoz added 6 commits April 23, 2015 18:19

Show a text preview instead of a bitmap preview of text

8d3f88c

+ fix the preview height

Cache the token value

e113d67

Use webDAV to have access to HTTP byte ranges

530c290

An idea to suggest that this is just a sample of the full text

a968877

Use authorization headers for better compatibility

58a87d0

+ add ellipsis + fix OC.generateUrl calls

Remove overflow+watermark

0729fc2

oparoz added 3 - To Review and removed 2 - Developing labels Apr 23, 2015

LukasReschke added a commit that referenced this pull request May 4, 2015

Merge pull request #15652 from oparoz/better-text-previews

17fedc8

Show a text preview instead of a bitmap preview of text

LukasReschke merged commit 17fedc8 into owncloud:master May 4, 2015

PVince81 mentioned this pull request May 13, 2015

Upload txt file, File has Chinese, Garbled #16229

Closed

LukasReschke mentioned this pull request May 19, 2015

Fix URL generation #16446

Merged

mmattel pushed a commit to mmattel/core that referenced this pull request May 22, 2015

Merge pull request owncloud#15652 from oparoz/better-text-previews

2b5275e

Show a text preview instead of a bitmap preview of text

danxuliu mentioned this pull request Feb 10, 2018

Add endpoint for public editor previews nextcloud/files_texteditor#71

Merged

MorrisJobke mentioned this pull request Apr 16, 2018

Fix user selectable text for public links for text files nextcloud/server#9206

Merged

lock bot locked as resolved and limited conversation to collaborators Aug 12, 2019

Show a text preview instead of a bitmap preview of text #15652

Show a text preview instead of a bitmap preview of text #15652

Conversation

oparoz commented Apr 15, 2015

A fix for

ghost commented Apr 15, 2015

oparoz commented Apr 15, 2015

oparoz commented Apr 15, 2015

oparoz commented Apr 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickvergessen commented Apr 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oparoz commented Apr 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oparoz commented Apr 16, 2015

ghost commented Apr 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oparoz commented Apr 23, 2015

jancborchardt commented Apr 23, 2015

PeterPablo commented Apr 23, 2015

scrutinizer-notifier commented Apr 23, 2015

ghost commented Apr 23, 2015

jancborchardt commented Apr 23, 2015

oparoz commented Apr 23, 2015

nickvergessen commented Apr 24, 2015

jancborchardt commented Apr 24, 2015

DeepDiver1975 commented May 4, 2015

DeepDiver1975 commented May 4, 2015

LukasReschke commented May 4, 2015