New gallery app #333

Merged
merged 118 commits into from Jan 28, 2013

Conversation

Projects
None yet
10 participants
Member

icewind1991 commented Dec 11, 2012

This is an almost complete rewrite of the gallery app (which is unmaintained afaik), almost all server side logic has been replaced with client side code making the server side code only responsible for thumbnail generation and getting the list of images.

This also fully ajaxyfies the gallery.

I'm not entirely happy about the look, maybe @jancborchardt can have another look at that.

cc, maybe @blizzz or @DeepDiver1975

icewind1991 and others added some commits Sep 7, 2012

@icewind1991 icewind1991 put filestorages in a namespace 3b8f539
@icewind1991 icewind1991 put gallery libs in the same namespace fc76116
@icewind1991 icewind1991 rework gallery scanner 6b5c145
@icewind1991 icewind1991 always store gallery thumbnails locally 4ac1e66
@icewind1991 icewind1991 rework of the gallery app wip 8d559a3
@BernhardPosselt @icewind1991 BernhardPosselt decreased unread timeout from 1s to .5s 5b2fd13
@DeepDiver1975 @icewind1991 DeepDiver1975 [tx-robot] updated from transifex a468d47
@BernhardPosselt @icewind1991 BernhardPosselt made links clickable on the whole width 325e8a0
@BernhardPosselt @icewind1991 BernhardPosselt If you click on a feed and then on a second feed, the first one is s…
…till loaded into the main view if the second one finished faster

: fixed
3c54ab2
@DeepDiver1975 @icewind1991 DeepDiver1975 No l10n for pdf for now d1c18db
@icewind1991 icewind1991 hide scrollbar during the slideshow 3d2755c
@icewind1991 icewind1991 make imageviewer background position fixed 1b86311
@icewind1991 icewind1991 merge master into filesystem 1a184af
@icewind1991 icewind1991 add getId to archive storage backend 8c710c5
@icewind1991 icewind1991 code style 6613494
@icewind1991 icewind1991 move storage backend test cases to their own namespace 1d54a0c
@icewind1991 icewind1991 Merge branch 'master' into filesystem 9b3fec4
@icewind1991 icewind1991 Merge branch 'master' into filesystem 19028f0
@icewind1991 icewind1991 Merge branch 'master' into filesystem ab9158b
@icewind1991 icewind1991 Merge branch 'filesystem' of github.com:owncloud/apps into filesystem 77396a2
@icewind1991 icewind1991 merge master into filesystem 8a6c596
@icewind1991 icewind1991 don't use depricated OC_Filesystem 13f8c84
@icewind1991 icewind1991 Merge branch 'master' into filesystem 6d35df9
@icewind1991 icewind1991 merge master into filesystem b4ca615
@icewind1991 icewind1991 Merge branch 'master' into filesystem a0ac4a9
@icewind1991 icewind1991 Merge branch 'master' into filesystem d5de736
@icewind1991 icewind1991 drop depricated is_readable and is_writable c14f73d
@icewind1991 icewind1991 Merge branch 'master' into filesystem 4ba2db1
@icewind1991 icewind1991 Merge branch 'master' into filesystem ffcd2be
@icewind1991 icewind1991 Merge branch 'master' into filesystem d1a0da7
@icewind1991 icewind1991 cleanup OC_Files a bit 6c5e104
@icewind1991 icewind1991 Merge branch 'master' into filesystem 38a133d
@icewind1991 icewind1991 Merge branch 'master' into filesystem 013d05f
@icewind1991 icewind1991 port to new searchByMime cca61d4
@icewind1991 icewind1991 fix gallery hooks adbef58
@icewind1991 icewind1991 merge master into filesystem e325e3d
@icewind1991 icewind1991 Merge branch 'master' into filesystem 84de06f
@icewind1991 icewind1991 Merge branch 'master' into filesystem 3c46133
@icewind1991 icewind1991 Merge branch 'master' into filesystem 0180b36
@icewind1991 icewind1991 Merge branch 'master' into filesystem f7b8f17
@icewind1991 icewind1991 Revert "[tx-robot] updated from transifex" due to merge conflicts
This reverts commit a468d47.
468cc74
@icewind1991 icewind1991 merge master into gallery c8d5486
@icewind1991 icewind1991 new thumbnail style for galleries 33a389f
@icewind1991 icewind1991 style albums f648fb1
@icewind1991 icewind1991 Merge branch 'master' into gallery 825410a
@icewind1991 icewind1991 Merge branch 'master' into filesystem e9fb3ff
@icewind1991 icewind1991 Merge branch 'master' into filesystem 6751ff6
@icewind1991 icewind1991 Merge branch 'master' into gallery 48311d5
@icewind1991 icewind1991 Merge branch 'master' into filesystem a78f597
@icewind1991 icewind1991 Merge branch 'master' into gallery 03ba34b
@icewind1991 icewind1991 Merge branch 'master' into gallery c44f2a5
@icewind1991 icewind1991 merge master into filesystem a595be4
@icewind1991 icewind1991 Merge branch 'master' into filesystem 19ecdb8
@icewind1991 icewind1991 Merge branch 'master' into gallery 3d3c42f
@icewind1991 icewind1991 Merge branch 'master' into filesystem 2f08572
@icewind1991 icewind1991 Merge branch 'master' into gallery 73328ba
@icewind1991 icewind1991 Merge branch 'master' into filesystem 2cc3958
@icewind1991 icewind1991 Merge branch 'master' into filesystem e6cc538
@icewind1991 icewind1991 Merge branch 'master' into filesystem 9d0f5c6
@icewind1991 icewind1991 Merge branch 'master' into filesystem 3488116
@icewind1991 icewind1991 Merge branch 'master' into filesystem a03d4ff
@icewind1991 icewind1991 merge master into gallery b260e9f
@icewind1991 icewind1991 merge master into filesystem b5de700
@icewind1991 icewind1991 Merge branch 'master' into filesystem 55136fd
@icewind1991 icewind1991 merge master into gallery 73e886c
@icewind1991 icewind1991 md5 archive paths to ensure we keep within the maximum id length 6cd6710
@icewind1991 icewind1991 prevent recursion in archive automount 837a32e
@icewind1991 icewind1991 Merge branch 'master' into filesystem 6b2bdb2
@icewind1991 icewind1991 Merge branch 'master' into gallery a6687b4
@icewind1991 icewind1991 Merge branch 'master' into filesystem e436166
@icewind1991 icewind1991 Merge branch 'master' into gallery 9ac8683
@icewind1991 icewind1991 Merge branch 'master' into filesystem f63816c
Owner

