#3601 [structured-commons] Wrong thumbnails at the "depictions" step when uploading (#3696)

* #3601 [structured-commons] Wrong thumbnails at the "depictions" step when uploading - fix imageview rendering code

* #3601 [structured-commons] Wrong thumbnails at the "depictions" step when uploading - show selected depictions at the top - ensure updates to nearby places are reflected

* #3601 [structured-commons] Wrong thumbnails at the "depictions" step when uploading - don't set imageUrl when the url is none

* #3601 [structured-commons] Wrong thumbnails at the "depictions" step when uploading - rewrite unit tests

* #3601 [structured-commons] Wrong thumbnails at the "depictions" step when uploading - minor cleanup

* #3601 [structured-commons] Wrong thumbnails at the "depictions" step when uploading - fix erroneous cache access

.idea/inspectionProfiles/Project_Default.xml

@@ -9,6 +9,7 @@
     </inspection_tool>
     <inspection_tool class="ControlFlowStatementWithoutBraces" enabled="true" level="ERROR" enabled_by_default="true" />
     <inspection_tool class="DefaultNotLastCaseInSwitch" enabled="true" level="WARNING" enabled_by_default="true" />
+    <inspection_tool class="ExplicitThis" enabled="true" level="WEAK WARNING" enabled_by_default="true" />
     <inspection_tool class="FieldMayBeFinal" enabled="true" level="WARNING" enabled_by_default="true" />
     <inspection_tool class="LocalCanBeFinal" enabled="true" level="WARNING" enabled_by_default="true">
       <option name="REPORT_VARIABLES" value="true" />

app/build.gradle

