Fixes #3359 Duplicate Photos in Contributions Page (#3515)

ashishkumar468 · web-flow · commit efdb00b5ee31 · 2020-03-18T13:47:51.000+03:00
* Fixes #3359 * Cache thumb url & imageUrl in local db * Use Fresco's ImageRequest to show images in ContributionViewHolder[this was the issue, we should have always used this to show the image] * Deleted DisplayableContribution (not needed anymore) * Exposed abstract function in ContributionDao to updateContribution * * Make position private in ContributionViewHolder * Remove MediaDataExtractor from ContributionsFragment * * Show placeholder image for Contributions while the image loads * setHasStableId's ContributionsAdapter * make Random variable private in ContributionViewHolder * replace local variable-if-with ternary operator in ContributionViewHolder * Fix indentation/formatting of ternary operator in ContributionViewHolder * I might revert this commit[I have reasons] * Create in-memory drawables in CVH's onBind, caches are bad, add mental overhead * Revert "I might revert this commit[I have reasons]" This reverts commit 627ac91. * minor formatting changes, reverted 627ac91 * uh-oh missed semicolon, java * minor formatting changes
diff --git a/app/src/main/java/fr/free/nrw/commons/Media.java b/app/src/main/java/fr/free/nrw/commons/Media.java
@@ -45,6 +45,7 @@ public Media[] newArray(int i) {
     };
 
     // Primary metadata fields
+    @Nullable
     public Uri localUri;
     public String thumbUrl;
     public String imageUrl;
diff --git a/app/src/main/java/fr/free/nrw/commons/contributions/ContributionDao.java b/app/src/main/java/fr/free/nrw/commons/contributions/ContributionDao.java
@@ -8,6 +8,8 @@
 import androidx.room.Query;
 import androidx.room.Transaction;
 
+import androidx.room.Update;
+import io.reactivex.disposables.Disposable;
 import java.util.List;
 
 import io.reactivex.Completable;
@@ -52,4 +54,7 @@ public void deleteAllAndSaveTransaction(List<Contribution> contributions){
 
     @Query("Delete FROM contribution WHERE state = :state")
     public abstract void deleteAll(int state);
+
+    @Update
+    public abstract Single<Integer> update(Contribution contribution);
 }
diff --git a/app/src/main/java/fr/free/nrw/commons/contributions/ContributionViewHolder.java b/app/src/main/java/fr/free/nrw/commons/contributions/ContributionViewHolder.java
@@ -1,34 +1,30 @@
 package fr.free.nrw.commons.contributions;
 
+import android.graphics.Color;
+import android.graphics.drawable.ColorDrawable;
+import android.net.Uri;
+import android.text.TextUtils;
 import android.view.View;
 import android.widget.LinearLayout;
 import android.widget.ProgressBar;
 import android.widget.TextView;
 
-import androidx.collection.LruCache;
+import androidx.annotation.Nullable;
 import androidx.recyclerview.widget.RecyclerView;
 
 import com.facebook.drawee.view.SimpleDraweeView;
 
-import org.apache.commons.lang3.StringUtils;
+import com.facebook.imagepipeline.request.ImageRequest;
+import com.facebook.imagepipeline.request.ImageRequestBuilder;
 
-import javax.inject.Inject;
-import javax.inject.Named;
 
 import butterknife.BindView;
 import butterknife.ButterKnife;
 import butterknife.OnClick;
-import fr.free.nrw.commons.MediaDataExtractor;
 import fr.free.nrw.commons.R;
 import fr.free.nrw.commons.contributions.ContributionsListAdapter.Callback;