DeepDiver1975 commented Dec 11, 2012

Is sharing back?
We lost this feature back on a patch release of OC4 - month ago.
Still listed as feature: http://owncloud.org/features/

Member

icewind1991 commented Dec 11, 2012

Not yet

Member

karlitschek commented Dec 12, 2012

Very nice. Way better :-) 👍

Member

schiessle commented Dec 12, 2012

Looks already really nice!

Some issues I discovered:

  • If the images are stored somewhere down the path, e.g. foo/bar/img.png and /foo doesn't contain any images than the images from /foo/bar/ aren't displayed in the gallery at all.
  • If I click on the slideshow button nothing happens
  • Clicking on a image immediately starts the download. I would expect to see the complete image in the browser. But than we would probably need more gui elements to download the images and maybe also to download a selection of images, a folder with images, etc. I'm sure @jancborchardt has some ideas how this could look like. But this is probably out of scope for the initial merge request.
Member

karlitschek commented Dec 12, 2012

Hmm. Issue 2 and 3 work for me. Do you have the files_imageviewer enabled?

Member

karlitschek commented Dec 12, 2012

And issue 1 seems to be an old friend again. ;-) Haven't we fixed that in stable45 already?

Member

schiessle commented Dec 12, 2012

You are right, files_imageviewer was not enabled. Yes, issue 1 was fixed in stable45...

Regarding the image viewer I wonder what might be the best solution:

  • gray out the slideshow button or hide it completely if the image viewer isn't loaded?
  • always load the image_viewer app if the gallery is loaded?

Generally this dependencies to other apps are not easy to handle. I would try to avoid this whenever it is possible. If a app is needed by (many) other apps maybe this is a sign that this functionality shouldn't be a app in the first place.
Having a button where nothing happens is a bad solution. Hiding a button if a additional app is not enabled can lead to many hidden features which magically appear/disappear if a app gets enabled/disabled. This is not really intuitive for the user. At the moment I would tend to say that if we can't avoid this dependencies than a app should load the apps it depends on automatically. This makes sure that the full functionality is always available.

Member

karlitschek commented Dec 12, 2012

the files_imageviewer is normally always enabled by default so this is only an issue for developers who haven't linked everything from the apps repo.

Member

jancborchardt commented Dec 12, 2012

Wohoo, awesome work!

