ListStore integration for Post list #8473

oguzkocer · 2018-10-24T04:22:12Z

This PR integrates the new ListStore into the post list. I wanted to separate this into multiple PRs, but because the changes are all connected, my attempts at separating it didn't give satisfactory results. Either the changes separated are too small, doesn't give enough context or creates too much merge conflicts for little gain. Instead of swimming against the tide, I wanted to open this PR as is and only separate it if the reviewer asks me to do so. In exchange, I'll add a lot of line comments in the PR itself to make it easier to review this.

I am going to make a certain bot angry, but oh well 🤷‍. Onto the changes..

We have been unable to add filters or searching to post list for some time due to pagination issues it would have caused. I have been working in the FluxC land for some time implementing a set of components which not only avoids the pagination issues, but also provides an easy way to add filters, search, order by, order or pretty much anything you can think of. This PR is the first one that utilizes these new components and it's considered to be the v1 integration of it.

Testing Instructions

Test a self-hosted site as well as a .com one. You don't have to try all of this on both, but at least verify that basics work on both type of sites.
Don't forget to rotate the screen at times. There are some rotation issues from before, but this PR shouldn't add any new ones.
Use the emulator's "Cellular" settings to try a slower connection. (or take a subway ride 🤷‍) You don't have to test everything on a slow connection, but after you test everything on a fast connection, you can try the basics for a different site or after logging out and logging in on a slower one. If you slow the connection after everything is loaded once, it'll still load pretty fast, so won't be as useful of a test.
If you find any issues, if you can try out the production version (or develop) to see if you can replicate the issue (where applicable), it'd be very helpful. If you can replicate it, that means it's most likely a non-related issue, so open a new issue and ping me in it so I can have a look to see if I can fix it quickly in this branch. If you can't, please document it in this PR.

Testing Steps

