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

Author: macgills

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) {
@@ -82,15 +82,13 @@ public Single<String> getP18ForItem(String entityId) {
             .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 void render() {
 
         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/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,5 +1,7 @@
 package fr.free.nrw.commons.upload;
 
+import static fr.free.nrw.commons.explore.depictions.DepictsClient.NO_DEPICTED_IMAGE;
+
 import android.net.Uri;
 import android.text.TextUtils;
 import android.view.LayoutInflater;
@@ -29,7 +31,6 @@ public class UploadDepictsRenderer extends Renderer<DepictedItem> {
     @BindView(R.id.description) TextView description;
     @BindView(R.id.depicted_image)
     SimpleDraweeView imageView;
-    private final static String NO_IMAGE_FOR_DEPICTION="No Image for Depiction";
 
     public UploadDepictsRenderer(UploadDepictsCallback listener) {
         this.listener = listener;
@@ -77,13 +78,15 @@ public void render() {
         checkedView.setChecked(item.isSelected());
         depictsLabel.setText(item.getName());
         description.setText(item.getDescription());
-        if (!TextUtils.isEmpty(item.getImageUrl())) {
-          if (!item.getImageUrl().equals(NO_IMAGE_FOR_DEPICTION)) {
-            imageView.setImageURI(Uri.parse(item.getImageUrl()));
-          }
+        final String imageUrl = item.getImageUrl();
+        final boolean imageUrlIsEmpty = TextUtils.isEmpty(imageUrl);
+        if (imageUrlIsEmpty) {
+            listener.fetchThumbnailUrlForEntity(item);
+        }
+        if (TextUtils.isEmpty(imageUrl) || imageUrl.equals(NO_DEPICTED_IMAGE)) {
+            imageView.setImageURI(UriUtil.getUriForResourceId(R.drawable.ic_wikidata_logo_24dp));
         } else {
-          imageView.setImageURI(UriUtil.getUriForResourceId(R.drawable.ic_wikidata_logo_24dp));
-          listener.fetchThumbnailUrlForEntity(item);
+            imageView.setImageURI(Uri.parse(imageUrl));
         }
     }
 

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

@@ -1,10 +1,10 @@
 package fr.free.nrw.commons.upload.depicts;
 
 import androidx.lifecycle.LiveData;
-import java.util.List;
-
 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
@@ -42,10 +42,9 @@ interface View {
          */
         void setDepictsList(List<DepictedItem> depictedItemList);
 
-        /**
-         * Set thumbnail image for depicted item
-         */
-        void onImageUrlFetched(String response, int position);
+
+      void updateUrlInAdapter(@NotNull DepictedItem depictedItem,
+          @NotNull String response);
     }
 
     interface UserActionListener extends BasePresenter<View> {

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

@@ -1,6 +1,8 @@
 package fr.free.nrw.commons.upload.depicts;
 
 import android.os.Bundle;
+import android.text.Editable;
+import android.util.Pair;
 import android.view.LayoutInflater;
 import android.view.View;
 import android.view.ViewGroup;
@@ -30,6 +32,7 @@
 import java.util.List;
 import java.util.concurrent.TimeUnit;
 import javax.inject.Inject;
+import org.jetbrains.annotations.NotNull;
 import timber.log.Timber;
 
 
@@ -142,13 +145,24 @@ public void setDepictsList(List<DepictedItem> depictedItemList) {
         }
     }
 
-    /**
-     * Set thumbnail image for depicted item
-     */
     @Override
-    public void onImageUrlFetched(String response, int position) {
-        adapter.getItem(position).setImageUrl(response);
-        adapter.notifyItemChanged(position);
+    public void updateUrlInAdapter(@NotNull DepictedItem depictedItem, @NotNull String response) {
+        final Pair<DepictedItem, Integer> itemAndPosition = returnItemAndPosition(depictedItem);
+        if (itemAndPosition != null) {
+            itemAndPosition.first.setImageUrl(response);
+            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)

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

@@ -6,10 +6,12 @@ import fr.free.nrw.commons.di.CommonsApplicationModule
 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.Flowable
 import io.reactivex.Scheduler
 import io.reactivex.Single
 import io.reactivex.android.schedulers.AndroidSchedulers
 import io.reactivex.disposables.CompositeDisposable
+import io.reactivex.functions.BiFunction
 import io.reactivex.processors.PublishProcessor
 import io.reactivex.schedulers.Schedulers
 import timber.log.Timber
@@ -36,24 +38,23 @@ class DepictsPresenter @Inject constructor(
     private var view = DUMMY
     private val compositeDisposable: CompositeDisposable = CompositeDisposable()
     private val searchTerm: PublishProcessor<String> = PublishProcessor.create()
-    val depictedItems: MutableLiveData<List<DepictedItem>> = MutableLiveData()
+    private val depictedItems: MutableLiveData<List<DepictedItem>> = MutableLiveData()
     private val idsToImageUrls = mutableMapOf<String, String>()
 
-    init {
+
+    override fun onAttachView(view: DepictsContract.View) {
+        this.view = view
         compositeDisposable.add(
             searchTerm
                 .observeOn(mainThreadScheduler)
                 .doOnNext { view.showProgress(true) }
-                .observeOn(ioScheduler)
-                .switchMap(repository::searchAllEntities)
-                .map { it.distinct() }
-                .map(::addImageUrlsFromCache)
+                .switchMap(::searchResultsWithTerm)
                 .observeOn(mainThreadScheduler)
                 .subscribe(
-                    {
+                    { (results, term) ->
                         view.showProgress(false)
-                        view.showError(it.isEmpty())
-                        depictedItems.value = it
+                        view.showError(results.isEmpty() && term.isNotEmpty())
+                        depictedItems.value = results
                     },
                     { t: Throwable? ->
                         view.showProgress(false)
@@ -64,19 +65,36 @@ class DepictsPresenter @Inject constructor(
         )
     }
 
+    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.getOrElse(item.id, { null })
                 ?.let { item.copy(imageUrl = it) }
                 ?: item
         }
 
-    override fun onAttachView(view: DepictsContract.View) {
-        this.view = view
-    }
-
     override fun onDetachView() {
         view = DUMMY
+        compositeDisposable.dispose()
+        idsToImageUrls.clear()
+
     }
 
     override fun onPreviousButtonClicked() {
@@ -106,7 +124,7 @@ class DepictsPresenter @Inject constructor(
     override fun verifyDepictions() {
         val selectedDepictions =
             repository.selectedDepictions
-        if (selectedDepictions != null && !selectedDepictions.isEmpty()) {
+        if (selectedDepictions != null && selectedDepictions.isNotEmpty()) {
             view.goToNextScreen()
         } else {
             view.noDepictionSelected()
@@ -122,19 +140,14 @@ class DepictsPresenter @Inject constructor(
         compositeDisposable.add(
             imageUrlFromNetworkOrCache(depictedItem)
                 .observeOn(AndroidSchedulers.mainThread())
-                .subscribe { response: String? ->
-                    if (response != null) {
-                        depictedItems.value =
-                            depictedItems.value!!.map {
-                                if (it.id == depictedItem.id) it.copy(imageUrl = response)
-                                else it
-                            }
-                    }
-                }
+                .subscribe(
+                    { view.updateUrlInAdapter(depictedItem, it) },
+                    { Timber.e(it) }
+                )
         )
     }
 
-    private fun imageUrlFromNetworkOrCache(depictedItem: DepictedItem) =
+    private fun imageUrlFromNetworkOrCache(depictedItem: DepictedItem): Single<String> =
         if (idsToImageUrls.containsKey(depictedItem.imageUrl))
             Single.just(idsToImageUrls[depictedItem.id])
         else

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

@@ -3,6 +3,7 @@ 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.Flowable
+import io.reactivex.processors.BehaviorProcessor
 import java.util.*
 import javax.inject.Inject
 import javax.inject.Singleton
@@ -13,7 +14,7 @@ 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 {
         private const val SEARCH_DEPICTS_LIMIT = 25
@@ -24,7 +25,7 @@ class DepictModel @Inject constructor(private val depictsInterface: DepictsInter
      */
     fun searchAllEntities(query: String): Flowable<List<DepictedItem>> {
         if (query.isBlank()) {
-            return Flowable.just(nearbyPlaces?.map { DepictedItem(it) } ?: emptyList())
+            return nearbyPlaces.map { it.map(::DepictedItem) }
         }
         return networkItems(query)
     }
@@ -38,7 +39,7 @@ class DepictModel @Inject constructor(private val depictsInterface: DepictsInter
     }
 
     fun cleanUp() {
-        nearbyPlaces = null
+        nearbyPlaces = BehaviorProcessor.createDefault(emptyList())
     }
 
 }

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

@@ -24,8 +24,7 @@
         android:paddingRight="@dimen/tiny_gap"
         app:layout_constraintLeft_toRightOf="@+id/depict_checkbox"
         app:layout_constraintTop_toTopOf="parent"
-        app:placeholderImage="@drawable/ic_wikidata_logo_24dp"
-        app:srcCompat="@drawable/ic_wikidata_logo_24dp"/>
+        app:placeholderImage="@drawable/ic_wikidata_logo_24dp"/>
 
     <TextView
         android:id="@+id/depicts_label"