The presentation mode has some issues: It takes long to load, and doesn’t give feedback. Sometimes I click presentation mode and, upon seeing nothing happens (except fullscreening), continue browsing, and then get the slideshow after some seconds. As soon as you click presentation mode, the screen should be greyed and the first image shown, if only in poor quality (upscaled thumbnail, improving as it is downloaded).

It’s unclear how to close presentation mode, and more confusing that a click nearly anywhere closes it. It should only be closable via an x in the top right corner (corresponding also to the top right where you start the presentation). Clicking on the image or on the background should advance to the next image instead of closing it. The »next« and »previous« buttons also need more padding to be better clickable.

That said, unknown image types shouldn’t be displayed in the gallery at all. (Why can’t SVG’s be displayed in the first place? Getting 404’s for them, but SVG is perfectly displayable by browsers.)

What do you think @icewind1991?

(I also fixed some stuff directly, namely slightly soften the album colors and fixing a layout problem which made unknown file types cause a gap.)

Owner

butonic commented Dec 13, 2012

At the moment you seem to delete a thumbnail whenever an image is written. Should't the postWrite hook create thumbnails?

Member

icewind1991 commented Dec 13, 2012

At the moment you seem to delete a thumbnail whenever an image is written. Should't the postWrite hook create thumbnails?

That can cause a high load during upload, and there is no reason to create thumbs for image that aren't viewed.
So creating them when they are needed seems like a better idea to me

Member

icewind1991 commented Dec 13, 2012

If the images are stored somewhere down the path, e.g. foo/bar/img.png and /foo doesn't contain any images than the images from /foo/bar/ aren't displayed in the gallery at all.

Fixed with the above comment

Member

icewind1991 commented Dec 13, 2012

@jancborchardt svg also has the issue that we cant generate thumbnails for it at the moment, this is a limitation by the graphics library we use

Member

jancborchardt commented Dec 13, 2012

@icewind1991 svg can just be displayed as is, and scaled down constrained
by the parent container. Only IE8- and Android 2.3- can’t handle it.
http://caniuse.com/#search=svg

Member

jancborchardt commented Dec 13, 2012

@icewind1991 oh and regarding the look, @karlitschek mentioned how the Pictures app in version 3 had this great hover-to-preview feature for folders (like in iPhoto, or what Speakerdeck.com does. We might want to do that again instead of the 4-arbitrary-pictures thing (even though I suggested this at the hackfest … ;) I didn’t remember we had the awesome solution in already in the past. But this is not required in this pull request, just something which would be cool in the future.

Member

icewind1991 commented Dec 13, 2012

@jancborchardt I had it like that before we had our discussion in berlin :)

But I'm not sure how that works if the images have a different aspect ratio

Owner

butonic commented Dec 13, 2012

@icewind1991 there are three possibilities for generating images:

  1. On demand: brings the server to its knees when viewing your holiday
    photos for the first time. IMHO not acceptable for end users.
  2. On write: the user has to wait for the extra time to generate the
    thumbnail to complete the write operation. IMHO still not nice but better
    than on demand. Especially considering that network bandwidth is likely to
    be the limiting factor here.
  3. Asynchronus: maybe best, but I need to see an example for one of the
    cron implementations to see how it works. I think I will also look into
    this for the seach_lucene.

@jancborchardt what is the best option usability wise. Is there some kind
of rule? When should we use which option.

PS: 4. Generate the thumb on the client and copy that over along with the
original image. Again a bandwidth / filesize tradeoff.
PPS: 5. Look into the image and extract the thumbnail if present. I think
jpegs have an option for that ... but only works for specific formats
Am 14.12.2012 00:29 schrieb "icewind1991" notifications@github.com:

@jancborchardt https://github.com/jancborchardt I had it like that
before we had our discussion in berlin :)

But I'm not sure how that works if the images have a different aspect ratio


Reply to this email directly or view it on GitHubhttps://github.com/owncloud/apps/pull/333#issuecomment-11358598.

Member

icewind1991 commented Dec 14, 2012

A thing to take into account with generating thumbnails on upload is that a zip upload can cause a large amount of pictures to be uploaded in one request, there is already a very noticeable delay while uploading a zip due to indexing

Owner

butonic commented Dec 14, 2012

The worst case happens when you are mounting an external storage with lots
of audio ore video files (or zips). Owncloud then tries to extract all the
meta information when you enter the mount point - over the wire (imagine
mountinga 1.5TB collection on a nas to an oc installation). Maybe another
usecase for asynchronus indexing?
Am 14.12.2012 01:15 schrieb "icewind1991" notifications@github.com:

