With lazy loading of contributions #3566

maskaravivek · 2020-03-23T06:30:25Z

Fixes #2904
Fixes #1993
Fixes #3667
Fixes #52

tests-checker

Could you please add tests to make sure this change works as expected?

codecov-io · 2020-03-23T06:38:03Z

Codecov Report

Merging #3566 into master will not change coverage.
The diff coverage is n/a.

@@           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.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb4f91c...bb4f91c. Read the comment docs.

* 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

…pps-android-commons into lazyContributions

.idea/codeStyles/codeStyleConfig.xml

app/src/main/java/fr/free/nrw/commons/Media.java

app/src/main/java/fr/free/nrw/commons/contributions/ContributionsListFragment.java

app/src/main/java/fr/free/nrw/commons/media/MediaInterface.java

app/src/main/java/fr/free/nrw/commons/upload/UploadService.java

macgills · 2020-05-20T12:29:05Z

I am seeing a few of these logs

2020-05-20 13:23:45.171 13978-14916/fr.free.nrw.commons.beta E/ContributionBoundaryCallback$fetchContributions: Failed to fetch contributions: Query map was null (parameter #3)
        for method MediaInterface.getMediaListForUser

should we be null checking

continuationStore.put(key, mwQueryResponse.continuation());

I haven't looked much at the structure of the returned response and what precisely a continuation is

maskaravivek · 2020-05-20T20:53:16Z

@macgills Thanks for testing the PR again. I have fixed the issue and have also addressed other code review comments.

Query map was null

Earlier, the end of the list was not handled so this error was coming.

macgills · 2020-05-21T07:59:11Z

app/src/main/java/fr/free/nrw/commons/media/MediaClient.java

@@ -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());


if null couldn't we just remove the key and then it all works as usual without the extra map

If we remove the key then it would be like fetching the first page. The API call would be made without the continuation params.

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

and the continuation map is never assigned but I am sure you are on that

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.

I should have said continuationExists map but it is resolved now.

This is a bit "upending the teatable" but it seems to me that this endpoint shouldn't use continuation at all, we could happily query using

but perhaps that is best suited for another ticket

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.

@macgills Is everything working in this PR now? It would be great if you could do another test.

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-commenter · 2020-05-21T18:34:25Z

Codecov Report

Merging #3566 into master will not change coverage.
The diff coverage is n/a.

@@           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.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f12c5f...5f12c5f. Read the comment docs.

macgills · 2020-05-22T11:26:19Z

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.

maskaravivek · 2020-05-26T09:33:05Z

@macgills I have fixed the UI issues. I have made the following changes:

am deleting and saving in a single transaction
have replaced a couple of usages of save with update
started persisting time information in dateUploaded column of Contribution DB. Earlier just date was persisted so all contributions uploaded on a particular day had the same timestamp.

app/src/main/java/fr/free/nrw/commons/contributions/ContributionsListPresenter.java

macgills · 2020-05-26T13:02:48Z

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 ioThreadScheduler.scheduleDirect is replaced with completeables that are added to a compositeDisposable so they can be gracefully cancelled.

May be for the best to return these from the dao eg

@Insert
public void saveSynchronous()
@Delete
public void deleteSynchronous()
@Transaction
public void saveAndDeleteSynchronous(){
deleteSynchronous()
saveSynchronous()
}

public Completeable saveAndDelete(){
return Completeable.fromAction{
saveAndDeleteSynchronous()
}
}

maskaravivek · 2020-05-26T18:53:43Z

Thanks for testing the PR @macgills.

I have updated the PR.

app/src/main/java/fr/free/nrw/commons/upload/UploadService.java

app/src/main/java/fr/free/nrw/commons/contributions/ContributionDao.java

macgills · 2020-05-27T08:52:49Z

@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

ashishkumar468

LGTM

neslihanturan · 2020-05-28T11:38:29Z

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.

macgills · 2020-05-28T11:45:09Z

@neslihanturan I just wanted verification a large data set is performant, that is good enough for my curiosity

neslihanturan

Thanks a lot @maskaravivek , this PR make the user feel the improvement.

tests-checker bot suggested changes Mar 23, 2020

View reviewed changes

maskaravivek mentioned this pull request Mar 23, 2020

In Contributions list, add an icon for images uploaded via Nearby that allows users to use that image in associated Wikipedia article(s) #872

Closed

maskaravivek mentioned this pull request Apr 15, 2020

Many Mnull requests #3653

Closed

maskaravivek force-pushed the lazyContributions branch 4 times, most recently from 84cd753 to c8c545c Compare April 17, 2020 04:42

maskaravivek added 4 commits April 17, 2020 00:06

With lazy loading of contributions

3adf1e3

With contribution fetch

27a8352

fix display of contributions

0537470

more changes

5dd362c

maskaravivek force-pushed the lazyContributions branch from c8c545c to 5dd362c Compare April 17, 2020 07:09

macgills reviewed Apr 20, 2020

View reviewed changes

app/src/main/java/fr/free/nrw/commons/di/CommonsApplicationModule.java Outdated Show resolved Hide resolved

maskaravivek added 5 commits April 20, 2020 16:41

Merge branch 'master' into lazyContributions

6e3c61a

Merge branch 'lazyContributions' of https://github.com/maskaravivek/a…

d0b127b

…pps-android-commons into lazyContributions

Merge branch 'master' into lazyContributions

25884ca

Fix build

2c1e271

minor fixes

1e12d31

maskaravivek marked this pull request as ready for review April 21, 2020 23:05

maskaravivek requested review from macgills, ashishkumar468 and misaochan April 21, 2020 23:05