Since this touches everything in post list anything that affects it should be tested. Here is a non-comprehensive list: (only because I can't think of all of it)

Do a fresh installation of the app (or logout/login) and visit the "Blog Posts" and check that it loads the posts.

A note on fresh installations: This will be only time the posts are loaded from scratch. After this, only the ids and modified dates of the post will be fetched, which means your subsequent visits or pull to refresh will be a lot faster and more bandwidth efficient (+90% difference on both of them in my tests)

Tap on a post and change the title or description and go back. The post should be updated and should say "Local changes".
Tap on a post, go to "Post settings" and add a featured image, tap on update button and check that it's reflected in the post list after a brief wait. (network request)

It looks like this doesn't work if the change only happens locally. Check #8472 for more details.

Tap on the floating button and create a draft by filling in title and/or content and going back. Check that it shows up in the list.
Disable your network, tap on the floating button and create a local draft by filling in the title and/or content. Check that it shows up in the list.
Re-enable your network and tap on publish on the remote draft you created 2 steps ago. Check that it's showing that it's uploading and it's refreshed after the upload succeeds.
Repeat the above step for the local draft you created.
If you have a site with more than 100 posts, use that and scroll as far as you can go. (or your as much as your patience allows) Check that more posts are loaded as you go and load more progress bar is visible at times. You can also overwrite the pageSize in PostListDescriptor in PostListFragment if you don't have a site with as many posts.
Change a post from another client. (Calypso, iOS etc) Pull to refresh the list and check that the post is updated in the post list.
Trash a post in the list and check that it's removed and a snackbar is shown to undo. Check that it's deleted in another client.
Trash a post in the list and tap on the undo bar to check that it's re-added to the list and it's not deleted from remote. (verify on remote after waiting a minute)
For bonus points, change the PostListDescriptorForRestSite or PostListDescriptorForXmlRpcSite by passing more parameters to it and try out different filters.

@malinajirka since you're leaving for a conference, I asked @kwonye for the review. If you do end up having time, you're welcome to review and/or test the PR, the more the merrier.

…ed to remote

…dapter

…to issue/list-store-integration-for-post-list

This commit adds a lot of comments to [ListStore] specific parts of PostListFragment and fixes a few small issues in it.

…list

wpmobilebot · 2018-10-24T04:26:21Z

	1 Warning
⚠️	PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 Danger

oguzkocer · 2018-10-24T05:09:27Z

WordPress/src/main/java/org/wordpress/android/ui/posts/adapters/PostListAdapter.kt

-        val imageUrl = featuredImageUrls.get(postId)
+    private fun showFeaturedImage(post: PostModel, imgFeatured: ImageView) {
+        var imageUrl: String? = null
+        if (post.featuredImageId != 0L) {


These actions should be taken in the bg thread, an issue is created for this #8474.

oguzkocer · 2018-10-24T05:12:03Z

WordPress/src/main/java/org/wordpress/android/ui/posts/adapters/PostListAdapter.kt

-     * called after the media (featured image) for a post has been downloaded - locate the post
-     * and set its featured image url to the passed url
-     */
-    fun mediaChanged(mediaModel: MediaModel) {


This is moved to PostListFragment.onMediaChanged.

oguzkocer · 2018-10-24T05:12:33Z

WordPress/src/main/java/org/wordpress/android/ui/posts/adapters/PostListAdapter.kt

-    }
-
-    @SuppressLint("StaticFieldLeak")
-    private inner class LoadPostsTask internal constructor(private val mLoadMode: LoadMode)


We no longer need this task as ListStore takes care of it for us.

oguzkocer · 2018-10-24T05:14:27Z

WordPress/src/main/res/layout/post_cardview_skeleton.xml

@@ -0,0 +1,72 @@
+<?xml version="1.0" encoding="utf-8"?>
+
+<android.support.v7.widget.CardView


We need to improve this skeleton layout and have a separate issue for it ##8475.

oguzkocer · 2018-10-24T05:15:45Z

WordPress/src/main/java/org/wordpress/android/ui/uploads/PostEvents.java

 public class PostEvents {
    public static class PostUploadStarted {
-        public final int mLocalBlogId;
+        public final PostModel post;


We need to access more than the siteId to be able to refresh the relevant row in post list, that's why this change was implemented for both PostUploadStarted and PostUploadCanceled.

I feel safer that we're accessing the id from the PostModel anyways, so 👍

oguzkocer · 2018-10-24T05:16:22Z

WordPress/src/main/java/org/wordpress/android/ui/posts/PostUtils.java

        }
    }

-    public static boolean postListsAreEqual(List<PostModel> lhs, List<PostModel> rhs) {


We no longer need these helpers.

oguzkocer · 2018-10-24T05:17:04Z

WordPress/src/main/java/org/wordpress/android/ui/posts/PostListFragment.kt


+    private var listManager: ListManager<PostModel>? = null
+    private var refreshListDataJob: Job? = null
+    private val listDescriptor: PostListDescriptor by lazy {


If you want to try different filters as mentioned in the PR, this is the value you need to modify.

oguzkocer · 2018-10-24T05:19:24Z

WordPress/src/main/java/org/wordpress/android/ui/posts/PostListFragment.kt

        super.onDestroy()
    }

+    override fun onResume() {


The only change in this method is the removal of loadPosts(LoadMode.IF_CHANGED) as it's no longer necessary with ListStore. It's moved for better organization, that's why it's showing as many changes. Here is the version in the base branch for easy comparison: https://github.com/wordpress-mobile/WordPress-Android/blob/feature/list-store-integration-for-post-list-master/WordPress/src/main/java/org/wordpress/android/ui/posts/PostListFragment.kt#L234-L249

oguzkocer · 2018-10-24T05:20:11Z

WordPress/src/main/java/org/wordpress/android/ui/posts/PostListFragment.kt

+        }
+    }
+
+    override fun onDetach() {


This method is unchanged, it's moved for better organization. Here is the version from base branch for easy comparison: https://github.com/wordpress-mobile/WordPress-Android/blob/feature/list-store-integration-for-post-list-master/WordPress/src/main/java/org/wordpress/android/ui/posts/PostListFragment.kt#L329-L337

oguzkocer · 2018-10-24T05:20:46Z

WordPress/src/main/java/org/wordpress/android/ui/posts/PostListFragment.kt

        fabView?.setOnClickListener { newPost() }

-        if (savedInstanceState == null) {
-            if (UploadService.hasPendingOrInProgressPostUploads()) {


These are not necessary anymore, ListStore will take care of it for us.

Good call removing this responsibility from the UI.

oguzkocer · 2018-10-24T05:21:36Z

WordPress/src/main/java/org/wordpress/android/ui/posts/PostListFragment.kt

        }
-
-        initSwipeToRefreshHelper()
+        refreshListManagerFromStore(listDescriptor, shouldRefreshFirstPageAfterLoading = (savedInstanceState == null))


We want to refresh the list if it's the first time the user is visiting the list. (not a config change) However, we want to do it only after the cached results are loaded.

oguzkocer · 2018-10-24T05:22:53Z

WordPress/src/main/java/org/wordpress/android/ui/posts/PostListFragment.kt

-                    if (!isAdded) {
-                        return@RefreshListener
-                    }
-                    if (!NetworkUtils.checkConnection(nonNullActivity)) {


This logic is moved to refreshPostList as it's the only place the list should be refreshed from.

oguzkocer · 2018-10-24T05:24:15Z

WordPress/src/main/java/org/wordpress/android/ui/posts/PostListFragment.kt

-        ActivityLauncher.addNewPostForResult(nonNullActivity, site, false)
-    }
-
-    override fun onResume() {


As mentioned here this is not actually removed, but just moved.

oguzkocer · 2018-10-24T05:24:50Z

WordPress/src/main/java/org/wordpress/android/ui/posts/PostListFragment.kt

        }
    }

-    override fun onDetach() {


As mentioned here this is not actually removed, but moved.

oguzkocer · 2018-10-24T05:25:40Z

WordPress/src/main/java/org/wordpress/android/ui/posts/PostListFragment.kt

-        dispatcher.dispatch(PostActionBuilder.newFetchPostsAction(payload))
-    }
-
-    private fun showLoadMoreProgress() {


These updates are done in updateListManager instead.

oguzkocer · 2018-10-24T05:27:49Z

WordPress/src/main/java/org/wordpress/android/ui/posts/PostListFragment.kt

-    override fun onLoadMore() {
-        if (canLoadMorePosts && !isFetchingPosts) {
-            requestPosts(true)
+    private fun trashPost(post: PostModel) {


Unfortunately this seems to have moved. The actual changes are minor. Here is the version from the base branch for easier comparison: https://github.com/wordpress-mobile/WordPress-Android/blob/feature/list-store-integration-for-post-list-master/WordPress/src/main/java/org/wordpress/android/ui/posts/PostListFragment.kt#L516-L584

Sorry about this move 😢

oguzkocer · 2018-10-24T05:28:35Z

WordPress/src/main/java/org/wordpress/android/ui/posts/PostListFragment.kt

+        snackbar.show()
+    }
+
+    private fun showPublishConfirmationDialog(post: PostModel) {


The only change is the isAdded check, here is the version from base branch for easy comparison: https://github.com/wordpress-mobile/WordPress-Android/blob/feature/list-store-integration-for-post-list-master/WordPress/src/main/java/org/wordpress/android/ui/posts/PostListFragment.kt#L494-L511

oguzkocer · 2018-10-24T05:30:14Z

WordPress/src/main/java/org/wordpress/android/ui/posts/PostListFragment.kt

-                loadPosts(LoadMode.IF_CHANGED)
+    @Subscribe(threadMode = ThreadMode.BACKGROUND)
+    @Suppress("unused")
+    fun onListChanged(event: OnListChanged) {


This action is triggered by FluxC whenever a list is updated. This could be just a state change or a result of a fetch. We need to check if the list is the one we are interested in and update the views accordingly.

oguzkocer · 2018-10-24T05:32:32Z

WordPress/src/main/java/org/wordpress/android/ui/posts/PostListFragment.kt

-            }
+    @Subscribe(threadMode = ThreadMode.BACKGROUND)
+    @Suppress("unused")
+    fun onListItemsChanged(event: OnListItemsChanged) {


Whenever there is a change for a specific "type", FluxC will emit this event. In this case, the type will represent the posts belonging to a specific site. We need to check if the type that's updated is the one we are interested in and update the views accordingly.

The only type we have in FluxC is for posts right now. However, once we have more types, this will be an important check. Also, if you switch the site, the "type" will actually change, so if a post for another site is updated, the list will not be refreshed, as expected.

oguzkocer · 2018-10-24T05:35:32Z

@kwonye @malinajirka I've added the line comments as mentioned in the PR to make it easier to review. Please let me know if anything is not clear.

Also, please note that I haven't added screenshots or gifs to this PR. I think it'd be better if we keep this PR code focused. Since we are merging it to a master branch, we can use the PR against develop more UI focused with design reviews and such. (design changes are pretty minor anyway)

malinajirka

Thanks @oguzkocer, great job as always ;)! It's no fun doing reviews when there isn't much I can suggest improving:D.

I didn't do a full review as I had just limited time + I haven't run the app at all.

I've left few super minor comments. Only thing I think we may want to consider is moving some logic from the PostListFragment to separate classes. The fragment has almost 1k lines and it's quite difficult to follow. Since we are making a lot of changes anyway I think it would make sense to try to lighten it a bit. It'd be great if we could move for example calculateDiff(), trash post related logic, media upload logic etc. I'm not sure whether it's doable and how much work it would require. But I just wanted to bring it up so we can all think about it.

malinajirka · 2018-10-24T08:47:17Z

WordPress/src/main/java/org/wordpress/android/ui/posts/PostListFragment.kt

+            if (listManager.size == 0) {
+                if (NetworkUtils.isNetworkAvailable(nonNullActivity)) {
+                    updateEmptyView(EmptyViewMessageType.NO_CONTENT)
+                }


What happens when the network is not available?

This is actually a small workaround I added, because the empty view would briefly show the no content view when we are actually fetching the first page. Having said that, I am not even 100% sure about that claim, as I might be getting confused in a sea of issues I dealt with. This certainly requires at least a comment or an improvement before this PR is merged in.

Great catch, thanks @malinajirka!

@malinajirka I've improved the empty view handling in ee5e054. It should be much clearer now.

It's still not perfect though. I think ideally we'd want the ListManager to carry that information and the empty view is updated only through ListManager changes. I do have some plans for that, but not right now.

malinajirka · 2018-10-24T09:10:08Z

WordPress/src/main/java/org/wordpress/android/ui/posts/adapters/PostListAdapter.kt

+                val view = layoutInflater.inflate(R.layout.post_cardview_skeleton, parent, false)
+                LoadingViewHolder(view)
+            }
+            else -> {


In order to follow Fail-Fast principle we may consider explicitly specifying VIEW_TYPE_POST and throw an exception in the else branch.

Agreed, fixed in 8719cdf.

The changeset looks bigger than it actually is in that commit due to indentation change. 😢

That commit also updates the FluxC hash. It includes some important changes made in FluxC's develop branch. If you already installed the app using this branch, you'll need to do a clean installation as the migration will fail. (had to change it due to a merge conflict) /cc @kwonye

malinajirka · 2018-10-24T09:13:33Z

WordPress/src/main/java/org/wordpress/android/ui/posts/adapters/PostListAdapter.kt

+            val context = holder.itemView.context

-            var cleanPostExcerpt = PostUtils.getPostListExcerptFromPost(post)
+            if (holder is PostViewHolder) {


AFAIU the holder should always be an instance of PostViewHolder. We may consider throwing an exception in the else branch or remove the if check so it crashes asap.

Agreed, fixed in 8719cdf.

oguzkocer · 2018-10-25T02:49:30Z

Thanks for the review @malinajirka! I have addressed the issues you have raised.

Only thing I think we may want to consider is moving some logic from the PostListFragment to separate classes. The fragment has almost 1k lines and it's quite difficult to follow. Since we are making a lot of changes anyway I think it would make sense to try to lighten it a bit.

I fully agree with your sentiment. Not only the fragment is too large, everything is intertwined at the moment. We need to move the logic to a view model, which will make this class much smaller. I gave this a shot yesterday night, but it's a significant undertaking and I don't think it'll happen, at least not before the merge.

kwonye

I spent yesterday looking at the code and spent an hour today testing and everything looks great!

I do agree with @malinajirka that PostListFragment is incredibly heavy and is something we should look at for modularizing, but it's not blocking release and not a high priority.

Great job!

kwonye · 2018-10-25T18:38:58Z

WordPress/src/main/java/org/wordpress/android/ui/posts/PostListFragment.kt

        fabView?.setOnClickListener { newPost() }

-        if (savedInstanceState == null) {
-            if (UploadService.hasPendingOrInProgressPostUploads()) {


Good call removing this responsibility from the UI.

kwonye · 2018-10-25T18:58:46Z

WordPress/src/main/java/org/wordpress/android/ui/uploads/PostEvents.java

 public class PostEvents {
    public static class PostUploadStarted {
-        public final int mLocalBlogId;
+        public final PostModel post;


I feel safer that we're accessing the id from the PostModel anyways, so 👍

oguzkocer added 22 commits October 16, 2018 20:41

A very rough first pass at integrating ListManager to post list

352cc3a

Update FluxC hash, add lots of todos to post list and minor fixes

e0bcee4

Featured images are properly handled in post list

766fe4d

Fix local changes not reflecting in post list

45f8c74

Correctly handle upload events in post list

d94b356

Add loading layout for post list

e4ac3aa

Refresh the post list after an upload

c1fb27f

Adds a workaround to local drafts briefly disappearing when it's push…

14c8eaf

…ed to remote

Skeleton view for loading posts in post list

3edc2dc

Remove unused property and enum and replace if with when in PostListA…

b014196

…dapter

Empty view in post list should be correctly updated now

36fc606

Fix an issue with comparing local items in post list

48f6e89

Basic hide post support added for post list

ae83827

Update FluxC hash and implement necessary changes in post list

1fe0f13

Update FluxC hash and resolve several todos in PostListFragment

29a71b7

Save trashed and uploaded post ids in the instance state

60e5209

Merge branch 'feature/list-store-integration-for-post-list-master' in…

be31015

…to issue/list-store-integration-for-post-list

Merge branch 'feature/list-store-integration-for-post-list-master' in…

5714744

…to issue/list-store-integration-for-post-list

Add a style for post list skeleton buttons

9adc90d

Major cleanup/reorder of PostListFragment and PostListAdapter

d399213

Second take at improving PostListFragment

2d1814f

This commit adds a lot of comments to [ListStore] specific parts of PostListFragment and fixes a few small issues in it.

Fix an issue for when a uploaded local draft doesn't show up in post …

f333500

…list

oguzkocer added the Post List label Oct 24, 2018

oguzkocer added this to the 11.2 milestone Oct 24, 2018

oguzkocer requested a review from kwonye October 24, 2018 04:22

oguzkocer mentioned this pull request Oct 24, 2018

Fetch the featured images in background thread in new Post List #8474

Closed

oguzkocer commented Oct 24, 2018

View reviewed changes

oguzkocer mentioned this pull request Oct 24, 2018

Improve loading skeleton for post list #8475

Closed

oguzkocer commented Oct 24, 2018

View reviewed changes

malinajirka reviewed Oct 24, 2018

View reviewed changes

oguzkocer added 2 commits October 24, 2018 19:17

Throw illegal exception if a view type is not handled in PostListAdapter

8719cdf

Improve empty view handling for list manager changes

ee5e054

kwonye approved these changes Oct 25, 2018

View reviewed changes

kwonye merged commit 6329695 into feature/list-store-integration-for-post-list-master Oct 25, 2018

kwonye deleted the issue/list-store-integration-for-post-list branch October 25, 2018 19:11

This was referenced Oct 26, 2018

Adds ListStore and PostStore integration for it (Master PR) wordpress-mobile/WordPress-FluxC-Android#950

Merged

ListStore integration for Post list (Master PR) #8484

Merged

oguzkocer mentioned this pull request Nov 12, 2018

Introduce PostListViewModel and use PagedList from FluxC #8587

Merged

malinajirka mentioned this pull request Apr 17, 2019

Merge feature/master post filters into develop #9656

Merged

		@@ -0,0 +1,72 @@
		<?xml version="1.0" encoding="utf-8"?>

		<android.support.v7.widget.CardView

ListStore integration for Post list #8473

ListStore integration for Post list #8473

Conversation

oguzkocer commented Oct 24, 2018

Testing Instructions

Testing Steps

Uh oh!

wpmobilebot commented Oct 24, 2018

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!

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

oguzkocer commented Oct 24, 2018

Uh oh!

malinajirka left a comment

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!

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!

oguzkocer commented Oct 25, 2018

Uh oh!

kwonye left a comment

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