A thing to take into account with generating thumbnails on upload is that
a zip upload can cause a large amount of pictures to be uploaded in one
request, there is already a very noticeable delay while uploading a zip due
to indexing


Reply to this email directly or view it on GitHubhttps://github.com/owncloud/apps/pull/333#issuecomment-11359987.

Member

jancborchardt commented Dec 14, 2012

@icewind1991 I recalled before the discussion in Berlin it was the weird spread out on hover, wasn’t it? Or that was probably the version you were fixing, sorry for the confusion. :\ Anyway: If the images have different aspect ratio, is it possible to crop each one in thumbnail generation (with the center as fixed point) so they fit a square?

@butonic the question when to generate thumbnails boils down to when to have extra wait time. I’d say it’s better to have upload take a bit longer (because you know something needs to be processed) but have a snappy display and browsing of albums (because you don’t really expect to need to wait when just opening a folder).
But this is not something which can be decided solely from the UX perspective – maybe we have to mix approaches a bit. Asynchronous also seems good, I take it also means generating thumbnails whenever there’s idle time.

Member

icewind1991 commented Dec 14, 2012

The above commits combined with #350 adds proper svg support

The only problem left with svg is that we can't add svg images to album thumbnails atm

Member

icewind1991 commented Dec 16, 2012

I'm currently thinking of a hybrid system where it generates thumbnails on write as long as it's only a few (say <=5) and generate the rest on demand.
That way in the normal use case of uploading your images trough the WI or WebDAV they will be created automatically but it won't kill ownCloud when mounting a large storage or uploading a large archive with images.

A background job to generate thumbnails can be added later.

Member

icewind1991 commented Dec 24, 2012

@jancborchardt what do you think is the best way to show album names?

Member

icewind1991 commented Dec 27, 2012

@jancborchardt for the album thumbnail, reverting 33a389f (and removing the thumbnails in data/$user/galley) will revert to the thumbnails I had before the meeting in berlin.

Are those the thumbnails you want?

Member

jancborchardt commented Jan 1, 2013

@icewind1991 super, thank you! Album names probably best overlay the albums on the bottom, in white typeface with a slight dark text-shadow. If you put the album name there I’ll go over the styling. :)

icewind1991 added some commits Jan 1, 2013

Member

icewind1991 commented Jan 1, 2013

@jancborchardt go ahead with the styling

@butonic @DeepDiver1975 @karlitschek @schiesbn anything else that needs to be changed before this can be merged into master

Member

karlitschek commented Jan 1, 2013

👍

Owner

DeepDiver1975 commented Jan 1, 2013

I'll test this tomorrow - tonight is LotR time ..... 💍

Contributor

tanghus commented Jan 2, 2013

tonight is LotR time ..... 💍

Thought you were getting married at first 😄 (not a big fantasy flick fan)

Owner

DeepDiver1975 commented Jan 2, 2013

There is one js error:

Uncaught TypeError: Cannot read property 'length' of undefined gallery.js:130
Gallery.view.viewAlbum gallery.js:130
(anonymous function) gallery.js:227
o jquery-1.7.2.min.js:2
p.fireWith jquery-1.7.2.min.js:2
e.extend.ready jquery-1.7.2.min.js:2
c.addEventListener.B
Owner

DeepDiver1975 commented Jan 2, 2013

slideshow button is not working

Owner

DeepDiver1975 commented Jan 2, 2013

image rotation works within the album view - as soon as the image is shown in the popup the rotation is broken

Owner

DeepDiver1975 commented Jan 2, 2013

oh yeah - sharing is still missing ;-)
to be honest we need to add sharing or remove it from the feature list and tell people why we failed to re-add sharing on albums for almost one year.

Owner

DeepDiver1975 commented Jan 2, 2013

folders with & and ä ü ö are encoded wrong

Owner

DeepDiver1975 commented Jan 2, 2013

finally: it's working with IE9 but not with IE8

Owner

DeepDiver1975 commented Jan 2, 2013

overall: I like it! THX

Owner

DeepDiver1975 commented Jan 2, 2013

Please add the unminified version of bigscreen.min.js and jquery.easing.min.js.
Actually due to the minifier we don't need any minified js anymore at all.

Owner

DeepDiver1975 commented Jan 2, 2013

And you can change the author within info.xml ;-)

Owner

DeepDiver1975 commented Jan 2, 2013

Folder names with & and äöü are now displayed correctly but the images inside are not loaded.

Member

icewind1991 commented Jan 2, 2013

@DeepDiver1975