@@ -78,6 +78,8 @@ dependencies {
     testImplementation "org.powermock:powermock-module-junit4:2.0.0-beta.5"
     testImplementation "org.powermock:powermock-api-mockito2:2.0.0-beta.5"
     testImplementation 'org.mockito:mockito-core:2.23.0'
+    testImplementation "com.jraska.livedata:testing-ktx:1.1.2"
+    testImplementation "androidx.arch.core:core-testing:2.1.0"
 
     // Android testing
     androidTestImplementation "org.jetbrains.kotlin:kotlin-stdlib-jdk7:$KOTLIN_VERSION"

app/src/main/java/fr/free/nrw/commons/explore/depictions/DepictsClient.java

@@ -32,7 +32,7 @@ public class DepictsClient {
 
     private final DepictsInterface depictsInterface;
     private final MediaInterface mediaInterface;
-    private static final String NO_DEPICTED_IMAGE = "No Image for Depiction";
+    public static final String NO_DEPICTED_IMAGE = "No Image for Depiction";
 
     @Inject
     public DepictsClient(DepictsInterface depictsInterface, MediaInterface mediaInterface) {
@@ -46,14 +46,16 @@ public class DepictsClient {
      */
     public Observable<DepictedItem> searchForDepictions(String query, int limit, int offset) {
         return depictsInterface.searchForDepicts(
-                query,
+            query,
-                String.valueOf(limit),
+            String.valueOf(limit),
-                Locale.getDefault().getLanguage(),
+            Locale.getDefault().getLanguage(),
-                Locale.getDefault().getLanguage(),
+            Locale.getDefault().getLanguage(),
-                String.valueOf(offset)
+            String.valueOf(offset)
         )
-                .flatMap(depictSearchResponse ->Observable.fromIterable(depictSearchResponse.getSearch()))
+            .toObservable()
-                .map(DepictedItem::new);
+            .flatMap( depictSearchResponse ->
+                Observable.fromIterable(depictSearchResponse.getSearch()))
+            .map(DepictedItem::new);
     }
 
     /**
@@ -80,15 +82,13 @@ public class DepictsClient {
             .map(claimsResponse -> {
                 final List<Statement_partial> imageClaim = claimsResponse.getClaims()
                     .get(WikidataProperties.IMAGE.getPropertyName());
-                if (imageClaim != null) {
                     final DataValueString dataValue = (DataValueString) imageClaim
                         .get(0)
                         .getMainSnak()
                         .getDataValue();
                     return getThumbnailUrl((dataValue.getValue()));
-                }
-                return NO_DEPICTED_IMAGE;
             })
+            .onErrorReturn(throwable -> NO_DEPICTED_IMAGE)
             .singleOrError();
     }
 

app/src/main/java/fr/free/nrw/commons/explore/depictions/SearchDepictionsRenderer.java

@@ -1,5 +1,7 @@
 package fr.free.nrw.commons.explore.depictions;
 
+import static fr.free.nrw.commons.explore.depictions.DepictsClient.NO_DEPICTED_IMAGE;
+
 import android.graphics.Bitmap;
 import android.net.Uri;
 import android.text.TextUtils;
@@ -45,9 +47,6 @@ public class SearchDepictionsRenderer extends Renderer<DepictedItem> {
 
     private DepictCallback listener;
 
-    int size = 0;
-    private final static String NO_IMAGE_FOR_DEPICTION = "No Image for Depiction";
-
     public SearchDepictionsRenderer(DepictCallback listener) {
         this.listener = listener;
     }
@@ -84,7 +83,7 @@ public class SearchDepictionsRenderer extends Renderer<DepictedItem> {
 
         Timber.e("line86"+item.getImageUrl());
         if (!TextUtils.isEmpty(item.getImageUrl())) {
-            if (!item.getImageUrl().equals(NO_IMAGE_FOR_DEPICTION) && !item.getImageUrl().equals(""))
+            if (!item.getImageUrl().equals(NO_DEPICTED_IMAGE) && !item.getImageUrl().equals(""))
             {
                 ImageRequest imageRequest = ImageRequestBuilder
                         .newBuilderWithSource(Uri.parse(item.getImageUrl()))

app/src/main/java/fr/free/nrw/commons/repository/UploadRemoteDataSource.java

@@ -1,14 +1,5 @@
 package fr.free.nrw.commons.repository;
 
-import fr.free.nrw.commons.upload.ImageCoordinates;
-import java.io.IOException;
-import java.util.Comparator;
-import java.util.List;
-import java.util.Locale;
-
-import javax.inject.Inject;
-import javax.inject.Singleton;
-
 import fr.free.nrw.commons.category.CategoriesModel;
 import fr.free.nrw.commons.category.CategoryItem;
 import fr.free.nrw.commons.contributions.Contribution;
@@ -16,14 +7,22 @@ import fr.free.nrw.commons.filepicker.UploadableFile;
 import fr.free.nrw.commons.location.LatLng;
 import fr.free.nrw.commons.nearby.NearbyPlaces;
 import fr.free.nrw.commons.nearby.Place;
+import fr.free.nrw.commons.upload.ImageCoordinates;
 import fr.free.nrw.commons.upload.SimilarImageInterface;
 import fr.free.nrw.commons.upload.UploadController;
 import fr.free.nrw.commons.upload.UploadModel;
 import fr.free.nrw.commons.upload.UploadModel.UploadItem;
 import fr.free.nrw.commons.upload.structure.depictions.DepictModel;
 import fr.free.nrw.commons.upload.structure.depictions.DepictedItem;
+import io.reactivex.Flowable;
 import io.reactivex.Observable;
 import io.reactivex.Single;
+import java.io.IOException;
+import java.util.Comparator;
+import java.util.List;
+import java.util.Locale;
+import javax.inject.Inject;
+import javax.inject.Singleton;
 
 /**
  * This class would act as the data source for remote operations for UploadActivity
@@ -189,15 +188,13 @@ public class UploadRemoteDataSource {
      * @param longitude
      * @return
      */
-    public Place getNearbyPlaces(double latitude, double longitude) {
+    public Place getNearbyPlaces(double latitude, double longitude) throws IOException {
-        try {
+        List<Place> fromWikidataQuery = nearbyPlaces
-            List<Place> fromWikidataQuery = nearbyPlaces.getFromWikidataQuery(new LatLng(latitude, longitude, 0.0f),
+            .getFromWikidataQuery(new LatLng(latitude, longitude, 0.0f),
-                    Locale.getDefault().getLanguage(),
+                Locale.getDefault().getLanguage(),
-                    NEARBY_RADIUS_IN_KILO_METERS);
+                NEARBY_RADIUS_IN_KILO_METERS);
-            return fromWikidataQuery.size() > 0 ? fromWikidataQuery.get(0) : null;
+        return fromWikidataQuery.size() > 0 ? fromWikidataQuery.get(0) : null;
-        } catch (IOException e) {
+
-            return null;
-        }
     }
 
     /**
@@ -220,9 +217,10 @@ public class UploadRemoteDataSource {
 
     /**
      * get all depictions
+     * @return
      */
 
-    public Observable<DepictedItem> searchAllEntities(String query) {
+    public Flowable<List<DepictedItem>> searchAllEntities(String query) {
         return depictModel.searchAllEntities(query);
     }
 

app/src/main/java/fr/free/nrw/commons/repository/UploadRepository.java

@@ -1,6 +1,8 @@
 package fr.free.nrw.commons.repository;
 
 import fr.free.nrw.commons.upload.ImageCoordinates;
+import io.reactivex.Flowable;
+import java.io.IOException;
 import java.util.Comparator;
 import java.util.List;
 
@@ -284,7 +286,7 @@ public class UploadRepository {
      * @return
      */
 
-    public Observable<DepictedItem> searchAllEntities(String query) {
+    public Flowable<List<DepictedItem>> searchAllEntities(String query) {
         return remoteDataSource.searchAllEntities(query);
     }
 
@@ -294,7 +296,7 @@ public class UploadRepository {
      * @param decLongitude
      * @return
      */
-    public Place checkNearbyPlaces(double decLatitude, double decLongitude) {
+    public Place checkNearbyPlaces(double decLatitude, double decLongitude) throws IOException {
         return remoteDataSource.getNearbyPlaces(decLatitude, decLongitude);
     }
 

app/src/main/java/fr/free/nrw/commons/upload/FileProcessor.kt

@@ -209,7 +209,7 @@ class FileProcessor @Inject constructor(
             .filter { it.size >= MIN_NEARBY_RESULTS }
             .take(1)
             .subscribe(
-                { depictsModel.nearbyPlaces = it },
+                { depictsModel.nearbyPlaces.offer(it) },
                 { Timber.e(it) }
             )
     }

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

@@ -1,34 +1,20 @@
 package fr.free.nrw.commons.upload;
 
-import android.graphics.Bitmap;
 import android.net.Uri;
 import android.text.TextUtils;
 import android.view.LayoutInflater;
 import android.view.View;
 import android.view.ViewGroup;
 import android.widget.CheckBox;
-import android.widget.ImageView;
 import android.widget.TextView;
-
-import androidx.annotation.Nullable;
-
-import com.facebook.common.executors.CallerThreadExecutor;
-import com.facebook.common.references.CloseableReference;
-import com.facebook.datasource.DataSource;
-import com.facebook.drawee.backends.pipeline.Fresco;
-import com.facebook.imagepipeline.core.ImagePipeline;
-import com.facebook.imagepipeline.datasource.BaseBitmapDataSubscriber;
-import com.facebook.imagepipeline.image.CloseableImage;
-import com.facebook.imagepipeline.request.ImageRequest;
-import com.facebook.imagepipeline.request.ImageRequestBuilder;
-import com.pedrogomez.renderers.Renderer;
-
 import butterknife.BindView;
 import butterknife.ButterKnife;
+import com.facebook.common.util.UriUtil;
+import com.facebook.drawee.view.SimpleDraweeView;
+import com.pedrogomez.renderers.Renderer;
 import fr.free.nrw.commons.R;
 import fr.free.nrw.commons.upload.structure.depictions.DepictedItem;
 import fr.free.nrw.commons.upload.structure.depictions.UploadDepictsCallback;
-import timber.log.Timber;
 
 /**
  * Depicts Renderer for setting up inflating layout,
@@ -42,8 +28,7 @@ public class UploadDepictsRenderer extends Renderer<DepictedItem> {
     TextView depictsLabel;
     @BindView(R.id.description) TextView description;
     @BindView(R.id.depicted_image)
-    ImageView imageView;
+    SimpleDraweeView imageView;
-    private final static String NO_IMAGE_FOR_DEPICTION="No Image for Depiction";
 
     public UploadDepictsRenderer(UploadDepictsCallback listener) {
         this.listener = listener;
@@ -91,45 +76,13 @@ public class UploadDepictsRenderer extends Renderer<DepictedItem> {
         checkedView.setChecked(item.isSelected());
         depictsLabel.setText(item.getName());
         description.setText(item.getDescription());
-        if (!TextUtils.isEmpty(item.getImageUrl())) {
+        final String imageUrl = item.getImageUrl();
-            if (!item.getImageUrl().equals(NO_IMAGE_FOR_DEPICTION))
+        if (TextUtils.isEmpty(imageUrl)) {
-                setImageView(Uri.parse(item.getImageUrl()), imageView);
+            imageView.setImageURI(UriUtil.getUriForResourceId(R.drawable.ic_wikidata_logo_24dp));
-        }else{
+            listener.fetchThumbnailUrlForEntity(item);
-            listener.fetchThumbnailUrlForEntity(item.getId(),item.getPosition());
+        } else {
+            imageView.setImageURI(Uri.parse(imageUrl));
         }
     }
 
-    /**
-     * Set thumbnail for the depicted item
-     */
-    private void setImageView(Uri imageUrl, ImageView imageView) {
-        ImageRequest imageRequest = ImageRequestBuilder
-                .newBuilderWithSource(imageUrl)
-                .setAutoRotateEnabled(true)
-                .build();
-
-        ImagePipeline imagePipeline = Fresco.getImagePipeline();
-        final DataSource<CloseableReference<CloseableImage>>
-                dataSource = imagePipeline.fetchDecodedImage(imageRequest, getContext());
-
-        dataSource.subscribe(new BaseBitmapDataSubscriber() {
-
-            @Override
-            public void onNewResultImpl(@Nullable Bitmap bitmap) {
-                if (dataSource.isFinished() && bitmap != null) {
-                    Timber.d("Bitmap loaded from url %s", imageUrl.toString());
-                    imageView.post(() -> imageView.setImageBitmap(Bitmap.createBitmap(bitmap)));
-                    dataSource.close();
-                }
-            }
-
-            @Override
-            public void onFailureImpl(DataSource dataSource) {
-                Timber.d("Error getting bitmap from image url %s", imageUrl.toString());
-                if (dataSource != null) {
-                    dataSource.close();
-                }
-            }
-        }, CallerThreadExecutor.getInstance());
-    }
 }

app/src/main/java/fr/free/nrw/commons/upload/depicts/DepictsContract.java

@@ -1,9 +1,10 @@
 package fr.free.nrw.commons.upload.depicts;
 
-import java.util.List;
+import androidx.lifecycle.LiveData;
-
 import fr.free.nrw.commons.BasePresenter;
 import fr.free.nrw.commons.upload.structure.depictions.DepictedItem;
+import java.util.List;
+import org.jetbrains.annotations.NotNull;
 
 /**
  * The contract with which DepictsFragment and its presenter would talk to each other
@@ -41,10 +42,8 @@ public interface DepictsContract {
          */
         void setDepictsList(List<DepictedItem> depictedItemList);
 
-        /**
+
-         * Set thumbnail image for depicted item
+        void onUrlFetched(@NotNull DepictedItem depictedItem, @NotNull String url);
-         */
-        void onImageUrlFetched(String response, int position);
     }
 
     interface UserActionListener extends BasePresenter<View> {
@@ -71,11 +70,8 @@ public interface DepictsContract {
          */
         void verifyDepictions();
 
-        /**
+        LiveData<List<DepictedItem>> getDepictedItems();
-         * Fetch thumbnail for the Wikidata Item
+
-         * @param entityId entityId of the item
+        void fetchThumbnailForEntityId(DepictedItem depictedItem);
-         * @param position position of the item
-         */
-        void fetchThumbnailForEntityId(String entityId, int position);
     }
 }

app/src/main/java/fr/free/nrw/commons/upload/depicts/DepictsFragment.java

@@ -1,6 +1,7 @@
 package fr.free.nrw.commons.upload.depicts;
 
 import android.os.Bundle;
+import android.util.Pair;
 import android.view.LayoutInflater;
 import android.view.View;
 import android.view.ViewGroup;
@@ -30,6 +31,7 @@ import java.util.ArrayList;
 import java.util.List;
 import java.util.concurrent.TimeUnit;
 import javax.inject.Inject;
+import org.jetbrains.annotations.NotNull;
 import timber.log.Timber;
 
 
@@ -66,6 +68,7 @@ public class DepictsFragment extends UploadBaseFragment implements DepictsContra
         super.onViewCreated(view, savedInstanceState);
         ButterKnife.bind(this, view);
         init();
+        presenter.getDepictedItems().observe(getViewLifecycleOwner(), this::setDepictsList);
     }
 
     /**
@@ -125,9 +128,11 @@ public class DepictsFragment extends UploadBaseFragment implements DepictsContra
 
     @Override
     public void showError(Boolean value) {
-        if (value)
+        if (value) {
-        depictsSearchContainer.setError(getString(R.string.no_depiction_found));
+            depictsSearchContainer.setError(getString(R.string.no_depiction_found));
-        else depictsSearchContainer.setErrorEnabled(false);
+        } else {
+            depictsSearchContainer.setErrorEnabled(false);
+        }
     }
 
     @Override
@@ -139,13 +144,24 @@ public class DepictsFragment extends UploadBaseFragment implements DepictsContra
         }
     }
 
-    /**
-     * Set thumbnail image for depicted item
-     */
     @Override
-    public void onImageUrlFetched(String response, int position) {
+    public void onUrlFetched(@NotNull DepictedItem depictedItem, @NotNull String url) {
-        adapter.getItem(position).setImageUrl(response);
+        final Pair<DepictedItem, Integer> itemAndPosition = returnItemAndPosition(depictedItem);
-        adapter.notifyItemChanged(position);
+        if (itemAndPosition != null) {
+            itemAndPosition.first.setImageUrl(url);
+            adapter.notifyItemChanged(itemAndPosition.second);
+        }
+    }
+
+    @Nullable
+    private Pair<DepictedItem,Integer> returnItemAndPosition(@NotNull DepictedItem depictedItem) {
+        for (int i = 0; i < adapter.getItemCount(); i++) {
+            final DepictedItem item = adapter.getItem(i);
+            if(item.getId().equals(depictedItem.getId())){
+                return new Pair<>(item, i);
+            }
+        }
+        return null;
     }
 
     @OnClick(R.id.depicts_next)
@@ -167,8 +183,8 @@ public class DepictsFragment extends UploadBaseFragment implements DepictsContra
      * Fetch thumbnail for the given entityId at the given position
      */
     @Override
-    public void fetchThumbnailUrlForEntity(String entityId, int position) {
+    public void fetchThumbnailUrlForEntity(DepictedItem depictedItem) {
-        presenter.fetchThumbnailForEntityId(entityId,position);
+        presenter.fetchThumbnailForEntityId(depictedItem);
     }
 
     /**

app/src/main/java/fr/free/nrw/commons/upload/depicts/DepictsInterface.java

@@ -2,6 +2,7 @@ package fr.free.nrw.commons.upload.depicts;
 
 import fr.free.nrw.commons.wikidata.model.DepictSearchResponse;
 import io.reactivex.Observable;
+import io.reactivex.Single;
 import org.wikipedia.wikidata.ClaimsResponse;
 import retrofit2.http.GET;
 import retrofit2.http.Query;
@@ -21,7 +22,7 @@ public interface DepictsInterface {
      * @param offset number of depictions already fetched useful in implementing pagination
      */
     @GET("/w/api.php?action=wbsearchentities&format=json&type=item&uselang=en")
-    Observable<DepictSearchResponse> searchForDepicts(@Query("search") String query, @Query("limit") String limit, @Query("language") String language, @Query("uselang") String uselang, @Query("continue") String offset);
+    Single<DepictSearchResponse> searchForDepicts(@Query("search") String query, @Query("limit") String limit, @Query("language") String language, @Query("uselang") String uselang, @Query("continue") String offset);
 
     @GET("/w/api.php?action=wbgetclaims&format=json&property=P18")
     Observable<ClaimsResponse> getImageForEntity(@Query("entity") String entityId);

app/src/main/java/fr/free/nrw/commons/upload/depicts/DepictsPresenter.java

@@ -1,152 +0,0 @@
-package fr.free.nrw.commons.upload.depicts;
-
-import static fr.free.nrw.commons.di.CommonsApplicationModule.IO_THREAD;
-import static fr.free.nrw.commons.di.CommonsApplicationModule.MAIN_THREAD;
-
-import fr.free.nrw.commons.explore.depictions.DepictsClient;
-import fr.free.nrw.commons.repository.UploadRepository;
-import fr.free.nrw.commons.upload.structure.depictions.DepictedItem;
-import io.reactivex.Observable;
-import io.reactivex.Scheduler;
-import io.reactivex.android.schedulers.AndroidSchedulers;
-import io.reactivex.disposables.CompositeDisposable;
-import io.reactivex.disposables.Disposable;
-import io.reactivex.schedulers.Schedulers;
-import java.lang.reflect.Proxy;
-import java.util.ArrayList;
-import java.util.List;
-import java.util.concurrent.TimeUnit;
-import javax.inject.Inject;
-import javax.inject.Named;
-import javax.inject.Singleton;
-import timber.log.Timber;
-
-/**
- * presenter for DepictsFragment
- */
-@Singleton
-public class DepictsPresenter implements DepictsContract.UserActionListener {
-
-    private static final DepictsContract.View DUMMY = (DepictsContract.View) Proxy
-            .newProxyInstance(
-                    DepictsContract.View.class.getClassLoader(),
-                    new Class[]{DepictsContract.View.class},
-                    (proxy, method, methodArgs) -> null);
-
-
-    private final Scheduler ioScheduler;
-    private final Scheduler mainThreadScheduler;
-    private DepictsContract.View view = DUMMY;
-    private UploadRepository repository;
-    private DepictsClient depictsClient;
-    private static int TIMEOUT_SECONDS = 15;
-
-    private CompositeDisposable compositeDisposable;
-
-    @Inject
-    public DepictsPresenter(UploadRepository uploadRepository, @Named(IO_THREAD) Scheduler ioScheduler,
-                            @Named(MAIN_THREAD) Scheduler mainThreadScheduler, DepictsClient depictsClient) {
-        this.repository = uploadRepository;
-        this.ioScheduler = ioScheduler;
-        this.mainThreadScheduler = mainThreadScheduler;
-        this.depictsClient = depictsClient;
-        compositeDisposable = new CompositeDisposable();
-    }
-
-    @Override
-    public void onAttachView(DepictsContract.View view) {
-        this.view = view;
-    }
-
-    @Override
-    public void onDetachView() {
-        this.view = DUMMY;
-    }
-
-    @Override
-    public void onPreviousButtonClicked() {
-        view.goToPreviousScreen();
-    }
-
-    @Override
-    public void onDepictItemClicked(DepictedItem depictedItem) {
-        repository.onDepictItemClicked(depictedItem);
-    }
-
-    /**
-     * asks the repository to fetch depictions for the query
-     *  @param query
-     */
-    @Override
-    public void searchForDepictions(String query) {
-        List<DepictedItem> depictedItemList = new ArrayList<>();
-        Observable<DepictedItem> distinctDepictsObservable = Observable
-                .fromIterable(repository.getSelectedDepictions())
-                .subscribeOn(ioScheduler)
-                .observeOn(mainThreadScheduler)
-                .doOnSubscribe(disposable -> {
-                    view.showProgress(true);
-                    view.setDepictsList(null);
-                })
-                .observeOn(ioScheduler)
-                .concatWith(
-                        repository.searchAllEntities(query)
-                )
-                .distinct();
-
-        Disposable searchDepictsDisposable = distinctDepictsObservable
-                .observeOn(mainThreadScheduler)
-                .subscribe(
-                    e -> {
-                        depictedItemList.add(e);
-                    },
-                        t -> {
-                            view.showProgress(false);
-                            view.showError(true);
-                            Timber.e(t);
-                        },
-                        () -> {
-                            view.showProgress(false);
-
-                            if (depictedItemList.isEmpty()) {
-                                view.showError(true);
-                            } else {
-                                view.showError(false);
-                                view.setDepictsList(depictedItemList);
-                            }
-                        }
-                );
-        compositeDisposable.add(searchDepictsDisposable);
-        view.setDepictsList(depictedItemList);
-    }
-
-    /**
-     * Check if depictions were selected
-     * from the depiction list
-     */
-    @Override
-    public void verifyDepictions() {
-        List<DepictedItem> selectedDepictions = repository.getSelectedDepictions();
-        if (selectedDepictions != null && !selectedDepictions.isEmpty()) {
-            view.goToNextScreen();
-        } else {
-            view.noDepictionSelected();
-        }
-    }
-
-    /**
-     * Fetch thumbnail for the Wikidata Item
-     * @param entityId entityId of the item
-     * @param position position of the item
-     */
-    @Override
-    public void fetchThumbnailForEntityId(String entityId, int position) {
-        compositeDisposable.add(depictsClient.getP18ForItem(entityId)
-                .subscribeOn(Schedulers.io())
-                .observeOn(AndroidSchedulers.mainThread())
-                .timeout(TIMEOUT_SECONDS, TimeUnit.SECONDS)
-                .subscribe(response -> {
-                    view.onImageUrlFetched(response,position);
-                }));
-    }
-}

app/src/main/java/fr/free/nrw/commons/upload/depicts/DepictsPresenter.kt

@@ -0,0 +1,152 @@
+package fr.free.nrw.commons.upload.depicts
+
+import androidx.lifecycle.LiveData
+import androidx.lifecycle.MutableLiveData
+import fr.free.nrw.commons.di.CommonsApplicationModule
+import fr.free.nrw.commons.explore.depictions.DepictsClient
+import fr.free.nrw.commons.explore.depictions.DepictsClient.NO_DEPICTED_IMAGE
+import fr.free.nrw.commons.repository.UploadRepository
+import fr.free.nrw.commons.upload.structure.depictions.DepictedItem
+import io.reactivex.Flowable
+import io.reactivex.Scheduler
+import io.reactivex.Single
+import io.reactivex.disposables.CompositeDisposable
+import io.reactivex.functions.BiFunction
+import io.reactivex.processors.PublishProcessor
+import timber.log.Timber
+import java.lang.reflect.Proxy
+import javax.inject.Inject
+import javax.inject.Named
+import javax.inject.Singleton
+
+/**
+ * presenter for DepictsFragment
+ */
+@Singleton
+class DepictsPresenter @Inject constructor(
+    private val repository: UploadRepository,
+    @param:Named(CommonsApplicationModule.IO_THREAD) private val ioScheduler: Scheduler,
+    @param:Named(CommonsApplicationModule.MAIN_THREAD) private val mainThreadScheduler: Scheduler,
+    private val depictsClient: DepictsClient
+) : DepictsContract.UserActionListener {
+
+    companion object {
+        private val DUMMY = proxy<DepictsContract.View>()
+    }
+
+    private var view = DUMMY
+    private val compositeDisposable: CompositeDisposable = CompositeDisposable()
+    private val searchTerm: PublishProcessor<String> = PublishProcessor.create()
+    private val depictedItems: MutableLiveData<List<DepictedItem>> = MutableLiveData()
+    private val idsToImageUrls = mutableMapOf<String, String>()
+
+    override fun onAttachView(view: DepictsContract.View) {
+        this.view = view
+        compositeDisposable.add(
+            searchTerm
+                .observeOn(mainThreadScheduler)
+                .doOnNext { view.showProgress(true) }
+                .switchMap(::searchResultsWithTerm)
+                .observeOn(mainThreadScheduler)
+                .subscribe(
+                    { (results, term) ->
+                        view.showProgress(false)
+                        view.showError(results.isEmpty() && term.isNotEmpty())
+                        depictedItems.value = results
+                    },
+                    { t: Throwable? ->
+                        view.showProgress(false)
+                        view.showError(true)
+                        Timber.e(t)
+                    }
+                )
+        )
+    }
+
+    private fun searchResultsWithTerm(it: String): Flowable<Pair<List<DepictedItem>, String>> {
+        return Flowable.zip(
+            searchResults(it),
+            Flowable.just(it),
+            BiFunction { results: List<DepictedItem>, term: String -> Pair(results, term) }
+        )
+    }
+
+    private fun searchResults(it: String): Flowable<List<DepictedItem>> {
+        return repository.searchAllEntities(it)
+            .subscribeOn(ioScheduler)
+            .map { repository.selectedDepictions + it }
+            .map { it.distinctBy(DepictedItem::id) }
+            .map(::addImageUrlsFromCache)
+    }
+
+    private fun addImageUrlsFromCache(depictions: List<DepictedItem>) =
+        depictions.map { item ->
+            idsToImageUrls[item.id]?.let { item.copy(imageUrl = it) } ?: item
+        }
+
+    override fun onDetachView() {
+        view = DUMMY
+        compositeDisposable.dispose()
+        idsToImageUrls.clear()
+    }
+
+    override fun onPreviousButtonClicked() {
+        view.goToPreviousScreen()
+    }
+
+    override fun onDepictItemClicked(depictedItem: DepictedItem) {
+        repository.onDepictItemClicked(depictedItem)
+    }
+
+    override fun getDepictedItems(): LiveData<List<DepictedItem>> {
+        return depictedItems;
+    }
+
+    /**
+     * asks the repository to fetch depictions for the query
+     * @param query
+     */
+    override fun searchForDepictions(query: String) {
+        searchTerm.onNext(query)
+    }
+
+    /**
+     * Check if depictions were selected
+     * from the depiction list
+     */
+    override fun verifyDepictions() {
+        if (repository.selectedDepictions.isNotEmpty()) {
+            view.goToNextScreen()
+        } else {
+            view.noDepictionSelected()
+        }
+    }
+
+    /**
+     * Fetch thumbnail for the Wikidata Item
+     * @param entityId entityId of the item
+     * @param position position of the item
+     */
+    override fun fetchThumbnailForEntityId(depictedItem: DepictedItem) {
+        compositeDisposable.add(
+            imageUrlFromNetworkOrCache(depictedItem)
+                .observeOn(mainThreadScheduler)
+                .filter { it != NO_DEPICTED_IMAGE }
+                .subscribe(
+                    { view.onUrlFetched(depictedItem, it) },
+                    { Timber.e(it) }
+                )
+        )
+    }
+
+    private fun imageUrlFromNetworkOrCache(depictedItem: DepictedItem): Single<String> =
+        if (idsToImageUrls.containsKey(depictedItem.id))
+            Single.just(idsToImageUrls[depictedItem.id])
+        else
+            depictsClient.getP18ForItem(depictedItem.id)
+                .subscribeOn(ioScheduler)
+                .doOnSuccess { idsToImageUrls[depictedItem.id] = it }
+}
+
+inline fun <reified T> proxy() = Proxy
+    .newProxyInstance(T::class.java.classLoader, arrayOf(T::class.java)) { _, _, _ -> null } as T

app/src/main/java/fr/free/nrw/commons/upload/mediaDetails/UploadMediaPresenter.java

@@ -16,7 +16,7 @@ import fr.free.nrw.commons.upload.SimilarImageInterface;
 import fr.free.nrw.commons.upload.UploadModel.UploadItem;
 import fr.free.nrw.commons.upload.mediaDetails.UploadMediaDetailsContract.UserActionListener;
 import fr.free.nrw.commons.upload.mediaDetails.UploadMediaDetailsContract.View;
-import io.reactivex.Observable;
+import io.reactivex.Maybe;
 import io.reactivex.Scheduler;
 import io.reactivex.disposables.CompositeDisposable;
 import io.reactivex.disposables.Disposable;
@@ -84,7 +84,7 @@ public class UploadMediaPresenter implements UserActionListener, SimilarImageInt
                                 checkNearbyPlaces(uploadItem);
                             }
                         },
-                        throwable -> Timber.e(throwable, "Error occurred in processing images"));
+                        throwable -> Timber.e(throwable, "Error occurred in pre-processing images"));
         compositeDisposable.add(uploadItemDisposable);
     }
 
@@ -93,14 +93,18 @@ public class UploadMediaPresenter implements UserActionListener, SimilarImageInt
      * @param uploadItem
      */
     private void checkNearbyPlaces(UploadItem uploadItem) {
-        Disposable checkNearbyPlaces = Observable.fromCallable(() -> repository
+        Disposable checkNearbyPlaces = Maybe.fromCallable(() -> repository
                 .checkNearbyPlaces(uploadItem.getGpsCoords().getDecLatitude(),
                         uploadItem.getGpsCoords().getDecLongitude()))
                 .subscribeOn(ioScheduler)
                 .observeOn(mainThreadScheduler)
-                .subscribe(place -> view.onNearbyPlaceFound(uploadItem, place),
+                .subscribe(place -> {
-                        throwable -> Timber.e(throwable, "Error occurred in processing images"));
+                        if (place != null) {
-        compositeDisposable.add(checkNearbyPlaces);
+                            view.onNearbyPlaceFound(uploadItem, place);
+                        }
+                    },
+                    throwable -> Timber.e(throwable, "Error occurred in processing images"));
+            compositeDisposable.add(checkNearbyPlaces);
     }
 
     /**

app/src/main/java/fr/free/nrw/commons/upload/structure/depictions/DepictModel.kt

@@ -2,7 +2,8 @@ package fr.free.nrw.commons.upload.structure.depictions
 
 import fr.free.nrw.commons.nearby.Place
 import fr.free.nrw.commons.upload.depicts.DepictsInterface
-import io.reactivex.Observable
+import io.reactivex.Flowable
+import io.reactivex.processors.BehaviorProcessor
 import java.util.*
 import javax.inject.Inject
 import javax.inject.Singleton
@@ -13,33 +14,32 @@ import javax.inject.Singleton
 @Singleton
 class DepictModel @Inject constructor(private val depictsInterface: DepictsInterface) {
 
-  var nearbyPlaces: MutableList<Place>? = null
+    var nearbyPlaces: BehaviorProcessor<List<Place>> = BehaviorProcessor.createDefault(emptyList())
 
-  companion object {
+    companion object {
-    private const val SEARCH_DEPICTS_LIMIT = 25
+        private const val SEARCH_DEPICTS_LIMIT = 25
-  }
-
-  /**
-   * Search for depictions
-   */
-  fun searchAllEntities(query: String): Observable<DepictedItem> {
-    if(query.isBlank()){
-      return Observable.fromIterable(nearbyPlaces?.map { DepictedItem(it) } ?: emptyList<DepictedItem>())
     }
-    return networkItems(query)
-  }
 
-  private fun networkItems(query: String): Observable<DepictedItem> {
+    /**
-    val language = Locale.getDefault().language
+     * Search for depictions
-    return depictsInterface.searchForDepicts(
+     */
-      query, "$SEARCH_DEPICTS_LIMIT", language, language, "0"
+    fun searchAllEntities(query: String): Flowable<List<DepictedItem>> {
-    )
+        if (query.isBlank()) {
-      .flatMap { Observable.fromIterable(it.search) }
+            return nearbyPlaces.map { it.map(::DepictedItem) }
-      .map(::DepictedItem)
+        }
-  }
+        return networkItems(query)
+    }
+
+    private fun networkItems(query: String): Flowable<List<DepictedItem>> {
+        val language = Locale.getDefault().language
+        return depictsInterface
+            .searchForDepicts(query, "$SEARCH_DEPICTS_LIMIT", language, language, "0")
+            .map { it.search.map(::DepictedItem) }
+            .toFlowable()
+    }
 
     fun cleanUp() {
-        nearbyPlaces = null
+        nearbyPlaces = BehaviorProcessor.createDefault(emptyList())
     }
 
 }

app/src/main/java/fr/free/nrw/commons/upload/structure/depictions/UploadDepictsCallback.java

@@ -6,5 +6,5 @@ package fr.free.nrw.commons.upload.structure.depictions;
 public interface UploadDepictsCallback {
     void depictsClicked(DepictedItem item);
 
-    void fetchThumbnailUrlForEntity(String entityId,int position);
+    void fetchThumbnailUrlForEntity(DepictedItem depictedItem);
 }

app/src/main/res/layout/layout_upload_depicts_item.xml

@@ -17,14 +17,14 @@
         app:layout_constraintLeft_toLeftOf="parent"
         app:layout_constraintTop_toTopOf="parent" />
 
-    <ImageView
+    <com.facebook.drawee.view.SimpleDraweeView
         android:id="@+id/depicted_image"
         android:layout_width="50dp"
         android:layout_height="50dp"
         android:paddingRight="@dimen/tiny_gap"
         app:layout_constraintLeft_toRightOf="@+id/depict_checkbox"
         app:layout_constraintTop_toTopOf="parent"
-        app:srcCompat="@drawable/ic_wikidata_logo_24dp"/>
+        app:placeholderImage="@drawable/ic_wikidata_logo_24dp"/>
 
     <TextView
         android:id="@+id/depicts_label"

app/src/test/kotlin/fr/free/nrw/commons/upload/DepictsPresenterTest.kt

@@ -1,22 +1,32 @@
 package fr.free.nrw.commons.upload
 
+import androidx.arch.core.executor.testing.InstantTaskExecutorRule
+import com.jraska.livedata.test
+import com.nhaarman.mockitokotlin2.never
+import com.nhaarman.mockitokotlin2.times
+import com.nhaarman.mockitokotlin2.verify
 import com.nhaarman.mockitokotlin2.whenever
-import fr.free.nrw.commons.category.CategoryItem
+import fr.free.nrw.commons.explore.depictions.DepictsClient
+import fr.free.nrw.commons.explore.depictions.DepictsClient.NO_DEPICTED_IMAGE
 import fr.free.nrw.commons.repository.UploadRepository
 import fr.free.nrw.commons.upload.depicts.DepictsContract
-import fr.free.nrw.commons.upload.depicts.DepictsFragment
 import fr.free.nrw.commons.upload.depicts.DepictsPresenter
 import fr.free.nrw.commons.upload.structure.depictions.DepictedItem
-import io.reactivex.Observable
+import io.reactivex.Flowable
+import io.reactivex.Single
 import io.reactivex.schedulers.TestScheduler
 import org.junit.Before
+import org.junit.Rule
 import org.junit.Test
-import org.mockito.ArgumentMatchers
 import org.mockito.Mock
-import org.mockito.Mockito.verify
 import org.mockito.MockitoAnnotations
 
+
 class DepictsPresenterTest {
+
+    @get:Rule
+    var testRule = InstantTaskExecutorRule()
+
     @Mock
     internal lateinit var repository: UploadRepository
 
@@ -25,15 +35,10 @@ class DepictsPresenterTest {
 
     private lateinit var depictsPresenter: DepictsPresenter
 
-    private lateinit var depictsFragment: DepictsFragment
-
     private lateinit var testScheduler: TestScheduler
 
-    private val depictedItems: ArrayList<DepictedItem> = ArrayList()
-
     @Mock
-    lateinit var depictedItem: DepictedItem
+    lateinit var depictsClient: DepictsClient
-
 
     /**
      * initial setup
@@ -43,79 +48,143 @@ class DepictsPresenterTest {
     fun setUp() {
         MockitoAnnotations.initMocks(this)
         testScheduler = TestScheduler()
-        depictedItem = DepictedItem("label", "desc", "", false, "entityId")
+        depictsPresenter = DepictsPresenter(repository, testScheduler, testScheduler, depictsClient)
-        depictedItems.add(depictedItem)
-        depictsPresenter = DepictsPresenter(repository, testScheduler, testScheduler, null)
-        depictsFragment = DepictsFragment()
         depictsPresenter.onAttachView(view)
     }
 
     @Test
-    fun searchEnglishDepictionsTest() {
+    fun `Search emission shows view progress`() {
-        whenever(repository.sortBySimilarity(ArgumentMatchers.anyString())).thenReturn(Comparator<CategoryItem> { _, _ -> 1 })
+        depictsPresenter.searchForDepictions("")
-        whenever(repository.selectedDepictions).thenReturn(depictedItems)
-        whenever(repository.searchAllEntities(ArgumentMatchers.anyString())).thenReturn(Observable.empty())
-        depictsPresenter.searchForDepictions("test")
-        verify(view).showProgress(true)
-        verify(view).setDepictsList(null)
         testScheduler.triggerActions()
         verify(view).showProgress(false)
     }
 
     @Test
-    fun searchOtherLanguageDepictions() {
+    fun `search results emission returns distinct results + selected items`() {
-        whenever(repository.sortBySimilarity(ArgumentMatchers.anyString())).thenReturn(Comparator<CategoryItem> { _, _ -> 1 })
+        val searchResults = listOf(depictedItem(), depictedItem())
-        whenever(repository.selectedDepictions).thenReturn(depictedItems)
+        whenever(repository.searchAllEntities("")).thenReturn(Flowable.just(searchResults))
-        whenever(repository.searchAllEntities(ArgumentMatchers.anyString())).thenReturn(Observable.empty())
+        val selectedItem = depictedItem(id = "selected")
-        depictsPresenter.searchForDepictions("वी")
+        whenever(repository.selectedDepictions).thenReturn(listOf(selectedItem))
-        verify(view).showProgress(true)
+        depictsPresenter.searchForDepictions("")
-        verify(view).setDepictsList(null)
         testScheduler.triggerActions()
         verify(view).showProgress(false)
+        verify(view).showError(false)
+        depictsPresenter.depictedItems
+            .test()
+            .assertValue(listOf(selectedItem, depictedItem()))
     }
 
     @Test
-    fun searchForNonExistingDepictions() {
+    fun `searchResults retrieve imageUrls from cache`() {
-        whenever(repository.sortBySimilarity(ArgumentMatchers.anyString())).thenReturn(Comparator<CategoryItem> { _, _ -> 1 })
+        val depictedItem = depictedItem()
-        whenever(repository.selectedDepictions).thenReturn(depictedItems)
+        whenever(depictsClient.getP18ForItem(depictedItem.id)).thenReturn(Single.just("url"))
-        whenever(repository.searchAllEntities(ArgumentMatchers.anyString())).thenReturn(Observable.empty())
+        depictsPresenter.fetchThumbnailForEntityId(depictedItem)
-        depictsPresenter.searchForDepictions("******")
+        testScheduler.triggerActions()
-        verify(view).showProgress(true)
+        val searchResults = listOf(depictedItem(), depictedItem())
-        verify(view).setDepictsList(null)
+        whenever(repository.searchAllEntities("")).thenReturn(Flowable.just(searchResults))
+        depictsPresenter.searchForDepictions("")
+        testScheduler.triggerActions()
+        depictsPresenter.depictedItems
+            .test()
+            .assertValue(listOf(depictedItem(imageUrl = "url")))
+    }
+
+    @Test
+    fun `empty search results with empty term do not show error`() {
+        whenever(repository.searchAllEntities("")).thenReturn(Flowable.just(emptyList()))
+        depictsPresenter.searchForDepictions("")
         testScheduler.triggerActions()
-        verify(view).setDepictsList(null)
         verify(view).showProgress(false)
+        verify(view).showError(false)
+        depictsPresenter.depictedItems
+            .test()
+            .assertValue(emptyList())
     }
 
     @Test
-    fun setSingleDepiction() {
+    fun `empty search results with non empty term do show error`() {
-        whenever(repository.sortBySimilarity(ArgumentMatchers.anyString())).thenReturn(Comparator<CategoryItem> { _, _ -> 1 })
+        whenever(repository.searchAllEntities("a")).thenReturn(Flowable.just(emptyList()))
-        whenever(repository.selectedDepictions).thenReturn(depictedItems)
+        depictsPresenter.searchForDepictions("a")
-        whenever(repository.searchAllEntities(ArgumentMatchers.anyString())).thenReturn(Observable.empty())
-        depictsPresenter.onDepictItemClicked(depictedItem)
-        depictsPresenter.verifyDepictions()
-        verify(view).goToNextScreen()
-    }
-
-    @Test
-    fun setMultipleDepictions() {
-        whenever(repository.sortBySimilarity(ArgumentMatchers.anyString())).thenReturn(Comparator<CategoryItem> { _, _ -> 1 })
-        whenever(repository.selectedDepictions).thenReturn(depictedItems)
-        whenever(repository.searchAllEntities(ArgumentMatchers.anyString())).thenReturn(Observable.empty())
-        depictsPresenter.onDepictItemClicked(depictedItem)
-        val depictedItem2 = DepictedItem("label2", "desc2", "", false, "entityid2")
-        depictsPresenter.onDepictItemClicked(depictedItem2)
-        depictsPresenter.verifyDepictions()
-        verify(view).goToNextScreen()
-    }
-
-    @Test
-    fun `on Search Exception Show Error And Stop Progress`() {
-        whenever(repository.searchAllEntities(ArgumentMatchers.anyString()))
-            .thenReturn(Observable.error(Exception()))
-        depictsPresenter.searchForDepictions("******")
         testScheduler.triggerActions()
+        verify(view).showProgress(false)
         verify(view).showError(true)
+        depictsPresenter.depictedItems
+            .test()
+            .assertValue(emptyList())
+    }
+
+    @Test
+    fun `search error shows error`() {
+        whenever(repository.searchAllEntities("")).thenReturn(Flowable.error(Exception()))
+        depictsPresenter.searchForDepictions("")
+        testScheduler.triggerActions()
         verify(view).showProgress(false)
+        verify(view).showError(true)
+    }
+
+    @Test
+    fun `onPreviousButtonClicked goes to previous screen`() {
+        depictsPresenter.onPreviousButtonClicked()
+        verify(view).goToPreviousScreen()
+    }
+
+    @Test
+    fun `onDepictItemClicked calls repository`() {
+        val depictedItem = depictedItem()
+        depictsPresenter.onDepictItemClicked(depictedItem)
+        verify(repository).onDepictItemClicked(depictedItem)
+    }
+
+    @Test
+    fun `verifyDepictions with non empty selectedDepictions goes to next screen`() {
+        whenever(repository.selectedDepictions).thenReturn(listOf(depictedItem()))
+        depictsPresenter.verifyDepictions()
+        verify(view).goToNextScreen()
+    }
+
+    @Test
+    fun `verifyDepictions with empty selectedDepictions goes to noDepictionSelected`() {
+        whenever(repository.selectedDepictions).thenReturn(emptyList())
+        depictsPresenter.verifyDepictions()
+        verify(view).noDepictionSelected()
+    }
+
+    @Test
+    fun `image urls fetched from network update the view`() {
+        val depictedItem = depictedItem()
+        whenever(depictsClient.getP18ForItem(depictedItem.id)).thenReturn(Single.just("url"))
+        depictsPresenter.fetchThumbnailForEntityId(depictedItem)
+        testScheduler.triggerActions()
+        verify(view).onUrlFetched(depictedItem, "url")
+    }
+
+    @Test
+    fun `image urls fetched from network filter NO_DEPICTED_IMAGE`() {
+        val depictedItem = depictedItem()
+        whenever(depictsClient.getP18ForItem(depictedItem.id))
+            .thenReturn(Single.just(NO_DEPICTED_IMAGE))
+        depictsPresenter.fetchThumbnailForEntityId(depictedItem)
+        testScheduler.triggerActions()
+        verify(view, never()).onUrlFetched(depictedItem, NO_DEPICTED_IMAGE)
+    }
+
+    @Test
+    fun `successive image urls fetched from cache`() {
+        val depictedItem = depictedItem()
+        whenever(depictsClient.getP18ForItem(depictedItem.id)).thenReturn(Single.just("url"))
+        depictsPresenter.fetchThumbnailForEntityId(depictedItem)
+        testScheduler.triggerActions()
+        verify(view).onUrlFetched(depictedItem, "url")
+        depictsPresenter.fetchThumbnailForEntityId(depictedItem)
+        testScheduler.triggerActions()
+        verify(view, times(2)).onUrlFetched(depictedItem, "url")
     }
 }
+
+fun depictedItem(
+    name: String = "label",
+    description: String = "desc",
+    imageUrl: String = "",
+    isSelected: Boolean = false,
+    id: String = "entityId"
+) = DepictedItem(name, description, imageUrl, isSelected, id)