kuuuurt / multiplatform-paging

Kotlin Multiplatform library for Pagination
Apache License 2.0
251 stars 22 forks source link

Trigger for calling loadNext() on iOS #6

Closed joreilly closed 3 years ago

joreilly commented 3 years ago

The sample that's in repo has following (for table view)

    if (indexPath.row == count - 1) {
      viewModel.pager.loadNext()
    }

In my SwiftUI iOS client right now I have

if data.shouldDisplayNextPage {
    nextPageView
}

....

private var nextPageView: some View {
    HStack {
        Spacer()
        VStack {
            ProgressView()
            Text("Loading next page...")
        }
        Spacer()
    }
    .onAppear(perform: {
        data.fetchNextData() 
    })
}                

data above is view model that calls in to shared KMP code. What I think I'm missing right now is how that shouldDisplayNextPage can be implemented. For example would it make sense to expose nextKey from Pager (or maybe there's already some way to infer this?) Could also perhaps be returned from call to loadNext()?

If you like I can create PR with this change?

(note above client code is in https://github.com/joreilly/MortyComposeKMM/tree/multiplatform-paging ...updated with 0.3.4 changes)

joreilly commented 3 years ago

Looking at code a bit more one other option is to expose currentPagingResult from iOS version of Pager....

kuuuurt commented 3 years ago

I feel like PagingResult is just an internal implementation and shouldn't be exposed outside of the Pager. If I'm getting your concern correctly, you just need to determine if there are more pages to be loaded, correct?

joreilly commented 3 years ago

Yes, that's correct. And I think what you said about PagingResult makes sense......perhaps then having that return value from loadNext might be another option...though also need to know this info from result of initial load as well.

kuuuurt commented 3 years ago

Hmm.. I've added a hasNextPage in v0.3.5 that can be observed on Pager on the iOS side. Internally, it just becomes false if the items from getItems is less than the supplied PagingConfig.pageSize. What do you think?

joreilly commented 3 years ago

Thanks, that sounds perfect....will give it a go later hopefully.

joreilly commented 3 years ago

I tried 0.3.5 and I'm getting other issues it seems with not always getting notified of new data on iOS. Looking at changes made I wonder if it's somehow related to following (from https://github.com/kuuuurt/multiplatform-paging/commit/83d3b558aad725939f34834ba1ce8d4f004cfee8)

-                _pagingData.offer(items)

+                _pagingData.value = items
kuuuurt commented 3 years ago

Ah might be because of the MutableStateFlow not emitting since it's the same object.

joreilly commented 3 years ago

The other thing I was wondering was whether maybe _hasNextPage could be set based on newPagingResult.nextKey. One other issue I was seeing was where number of items I was getting back didn't match pageSize that I had set (in case for example where you can't set page size in query).

kuuuurt commented 3 years ago

The pageSize should be used when you query your items and the API or your backing database should be querying based on that size. It's the assumption that if you're not able to get items of that size, then you should be at the last page. Why do you think that the nextKey can be an indicator of the last page? The nextKey's value would change depending on the current page that's why I don't think we can use it to determine if it's the last page already.

I also pushed v0.3.6 for the loading fix.

joreilly commented 3 years ago

I guess it depends on how nextKey is being determined. In this project it's part of response so it implies whether there's another page or not.....but I guess that won't always be the case.

joreilly commented 3 years ago

btw I'm seeing same issue with not being notified of new data in 0.3.6. Just curious what issue was with the call to offer() you had originally?

kuuuurt commented 3 years ago

Whoops, sorry for being trigger happy. I was juggling a few things with work last night. I've pushed out 0.3.7 and it should be working fine now. I just switched to a MutableStateFlow since AFAIK that's where ConflatedBroadcastChannel is headed.

As for your issue with the pageSize, if you cannot control the size through your code, I'm assuming that the response is returning a fixed amount of items when paginated. If that's the case, maybe you can tweak your pageSize to match that as a workaround while we look for a better API design to cater to this use case. Would that work?

joreilly commented 3 years ago

Just tried out 0.3.7 and looks good, thanks!

joreilly commented 3 years ago

btw I had only tested on iOS that time.....get following when I build on Android

> Could not resolve all files for configuration ':androidApp:debugRuntimeClasspath'.
   > Could not find com.kuuuurt:multiplatform-paging-androidDebug:0.3.7.
     Required by:
         project :androidApp > project :shared > com.kuuuurt:multiplatform-paging:0.3.7
kuuuurt commented 3 years ago

Whoops. Seems like the upload failed just for android. Could you try it again? I republished the same version

joreilly commented 3 years ago

Changes have been merged to main branch of https://github.com/joreilly/MortyComposeKMM