#3493 App freezes for 15 seconds when you press Next in UploadMediaDetailsFragment (#3499)

macgills · web-flow · commit 4bd7a5b1e27a · 2020-03-13T23:47:20.000+10:00
* #3493 App freezes for 15 seconds when you press Next in UploadMediaDetailsFragment - add apropriate schedulers and convert justs to fromCallable * #3493 App freezes for 15 seconds when you press Next in UploadMediaDetailsFragment - remove test for removed functionality * #3493 App freezes for 15 seconds when you press Next in UploadMediaDetailsFragment - replace kotlin with java
diff --git a/app/src/main/java/fr/free/nrw/commons/repository/UploadRemoteDataSource.java b/app/src/main/java/fr/free/nrw/commons/repository/UploadRemoteDataSource.java
@@ -179,11 +179,10 @@ public Observable<UploadItem> preProcessImage(UploadableFile uploadableFile, Pla
      * ask the UplaodModel for the image quality of the UploadItem
      *
      * @param uploadItem
-     * @param shouldValidateTitle
      * @return
      */
-    public Single<Integer> getImageQuality(UploadItem uploadItem, boolean shouldValidateTitle) {
-        return uploadModel.getImageQuality(uploadItem, shouldValidateTitle);
+    public Single<Integer> getImageQuality(UploadItem uploadItem) {
+        return uploadModel.getImageQuality(uploadItem);
     }
 
     /**
diff --git a/app/src/main/java/fr/free/nrw/commons/repository/UploadRepository.java b/app/src/main/java/fr/free/nrw/commons/repository/UploadRepository.java
@@ -187,11 +187,10 @@ public Observable<UploadItem> preProcessImage(UploadableFile uploadableFile, Pla
      * query the RemoteDataSource for image quality
      *
      * @param uploadItem
-     * @param shouldValidateTitle
      * @return
      */
-    public Single<Integer> getImageQuality(UploadItem uploadItem, boolean shouldValidateTitle) {
-        return remoteDataSource.getImageQuality(uploadItem, shouldValidateTitle);
+    public Single<Integer> getImageQuality(UploadItem uploadItem) {
+        return remoteDataSource.getImageQuality(uploadItem);
     }
 
     /**
diff --git a/app/src/main/java/fr/free/nrw/commons/upload/ImageProcessingService.java b/app/src/main/java/fr/free/nrw/commons/upload/ImageProcessingService.java
@@ -1,23 +1,21 @@
 package fr.free.nrw.commons.upload;
 
-import android.content.Context;
-
-import org.apache.commons.lang3.StringUtils;
-
-import javax.inject.Inject;
-import javax.inject.Singleton;
+import static fr.free.nrw.commons.utils.ImageUtils.EMPTY_TITLE;
+import static fr.free.nrw.commons.utils.ImageUtils.FILE_NAME_EXISTS;
+import static fr.free.nrw.commons.utils.ImageUtils.IMAGE_OK;
 
+import android.content.Context;
 import fr.free.nrw.commons.media.MediaClient;
 import fr.free.nrw.commons.nearby.Place;
 import fr.free.nrw.commons.utils.ImageUtils;
 import fr.free.nrw.commons.utils.ImageUtilsWrapper;
 import io.reactivex.Single;
+import io.reactivex.schedulers.Schedulers;
+import javax.inject.Inject;
+import javax.inject.Singleton;
+import org.apache.commons.lang3.StringUtils;
 import timber.log.Timber;
 
-import static fr.free.nrw.commons.utils.ImageUtils.EMPTY_TITLE;
-import static fr.free.nrw.commons.utils.ImageUtils.FILE_NAME_EXISTS;
-import static fr.free.nrw.commons.utils.ImageUtils.IMAGE_OK;
-
 /**
  * Methods for pre-processing images to be uploaded
  */
@@ -41,38 +39,33 @@ public ImageProcessingService(FileUtilsWrapper fileUtilsWrapper,
         this.mediaClient = mediaClient;
     }
 
-    /**
-     * Check image quality before upload
-     * - checks duplicate image
-     * - checks dark image
-     * - checks geolocation for image
-     * - check for valid title
-     */
-    Single<Integer> validateImage(UploadModel.UploadItem uploadItem, boolean checkTitle) {
-        int currentImageQuality = uploadItem.getImageQuality();
-        Timber.d("Current image quality is %d", currentImageQuality);
-        if (currentImageQuality == ImageUtils.IMAGE_KEEP) {
-            return Single.just(ImageUtils.IMAGE_OK);
-        }
-        Timber.d("Checking the validity of image");
-        String filePath = uploadItem.getMediaUri().getPath();
-        Single<Integer> duplicateImage = checkDuplicateImage(filePath);
-        Single<Integer> wrongGeoLocation = checkImageGeoLocation(uploadItem.getPlace(), filePath);
-        Single<Integer> darkImage = checkDarkImage(filePath);
-        Single<Integer> itemTitle = checkTitle ? validateItemTitle(uploadItem) : Single.just(ImageUtils.IMAGE_OK);
-        Single<Integer> checkFBMD = checkFBMD(filePath);
-        Single<Integer> checkEXIF = checkEXIF(filePath);
-
-        Single<Integer> zipResult = Single.zip(duplicateImage, wrongGeoLocation, darkImage, itemTitle,
-                (duplicate, wrongGeo, dark, title) -> {
-                    Timber.d("Result for duplicate: %d, geo: %d, dark: %d, title: %d", duplicate, wrongGeo, dark, title);
-                    return duplicate | wrongGeo | dark | title;
-                });
-        return Single.zip(zipResult, checkFBMD , checkEXIF , (zip , fbmd , exif)->{
-            Timber.d("zip:" + zip + "fbmd:" + fbmd + "exif:" + exif);
-            return zip | fbmd | exif;
-                });
+  /**
+   * Check image quality before upload - checks duplicate image - checks dark image - checks
+   * geolocation for image - check for valid title
+   */
+  Single<Integer> validateImage(UploadModel.UploadItem uploadItem) {
+    int currentImageQuality = uploadItem.getImageQuality();
+    Timber.d("Current image quality is %d", currentImageQuality);
+    if (currentImageQuality == ImageUtils.IMAGE_KEEP) {
+      return Single.just(ImageUtils.IMAGE_OK);
     }
+    Timber.d("Checking the validity of image");
+    String filePath = uploadItem.getMediaUri().getPath();
+
+    return Single.zip(
+        checkDuplicateImage(filePath),
+        checkImageGeoLocation(uploadItem.getPlace(), filePath),
+        checkDarkImage(filePath),
+        validateItemTitle(uploadItem),
+        checkFBMD(filePath),
+        checkEXIF(filePath),
+        (duplicateImage, wrongGeoLocation, darkImage, itemTitle, fbmd, exif) -> {
+          Timber.d("duplicate: %d, geo: %d, dark: %d, title: %d" + "fbmd:" + fbmd + "exif:" + exif,
+              duplicateImage, wrongGeoLocation, darkImage, itemTitle);
+          return duplicateImage | wrongGeoLocation | darkImage | itemTitle | fbmd | exif;
+        }
+    );
+  }
 
     /**
      * We want to discourage users from uploading images to Commons that were taken from Facebook.
@@ -113,7 +106,8 @@ private Single<Integer> validateItemTitle(UploadModel.UploadItem uploadItem) {
                 .map(doesFileExist -> {
                     Timber.d("Result for valid title is %s", doesFileExist);
                     return doesFileExist ? FILE_NAME_EXISTS : IMAGE_OK;
-                });
+                })
+                .subscribeOn(Schedulers.io());
     }
 
     /**
@@ -124,14 +118,14 @@ private Single<Integer> validateItemTitle(UploadModel.UploadItem uploadItem) {
      */
     private Single<Integer> checkDuplicateImage(String filePath) {
         Timber.d("Checking for duplicate image %s", filePath);
-        return Single.fromCallable(() ->
-                fileUtilsWrapper.getFileInputStream(filePath))
+        return Single.fromCallable(() -> fileUtilsWrapper.getFileInputStream(filePath))
                 .map(fileUtilsWrapper::getSHA1)
                 .flatMap(mediaClient::checkFileExistsUsingSha)
                 .map(b -> {
                     Timber.d("Result for duplicate image %s", b);
                     return b ? ImageUtils.IMAGE_DUPLICATE : ImageUtils.IMAGE_OK;
-                });
+                })
+                .subscribeOn(Schedulers.io());
     }
 
     /**
@@ -164,7 +158,8 @@ private Single<Integer> checkImageGeoLocation(Place place, String filePath) {
                         return Single.just(ImageUtils.IMAGE_OK);
                     }
                     return imageUtilsWrapper.checkImageGeolocationIsDifferent(geoLocation, place.getLocation());
-                });
+                })
+                .subscribeOn(Schedulers.io());
     }
 }
 
diff --git a/app/src/main/java/fr/free/nrw/commons/upload/ReadFBMD.java b/app/src/main/java/fr/free/nrw/commons/upload/ReadFBMD.java
@@ -1,48 +1,48 @@
 package fr.free.nrw.commons.upload;
 
+import fr.free.nrw.commons.utils.ImageUtils;
+import io.reactivex.Single;
 import java.io.FileInputStream;
 import java.io.IOException;
-
 import javax.inject.Inject;
 import javax.inject.Singleton;
 
-import fr.free.nrw.commons.utils.ImageUtils;
-import io.reactivex.Single;
-
 /**
- * We want to discourage users from uploading images to Commons that were taken from Facebook.
- * This attempts to detect whether an image was downloaded from Facebook by heuristically
- * searching for metadata that is specific to images that come from Facebook.
+ * We want to discourage users from uploading images to Commons that were taken from Facebook. This
+ * attempts to detect whether an image was downloaded from Facebook by heuristically searching for
+ * metadata that is specific to images that come from Facebook.
  */
 @Singleton
 public class ReadFBMD {
 
-    @Inject
-    public ReadFBMD() {
-    }
-
-    public Single<Integer> processMetadata(String path) {
-        try {
-            int psBlockOffset;
-            int fbmdOffset;
-
-            try (FileInputStream fs = new FileInputStream(path)) {
-                byte[] bytes = new byte[4096];
-                fs.read(bytes);
-                fs.close();
-                String fileStr = new String(bytes);
-                psBlockOffset = fileStr.indexOf("8BIM");
-                fbmdOffset = fileStr.indexOf("FBMD");
-            }
+  @Inject
+  public ReadFBMD() {
+  }
+
+  public Single<Integer> processMetadata(String path) {
+    return Single.fromCallable(() -> {
+      try {
+        int psBlockOffset;
+        int fbmdOffset;
+
+        try (FileInputStream fs = new FileInputStream(path)) {
+          byte[] bytes = new byte[4096];
+          fs.read(bytes);
+          fs.close();
+          String fileStr = new String(bytes);
+          psBlockOffset = fileStr.indexOf("8BIM");
+          fbmdOffset = fileStr.indexOf("FBMD");
+        }
 
-            if (psBlockOffset > 0 && fbmdOffset > 0
-                    && fbmdOffset > psBlockOffset && fbmdOffset - psBlockOffset < 0x80) {
-                return Single.just(ImageUtils.FILE_FBMD);
-            }
-        } catch (IOException e) {
-            e.printStackTrace();
+        if (psBlockOffset > 0 && fbmdOffset > 0
+            && fbmdOffset > psBlockOffset && fbmdOffset - psBlockOffset < 0x80) {
+          return ImageUtils.FILE_FBMD;
         }
-        return Single.just(ImageUtils.IMAGE_OK);
-    }
+      } catch (IOException e) {
+        e.printStackTrace();
+      }
+      return ImageUtils.IMAGE_OK;
+    });
+  }
 }
 
diff --git a/app/src/main/java/fr/free/nrw/commons/upload/UploadModel.java b/app/src/main/java/fr/free/nrw/commons/upload/UploadModel.java
@@ -119,8 +119,8 @@ public Observable<UploadItem> preProcessImage(UploadableFile uploadableFile,
         return Observable.just(getUploadItem(uploadableFile, place, source, similarImageInterface));
     }
 
-    public Single<Integer> getImageQuality(UploadItem uploadItem, boolean checkTitle) {
-        return imageProcessingService.validateImage(uploadItem, checkTitle);
+    public Single<Integer> getImageQuality(UploadItem uploadItem) {
+        return imageProcessingService.validateImage(uploadItem);
     }
 
     private UploadItem getUploadItem(UploadableFile uploadableFile,
@@ -378,4 +378,4 @@ public int hashCode() {
         }
     }
 
-}
+}
diff --git a/app/src/main/java/fr/free/nrw/commons/upload/mediaDetails/UploadMediaDetailFragment.java b/app/src/main/java/fr/free/nrw/commons/upload/mediaDetails/UploadMediaDetailFragment.java
@@ -251,7 +251,7 @@ private void showInfoAlert(int titleStringID, int messageStringId) {
     @OnClick(R.id.btn_next)
     public void onNextButtonClicked() {
         uploadItem.setDescriptions(descriptionsAdapter.getDescriptions());
-        presenter.verifyImageQuality(uploadItem, true);
+        presenter.verifyImageQuality(uploadItem);
     }
 
     @OnClick(R.id.btn_previous)
diff --git a/app/src/main/java/fr/free/nrw/commons/upload/mediaDetails/UploadMediaDetailsContract.java b/app/src/main/java/fr/free/nrw/commons/upload/mediaDetails/UploadMediaDetailsContract.java
@@ -43,7 +43,7 @@ interface UserActionListener extends BasePresenter<View> {
         void receiveImage(UploadableFile uploadableFile, @Contribution.FileSource String source,
                 Place place);
 
-        void verifyImageQuality(UploadItem uploadItem, boolean validateTitle);
+        void verifyImageQuality(UploadItem uploadItem);
 
         void setUploadItem(int index, UploadItem uploadItem);
 
diff --git a/app/src/main/java/fr/free/nrw/commons/upload/mediaDetails/UploadMediaPresenter.java b/app/src/main/java/fr/free/nrw/commons/upload/mediaDetails/UploadMediaPresenter.java
@@ -111,14 +111,14 @@ private void checkNearbyPlaces(UploadItem uploadItem) {
      * asks the repository to verify image quality
      *
      * @param uploadItem
-     * @param validateTitle
      */
     @Override
-    public void verifyImageQuality(UploadItem uploadItem, boolean validateTitle) {
+    public void verifyImageQuality(UploadItem uploadItem) {
         view.showProgress(true);
-        Disposable imageQualityDisposable = repository
-                .getImageQuality(uploadItem, true)
-                .subscribeOn(ioScheduler)
+
+        compositeDisposable.add(
+            repository
+                .getImageQuality(uploadItem)
                 .observeOn(mainThreadScheduler)
                 .subscribe(imageResult -> {
                             view.showProgress(false);
@@ -129,9 +129,8 @@ public void verifyImageQuality(UploadItem uploadItem, boolean validateTitle) {
                             view.showMessage("" + throwable.getLocalizedMessage(),
                                     R.color.color_error);
                             Timber.e(throwable, "Error occurred while handling image");
-                        });
-
-        compositeDisposable.add(imageQualityDisposable);
+                        })
+        );
     }
 
     /**
diff --git a/app/src/main/java/fr/free/nrw/commons/utils/ImageUtilsWrapper.java b/app/src/main/java/fr/free/nrw/commons/utils/ImageUtilsWrapper.java
@@ -1,31 +1,30 @@
 package fr.free.nrw.commons.utils;
 
-import javax.inject.Inject;
-import javax.inject.Singleton;
-
 import fr.free.nrw.commons.location.LatLng;
 import io.reactivex.Single;
 import io.reactivex.schedulers.Schedulers;
+import javax.inject.Inject;
+import javax.inject.Singleton;
 
 @Singleton
 public class ImageUtilsWrapper {
 
-    @Inject
-    public ImageUtilsWrapper() {
+  @Inject
+  public ImageUtilsWrapper() {
 
-    }
+  }
 
-    public Single<Integer> checkIfImageIsTooDark(String bitmapPath) {
-        return Single.just(ImageUtils.checkIfImageIsTooDark(bitmapPath))
-                .subscribeOn(Schedulers.computation())
-                .observeOn(Schedulers.computation());
-    }
+  public Single<Integer> checkIfImageIsTooDark(String bitmapPath) {
+    return Single.fromCallable(() -> ImageUtils.checkIfImageIsTooDark(bitmapPath))
+        .subscribeOn(Schedulers.computation());
+  }
 
-    public Single<Integer> checkImageGeolocationIsDifferent(String geolocationOfFileString, LatLng latLng) {
-        boolean isImageGeoLocationDifferent = ImageUtils.checkImageGeolocationIsDifferent(geolocationOfFileString, latLng);
-        return Single.just(isImageGeoLocationDifferent)
-                .subscribeOn(Schedulers.computation())
-                .observeOn(Schedulers.computation())
-                .map(isDifferent -> isDifferent ? ImageUtils.IMAGE_GEOLOCATION_DIFFERENT : ImageUtils.IMAGE_OK);
-    }
+  public Single<Integer> checkImageGeolocationIsDifferent(String geolocationOfFileString,
+      LatLng latLng) {
+    return Single.fromCallable(
+        () -> ImageUtils.checkImageGeolocationIsDifferent(geolocationOfFileString, latLng))
+        .subscribeOn(Schedulers.computation())
+        .map(isDifferent -> isDifferent ? ImageUtils.IMAGE_GEOLOCATION_DIFFERENT
+            : ImageUtils.IMAGE_OK);
+  }
 }
diff --git a/app/src/test/kotlin/fr/free/nrw/commons/upload/ImageProcessingServiceTest.kt b/app/src/test/kotlin/fr/free/nrw/commons/upload/ImageProcessingServiceTest.kt
diff --git a/app/src/test/kotlin/fr/free/nrw/commons/upload/UploadMediaPresenterTest.kt b/app/src/test/kotlin/fr/free/nrw/commons/upload/UploadMediaPresenterTest.kt
diff --git a/app/src/test/kotlin/fr/free/nrw/commons/upload/UploadModelTest.kt b/app/src/test/kotlin/fr/free/nrw/commons/upload/UploadModelTest.kt

Original file line number	Diff line number	Diff line change
`@@ -179,11 +179,10 @@ public Observable<UploadItem> preProcessImage(UploadableFile uploadableFile, Pla`
`179`	`179`	`* ask the UplaodModel for the image quality of the UploadItem`
`180`	`180`	`*`
`181`	`181`	`* @param uploadItem`
`182`		`- * @param shouldValidateTitle`
`183`	`182`	`* @return`
`184`	`183`	`*/`
`185`		`- public Single<Integer> getImageQuality(UploadItem uploadItem, boolean shouldValidateTitle) {`
`186`		`- return uploadModel.getImageQuality(uploadItem, shouldValidateTitle);`
	`184`	`+ public Single<Integer> getImageQuality(UploadItem uploadItem) {`
	`185`	`+ return uploadModel.getImageQuality(uploadItem);`
`187`	`186`	`}`
`188`	`187`
`189`	`188`	`/**`
Original file line number	Diff line number	Diff line change
`@@ -187,11 +187,10 @@ public Observable<UploadItem> preProcessImage(UploadableFile uploadableFile, Pla`
`187`	`187`	`* query the RemoteDataSource for image quality`
`188`	`188`	`*`
`189`	`189`	`* @param uploadItem`
`190`		`- * @param shouldValidateTitle`
`191`	`190`	`* @return`
`192`	`191`	`*/`
`193`		`- public Single<Integer> getImageQuality(UploadItem uploadItem, boolean shouldValidateTitle) {`
`194`		`- return remoteDataSource.getImageQuality(uploadItem, shouldValidateTitle);`
	`192`	`+ public Single<Integer> getImageQuality(UploadItem uploadItem) {`
	`193`	`+ return remoteDataSource.getImageQuality(uploadItem);`
`195`	`194`	`}`
`196`	`195`
`197`	`196`	`/**`
Original file line number	Diff line number	Diff line change
`@@ -119,8 +119,8 @@ public Observable<UploadItem> preProcessImage(UploadableFile uploadableFile,`
`119`	`119`	`return Observable.just(getUploadItem(uploadableFile, place, source, similarImageInterface));`
`120`	`120`	`}`
`121`	`121`
`122`		`- public Single<Integer> getImageQuality(UploadItem uploadItem, boolean checkTitle) {`
`123`		`- return imageProcessingService.validateImage(uploadItem, checkTitle);`
	`122`	`+ public Single<Integer> getImageQuality(UploadItem uploadItem) {`
	`123`	`+ return imageProcessingService.validateImage(uploadItem);`
`124`	`124`	`}`
`125`	`125`
`126`	`126`	`private UploadItem getUploadItem(UploadableFile uploadableFile,`
`@@ -378,4 +378,4 @@ public int hashCode() {`
`378`	`378`	`}`
`379`	`379`	`}`
`380`	`380`
`381`		`-}`
	`381`	`+}`
Original file line number	Diff line number	Diff line change
`@@ -251,7 +251,7 @@ private void showInfoAlert(int titleStringID, int messageStringId) {`
`251`	`251`	`@OnClick(R.id.btn_next)`
`252`	`252`	`public void onNextButtonClicked() {`
`253`	`253`	`uploadItem.setDescriptions(descriptionsAdapter.getDescriptions());`
`254`		`- presenter.verifyImageQuality(uploadItem, true);`
	`254`	`+ presenter.verifyImageQuality(uploadItem);`
`255`	`255`	`}`
`256`	`256`
`257`	`257`	`@OnClick(R.id.btn_previous)`