Infinite scroll for People Management by oguzkocer · Pull Request #4120 · wordpress-mobile/WordPress-Android

oguzkocer · 2016-05-24T15:36:19Z

Relevant #3901. This PR adds infinite scroll to people list. As per our discussion on Slack, we remove the users list for a site when you visit the people page, except for the first 20 users so we don't show an empty list. The idea behind this implementation is to make sure the removed users don't show up in that list.

When a fresh list of users is received from remote, meaning the request offset parameter is 0, we remove ALL users for that site and fill it with the users from the response. From then on, when you scroll down to the end of list, a new page will be requested and we'll just save those users on top of the ones we already have. I've implemented the same progress bar we use in the reader posts page to show that we're loading more users. I just centered it because it was blocking the gravatar for the users (it's on the left for reader).

I moved PeopleAdapter into PeopleListFragment to handle end of list condition a bit easier. Initially I tried to add a listener to the adapter which was the fragment and that'd trigger the activity to make the request, but then I thought it was too long of a chain. Moving the adapter inside of the fragment made things simpler.

Question:

In deletePeopleForLocalBlogIdExceptForFirstPage method of PeopleTable I was unable to add the table's name people as a query parameter. The query to delete n users from the end of a table turned out to be a bit complicated. I'd appreciate any suggestions on improving that, as well as finding a way to add the PEOPLE_TABLE as a parameter.

Known issue:

This is unrelated to this PR but I've recently found out that public View getView(int position, View convertView, ViewGroup parent) is called 3 times for each row in PeopleAdapter. I was unable to find the source of the issue so far and will be handling it separately.

To test: Navigate to a blog where there is more than 20 users, go into People page and scroll to the end of list. You should see a loading progress bar and once that's gone, you should be able to scroll to see more users.

/cc @astralbodies, @hypest, @jleandroperez

Needs review: @nbradbury

…list

PeopleAdapter also has been moved inside PeopleListFragment to better handle the end of list listener.

…ng users

…eners

nbradbury · 2016-05-24T16:01:59Z

WordPress/src/main/java/org/wordpress/android/datasets/PeopleTable.java