-import fr.free.nrw.commons.contributions.model.DisplayableContribution;
-import fr.free.nrw.commons.di.ApplicationlessInjection;
-import fr.free.nrw.commons.upload.FileUtils;
-import io.reactivex.android.schedulers.AndroidSchedulers;
-import io.reactivex.disposables.CompositeDisposable;
-import io.reactivex.disposables.Disposable;
-import io.reactivex.schedulers.Schedulers;
-import timber.log.Timber;
+import java.util.HashMap;
+import java.util.Random;
 
 public class ContributionViewHolder extends RecyclerView.ViewHolder {
 
@@ -41,32 +37,32 @@ public class ContributionViewHolder extends RecyclerView.ViewHolder {
     @BindView(R.id.contributionProgress) ProgressBar progressView;
     @BindView(R.id.failed_image_options) LinearLayout failedImageOptions;
 
-    @Inject
-    MediaDataExtractor mediaDataExtractor;
-
-    @Inject
-    @Named("thumbnail-cache")
-    LruCache<String, String> thumbnailCache;
-
-    private DisplayableContribution contribution;
-    private CompositeDisposable compositeDisposable = new CompositeDisposable();
     private int position;
+    private Contribution contribution;
+    private Random random = new Random();
 
     ContributionViewHolder(View parent, Callback callback) {
         super(parent);
         ButterKnife.bind(this, parent);
         this.callback=callback;
     }
 
-    public void init(int position, DisplayableContribution contribution) {
-        ApplicationlessInjection.getInstance(itemView.getContext())
-                .getCommonsApplicationComponent().inject(this);
-        this.position=position;
+    public void init(int position, Contribution contribution) {
         this.contribution = contribution;
-        fetchAndDisplayThumbnail(contribution);
+        this.position = position;
+        imageView.getHierarchy().setPlaceholderImage(new ColorDrawable(
+            Color.argb(100, random.nextInt(256), random.nextInt(256), random.nextInt(256))));
+        String imageSource = chooseImageSource(contribution.thumbUrl, contribution.getLocalUri());
+        if (!TextUtils.isEmpty(imageSource)) {
+            final ImageRequest imageRequest =
+                ImageRequestBuilder.newBuilderWithSource(Uri.parse(imageSource))
+                    .setProgressiveRenderingEnabled(true)
+                    .build();
+            imageView.setImageRequest(imageRequest);
+        }
         titleView.setText(contribution.getDisplayTitle());
 
-        seqNumView.setText(String.valueOf(contribution.getPosition() + 1));
+        seqNumView.setText(String.valueOf(position + 1));
         seqNumView.setVisibility(View.VISIBLE);
 
         switch (contribution.getState()) {
@@ -104,40 +100,18 @@ public void init(int position, DisplayableContribution contribution) {
     }
 
     /**
-     * This method fetches the thumbnail url from file name
-     * If the thumbnail url is present in cache, then it is used otherwise API call is made to fetch the thumbnail
-     * This can be removed once #2904 is in place and contribution contains all metadata beforehand
-     * @param contribution
+     * Returns the image source for the image view, first preference is given to thumbUrl if that is
+     * null, moves to local uri and if both are null return null
+     *
+     * @param thumbUrl
+     * @param localUri
+     * @return
      */
-    private void fetchAndDisplayThumbnail(DisplayableContribution contribution) {
-        String keyForLRUCache = contribution.getFilename();
-        String cacheUrl = thumbnailCache.get(keyForLRUCache);
-        if (!StringUtils.isBlank(cacheUrl)) {
-            imageView.setImageURI(cacheUrl);
-            return;
-        }
-
-        imageView.setBackground(null);
-        if ((contribution.getState() != Contribution.STATE_COMPLETED) && FileUtils.fileExists(
-                contribution.getLocalUri())) {
-            imageView.setImageURI(contribution.getLocalUri());
-        } else {
-            Timber.d("Fetching thumbnail for %s", contribution.getFilename());
-            Disposable disposable = mediaDataExtractor
-                    .getMediaFromFileName(contribution.getFilename())
-                    .subscribeOn(Schedulers.io())
-                    .observeOn(AndroidSchedulers.mainThread())
-                    .subscribe(media -> {
-                        thumbnailCache.put(keyForLRUCache, media.getThumbUrl());
-                        imageView.setImageURI(media.getThumbUrl());
-                    });
-            compositeDisposable.add(disposable);
-        }
-
-    }
-
-    public void clear() {
-        compositeDisposable.clear();
+    @Nullable
+    private String chooseImageSource(String thumbUrl, Uri localUri) {
+        return !TextUtils.isEmpty(thumbUrl) ? thumbUrl :
+            localUri != null ? localUri.toString() :
+                null;
     }
 
     /**
diff --git a/app/src/main/java/fr/free/nrw/commons/contributions/ContributionsContract.java b/app/src/main/java/fr/free/nrw/commons/contributions/ContributionsContract.java
@@ -26,10 +26,15 @@ public interface View {
     }
 
     public interface UserActionListener extends BasePresenter<ContributionsContract.View> {
+
         Contribution getContributionsWithTitle(String uri);
 
         void deleteUpload(Contribution contribution);
 
         Media getItemAtPosition(int i);
+
+        void updateContribution(Contribution contribution);
+
+        void fetchMediaDetails(Contribution contribution);
     }
 }
diff --git a/app/src/main/java/fr/free/nrw/commons/contributions/ContributionsFragment.java b/app/src/main/java/fr/free/nrw/commons/contributions/ContributionsFragment.java
@@ -20,6 +20,8 @@
 import androidx.fragment.app.FragmentManager.OnBackStackChangedListener;
 import androidx.fragment.app.FragmentTransaction;
 
+import fr.free.nrw.commons.MediaDataExtractor;
+import io.reactivex.disposables.Disposable;
 import java.util.List;
 
 import javax.inject.Inject;
@@ -216,6 +218,12 @@ public void openMediaDetail(int position) {
             public Contribution getContributionForPosition(int position) {
                 return (Contribution) contributionsPresenter.getItemAtPosition(position);
             }
+
+            @Override
+            public void fetchMediaUriFor(Contribution contribution) {
+                Timber.d("Fetching thumbnail for %s", contribution.filename);
+                contributionsPresenter.fetchMediaDetails(contribution);
+            }
         });
 
         if(null==mediaDetailPagerFragment){
diff --git a/app/src/main/java/fr/free/nrw/commons/contributions/ContributionsListAdapter.java b/app/src/main/java/fr/free/nrw/commons/contributions/ContributionsListAdapter.java
@@ -1,5 +1,8 @@
 package fr.free.nrw.commons.contributions;
 
+import android.os.Handler;
+import android.os.Looper;
+import android.text.TextUtils;
 import android.view.LayoutInflater;
 import android.view.ViewGroup;
 
@@ -10,7 +13,6 @@
 import java.util.List;
 
 import fr.free.nrw.commons.R;
-import fr.free.nrw.commons.contributions.model.DisplayableContribution;
 
 /**
  * Represents The View Adapter for the List of Contributions  
@@ -22,7 +24,7 @@ public class ContributionsListAdapter extends RecyclerView.Adapter<ContributionV
 
     public ContributionsListAdapter(Callback callback) {
         this.callback = callback;
-        contributions=new ArrayList<>();
+        contributions = new ArrayList<>();
     }
 
     /**
@@ -41,22 +43,27 @@ public ContributionViewHolder onCreateViewHolder(@NonNull ViewGroup parent, int
     @Override
     public void onBindViewHolder(@NonNull ContributionViewHolder holder, int position) {
         final Contribution contribution = contributions.get(position);
-        DisplayableContribution displayableContribution = new DisplayableContribution(contribution,
-                position);
-        holder.init(position, displayableContribution);
+        if (TextUtils.isEmpty(contribution.getThumbUrl())
+            && contribution.getState() == Contribution.STATE_COMPLETED) {
+            callback.fetchMediaUriFor(contribution);
+        }
+
+        holder.init(position, contribution);
     }
 
     @Override
     public int getItemCount() {
         return contributions.size();
     }
 
-    public void setContributions(List<Contribution> contributionList) {
-        if(null!=contributionList) {
-            this.contributions.clear();
-            this.contributions.addAll(contributionList);
-            notifyDataSetChanged();
-        }
+    public void setContributions(@NonNull List<Contribution> contributionList) {
+        contributions = contributionList;
+        notifyDataSetChanged();
+    }
+
+    @Override
+    public long getItemId(int position) {
+        return contributions.get(position)._id;
     }
 
     public interface Callback {
@@ -68,5 +75,7 @@ public interface Callback {
         void openMediaDetail(int contribution);
 
         Contribution getContributionForPosition(int position);
+
+        void fetchMediaUriFor(Contribution contribution);
     }
 }
diff --git a/app/src/main/java/fr/free/nrw/commons/contributions/ContributionsListFragment.java b/app/src/main/java/fr/free/nrw/commons/contributions/ContributionsListFragment.java
@@ -90,6 +90,7 @@ public void setCallback(Callback callback) {
 
     private void initAdapter() {
         adapter = new ContributionsListAdapter(callback);
+        adapter.setHasStableIds(true);
     }
 
     @Override
diff --git a/app/src/main/java/fr/free/nrw/commons/contributions/ContributionsLocalDataSource.java b/app/src/main/java/fr/free/nrw/commons/contributions/ContributionsLocalDataSource.java
@@ -74,4 +74,8 @@ public Completable saveContributions(List<Contribution> contributions) {
     public void set(String key, long value) {
         defaultKVStore.putLong(key,value);
     }
+
+    public Single<Integer> updateContribution(Contribution contribution) {
+        return contributionDao.update(contribution);
+    }
 }
diff --git a/app/src/main/java/fr/free/nrw/commons/contributions/ContributionsPresenter.java b/app/src/main/java/fr/free/nrw/commons/contributions/ContributionsPresenter.java
@@ -12,6 +12,7 @@
 import androidx.lifecycle.LiveData;
 import androidx.lifecycle.Observer;
 
+import fr.free.nrw.commons.MediaDataExtractor;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
@@ -62,6 +63,10 @@ public class ContributionsPresenter implements UserActionListener {
 
     @Inject
     SessionManager sessionManager;
+
+    @Inject
+    MediaDataExtractor mediaDataExtractor;
+
     private LifecycleOwner lifeCycleOwner;
 
     @Inject
@@ -180,4 +185,24 @@ public Media getItemAtPosition(int i) {
         }
         return contributionList.get(i);
     }
+
+    @Override
+    public void updateContribution(Contribution contribution) {
+        compositeDisposable.add(repository
+            .updateContribution(contribution)
+            .subscribeOn(ioThreadScheduler)
+            .subscribe());
+    }
+
+    @Override
+    public void fetchMediaDetails(Contribution contribution) {
+        compositeDisposable.add(mediaDataExtractor
+            .getMediaFromFileName(contribution.filename)
+            .subscribeOn(Schedulers.io())
+            .observeOn(AndroidSchedulers.mainThread())
+            .subscribe(media -> {
+                contribution.thumbUrl=media.thumbUrl;
+                updateContribution(contribution);
+            }));
+    }
 }
diff --git a/app/src/main/java/fr/free/nrw/commons/contributions/ContributionsRepository.java b/app/src/main/java/fr/free/nrw/commons/contributions/ContributionsRepository.java
@@ -61,4 +61,8 @@ public void set(String key, long value) {
     public long getLong(String key) {
         return localDataSource.getLong(key);
     }
+
+    public Single<Integer> updateContribution(Contribution contribution) {
+        return localDataSource.updateContribution(contribution);
+    }
 }
diff --git a/app/src/main/java/fr/free/nrw/commons/contributions/model/DisplayableContribution.java b/app/src/main/java/fr/free/nrw/commons/contributions/model/DisplayableContribution.java
diff --git a/app/src/main/java/fr/free/nrw/commons/di/CommonsApplicationComponent.java b/app/src/main/java/fr/free/nrw/commons/di/CommonsApplicationComponent.java
@@ -53,8 +53,6 @@ public interface CommonsApplicationComponent extends AndroidInjector<Application
 
     void inject(PicOfDayAppWidget picOfDayAppWidget);
 
-    void inject(ContributionViewHolder viewHolder);
-
     Gson gson();
 
     @Component.Builder

Original file line number	Diff line number	Diff line change
`@@ -26,10 +26,15 @@ public interface View {`
`26`	`26`	`}`
`27`	`27`
`28`	`28`	`public interface UserActionListener extends BasePresenter<ContributionsContract.View> {`
	`29`	`+`
`29`	`30`	`Contribution getContributionsWithTitle(String uri);`
`30`	`31`
`31`	`32`	`void deleteUpload(Contribution contribution);`
`32`	`33`
`33`	`34`	`Media getItemAtPosition(int i);`
	`35`	`+`
	`36`	`+ void updateContribution(Contribution contribution);`
	`37`	`+`
	`38`	`+ void fetchMediaDetails(Contribution contribution);`
`34`	`39`	`}`
`35`	`40`	`}`
Original file line number	Diff line number	Diff line change
`@@ -90,6 +90,7 @@ public void setCallback(Callback callback) {`
`90`	`90`
`91`	`91`	`private void initAdapter() {`
`92`	`92`	`adapter = new ContributionsListAdapter(callback);`
	`93`	`+ adapter.setHasStableIds(true);`
`93`	`94`	`}`
`94`	`95`
`95`	`96`	`@Override`
Original file line number	Diff line number	Diff line change
`@@ -74,4 +74,8 @@ public Completable saveContributions(List<Contribution> contributions) {`
`74`	`74`	`public void set(String key, long value) {`
`75`	`75`	`defaultKVStore.putLong(key,value);`
`76`	`76`	`}`
	`77`	`+`
	`78`	`+ public Single<Integer> updateContribution(Contribution contribution) {`
	`79`	`+ return contributionDao.update(contribution);`
	`80`	`+ }`
`77`	`81`	`}`
Original file line number	Diff line number	Diff line change
`@@ -61,4 +61,8 @@ public void set(String key, long value) {`
`61`	`61`	`public long getLong(String key) {`
`62`	`62`	`return localDataSource.getLong(key);`
`63`	`63`	`}`
	`64`	`+`
	`65`	`+ public Single<Integer> updateContribution(Contribution contribution) {`
	`66`	`+ return localDataSource.updateContribution(contribution);`
	`67`	`+ }`
`64`	`68`	`}`