Feature/placed pages pagination

[danielebogo]

This PR allows the pages list to load all the pages till the end, without using the infinite scroll feature as before.

Page List has a new WPContentSyncHelper called PageContentSyncHelper in charge to recursively call syncMoreContent till it has no more content.

To test:

• Open a blog and check if all the pages are loaded

@aerych do you mind to have a look since we already spoke about this?

[aerych]

Heya @danielebogo
Thanks for tackling this. I noticed a few things in the review and testing that I think we should consider.

Apart from the comments in the code, when I started testing in the simulator I noticed the page list will stutter or momentarily freeze when a batch of pages are saved and the list is updated. This doesn't seem like a great experience for the user. I think the experience would be better to sync everything before refreshing the list.

In this patch the ViewController syncs in batches. This made sense when we had the Load More functionality but since we want to load all pages it would be better for the service to do this job. I'd consider updating the service so it would fetch all pages and when everything was synced then execute its success block. This would probably need to be a new method to handle the logic so it doesn't conflict with the way the post list syncs. This would let the list refresh once and keep the user experience a bit nicer. The first sync might end up showing the Loading message for a while but that should only happen once.

Thoughts?

[aerych]

Here we're subclassing in order to override one method where the method body is largely duplicated.

For a lighter touch, and to avoid duplication, it might make sense to instead override syncContentEnded in PageListViewController like so:

override func syncContentEnded(_ syncHelper: WPContentSyncHelper) {
        if syncHelper.hasMoreContent {
            syncHelper.syncMoreContent()
        } else {
            super.syncContentEnded(syncHelper)
        }
    }

What do you think?

[aerych]

I think the change I suggested above would avoid the need for this or for customizing the getter/setter below.

[danielebogo]

Thanks @aerych for the hint. Yes you are right, the freeze happened because of the purgeLocalSync that was applied for every batch.
So the fix now affect only the PostService. The syncPost method use a recursion to load and sync/save the remote posts at the end. In this way the sync delegate use one function only and doesn't call the load more anymore (that actually is a duplication too).
Let me know what you think 😉!

[aerych]

Heya @danielebogo! Loving this change. I took it for a spin with a few different blogs. Blogs with a few dozen pages are super fast. I stress tested with a blog that had ~2500 pages and while it took ages to load it worked as expected.

Just wanting to note on the blog with 2500 pages, there was a pause navigating from the site details screen to the page list, I think due to the hierarchy being processed. We'll have an opportunity to improve performance there but that behavior is not really related to this PR, and it should be a very rare issue so let's address it later.

I noted a nitpicky thing about our style guide but other than that we can ! Thank ya sir :)

[aerych]

This is super nitpicky. Our objc style guide calls for dot notation only for @proprties. Method calls in particular we like to retain the bracket syntax: [@[] mutableCopy].

[aerych]

And then also bracket notation here [loadedPosts copy]

[danielebogo]

Thanks @aerych ! I fixed the brackets syntax and I tested it with the ~2500 pages blog! Everything works fine!