Folder names with & and äöü are now displayed correctly but the images inside are not loaded.

I can't reproduce this problem

The slideshow library keeps giving problems, I'll look for another one

Owner

DeepDiver1975 commented Jan 2, 2013

try that folder name:

p+i&ct u r äs
Member

icewind1991 commented Jan 3, 2013

@DeepDiver1975 fixed the problem

Owner

DeepDiver1975 commented Jan 4, 2013

folder 'p+i&ct u r äs' is now working.
Still open:

  • javascript error
  • image rotation
  • minified js

I'll retest IE after the above is fixed.

THX a lot!

Owner

DeepDiver1975 commented Jan 18, 2013

@owncloud-bot retest this please

LukasReschke referenced this pull request Jan 22, 2013

Merged

No inline JS - Work in progress #472

37 of 47 tasks complete

@LukasReschke LukasReschke and 1 other commented on an outdated diff Jan 25, 2013

gallery/templates/index.php
@@ -1,84 +1,57 @@
<script type="text/javascript">
-
-var root = "<?php echo OCP\Util::sanitizeHTML($_['root']); ?>";
-
-$(document).ready(function() {
- $("a[rel=images]").fancybox({
- 'titlePosition': 'inside'
- });
-});
-
+ Gallery.images = <?php echo json_encode($_['images']);?>
@LukasReschke

LukasReschke Jan 25, 2013

Member

This must be moved to an external JS file due to our CSP header.

@icewind1991

icewind1991 Jan 25, 2013

Member

Already fixed it locally

Member

icewind1991 commented Jan 28, 2013

Gallery sharing is implemented.

@schiesbn @DeepDiver1975 can we merge this?

Contributor

owncloud-bot commented Jan 28, 2013

I'll retest today. Thx

Member

karlitschek commented Jan 28, 2013

Sweet!! 👍 Ups. I already gave a 👍 earlier but nevermind :-)

Owner

DeepDiver1975 commented Jan 28, 2013

js error:

Uncaught TypeError: Object [object Object] has no method 'fancybox' gallery.js:205
Gallery.view.viewAlbum gallery.js:205
(anonymous function) gallery.js:311
f.Callbacks.o jquery-1.7.2.min.js:2
f.Callbacks.p.fireWith jquery-1.7.2.min.js:2
f.Callbacks.p.fire jquery-1.7.2.min.js:2
(anonymous function) gallery.js:25
f.Callbacks.o jquery-1.7.2.min.js:2
f.Callbacks.p.fireWith jquery-1.7.2.min.js:2
w jquery-1.7.2.min.js:4
f.support.ajax.f.ajaxTransport.send.d jquery-1.7.2.min.js:4
Owner

DeepDiver1975 commented Jan 28, 2013

image viewer was disabled - sorry for the noise

LukasReschke merged commit 04f2e2f into master Jan 28, 2013

1 check passed

default Merged build finished.
Details

LukasReschke deleted the gallery branch Jan 28, 2013

Owner

DeepDiver1975 commented Jan 28, 2013

@icewind1991 have you taken care of the image rotation?
I still see some images in wrong rotation within the imageviewer where they are correctly displayed on the overview.

Member

karlitschek commented Jan 28, 2013

Perhaps gallery should check if image_viewer is enabled and print an error message if not. I saw the code somewhere else but can't remember where.

Owner

DeepDiver1975 commented Jan 28, 2013

@LukasReschke your merge came a bit too early as I'm still testing. In addition the gallery required the new filesystem branch in core to be merged. Well - shit happens I guess ;-)

Member

LukasReschke commented Jan 28, 2013

hides himself

Sorry for that :-/

(Same for #289 - notice for myself: Always check in which tab you are...)

Big sorry again :oops:

Owner

DeepDiver1975 commented Jan 28, 2013

@icewind1991 works now in IE9 - in IE8 there is a javascript error:

SCRIPT438: Das Objekt unterstützt die Eigenschaft oder Methode "bind" nicht 
gallery.js, Zeile 137 Zeichen 3

@butonic can you help out here? THX

LukasReschke restored the gallery branch Jan 28, 2013

LukasReschke referenced this pull request Jan 28, 2013

Merged

Gallery #485

Member

LukasReschke commented Jan 28, 2013

There we go: #485

Owner

butonic commented Jan 28, 2013

IE8 has no native bind: http://kangax.github.com/es5-compat-table/ The jquery polyfill is $.proxy(). Considering performance try to live without binding: http://jsperf.com/bind-vs-jquery-proxy/5

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