Adding Lazy Image Load for Icons #6692

rnveach · 2014-01-09T02:31:08Z

This is my first fork and pull request, so I hope nothing is wrong with it.
I originally submitted a feature request for changing always loading file icon images to lazy loading them to save on bandwidth when opening a folder with many pictures inside it.
The original issue can be read here at #6591 and all my reasoning for the need of this addition.

I have made small changes to the owncloud's core app "files" that achieves this functionality and am submitting it here so hopefully it can be incorporated.
The main addition is the JQuery Lazy Load library that I found at http://www.appelsiini.net/projects/lazyload , which can be used, as is, with no modifications. The library is released under the MIT License, which I believe it compatible with owncloud's APGL. Correct me if I am wrong.
The main changes to the file listing is replacing the background image style for the icon and putting it into a custom attribute that Lazy Load recognizes and works with.
The only other change is the call to the Lazy Load function in JS to monitor the user scrolling and position in the document.

I have tested this with owncloud 6.0.0a and found no issues in IE8 and FF26. The images that are not in view don't load when opening a directory (verified with firebug's net panel), the images are loaded correctly when scrolled into view, and cached images still show up rather instantly. I didn't see any problems with applying the changes to the master in git.

ghost · 2014-01-09T02:32:48Z

Can one of the admins verify this patch?

DeepDiver1975 · 2014-01-09T08:06:17Z

@owncloud-bot ok to test

DeepDiver1975 · 2014-01-09T08:10:58Z

@rnveach Thanks a lot for your contribution!
Contributions to the core repo require a signed contributors agreement http://owncloud.org/about/contributor-agreement/

Alternatively you can add a comment here where you state that this contribution is MIT licensed.

Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

DeepDiver1975 · 2014-01-09T08:22:21Z

Quick test on chromium - awesome 👍

PVince81 · 2014-01-09T09:30:24Z

apps/files/js/filelist.js

@@ -701,6 +701,8 @@ var FileList={
 				$connector.show();
 			}
 		}
+
+		$(".lazy").lazyload();


Can you maybe restrict the jQuery selector to the file list ? This way it won't try to find the class in the whole page.
Something like $('#filesTable .lazy')

PVince81 · 2014-01-09T09:31:28Z

@rnveach nice one!

