-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
With lazy loading of contributions #3566
With lazy loading of contributions #3566
Conversation
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.
Could you please add tests to make sure this change works as expected?
Codecov Report
@@ Coverage Diff @@
## master #3566 +/- ##
========================================
Coverage 7.82% 7.82%
Complexity 347 347
========================================
Files 294 294
Lines 12675 12675
Branches 1015 1015
========================================
Hits 992 992
Misses 11611 11611
Partials 72 72 Continue to review full report at Codecov.
|
84cd753
to
c8c545c
Compare
c8c545c
to
5dd362c
Compare
* With lazy loading of contributions * With contribution fetch * fix display of contributions * BugFixes Lazy Load Contributions * Use PageId as primary Key * show progress in CLF only whenn contributions are empty (graceful pagination) * Migration 1_2 Co-authored-by: Vivek Maskara <maskaravivek@gmail.com>
app/src/main/java/fr/free/nrw/commons/di/CommonsApplicationModule.java
Outdated
Show resolved
Hide resolved
app/src/main/java/fr/free/nrw/commons/contributions/ContributionsListFragment.java
Outdated
Show resolved
Hide resolved
app/src/main/java/fr/free/nrw/commons/media/MediaInterface.java
Outdated
Show resolved
Hide resolved
app/src/main/java/fr/free/nrw/commons/media/MediaInterface.java
Outdated
Show resolved
Hide resolved
app/src/main/java/fr/free/nrw/commons/upload/UploadService.java
Outdated
Show resolved
Hide resolved
I am seeing a few of these logs
should we be null checking
I haven't looked much at the structure of the returned response and what precisely a continuation is |
@macgills Thanks for testing the PR again. I have fixed the issue and have also addressed other code review comments.
Earlier, the end of the list was not handled so this error was coming. |
@@ -123,7 +140,12 @@ public MediaClient(MediaInterface mediaInterface, MediaDetailInterface mediaDeta | |||
|| null == mwQueryResponse.query().pages()) { | |||
return Observable.empty(); | |||
} | |||
continuationStore.put(key, mwQueryResponse.continuation()); | |||
if(mwQueryResponse.continuation() != null) { | |||
continuationStore.put(key, mwQueryResponse.continuation()); |
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.
if null couldn't we just remove the key and then it all works as usual without the extra map
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.
If we remove the key then it would be like fetching the first page. The API call would be made without the continuation params.
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 then the Db update would return the same data, not retriggering a boundaryCallback as the list would be unchanged, I think.
Though I guess now the api is set up with a continuation... I am quite unsure of how to proceed
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 the continuation map is never assigned but I am sure you are on that
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.
Though I guess now the api is set up with a continuation... I am quite unsure of how to proceed
Yeah, right. As the API is set up with a continuation it will start fetching all the pages again. On a related note, the current logic of fetching pages(that I added in this PR) is very naive and can be improved. For now, i wanted to keep things simple.
and the continuation map is never assigned but I am sure you are on that
I didn't understand this comment.
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 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 we can use aistart
and aiend
parameter to fetch contributions based on timestamp but I don't think that can replace continuation. With start and end timestamp, there's no way to be sure how many contributions would be returned. For eg. recently I uploaded around 2000 images in a day. The API would then return all images in one call. It would defeat the purpose of pagination.
To limit it to 10 images per call we will have to use ailimit
and then continuation
will still come into picture.
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.
@macgills Is everything working in this PR now? It would be great if you could do another test.
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 a little trickier granted but if at top of list we request media since now and reorder it so it is oldest first it hoovers along nicely.
Bottom of the list request since last item timestamp + newest first.
Again perhaps a discussion for another ticket. I will get to testing shortly
Codecov Report
@@ Coverage Diff @@
## master #3566 +/- ##
========================================
Coverage 7.81% 7.81%
Complexity 334 334
========================================
Files 295 295
Lines 12352 12352
Branches 992 992
========================================
Hits 965 965
Misses 11319 11319
Partials 68 68 Continue to review full report at Codecov.
|
The ux after uploading is not as good, you have to scroll up to see the in progress upload and then when it is done the list jumps down to the next item and you have to scroll up again to see the now uploaded item. Maybe if the deleting and saving of the inProgress/complete Contribution were in a transaction it would look better. |
@macgills I have fixed the UI issues. I have made the following changes:
|
app/src/main/java/fr/free/nrw/commons/contributions/ContributionsListPresenter.java
Outdated
Show resolved
Hide resolved
The UI is better but we still don't scroll to the new uploading item however all things considered I think that is an acceptable concession. I am happy with this PR once May be for the best to return these from the dao eg
|
Thanks for testing the PR @macgills. I have updated the PR. |
app/src/main/java/fr/free/nrw/commons/upload/UploadService.java
Outdated
Show resolved
Hide resolved
app/src/main/java/fr/free/nrw/commons/contributions/ContributionDao.java
Outdated
Show resolved
Hide resolved
@neslihanturan could you give this a quick test? I think you have a far greater number of contributions than I do. Otherwise looks good, has my approval |
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.
LGTM
Scrolling is a lot smoother, my first insights about this Pr is very positive. Can you emphasize if there is a function that must be tested. |
@neslihanturan I just wanted verification a large data set is performant, that is good enough for my curiosity |
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.
Thanks a lot @maskaravivek , this PR make the user feel the improvement.
Fixes #2904
Fixes #1993
Fixes #3667
Fixes #52