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

Add direct preview link #6599

Merged
merged 6 commits into from
Sep 27, 2017
Merged

Add direct preview link #6599

merged 6 commits into from
Sep 27, 2017

Conversation

rullzer
Copy link
Member

@rullzer rullzer commented Sep 21, 2017

Should fix #2523

Turns out it was easier to just do it then to tell @jancborchardt to shut up again 💋 😉

Requires:

CC: @eppfel

@jancborchardt
Copy link
Member

Also cc the others from issue #2523 for review: @jm-andonegi @Cerberus-tm @oliverpool @shyamal890 @drjagan @io-node @Happyfeet01– please check it out! :)

@rullzer rullzer added 1. to develop Accepted and waiting to be taken care of and removed 3. to review Waiting for reviews labels Sep 21, 2017
@rullzer
Copy link
Member Author

rullzer commented Sep 21, 2017

Ok so we need actual samesite cookie work here to make it work. So this is not as simple... I'll look into that.

@@ -92,7 +92,7 @@
</div>
<div class="directLink">
<label for="directLink"><?php p($l->t('Direct link')) ?></label>
<input id="directLink" type="text" readonly value="<?php p($_['downloadURL']); ?>">
<input id="directLink" type="text" readonly value="<?php p($_['previewURL']); ?>">
Copy link
Member

Choose a reason for hiding this comment

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

This will not work when you share a file that is not an image, right? We should still show the direct download link then.

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't, for non preview types it is set to the downloadurl in the controller.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I've missed that it is being set there, nevermind.

@rullzer
Copy link
Member Author

rullzer commented Sep 24, 2017

This requires #6630 to work properly. I'll update once that is in.

@MorrisJobke
Copy link
Member

This requires #6630 to work properly. I'll update once that is in.

Done. Now don our work ;)

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@codecov
Copy link

codecov bot commented Sep 25, 2017

Codecov Report

Merging #6599 into master will decrease coverage by 0.01%.
The diff coverage is 9.37%.

@@             Coverage Diff              @@
##             master    #6599      +/-   ##
============================================
- Coverage     53.06%   53.05%   -0.02%     
- Complexity    22567    22580      +13     
============================================
  Files          1417     1417              
  Lines         87800    87833      +33     
  Branches       1340     1340              
============================================
+ Hits          46594    46599       +5     
- Misses        41206    41234      +28
Impacted Files Coverage Δ Complexity Δ
apps/files_sharing/templates/public.php 0% <ø> (ø) 0 <0> (ø) ⬇️
core/routes.php 0% <0%> (ø) 0 <0> (ø) ⬇️
...sharing/lib/Controller/PublicPreviewController.php 36.36% <0%> (-22.46%) 18 <8> (+8)
lib/private/Preview/Generator.php 84.56% <33.33%> (-1.06%) 44 <0> (+2)
...s/files_sharing/lib/Controller/ShareController.php 46.59% <66.66%> (+0.21%) 61 <0> (+1) ⬆️
lib/private/Server.php 84.24% <0%> (-0.13%) 123% <0%> (ø)
...vate/Authentication/Token/DefaultTokenProvider.php 94.31% <0%> (+0.06%) 26% <0%> (ø) ⬇️
lib/private/Security/CertificateManager.php 92.07% <0%> (+0.99%) 39% <0%> (ø) ⬇️
lib/private/Files/Cache/Propagator.php 96.2% <0%> (+1.26%) 16% <0%> (ø) ⬇️

@rullzer rullzer added 3. to review Waiting for reviews and removed 1. to develop Accepted and waiting to be taken care of labels Sep 25, 2017
@rullzer
Copy link
Member Author

rullzer commented Sep 25, 2017

DONE! Lets get this in as well so @jancborchardt owes me a 🍺 :)

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Works, but for example for text files this is quite suboptimal, because you get the preview instead :/

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

And the unit tests fail

@rullzer rullzer added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Sep 25, 2017
@rullzer
Copy link
Member Author

rullzer commented Sep 25, 2017

Test should be fixed. The preview endpoint is now only used for image types we can generate previews for.

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Tested and works 👍

@X4LD1M0
Copy link

X4LD1M0 commented Sep 26, 2017

Just a question, I am trying out this feature, am I correct I take the share-link and append "/preview" to make it work? Or is there an automated way to get this specific link?

@rullzer
Copy link
Member Author

rullzer commented Sep 26, 2017

@io-node that is correct.

However, if you just share a picture via public link. It should show up in the text feel below the preview.

@X4LD1M0
Copy link

X4LD1M0 commented Sep 26, 2017

OK so if i understand it correctly:

  1. Create a share link of photo
  2. Copy share link
  3. Past share link where required and append '/preview'

It does work as mentioned above. I assume once 13 is being released it will be noted somewhere (on the web-page) how to use this.

@rullzer
Copy link
Member Author

rullzer commented Sep 26, 2017

@io-node correct.

But if you open the share link. There should be a button (download) and a field direct link below the image.

@X4LD1M0
Copy link

X4LD1M0 commented Sep 26, 2017

Yes the 'direct link' i can see just under the image but this can only be used internally for users who have an account (and access to the relevant folder). I just expected there be a separate box/link to provide the preview link (if a preview is possible)

So to summarise, I expected a standard link (as normal), and (if preview is available) a preview link. This way you can share a photo still easy in the normal way (including the NC theming) and have a separate link which can be used if using it on other sites as a direct link.

@rullzer
Copy link
Member Author

rullzer commented Sep 26, 2017

No no the link is at the public page:

preview

See the bottom selected text

@X4LD1M0
Copy link

X4LD1M0 commented Sep 26, 2017

Thank you 😊 sorry for the confusion. I get now what you mean. OK that works also. As an 'expert' one can always use the normal link and add the "/preview" behind it, but for novice users they can use the method you mentioned.

Do like how it works and does exactly 'as is written on the box' 😄

I do have a little comment, since it does any image MIME type of files, the problem is with GIF animations it won't work, since it will only do a (static) preview. Can I assume this is an accepted limitation? Or is there a way to make an animated preview for GIFs?

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Looks good! Finally ;)

@MorrisJobke MorrisJobke merged commit 5f25dd7 into master Sep 27, 2017
@MorrisJobke MorrisJobke deleted the fix_2523 branch September 27, 2017 21:28
@eppfel
Copy link
Member

eppfel commented Sep 28, 2017

Awesome everyone! 😍 Sry, for being absent in recent time. Moving to Finland 🇫🇮 New job and all.

@DarkScientist
Copy link

Hi, any way to get a direct link to the image file ? I want a link who point to a .jpeg file for example...

@schniewmatz
Copy link

Hi thank you for this advance :)
Is there any option to make this work also for .svg files?

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

Successfully merging this pull request may close these issues.

Make Nextcloud a great image hosting/upload service
9 participants