Did you test it with file upload ?
When uploading files, those file elements might be added at the bottom of the list (where they aren't visible yet), so scrolling to those should also trigger the lazy loading.

PVince81 · 2014-01-09T09:32:17Z

CC @owncloud/designers @georgehrke

fossxplorer · 2014-01-09T13:10:03Z

@PVince81 Perhaps not the right place to discuss this, but can a similar functionality be implemented for Gallery thumbnails as well as for public gallery sharing? Currently gallery loads every thumbnail in both views and it's unfortunate when you have many 100/1000s of images inside one folder.

DeepDiver1975 · 2014-01-09T13:13:59Z

@fossxplorer good catch - THX - Can you open a ticket on the apps repo - just to make sure we don't forget? THX

rnveach · 2014-01-10T02:00:42Z

I updated the trashbin to also lazy load images since the same code was connected to the normal directory listing and there was an error before that I didn't notice.

@PVince81 : I added the restricted selector.
I did test uploads. It currently isn't being lazy loaded, but just normal loaded. Regardless, the images load fine. Renaming looks the same since the preview icon is connected to the file name.
I will try to work on these to change them to being lazy loaded.

For the "jquery.lazyload.js" should we switch to the minified source?

PVince81 · 2014-01-10T08:40:22Z

@rnveach thanks.

Please leave the non-minified source for "jquery.lazyload.js". ownCloud has its own minifier when DEBUG mode is disabled. Having the real source makes it easier to debug in case of bugs 😄

DeepDiver1975 · 2014-01-10T08:55:12Z

And the unminified version has to be kept due to licensing.

Von Samsung Mobile gesendet

-------- Ursprüngliche Nachricht --------
Von: Vincent Petry [email protected]
Datum:10.01.2014 09:40 (GMT+01:00)
An: owncloud/core [email protected]
Cc: Thomas Müller [email protected]
Betreff: Re: [core] Adding Lazy Image Load for Icons (#6692)

@rnveach thanks.

Please leave the non-minified source for "jquery.lazyload.js". ownCloud has its own minifier when DEBUG mode is disabled. Having the real source makes it easier to debug in case of bugs

—
Reply to this email directly or view it on GitHub.

DeepDiver1975 · 2014-01-13T14:54:29Z

@rnveach your pull request can no longer be merged - can I ask you to rebase your branch? Thanks a lot!

rnveach · 2014-01-15T03:16:41Z

I finished the rebase. All the code should be up.
I modified and tested dir listing, trash listing, uploading a file, and drag and drop. I decided doing lazy load on the rename would be pointless since the image should already be loaded by that time.
I'm done with my base I believe unless there are any issues.

PVince81 · 2014-01-15T11:45:43Z

apps/files/js/filelist.js

@@ -97,8 +100,9 @@ var FileList={
 		tr.append(td);
 		return tr;
 	},
-	addFile:function(name, size, lastModified, loading, hidden, param) {
+	addFile:function(name, etag, size, lastModified, loading, hidden, param) {


I'm not sure about this, as it's changing the function's signature as other apps might be using it (not sure for what though).
Another idea would to pass it through the "param" attribute for now.
Do you mind changing this ? (minor)

Ideally I intend to change this method in OC7 to receive a hash array as single argument, where API changes are acceptable.
But in case we want to backport your fix to OC6 it would be safer to keep the function signature as is.

PVince81 · 2014-01-15T11:56:46Z

I tried the following steps:

Create subdir "pics"
Share "subdir" with link and copy the link
Using the sync client, upload many pics into that dir (to make sure it will scroll)
Open the link you copied
For some reason, the preview don't appear on your branch, maybe the preview URL is wrong ?

rnveach · 2014-01-15T19:00:10Z

@PVince81

I made the requested change.
I actually added this to deal with performance. :)

The way the lazy load works is it creates an array of images on the page that it is to watch and maintain. With drag and drop, recalling the lazy load function doesn't remove the old array, so the library keeps trying to work with the old array even if the images aren't displayed on the page anymore. Atleast this is what I was seeing with FF.
Since the array is local to the library and encapsulated inside a local function, I don't really have direct access to the array. Everything is local so you can add multiple lazy load groups without them interfering with each other too.

If this is still unacceptable, we can discuss this in IRC on a better way to deal with it.

This was an issue with the share pages not having the required JS file. I fixed this and it and the images work on my end.

PVince81 · 2014-01-19T11:24:20Z

@rnveach can you please state the license as requested in #6692 (comment) ? Thanks 😄

PVince81 · 2014-01-19T11:26:56Z

there are two removeLazyLoad calls, one in "update()" which is correct as the whole file list is reloaded.
I was referring to the second one that is done only when one or more files are uploaded, which usually results in them being added into the list (the list is not reloaded completely), so I don't think we need to reset the whole lazyload array. What you could do is call "lazyLoad()" on the specific "tr" that has been added into the list after upload (not the whole list as you've done before).
Does it make sense ?

PVince81 · 2014-01-19T11:29:19Z

apps/files/js/file-upload.js

 									FileActions.display(tr.find('td.filename'), true);
+									$('#fileList .lazy').lazyload();


I think hee you only need to call lazyLoad() on the tr instead of the whole list:
tr.lazyLoad()

This idea goes together with my comment about removeLazyLoad()
This should improve performance when the file list is very big.

PVince81 · 2014-01-19T11:50:30Z

Had a quick test in Chrome and it works well.
I'll do a full test with other browsers once we've solved the comments above.
Note that the trashbin doesn't show real previews, just icons, but it seems to be a different issue that exists on master as well.

ghost · 2014-01-24T03:52:11Z

Test failed.
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/2758/

DeepDiver1975 · 2014-02-20T13:19:59Z

@rnveach can you please rebase this? THX

PVince81 · 2014-02-20T13:24:38Z

@rnveach let me know if you need help for rebasing.

images go away

scrutinizer-notifier · 2014-02-22T04:18:46Z

A new inspection was created.

DeepDiver1975 · 2014-04-02T06:42:18Z

@rnveach once more a rebase is necessary - sorry for that.
Please add a comment once done - @PVince81 and/or myself will catchup then and I hope we can merge this asap - THX

PVince81 · 2014-04-30T09:47:55Z

This is not needed any more since the introduction of infinite scrolling on master, because it implicitly lazy loads previews by only rendering in a paginated manner.
Had a chat with @rnveach on IRC and he's ok about this.

Closing now, thanks.

PVince81 reviewed Jan 9, 2014
View reviewed changes

PVince81 reviewed Jan 15, 2014
View reviewed changes

PVince81 reviewed Jan 19, 2014
View reviewed changes

rnveach added 5 commits February 21, 2014 22:52

added lazy image load for icons in the directory listing

785353e

restricted jquery selector to file list

5be2aca

added lazy image load for icons in the trashbin directory listing

f649722

added lazy load images for uploaded files

758302b

fixed method call with changes made to FileList.addFile

17fa519

rnveach added 5 commits February 21, 2014 22:57

added lazy image load for icons when the user is dragging and dropping

bece2b2

added code to remove lazy load before adding new entires or when those

db8989c

images go away

added lazy image load for icons in the share directory listing

97ede82

made requested change to put etag into param

8249a71

optimizations on single elements

001fa95

tanghus mentioned this pull request Mar 25, 2014

[WIP] Seamless layout for pictures owncloud-archive/gallery-old#17

Closed

5 tasks

PVince81 closed this Apr 30, 2014

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding Lazy Image Load for Icons #6692

Adding Lazy Image Load for Icons #6692

rnveach commented Jan 9, 2014

ghost commented Jan 9, 2014

DeepDiver1975 commented Jan 9, 2014

DeepDiver1975 commented Jan 9, 2014

DeepDiver1975 commented Jan 9, 2014

PVince81 Jan 9, 2014

PVince81 commented Jan 9, 2014

PVince81 commented Jan 9, 2014

fossxplorer commented Jan 9, 2014

DeepDiver1975 commented Jan 9, 2014

rnveach commented Jan 10, 2014

PVince81 commented Jan 10, 2014

DeepDiver1975 commented Jan 10, 2014

DeepDiver1975 commented Jan 13, 2014

rnveach commented Jan 15, 2014

PVince81 Jan 15, 2014

PVince81 commented Jan 15, 2014

rnveach commented Jan 15, 2014

PVince81 commented Jan 19, 2014

PVince81 commented Jan 19, 2014

PVince81 Jan 19, 2014

PVince81 commented Jan 19, 2014

ghost commented Jan 24, 2014

DeepDiver1975 commented Feb 20, 2014

PVince81 commented Feb 20, 2014

scrutinizer-notifier commented Feb 22, 2014

DeepDiver1975 commented Apr 2, 2014

PVince81 commented Apr 30, 2014

		FileActions.display(tr.find('td.filename'), true);
		$('#fileList .lazy').lazyload();

Adding Lazy Image Load for Icons #6692

Adding Lazy Image Load for Icons #6692

Conversation

rnveach commented Jan 9, 2014

ghost commented Jan 9, 2014

DeepDiver1975 commented Jan 9, 2014

DeepDiver1975 commented Jan 9, 2014

DeepDiver1975 commented Jan 9, 2014

PVince81 Jan 9, 2014

Choose a reason for hiding this comment

PVince81 commented Jan 9, 2014

PVince81 commented Jan 9, 2014

fossxplorer commented Jan 9, 2014

DeepDiver1975 commented Jan 9, 2014

rnveach commented Jan 10, 2014

PVince81 commented Jan 10, 2014

DeepDiver1975 commented Jan 10, 2014

DeepDiver1975 commented Jan 13, 2014

rnveach commented Jan 15, 2014

PVince81 Jan 15, 2014

Choose a reason for hiding this comment

PVince81 commented Jan 15, 2014

rnveach commented Jan 15, 2014

PVince81 commented Jan 19, 2014

PVince81 commented Jan 19, 2014

PVince81 Jan 19, 2014

Choose a reason for hiding this comment

PVince81 commented Jan 19, 2014

ghost commented Jan 24, 2014

DeepDiver1975 commented Feb 20, 2014

PVince81 commented Feb 20, 2014

scrutinizer-notifier commented Feb 22, 2014

DeepDiver1975 commented Apr 2, 2014

PVince81 commented Apr 30, 2014