Infinite scrolling for files app #7167

PVince81 · 2014-02-12T10:02:45Z

Adds in-memory infinite scroll: the whole files list is loaded into a JS array but only the next 20 table rows are rendered, and then the next 20, etc when scrolling.

DeepDiver1975 · 2014-02-12T22:22:22Z

@PVince81 What is your plan regarding the share information for the 20 loaded files?

From my point of view any share information should be delivered together with the file information.
This will reduce the number of ajax calls - because as of today share information for the complete folder is loaded in an extra call.

an example JSON response could look like this:

[
   {
      "fileId" : 6967,
      "shares" : [
         {
            "type" : "files",
            "shareType" : 5,
            "permission" : 1,
            "shareId" : 241,
            "name" : "07-12-11_no231_weihnachtsmann.jpg",
            "url" : "http://localhost/core-stable5/public.php?service=files&t=0e72c30928c1faed00ff0602771b24bf",
            "shareWith" : "user@example.com"
         },
         {
            "permission" : 1,
            "shareType" : 4,
            "type" : "files",
            "shareWith" : "user@example.com",
            "name" : "07-12-11_no231_weihnachtsmann.jpg",
            "url" : "http://localhost/core-stable5/public.php?service=files&t=558af554cd3d0b41648c01b4b0ba6f8a",
            "shareId" : 242
         }
      ],
      "path" : "/07-12-11_no231_weihnachtsmann.jpg"
   },
   {
      "fileId" : 185,
      "path" : "/Beamer JVC HD1.jpg",
      "shares" : [
         {
            "type" : "files",
            "shareType" : 5,
            "permission" : 1,
            "shareId" : 241,
            "shareWith" : "user@example.com",
            "url" : "http://localhost/core-stable5/public.php?service=files&t=0e72c30928c1faed00ff0602771b24bf",
            "name" : "Beamer JVC HD1.jpg"
         },
         {
            "permission" : 1,
            "type" : "files",
            "shareType" : 4,
            "name" : "Beamer JVC HD1.jpg",
            "url" : "http://localhost/core-stable5/public.php?service=files&t=558af554cd3d0b41648c01b4b0ba6f8a",
            "shareWith" : "user@example.com",
            "shareId" : 242
         },
         {
            "shareId" : 291,
            "url" : "http://localhost/core-stable5/public.php?service=files&t=b88b2771c1c1a48beb9bcb0334123a7c",
            "name" : "Beamer JVC HD1.jpg",
            "shareWith" : null,
            "type" : "file",
            "shareType" : 3,
            "permission" : 1
         }
      ]
   }
]

PVince81 · 2014-02-13T08:53:15Z

@DeepDiver1975 at the moment ALL files are loaded in a JS array but only the 20 first are rendered in the table, so having the whole sharing should currently work.

But you are right, I could try making the share data available along with the file data instead, maybe a task for the ajaxification here: #6968
But I'm afraid that the PR there is already too big so not sure about adding new stuff... should we get it merged first ? (step by step / incremental)

PVince81 · 2014-02-13T09:11:38Z

@DeepDiver1975 I'll discuss this with Björn tomorrow as I still need to get familiar with the sharing API and will either add it into that PR or separately (a mix between the sharing task and files app performance improvements)

DeepDiver1975 · 2014-02-13T09:19:11Z

But I'm afraid that the PR there is already too big so not sure about adding new stuff... should we get it merged first ? (step by step / incremental)

For sure - my intention was to talk/discuss about the overall picture and concept.

PVince81 · 2014-02-13T15:13:03Z

@owncloud/designers question about infinite scroll.
Imagine you see the first 20 of a long file list, sorted from A to F alphabetically.
Scrolling down would give you G, H, etc
Let's say you upload a file that starts with "Z" or rename the file "A" to "Z".
What is supposed to happen ?
Do we:

Reload the whole list ? (making the infinite scroll start from the first "page")
Append/insert the file into the list ?

For 2) we could try and find out whether the file belongs to the current displayed range, if yes insert it, if not don't show it and let it appear as soon as the user scrolls down.

What do you think ?

PVince81 · 2014-02-14T16:00:39Z

Just tried this in IE8 (VM, not so strong computer) with 10000 files.
After the initial scan it took about 17 seconds to load the file list from the server, and then the scrolling went well.

Of course if you try and scroll to the bottom it will become slower over time, but at least you can open that folder without crashing the browser.

I'm surprised that even IE8 isn't bothered when loading 10000 file entries into a JS array.

PVince81 · 2014-02-14T16:03:12Z

CC @jancborchardt

I think this is already a nice improvement. I suggest we look into "API wise pagination" and "virtual scrolling" later (separate PR/future)

I'll try and optimize this one here a bit more.
I still have a few TODOs like make select all work with the trashbin.

karlitschek · 2014-02-14T16:17:12Z

I agree. This is a nice improvement and kind of "good enough" TM because 10000 file in one directory is a corner case anyways.
Great work!!

