Skip to content
This repository has been archived by the owner on Nov 14, 2018. It is now read-only.

use slideshow as image preview in files & public shares #1123

Merged
merged 3 commits into from May 25, 2013

Conversation

butonic
Copy link
Contributor

@butonic butonic commented May 22, 2013

This PR makes the slideshow available in the files app and in public file listings.
see #436

@jancborchardt @icewind1991 please review. I commented a line in the window.onhashchange function in gallery.js as it produced additional clicks on images which caused weird errors. The slideshow would not update the hash in the url when switching to the next image anyway.

@karlitschek This obsoletes the files_imageviewer app, which is no longer necessary in future releases. It is replaced by a unified user experience for viewing files in files, gallery and public shared files.

@sualko This supersedes your PR #1111 Maybe you can test this, too and give me your feedback, too. If you think this is an improvement please also close your PR.

@butonic
Copy link
Contributor Author

butonic commented May 22, 2013

@kabum can I get your attention for this PR as well?

@karlitschek
Copy link
Contributor

awesome 👍

@@ -395,10 +312,10 @@ window.onhashchange = function () {
}
console.log(OC.dirname(album));
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be removed ;)

@MorrisJobke
Copy link
Contributor

👍 Works like a charm. No JS errors. Tested in Firefox 20 and Chromium 27 on a linux machine.

@MorrisJobke
Copy link
Contributor

@butonic Concerning the function onhashchange: Did this update the url? Or what is it good for?

@butonic
Copy link
Contributor Author

butonic commented May 22, 2013

I only commented it because I have not looked to deep into the handler and was for the most part just moving code around

@butonic
Copy link
Contributor Author

butonic commented May 22, 2013

@kabum @icewind1991 I think it is used to automagically open the slideshow with an image specified after the url hash. It would allow you to send a link that directly points to an image. The hash would however not update when the slideshow progressed to the next image ... but this should be fixed in a separate PR. Maybe even with a way to link to publicly shared files that then open in the slideshow ... how could we best encode that in the hash? sth like ...#slideshow/<path to image>?

Gallery.view.viewAlbum(album);
} else {
Gallery.view.viewAlbum(OC.dirname(album));
$('#gallery a.image[data-path="' + album + '"]').click();
//$('#gallery a.image[data-path="' + album + '"]').click();
Copy link
Contributor

Choose a reason for hiding this comment

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

Commenting this out breaks some of the navigation within the gallery app using the back/foreward buttons

@icewind1991
Copy link
Contributor

The onhashchange is used to make the back and foreward buttons work within the gallery app

@icewind1991
Copy link
Contributor

Works fine except for the onhashchange within the gallery app

@butonic
Copy link
Contributor Author

butonic commented May 22, 2013

The browsers back an forward buttons?

icewind1991 notifications@github.com schrieb:

The onhashchange is used to make the back and foreward buttons work
within the gallery app


Reply to this email directly or view it on GitHub:
#1123 (comment)

Diese Nachricht wurde von meinem Android-Mobiltelefon mit K-9 Mail gesendet.

@icewind1991
Copy link
Contributor

Yes

@sualko
Copy link

sualko commented May 23, 2013

@butonic Your PR didn't supersede my PR, because not everybody wants to enable the picture app.

@butonic
Copy link
Contributor Author

butonic commented May 23, 2013

@icewind1991 I think I found a solution by only starting the slideshow when the hash differs. please review.

@sualko are you interested in extracting the slideshow as a standaone app?

@icewind1991
Copy link
Contributor

@butonic that seems to work fine 👍

@MorrisJobke
Copy link
Contributor

Doesn't work for me. The has is only appended at start of the slideshow. Shouldn't it be updated on image change?

@butonic
Copy link
Contributor Author

butonic commented May 23, 2013

Nope. But the forward backward buttons now allow going to and. From the slideshow :)

PR for url changing with slideshow progress welcome.

Morris Jobke notifications@github.com schrieb:

Doesn't work for me. The has is only appended at start of the
slideshow. Shouldn't it be updated on image change?


Reply to this email directly or view it on GitHub:
#1123 (comment)

Diese Nachricht wurde von meinem Android-Mobiltelefon mit K-9 Mail gesendet.

@MorrisJobke
Copy link
Contributor

@butonic but only once. So from the slideshow back to the gallery app

@sualko
Copy link

sualko commented May 23, 2013

@butonic I don't need a slideshow. So I suggest to publish both apps, and let the user decide which one they want to use to view their pictures.

@jancborchardt
Copy link
Contributor

No, that's not how we work in ownCloud. We don't offload these stupid decisions to users, but solve them in the software. We're building a product, not yet another Linux distribution.

I'm with @butonic here, the "Pictures" app and the mechanism for previewing Pictures in the Files app should be one and the same.

ownCloud works on the concept of apps, and an app for pictures, photos, images is one of the most important.

@MorrisJobke
Copy link
Contributor

@jancborchardt 👍

@MorrisJobke
Copy link
Contributor

I'll open a new issue, which contains the "change hash while moving through the slideshow" feature.

MorrisJobke added a commit that referenced this pull request May 25, 2013
use slideshow as image preview in files & public shares
@MorrisJobke MorrisJobke merged commit 1751113 into master May 25, 2013
@MorrisJobke MorrisJobke deleted the slideshow_as_image_preview branch May 25, 2013 09:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants