Introduce lazy loading #88

daniellandau · 2017-01-07T21:44:22Z

This is related to #12

eppfel · 2017-01-08T11:46:04Z

Thanks, for that detailed work. I would move the detailed explanation here, because you not only explain the problem, but also your solution.

I ping @nextcloud/contacts @ChristophWurst for review!

skjnldsv

Amazing work! It was next on my todo list! 🎉
I will review it deeply as soon as possible!

skjnldsv · 2017-01-08T13:31:51Z

templates/group.html

@@ -1 +1 @@
-<a ng-href="#/{{ctrl.group}}">{{ctrl.group}}</a>
+<a ng-href="#/{{ctrl.group}}">{{ctrl.group}} ({{ctrl.groupCount}})</a>


Please format menus according to guidelines: https://docs.nextcloud.com/server/11/developer_manual/app/css.html#menus

skjnldsv · 2017-01-08T16:43:13Z

js/filters/counterFormatter_filter.js

@@ -0,0 +1,12 @@
+// from https://docs.nextcloud.com/server/11/developer_manual/app/css.html#menus
+angular.module('contactsApp')


Please add tests (seehttps://github.com/owncloud/contacts/blob/31e403af1db859f85e84c2337134e20e5719c825/js/tests/filters/counterFormatter_filter.js)

skjnldsv · 2017-01-08T16:43:59Z

templates/group.html

@@ -1 +1 @@
-<a ng-href="#/{{ctrl.group}}">{{ctrl.group}} ({{ctrl.groupCount}})</a>
+<a ng-href="#/{{ctrl.group}}">{{ ctrl.group }} ({{ ctrl.groupCount | counterFormatter }})</a>


Better, but you need to use the li element
<li class="app-navigation-entry-utils-counter">{{ count | counterFormatter }}</li>

In order to get right-aligning instead of parentheses I gather. Sure thing, fixed.

skjnldsv · 2017-01-08T16:57:51Z

templates/groupList.html

@@ -1 +1 @@
-<li ng-repeat="group in ctrl.groups" group data="group" ng-click="ctrl.setSelected(group)" ng-class="{active: group === ctrl.getSelected}"></li>
+<li ng-repeat="group in ctrl.groups" group group-name="group[0]" group-count="group[1]" ng-click="ctrl.setSelected(group[0])" ng-class="{active: group[0] === ctrl.getSelected}"></li>


you could do something like <li ng-repeat="(group, count) in ctrl.groups" group data="group" count="count" ng-click="ctrl.setSelected(group)" ng-class="{active: group === ctrl.getSelected}"></li> for better readibility :)

See my previous work: https://github.com/owncloud/contacts/pull/341/files

Oh, did not know that Angular does list destructuring, will fix.

Hmm, it seems that that syntax is only used for going through the keys and values in an object.

Isn't there an alternative?

The order of keys in an object is arbitrary, so I don't want to have the whole ctrl.groups be an object. We want to guarantee All Contacts is first.

Too bad, it would have been great to improve the readability. But it's clearly not that important :)

skjnldsv · 2017-01-08T17:02:14Z

js/components/contactList/contactList_controller.js

-				ctrl.setSelectedId(ctrl.contactList[0].uid());
-			}
+			var firstNames = ctrl.contactList.slice(0, 20).map(function (c) { return c.displayName(); });
+			ContactService.getFullContacts(firstNames);


We still don't want to load the first contact in mobile mode.
We need a condition to prevent that like we had before ;)

We now never blindly load the first contact, so not on mobile either.

By selecting the first uid after load, we display the first contact.
In mobile, since the list view and detail views are split, we need to stay on the contact list only. :)

I don't think we are selecting the first uid at all. I tested with both a responsive design mode and an actual mobile browser.

So the right app panel is blank on your contact installation? On fresh click of your.domain/index.php/apps/contacts, the app doesn't load a contact?

~~Strange, it changes the url with an uid and then load a contact in my installation.~~
Anyway, we need to keep the old mechanic in place.

If mobile, do not load contact details and stay on the contacts list EXCEPT if there is a matching uid specified in the url

If not in mobile, load the first contact

Edit: error in my environment. It doesn't load a thing indeed.

Ok, added that back in. It adds ~400 ms rendering time, but in the scheme of things that's probably fine.

You're awesome! :)

skjnldsv · 2017-01-08T17:03:28Z

If no contacts, I get errors on the console:

Uncaught TypeError: Cannot read property 'length' of undefined
    at contactList_controller.js:22

TODO: fix if no contacts

daniellandau · 2017-01-08T17:15:47Z

I added a defense in contactList_controller.

