-
Notifications
You must be signed in to change notification settings - Fork 551
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add Pagination #1681
feat: Add Pagination #1681
Conversation
app/src/main/java/org/fossasia/openevent/general/event/EventsDataSourceFactory.kt
Outdated
Show resolved
Hide resolved
@iamareebjamal @nikit19 please do an initial review after that I'll be moving forward to integrate for similar and search event API calls |
|
||
compositeDisposable += mutableEventsPaged | ||
.subscribeOn(Schedulers.io()) | ||
.observeOn(AndroidSchedulers.mainThread()) | ||
.doOnSubscribe { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doFinally Block is not working
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cannot get it, why it is not working!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iamareebjamal can you tell why doFinally Block is not working here because of that shimmer layout do not stop everything else just works fine. is it because pagination requires continuous API calls to different pages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iamareebjamal please a little bit of help here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will try locally today
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please do. I'm not getting any time these days
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iamareebjamal i tried with calling doFinally in the data source class and it was working there,i might be right that it RxPagedList don't call doFinally as it subscribes again and again to fetch all the pages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i was searching for some solution the only way i found is to send a mutableprogress object to data source and update that on doFinally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pushed it but I don't think so it is the best possible method to implement it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, doFinally will never be called in case of Observable or Flowable because the stream is never completed
@@ -57,7 +59,7 @@ class EventsListAdapter( | |||
* The function to call when the adapter has to be cleared of items | |||
*/ | |||
fun clear() { | |||
this.submitList(emptyList()) | |||
// this.submitList() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how to pass empty paged list I couldn't get it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this emptyList()
method returning earlier?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
empty list returns a list whereas the adapter required a pagedlist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thinking to use
this.currentList?.clear()
would that work @liveHarshit
app/build.gradle
Outdated
@@ -36,11 +36,11 @@ android { | |||
shrinkResources true | |||
minifyEnabled true | |||
proguardFiles getDefaultProguardFile('proguard-android.txt'), 'proguard-rules.pro' | |||
buildConfigField "String", "DEFAULT_BASE_URL", '"https://api.eventyay.com/v1/"' | |||
buildConfigField "String", "DEFAULT_BASE_URL", '"https://open-event-api-dev.herokuapp.com/v1/"' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will be reverting this
@nikit19 any update on this |
@iamareebjamal any update regarding this ? |
Take peer reviews |
@nikit19 @liveHarshit please review |
@@ -57,7 +59,7 @@ class EventsListAdapter( | |||
* The function to call when the adapter has to be cleared of items | |||
*/ | |||
fun clear() { | |||
this.submitList(emptyList()) | |||
// this.submitList() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this emptyList()
method returning earlier?
|
||
compositeDisposable += mutableEventsPaged | ||
.subscribeOn(Schedulers.io()) | ||
.observeOn(AndroidSchedulers.mainThread()) | ||
.doOnSubscribe { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cannot get it, why it is not working!
if (similarEventsListAdapter.currentList.size != similarEvents.size) similarEvents.shuffle() | ||
similarEventsListAdapter.submitList(similarEvents) | ||
// if (similarEventsListAdapter.currentList.size != similarEvents.size) similarEvents.shuffle() | ||
// similarEventsListAdapter.submitList(similarEvents) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will implement pagination here as well it uses same adapters event view holder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iamareebjamal should i implement pagination here as well because this method won't no longer work as the adapter expects as pagedlist and we are passing a normal list here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And there would be a heavy network usage if I implement pagination here as well I suggest we should create a seperate adapter for this and use this as a normal list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iamareebjamal updated
eventService.getEventsByLocationPaged(query, requestedPage) | ||
.withDefaultSchedulers() | ||
.doFinally { | ||
mutableProgress.value = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would this approach be okay? i couldn't find any other method to get this working @iamareebjamal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is getEventsByLocationPaged
returning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is returning a Observable Paged List
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You cannot use doFinally here, move this call inside subscribe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay but inside the individual call of page finally is working
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should i add a check using using pageNo if equal pageNo == 1 then update the value because we want to hide the shimmer only once
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's reasonable
@@ -60,6 +74,12 @@ class EventsViewModel( | |||
} else { | |||
mutableProgress.value = false | |||
} | |||
|
|||
mutableProgress.observeForever { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iamareebjamal I can't observer in the ViewModel i guess. can i ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, use MediatorLiveData
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iamareebjamal updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iamareebjamal should I implement pagination for search events as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Build is failing. Yeah, implement that in other PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some unrelated changes will undo them and fix spotless. Rest all good?
d3d64b3
to
9c2fc88
Compare
@anhanh11001 @liveHarshit Please test |
@iamareebjamal @aggarwalpulkit596 App is crashing while navigating to another fragment and press back button while loading -
|
@liveHarshit will check |
not able to reproduce it @liveHarshit |
While loading goes to notification fragment and press back button. It's crashing in my device. Let me check again. |
|
@liveHarshit @iamareebjamal please review there is only issue now this always shows up because previously observer was only called upon a valid api request not with paged list it is called as soon api call is made so we need to check additional param whether showShimmer is false or not, just an observable on event list can't work |
Please fix Travis also. |
Probably a spotless error will fix it but please first check for any crashes 😅 |
@aggarwalpulkit596 It is not showing upcoming events. |
also |
Oh the logic must have been changed i forgot will change the end point |
Forgot that will fix it rest all good ? |
I don't see Infinite Scrolling in my device. |
Did you try with changing the base url ther aren't many events in the staging server try with prod server |
Yes I have tried with Singapore and Delhi location with the release API. For what location you are testing? |
Wait let me share a gif with you |
@@ -65,28 +88,14 @@ class EventsViewModel( | |||
fun isConnected(): Boolean = mutableConnectionLiveData.value ?: false | |||
|
|||
fun clearEvents() { | |||
mutableEvents.value = null | |||
|
|||
// mutableEventsPaged |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iamareebjamal couldn't find anything to clear this out please help out here
@@ -99,6 +99,18 @@ class EventService( | |||
} | |||
} | |||
|
|||
fun getEventsByLocationPaged(locationName: String?, page: Int): Single<List<Event>> { | |||
val query = "[{\"name\":\"location-name\",\"op\":\"ilike\",\"val\":\"%$locationName%\"}," + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@liveHarshit updated query for upcoming events
@liveHarshit rest all good ? |
What is the minimum number of events required for infinite scrolling? |
Because after filtering events, I can list max 5 number of events and infinite scrolling is not working. |
It is 6 but can be changed in config object and it is showing because of
|
PagedList | ||
.Config | ||
.Builder() | ||
.setPageSize(10) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@liveHarshit it is 10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. Then there are three bugs to solve that I've already told you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already @iamareebjamal for a little bit of assistance as i am unable to find anything for @liveHarshit you can also suggest something if you find anything 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Firstly you need to modify the logic for showing no events card and shimmer effect. Both are in conflicts right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also how the swipe refresh is connected with both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
triggered with a zero size list
Oh, got it. So here we cannot compare the size of the list, but
hat whether the intial request is completer or not
In that case, can we check if there are no events?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can only be checked after the first request is completed that is progress live data is set to false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then show shimmer until the request will complete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also for swipe to refresh we need to clear the mutableEventsPaged but that is observable before it was a live data so its value can be set to null and I can't find anything to set it source factory to null or zero-sized list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then show shimmer until the request will complete.
but that's the point I need to check 2 separate live data values and then work accordingly will find some workaround for that definitely but couldn't find anything for observable disposable.clear() would work I guess then?
@liveHarshit updated,everything is working fine now |
Please test it by removing the filter and change the debug API to release. |
upcoming event filter? Cool |
Any updates on this? If not then I'm taking this up as it is an issue listed in GSoC Milestones. @aggarwalpulkit596 |
Would wind up by end of the week |
@aggarwalpulkit596 Thanks |
Fixes #84
Changes: Add Pagination for Searching Location Events
Screenshots for the change:
![videotogif_2019 05 02_19 17 29](https://user-images.githubusercontent.com/29139786/57079967-25586d80-6d0f-11e9-96b9-3cd8f5f71f53.gif)
Infinite Scrolling