jancborchardt · 2014-02-17T15:49:18Z

Cool! After the scroll threshold improvement as we talked about, I’m fine with this. :) Design-wise this is a 👍 although I can’t judge if the code is ok.

PVince81 · 2014-02-17T15:50:17Z

Thanks for the design review.
I still need to fix the trashbin "restore" with select all and test the files app more thoroughly.

PVince81 · 2014-02-17T17:30:13Z

Please review #7195 first, which fixes the "select all" case when not all files are in the list

jancborchardt · 2014-02-20T16:44:52Z

@PVince81 #7195 is in, can you fix the failed stuff and rebase?

PVince81 · 2014-04-02T14:42:57Z

Rebased on top of the ajaxification branch which is itself rebased on top of master so needs to be retested.

@MorrisJobke if you like you can test this branch instead of the ajaxification one... at least this way we can get the infinite scroll stuff in faster...

Then I'll need someone to look at the code.

PVince81 · 2014-04-02T14:47:11Z

@schiesbn if you like I can guide you through the new code on this branch for a code review ?

schiessle · 2014-04-02T15:19:52Z

reviewed the code together with @PVince81, looks good 👍

PVince81 · 2014-04-03T16:33:33Z

Bug: renaming a file puts it back to the beginning
When uploading/creating a new file: insert at the correct position if it exists, else don't. Do insert into internal JSON array.

MorrisJobke · 2014-04-03T20:04:34Z

@PVince81 Rebase please (or just fast-forward ;))

PVince81 · 2014-04-04T08:08:42Z

Rebased.
You might want to hold testing for this until I've fixed the points above.

PVince81 · 2014-04-04T14:17:00Z

Open a folder with 40 files, delete the first 20 without scrolling down: next page should be loaded atuomatically, but isn't.

PVince81 · 2014-04-04T14:18:25Z

Open folder with 40 files without scrollng, click "select all" then unselect the first one. Total selected files does not match. Selection code needs to be revamped to be based off the "FileList.files" array instead of DOM.

I predict many future issues with this because many apps rely on the file names being in the DOM (for example slideshow). These apps will have to be adapted to rely on the FileList.files array.

PVince81 · 2014-04-08T14:50:48Z

Rebased and fixed selection to be now based on the internal array instead of the visible checkboxes...

Additional issues:

Drag and drop shadow has wrong file name order

PVince81 · 2014-04-08T15:34:33Z

- moved file selection code to FileList - fix selection summary when all files are selected - nextPage now auto-selects files if "select all" checkbox is checked - fixed trashbin to use the same selection logic as FileList

The FileList.files model is now updated with file operations. Adding files to the list will add to the model first, then to the DOM. If the insertion point isn't visible yet, the file won't be added to the DOM until the user scrolls down. Updated unit tests to include checking for the correct insertion point.

Removed "insert" flag, inserting is by default for FileList.add(). Added "animate" flag to FileList.add(). Added logic to correctly detect when to insert/append elements whenever the insertion point is visible or not. Fixed "render next page" logic to work correctly when many pages of files have been added.

The file selection is now based on the internal model array FileList.files instead of the visible checkboxes. This makes it possible to virtually select files that haven't been rendered yet (select all, then deselect a visible one) Added more unit tests for selection (with shift and ctrl as well)

Now using _.bind() for event handlers so we can use "this" which is more readable than a static access to FileList.

Fixed drag and drop code to use FileList.getSelectedFiles() instead of the visible DOM elements.

PVince81 · 2014-04-28T12:59:33Z

Rebased.

scrutinizer-notifier · 2014-04-28T12:59:44Z

A new inspection was created.

schiessle · 2014-04-28T14:02:41Z

went through the code together with @PVince81 and saw how it works 👍

scrutinizer-notifier · 2014-04-28T14:52:15Z

A new inspection was created.

DeepDiver1975 · 2014-04-28T15:12:17Z

NICE! 👍

ghost · 2014-04-28T15:32:38Z

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

Infinite scrolling for files app

PVince81 · 2014-04-28T15:39:37Z

Thanks! 🍸

ghost · 2014-04-28T16:47:28Z

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

alanv72 · 2014-04-28T23:09:00Z

Trying to cherry-pick this branch for a OC6.02 install. After merging just the modified files, I'm getting this error in the owncloud.log file... {"app":"PHP","message":"Call to a member function getType() on a non-object at /www/beta/apps/files/index.php#45","level":3,"time":"2014-04-28T23:05:40+00:00"}

Any suggestions?

DeepDiver1975 · 2014-04-29T05:32:56Z

Use the master branch if you want to test thus feature. Cherry-pick will not work as many other changes are necessary

Von Samsung Mobile gesendet