skjnldsv · 2017-01-08T17:47:46Z

js/services/contact_service.js

 								} else {
-									// custom console
+									// eslint-disable-next-line no-console


daniellandau · 2017-01-10T20:50:03Z

@skjnldsv did you want me to still do things? Like rebase with signed off commits?

skjnldsv · 2017-01-10T20:53:06Z

Not really. But we need to wait for the 3rdparty patch/sabre release.

jancborchardt · 2017-01-24T00:59:15Z

Any idea when this sabre patch release is coming? cc @evert

daniellandau · 2017-01-24T11:06:19Z

I don't know. I asked on their IRC channel a couple of weeks ago and got no reply. I asked two days ago on Github (https://github.com/fruux/sabre-dav/issues/936), but haven't gotten any reply yet.

Henni · 2017-01-24T12:14:08Z

I'd suggest to split this up into two separate pull requests:

one for the the partial carddav requests, which is blocked by sabre-dav
and another one which loads the contacts list piece by piece

I'd like to merge the second one as soon as possible, because it might introduce a nice performance improvement and also to reduce merge conflicts in the future.

@daniellandau what do you think about this? would you do the job of splitting this PR up?

daniellandau · 2017-01-24T12:22:00Z

Do you mean as the second part the thing that renders the contact list piece by piece? I'd rather just merge the sabre-dav patch and merge this whole thing. All in all that would be much less maintenance work in my opinion, and the sabre-dav patch is merged in all branches upstream, so it shouldn't cause conflicts or problems later on either.

Henni · 2017-01-24T13:00:24Z

@daniellandau shouldn't cause any conflicts. But I also understand that the guys over at nextcloud/server don't want to patch anything unreleased into their 3rdparty repo.

Let's wait a bit longer for a response by sabre/dav.

daniellandau · 2017-03-02T20:22:58Z

Just a quick update, the sabre/dav release has come, and the current PR on 3rdparty is nextcloud/3rdparty#40

Signed-off-by: Daniel Landau <daniel@landau.fi>

It's a really heavy operation that hangs the browser Signed-off-by: Daniel Landau <daniel@landau.fi>

If we render the whole thing at once, the browser hangs. This way we can render the beginning of the list and the user can start to either scroll or search for better results. Signed-off-by: Daniel Landau <daniel@landau.fi>

Signed-off-by: Daniel Landau <daniel@landau.fi>

This requires an updated SabreDav in core. The contents of address-data are ignored before https://github.com/fruux/sabre-dav/issues/889 Signed-off-by: Daniel Landau <daniel@landau.fi>

Signed-off-by: Daniel Landau <daniel@landau.fi>

This is to ensure that they really are loaded when changeContact is run Signed-off-by: Daniel Landau <daniel@landau.fi>

Signed-off-by: Daniel Landau <daniel@landau.fi>

daniellandau · 2017-03-16T20:18:15Z

I rebased the PR to current master, ping @skjnldsv.

codecov-io · 2017-03-17T00:02:09Z

Codecov Report

Merging #88 into master will decrease coverage by 0.53%.
The diff coverage is 6.45%.

@@            Coverage Diff             @@
##           master      #88      +/-   ##
==========================================
- Coverage   16.42%   15.89%   -0.54%     
==========================================
  Files          50       51       +1     
  Lines        1023     1095      +72     
==========================================
+ Hits          168      174       +6     
- Misses        855      921      +66

Impacted Files	Coverage Δ
js/components/group/group_directive.js	`50% <ø> (ø)`	✅
js/models/contact_model.js	`40.6% <ø> (ø)`	✅
...ts/newContactButton/newContactButton_controller.js	`8.33% <0%> (ø)`	✅
js/services/contact_service.js	`0.74% <0%> (-0.27%)`	❌
...onents/contactDetails/contactDetails_controller.js	`2% <0%> (ø)`	✅
js/components/groupList/groupList_controller.js	`6.66% <0%> (ø)`	✅
js/filters/counterFormatter_filter.js	`100% <100%> (ø)`
...s/components/contactList/contactList_controller.js	`1.76% <2.94%> (+0.55%)`	✅

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2692185...e628a73. Read the comment docs.

irgendwie · 2017-03-18T23:57:43Z

If I'm not mistaken, the REPORT functionality (https://github.com/fruux/sabre-dav/issues/889) is only included starting from sabre-dav 3.0.10 (https://github.com/fruux/sabre-dav/blob/master/CHANGELOG.md#3010-2016--) which isn't released (by now?) and only sabre-dav 3.2.1 (https://github.com/fruux/sabre-dav/blob/master/CHANGELOG.md#321-2017-01-28) seems to support the REPORT functionality .

sabre-dav in Nextcloud 9 was only bumped to sabre-dav 3.0.9 (https://github.com/nextcloud/3rdparty/tree/stable9/sabre), which means we wouldn't support Nextcloud 9 anymore by now.

And sabre-dav in Nextcloud 10 was also only bumped to sabre-dav 3.0.9 (https://github.com/nextcloud/3rdparty/tree/stable10/sabre), which also means no support for Nextcloud 10 anymore by now.

And sabre-dav in Nextcloud 11 was also only bumped to sabre-dav 3.2.0 (https://github.com/nextcloud/3rdparty/tree/stable11/sabre), which also means no support for Nextcloud 11 anymore by now.

👎 I'm not willing to merge this PR into the master branch, which would basically mean that the next version of contacts would only be compatible with Nextcloud 12.
We need a solution for this, before we merge this.

---> After some testing, if the REPORT feature is not supported, it still seems to work (requested properties are ignored?)

daniellandau · 2017-03-20T09:11:22Z

---> After some testing, if the REPORT feature is not supported, it still seems to work (requested properties are ignored?)

Yes, the properties are simply ignored. There's some extra network traffic when the browsers asks for the visible cards again (as they are already fully downloaded in the beginning).

jancborchardt · 2017-03-27T22:47:01Z

@LukasReschke @MorrisJobke care to check this out? :) Would be great to get your opinion on this.

MorrisJobke · 2017-03-27T23:07:22Z

@LukasReschke @MorrisJobke care to check this out? :) Would be great to get your opinion on this.

Sadly I don't have time for this :/

jancborchardt · 2017-03-28T00:05:05Z

Anyone else who can help here? @rullzer @schiessle @nickvergessen @georgehrke

skjnldsv · 2017-04-19T17:17:51Z

🎉

jancborchardt · 2017-04-23T04:05:54Z

Great work everyone! :)

timreeves · 2017-05-06T10:17:24Z

Does "renders the contact list piece by piece" translate to "infinite scrolling", 'cos that would be really great. I have 4 adress books shared with different people, totalling around 1300 contacts, and the load time is lousy, even on a fast linux PC with fast cable internet. It would be great, really great, if I could select which of the adress books should be displayed (BEFORE they are) - I would have thought that an elementary feature.

And a client of mine uploaded about 15.000 contacts to one adress book; since then it has been unusable and would have to be deleted manually from the DB (igitt).

So yes, infinite scrolling / lazy loading or whatever we call it would imho be neccessary for professional use. Or even/also the possibility to limit the display to first name / last name beginning with...

Many thanks to all working on this!

daniellandau mentioned this pull request Jan 7, 2017

Introduce lazy loading to shorten waiting time on open #12

Closed

eppfel added 3. to review Waiting for reviews enhancement New feature or request labels Jan 8, 2017

skjnldsv requested changes Jan 8, 2017

View reviewed changes

skjnldsv added this to the 1.5.3 milestone Jan 8, 2017

skjnldsv reviewed Jan 8, 2017

View reviewed changes

js/services/contact_service.js

} else {

// custom console

// eslint-disable-next-line no-console

Copy link

Member

skjnldsv Jan 8, 2017

Choose a reason for hiding this comment

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

Nice! 😆

skjnldsv removed this from the 1.5.3 milestone Jan 17, 2017

skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jan 23, 2017

daniellandau mentioned this pull request Jan 24, 2017

Number of contacts should be displayed in the navigation row of the group #17

Closed

daniellandau added 6 commits March 16, 2017 21:44

Add XML markup for limiting results

deeaa43

Signed-off-by: Daniel Landau <daniel@landau.fi>

Show contact count in left sidebar (fixes nextcloud#17)

7643e81

Signed-off-by: Daniel Landau <daniel@landau.fi>

Never automatically load first contact

5c5f3e8

It's a really heavy operation that hangs the browser Signed-off-by: Daniel Landau <daniel@landau.fi>

Limit the amount of contacts that are rendered at once

b01ee95

If we render the whole thing at once, the browser hangs. This way we can render the beginning of the list and the user can start to either scroll or search for better results. Signed-off-by: Daniel Landau <daniel@landau.fi>

Get rid of nlimit, that didn't go anywhere

0575638

Signed-off-by: Daniel Landau <daniel@landau.fi>

Get relevant fields for listing and searching

1df38ad

This requires an updated SabreDav in core. The contents of address-data are ignored before https://github.com/fruux/sabre-dav/issues/889 Signed-off-by: Daniel Landau <daniel@landau.fi>

daniellandau added 10 commits March 16, 2017 21:53

Get images as needed for visible rows

028df62

Signed-off-by: Daniel Landau <daniel@landau.fi>

Add support for multiple people with the same name

1c9a6b2

Signed-off-by: Daniel Landau <daniel@landau.fi>

Add eslint comment

096068a

Signed-off-by: Daniel Landau <daniel@landau.fi>

Move uid watching to addressBooks callback

dd871cf

This is to ensure that they really are loaded when changeContact is run Signed-off-by: Daniel Landau <daniel@landau.fi>

Update comment

953a3dc

Signed-off-by: Daniel Landau <daniel@landau.fi>

Format counts according to guide line

6cc225b

Signed-off-by: Daniel Landau <daniel@landau.fi>

Add test for counterFormatter

993e1e6

Signed-off-by: Daniel Landau <daniel@landau.fi>

Defend against missing contacts

203994b

Signed-off-by: Daniel Landau <daniel@landau.fi>

Change styling

6821004

Signed-off-by: Daniel Landau <daniel@landau.fi>

Do load the first contact on desktop

e628a73

Signed-off-by: Daniel Landau <daniel@landau.fi>

daniellandau force-pushed the introduce-lazy-loading branch from a5b210b to e628a73 Compare March 16, 2017 20:12

irgendwie mentioned this pull request Mar 23, 2017

problems with huge addressbooks #176

Closed

irgendwie merged commit daece9d into nextcloud:master Apr 19, 2017

skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Apr 19, 2017

irgendwie mentioned this pull request May 8, 2017

[stable 11] update sabre/dav 3.2.0 to 3.2.2 nextcloud/3rdparty#50

Closed

irgendwie mentioned this pull request Jun 6, 2017

FEATURE REQUEST: Caching for Contact Avatars #228

Closed

skjnldsv mentioned this pull request Sep 19, 2017

Very long time display of the contact list #222

Closed

		@@ -1 +1 @@
		<a ng-href="#/{{ctrl.group}}">{{ctrl.group}}</a>
		<a ng-href="#/{{ctrl.group}}">{{ctrl.group}} ({{ctrl.groupCount}})</a>

		@@ -0,0 +1,12 @@
		// from https://docs.nextcloud.com/server/11/developer_manual/app/css.html#menus
		angular.module('contactsApp')

		@@ -1 +1 @@
		<a ng-href="#/{{ctrl.group}}">{{ctrl.group}} ({{ctrl.groupCount}})</a>
		<a ng-href="#/{{ctrl.group}}">{{ ctrl.group }} ({{ ctrl.groupCount \| counterFormatter }})</a>

		@@ -1 +1 @@
		<li ng-repeat="group in ctrl.groups" group data="group" ng-click="ctrl.setSelected(group)" ng-class="{active: group === ctrl.getSelected}"></li>
		<li ng-repeat="group in ctrl.groups" group group-name="group[0]" group-count="group[1]" ng-click="ctrl.setSelected(group[0])" ng-class="{active: group[0] === ctrl.getSelected}"></li>

Introduce lazy loading #88

Introduce lazy loading #88

Conversation

daniellandau commented Jan 7, 2017

eppfel commented Jan 8, 2017 • edited Loading

skjnldsv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skjnldsv Jan 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skjnldsv Jan 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skjnldsv commented Jan 8, 2017 • edited Loading

daniellandau commented Jan 8, 2017

Choose a reason for hiding this comment

daniellandau commented Jan 10, 2017

skjnldsv commented Jan 10, 2017

jancborchardt commented Jan 24, 2017

daniellandau commented Jan 24, 2017

Henni commented Jan 24, 2017

daniellandau commented Jan 24, 2017

Henni commented Jan 24, 2017

daniellandau commented Mar 2, 2017

daniellandau commented Mar 16, 2017

codecov-io commented Mar 17, 2017 • edited Loading

Codecov Report

irgendwie commented Mar 18, 2017

daniellandau commented Mar 20, 2017

jancborchardt commented Mar 27, 2017

MorrisJobke commented Mar 27, 2017

jancborchardt commented Mar 28, 2017

skjnldsv commented Apr 19, 2017

jancborchardt commented Apr 23, 2017

timreeves commented May 6, 2017

eppfel commented Jan 8, 2017 •

edited

Loading

skjnldsv Jan 8, 2017 •

edited

Loading

skjnldsv Jan 8, 2017 •

edited

Loading

skjnldsv commented Jan 8, 2017 •

edited

Loading

codecov-io commented Mar 17, 2017 •

edited

Loading