+        if (size > fetchLimit) {
+            int deleteCount = size - fetchLimit;
+            String[] args = new String[]{Integer.toString(deleteCount), Integer.toString(localTableBlogId)};
+            getWritableDb().delete(PEOPLE_TABLE, "person_id " +


Rather than hard-code "people" here, how about "person_id IN (SELECT person_id FROM " + PEOPLE_TABLE + "WHERE..."

I've added a question to the PR description just for this 👊, because I was trying to use ? and pass the PEOPLE_TABLE as an argument. Working on that sql statement probably fried my brain a bit. 😢

nbradbury · 2016-05-24T16:07:33Z

This is unrelated to this PR but I've recently found out that public View getView(int position, View convertView, ViewGroup parent) is called 3 times for each row in PeopleAdapter. I was unable to find the source of the issue so far and will be handling it separately.

I remember running into this same problem in the past. Replacing the ListView with a RecyclerView should fix it (but you probably want to make that a separate PR, since it's not a simple change).

nbradbury · 2016-05-24T16:14:28Z

WordPress/src/main/java/org/wordpress/android/ui/people/PeopleListFragment.java

 import java.util.List;

 public class PeopleListFragment extends ListFragment implements OnItemClickListener {
    private static String ARG_LOCAL_TABLE_BLOG_ID = "local_table_blog_id";


Just noticed this should be final.

nbradbury · 2016-05-25T19:10:41Z

WordPress/src/main/java/org/wordpress/android/datasets/PeopleTable.java

+                    Integer.toString(localTableBlogId),
+                    Integer.toString(deleteCount)
+            };
+            getWritableDb().delete(PEOPLE_TABLE, "local_blog_id=? AND person_id " +


For future reference, you can use numbered arguments (like ?1) in queries so you don't need to pass the same argument twice. Example:

String[] args = new String[] { Integer.toString(localTableBlogId), Integer.toString(deleteCount) }; getWritableDb().delete(PEOPLE_TABLE, "local_blog_id=?1 AND person_id " + "IN (SELECT person_id FROM " + PEOPLE_TABLE + " WHERE local_blog_id=?1 " + "ORDER BY display_name DESC LIMIT ?2)", args);

TIL, thanks!

nbradbury · 2016-05-25T20:16:20Z

Nice work on the switch to a RecyclerView - ~~but unfortunately the change seems to have broken infinite scroll. I can no longer scroll to get more than the initial 20 users.~~

nbradbury · 2016-05-25T20:22:59Z

Ignore my previous comment about infinite scroll not working - that was apparently caused by some code changes I made while testing. Sorry for the false alarm!

…e sql

oguzkocer · 2016-05-26T11:42:39Z

@nbradbury I've addressed all your feedback. fdec9be is the best way I could find that ensures the progress bar is visible at correct times. The PR should be ready for another pass. Thanks!

nbradbury · 2016-05-26T12:52:45Z

WordPress/src/main/java/org/wordpress/android/ui/people/PeopleListFragment.java

            return mPeopleList.size();
        }

+        @Override


In addition to this, the adapter's constructor should have setHasStableIds(true);

I swear I had that! It didn't even work at first when I added it because I removed the getItemId while I was changing to RecyclerView. I guess I forgot to put it back in, so sorry, fixing now!

oguzkocer · 2016-05-26T14:33:32Z

@nbradbury I've improved the offline mode in 821625e. We don't remove the cached users if there is no connectivity anymore (as you suggested) and we don't try to make a call to the server for any action. I've checked our codebase and generally we silently fail if there is no connection, but I think it'd be better if we tell the user about it. For now, I've used the same errors we use if the request actually fails, however I feel like it'd be better if we had a general error string to use in these situations, maybe something like "No network connection". What do you think?

nbradbury · 2016-05-26T17:53:46Z

WordPress/src/main/java/org/wordpress/android/ui/people/PeopleManagementActivity.java

    @Override
    public void onRoleChanged(long personID, int localTableBlogId, String newRole) {
+        if(!NetworkUtils.isNetworkAvailable(this)) {
+            ToastUtils.showToast(this, R.string.error_update_role, ToastUtils.Duration.LONG);


Since the error is due to no network, it'd be nicer to tell the user that's what the problem is. How about R.string.no_network_message instead?

…inite-scroll

jleandroperez · 2016-06-03T17:59:53Z

WordPress/src/main/java/org/wordpress/android/ui/people/utils/PeopleUtils.java

        };

+        Map<String, String> params = new HashMap<>();
+        params.put("number", Integer.toString(PeopleUtils.FETCH_USERS_LIMIT));


Guys, heads up. Not sure if we're hitting the exact same endpoint, or there's a difference. But on my end, the number parameter appears to be disregarded.

Instead, i'm using page. (Checked with dotcom source!).

Okay. Update. Apparently Users deal with offset, while Followers deal with page

Thanks for the heads up @jleandroperez. I am currently working on the follower section and I'll make sure we use the correct parameters for this. We currently use the default values, so probably that's why I didn't notice the issue before and just used what the documentation said here: https://developer.wordpress.com/docs/api/1.1/get/sites/%24site/users/

oguzkocer added 11 commits May 24, 2016 13:32

Rename refreshUsersList as fetchUsersList in PeopleManagementActivity

7ae0be3

Adds offset parameter to fetchList and returns if it's at the end of …

117d7fe

…list

Use offset parameter and only remove users from blog if it's a fresh …

9b40515

…list

Fetch users ordered by their display name in ascending order

49e3217

Fetch more users when the user scrolls to the end

c250767

PeopleAdapter also has been moved inside PeopleListFragment to better handle the end of list listener.

Adds count method for people table and uses correct offset for fetchi…

ea274d7

…ng users

Check if end of list is reached before fetching people list

8c6accc

If the people management activity is recreated, make sure to set list…

5aa934c

…eners

Save end of list reached flag in savedInstanceState

ff286e7

When People page is created, remove all users except for 1st page

314989f

Adds a progressbar showing more people are loading

36046b5

oguzkocer added the People Management label May 24, 2016

oguzkocer added this to the 5.4 milestone May 24, 2016

nbradbury self-assigned this May 24, 2016

nbradbury reviewed May 24, 2016
View reviewed changes

maxme removed this from the 5.4 milestone May 25, 2016

oguzkocer added 6 commits May 25, 2016 13:45

Improve the sql query in deletePeopleForLocalBlogIdExceptForFirstPage

55b23e3

Don't make a fetch request when one is already going on

127c1ca

PeopleListFragment refactored to use RecyclerView

2eb3b02

Adds PeopleItemDecoration as people list divider

70774b9

On click implemented for RecyclerView in people list

85c8ef4

Show progress bar if the list is empty

9dcd77c

nbradbury reviewed May 25, 2016
View reviewed changes

Use numbered arguments in deletePeopleForLocalBlogIdExceptForFirstPag…

113f43a

…e sql

oguzkocer force-pushed the feature/people-infinite-scroll branch from b5ba30f to 113f43a Compare May 26, 2016 09:57

oguzkocer added 3 commits May 26, 2016 13:00

Optimize PeopleAdapter by using stable ids

83851c7

Better onClick handle for PeopleViewHolder

80af60e

Trigger the people list fetch from fragment instead of activity

fdec9be

nbradbury reviewed May 26, 2016
View reviewed changes

oguzkocer added 2 commits May 26, 2016 16:05

setHasStableIds to true in PeopleAdapter

7654d95

Better offline support for people management

821625e

nbradbury reviewed May 26, 2016
View reviewed changes

oguzkocer added 2 commits May 26, 2016 21:05

Merge branch 'feature/people-management-sync' into feature/people-inf…

5bdc913

…inite-scroll

Use checkConnection of NetworkUtils instead of isNetworkAvailable

92218f6

nbradbury merged commit bbf7170 into feature/people-management-sync May 26, 2016

nbradbury deleted the feature/people-infinite-scroll branch May 26, 2016 18:36

jleandroperez reviewed Jun 3, 2016
View reviewed changes

Conversation

oguzkocer commented May 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nbradbury commented May 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nbradbury commented May 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nbradbury commented May 25, 2016

Uh oh!

oguzkocer commented May 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oguzkocer commented May 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nbradbury commented May 24, 2016 •

edited

Loading

nbradbury commented May 25, 2016 •

edited

Loading