-------- Ursprüngliche Nachricht --------
Von: alanv72 notifications@github.com
Datum:29.04.2014 01:09 (GMT+01:00)
An: owncloud/core core@noreply.github.com
Cc: Thomas Müller thomas.mueller@tmit.eu
Betreff: Re: [core] Infinite scrolling for files app (#7167)

Trying to cherry-pick this branch for a OC6.02 install. After merging just the modified files, I'm getting this error in the owncloud.log file... {"app":"PHP","message":"Call to a member function getType() on a non-object at /www/beta/apps/files/index.php#45","level":3,"time":"2014-04-28T23:05:40+00:00"}

Any suggestions?

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

alanv72 · 2014-04-29T14:58:32Z

Thanks! Sorry for the newbie question.. but what's the best way to know what pull request made it into the owncloud community editions? For example, how can I tell if this made it into 6.0.3?

PVince81 · 2014-04-29T15:03:50Z

Without juggling with git it's a bit hard to see.
Normally you could have a look at the changelog of 6.0.3 if it's in.

But generally speaking the stable branches (OC 6) only get bugfixes, no new features.
New features are developed on master, the next big version which is OC 7.

Even easier is to just ask in the PR 😉

alanv72 · 2014-04-29T21:26:10Z

Thanks for the info guys. Still trying to learn how to use github efficiently...

DeepDiver1975 mentioned this pull request Feb 13, 2014

Files browser: endless scroll of files list #251

Closed

PVince81 mentioned this pull request Feb 18, 2014

Having 5132 deleted files results in a unresponsive webbrowser #2860

Closed

MorrisJobke added Enhancement labels Feb 19, 2014

PVince81 mentioned this pull request Mar 6, 2014

Files app scrolling / progressive load with many files #7597

Closed

PVince81 changed the title ~~[WIP] Infinite scrolling for files app~~ Infinite scrolling for files app Apr 2, 2014

PVince81 mentioned this pull request Apr 2, 2014

Ajaxify files list for files + trashbin + public page #6968

Merged

PVince81 mentioned this pull request Apr 4, 2014

Use FileList.files instead of DOM to get all images owncloud-archive/gallery-old#22

Merged

Vincent Petry added 11 commits April 28, 2014 14:55

Fix file selection for infinite scrolling

9c2fbea

- moved file selection code to FileList - fix selection summary when all files are selected - nextPage now auto-selects files if "select all" checkbox is checked - fixed trashbin to use the same selection logic as FileList

Make sure there are always enough elements visible on the page

8909b57

Fix trashbin previews and "delete selected"

a952d80

Fixed drag shadow file sorting

3159c2e

Cleanup and fix trashbin "clear all files" operation

3c006a5

Cleanup of event handlers

c29e8b0

Now using _.bind() for event handlers so we can use "this" which is more readable than a static access to FileList.

Fixed selection summary calculation issue

a15b68c

Fixed drag and drop into folder and onto breadcrumb

f99f451

Fixed drag and drop code to use FileList.getSelectedFiles() instead of the visible DOM elements.

typos, naming, remove unused code, identation

bf61d84

DeepDiver1975 added a commit that referenced this pull request Apr 28, 2014

Merge pull request #7167 from owncloud/files-ajaxload-infscroll

e055a41

Infinite scrolling for files app

DeepDiver1975 merged commit e055a41 into master Apr 28, 2014

DeepDiver1975 deleted the files-ajaxload-infscroll branch April 28, 2014 15:39

PVince81 mentioned this pull request Apr 30, 2014

Clear trashbin selection after action #7161

Closed

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

Infinite scrolling for files app #7167

Infinite scrolling for files app #7167

Conversation

PVince81 commented Feb 12, 2014

DeepDiver1975 commented Feb 12, 2014

PVince81 commented Feb 13, 2014

PVince81 commented Feb 13, 2014

DeepDiver1975 commented Feb 13, 2014

PVince81 commented Feb 13, 2014

PVince81 commented Feb 14, 2014

PVince81 commented Feb 14, 2014

karlitschek commented Feb 14, 2014

jancborchardt commented Feb 17, 2014

PVince81 commented Feb 17, 2014

PVince81 commented Feb 17, 2014

jancborchardt commented Feb 20, 2014

PVince81 commented Apr 2, 2014

PVince81 commented Apr 2, 2014

schiessle commented Apr 2, 2014

PVince81 commented Apr 3, 2014

MorrisJobke commented Apr 3, 2014

PVince81 commented Apr 4, 2014

PVince81 commented Apr 4, 2014

PVince81 commented Apr 4, 2014

PVince81 commented Apr 8, 2014

PVince81 commented Apr 8, 2014

PVince81 commented Apr 28, 2014

scrutinizer-notifier commented Apr 28, 2014

schiessle commented Apr 28, 2014

scrutinizer-notifier commented Apr 28, 2014

DeepDiver1975 commented Apr 28, 2014

ghost commented Apr 28, 2014

PVince81 commented Apr 28, 2014

ghost commented Apr 28, 2014

alanv72 commented Apr 28, 2014

DeepDiver1975 commented Apr 29, 2014

alanv72 commented Apr 29, 2014

PVince81 commented Apr 29, 2014

alanv72 commented Apr 29, 2014