From 4bd7a5b1e27ace6730d55abcf4b929bad5a15021 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Se=C3=A1n=20Mac=20Gillicuddy?= Date: Fri, 13 Mar 2020 13:47:20 +0000 Subject: [PATCH] #3493 App freezes for 15 seconds when you press Next in UploadMediaDetailsFragment (#3499) * #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 --- .../repository/UploadRemoteDataSource.java | 5 +- .../commons/repository/UploadRepository.java | 5 +- .../upload/ImageProcessingService.java | 87 +++++++++---------- .../fr/free/nrw/commons/upload/ReadFBMD.java | 64 +++++++------- .../free/nrw/commons/upload/UploadModel.java | 6 +- .../UploadMediaDetailFragment.java | 2 +- .../UploadMediaDetailsContract.java | 2 +- .../mediaDetails/UploadMediaPresenter.java | 15 ++-- .../nrw/commons/utils/ImageUtilsWrapper.java | 35 ++++---- .../upload/ImageProcessingServiceTest.kt | 22 ++--- .../upload/UploadMediaPresenterTest.kt | 6 +- .../nrw/commons/upload/UploadModelTest.kt | 4 +- 12 files changed, 118 insertions(+), 135 deletions(-) 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 index 047574a1a7..2263eb5f3e 100644 --- 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 preProcessImage(UploadableFile uploadableFile, Pla * ask the UplaodModel for the image quality of the UploadItem * * @param uploadItem - * @param shouldValidateTitle * @return */ - public Single getImageQuality(UploadItem uploadItem, boolean shouldValidateTitle) { - return uploadModel.getImageQuality(uploadItem, shouldValidateTitle); + public Single 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 index 0e84f9f379..877327c148 100644 --- 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 preProcessImage(UploadableFile uploadableFile, Pla * query the RemoteDataSource for image quality * * @param uploadItem - * @param shouldValidateTitle * @return */ - public Single getImageQuality(UploadItem uploadItem, boolean shouldValidateTitle) { - return remoteDataSource.getImageQuality(uploadItem, shouldValidateTitle); + public Single 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 index 31386d9d65..d9fd8a0346 100644 --- 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 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 duplicateImage = checkDuplicateImage(filePath); - Single wrongGeoLocation = checkImageGeoLocation(uploadItem.getPlace(), filePath); - Single darkImage = checkDarkImage(filePath); - Single itemTitle = checkTitle ? validateItemTitle(uploadItem) : Single.just(ImageUtils.IMAGE_OK); - Single checkFBMD = checkFBMD(filePath); - Single checkEXIF = checkEXIF(filePath); - - Single 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 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 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 validateItemTitle(UploadModel.UploadItem uploadItem) { */ private Single 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 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 index 99b2a5a5e1..e98ab9ec5f 100644 --- 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 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 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 index 9630d83e99..172d7a360c 100644 --- 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 preProcessImage(UploadableFile uploadableFile, return Observable.just(getUploadItem(uploadableFile, place, source, similarImageInterface)); } - public Single getImageQuality(UploadItem uploadItem, boolean checkTitle) { - return imageProcessingService.validateImage(uploadItem, checkTitle); + public Single getImageQuality(UploadItem uploadItem) { + return imageProcessingService.validateImage(uploadItem); } private UploadItem getUploadItem(UploadableFile uploadableFile, @@ -378,4 +378,4 @@ public int hashCode() { } } -} \ No newline at end of file +} 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 index 1c6140b408..8b0b8c39f7 100644 --- 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 index b442d79396..280128999d 100644 --- 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 { 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 index 3cab3c7537..a468ee15e3 100644 --- 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 index 5cfaf08ec5..bb40ee1cf4 100644 --- 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 checkIfImageIsTooDark(String bitmapPath) { - return Single.just(ImageUtils.checkIfImageIsTooDark(bitmapPath)) - .subscribeOn(Schedulers.computation()) - .observeOn(Schedulers.computation()); - } + public Single checkIfImageIsTooDark(String bitmapPath) { + return Single.fromCallable(() -> ImageUtils.checkIfImageIsTooDark(bitmapPath)) + .subscribeOn(Schedulers.computation()); + } - public Single 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 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 index cb42067cff..d64905f79f 100644 --- a/app/src/test/kotlin/fr/free/nrw/commons/upload/ImageProcessingServiceTest.kt +++ b/app/src/test/kotlin/fr/free/nrw/commons/upload/ImageProcessingServiceTest.kt @@ -88,7 +88,7 @@ class u { @Test fun validateImageForKeepImage() { `when`(uploadItem.imageQuality).thenReturn(ImageUtils.IMAGE_KEEP) - val validateImage = imageProcessingService!!.validateImage(uploadItem, false) + val validateImage = imageProcessingService!!.validateImage(uploadItem) assertEquals(ImageUtils.IMAGE_OK, validateImage.blockingGet()) } @@ -96,13 +96,13 @@ class u { fun validateImageForDuplicateImage() { `when`(mediaClient!!.checkFileExistsUsingSha(ArgumentMatchers.anyString())) .thenReturn(Single.just(true)) - val validateImage = imageProcessingService!!.validateImage(uploadItem, false) + val validateImage = imageProcessingService!!.validateImage(uploadItem) assertEquals(ImageUtils.IMAGE_DUPLICATE, validateImage.blockingGet()) } @Test fun validateImageForOkImage() { - val validateImage = imageProcessingService!!.validateImage(uploadItem, false) + val validateImage = imageProcessingService!!.validateImage(uploadItem) assertEquals(ImageUtils.IMAGE_OK, validateImage.blockingGet()) } @@ -110,7 +110,7 @@ class u { fun validateImageForDarkImage() { `when`(imageUtilsWrapper?.checkIfImageIsTooDark(ArgumentMatchers.anyString())) .thenReturn(Single.just(ImageUtils.IMAGE_DARK)) - val validateImage = imageProcessingService!!.validateImage(uploadItem, false) + val validateImage = imageProcessingService!!.validateImage(uploadItem) assertEquals(ImageUtils.IMAGE_DARK, validateImage.blockingGet()) } @@ -118,23 +118,15 @@ class u { fun validateImageForWrongGeoLocation() { `when`(imageUtilsWrapper!!.checkImageGeolocationIsDifferent(ArgumentMatchers.anyString(), any(LatLng::class.java))) .thenReturn(Single.just(ImageUtils.IMAGE_GEOLOCATION_DIFFERENT)) - val validateImage = imageProcessingService!!.validateImage(uploadItem, false) + val validateImage = imageProcessingService!!.validateImage(uploadItem) assertEquals(ImageUtils.IMAGE_GEOLOCATION_DIFFERENT, validateImage.blockingGet()) } - @Test - fun validateImageForFileNameExistsWithCheckTitleOff() { - `when`(mediaClient?.checkPageExistsUsingTitle(ArgumentMatchers.anyString())) - .thenReturn(Single.just(true)) - val validateImage = imageProcessingService!!.validateImage(uploadItem, false) - assertEquals(ImageUtils.IMAGE_OK, validateImage.blockingGet()) - } - @Test fun validateImageForFileNameExistsWithCheckTitleOn() { `when`(mediaClient?.checkPageExistsUsingTitle(ArgumentMatchers.anyString())) .thenReturn(Single.just(true)) - val validateImage = imageProcessingService!!.validateImage(uploadItem, true) + val validateImage = imageProcessingService!!.validateImage(uploadItem) assertEquals(ImageUtils.FILE_NAME_EXISTS, validateImage.blockingGet()) } -} \ No newline at end of file +} 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 index eb7b2322bd..a16af59bbb 100644 --- a/app/src/test/kotlin/fr/free/nrw/commons/upload/UploadMediaPresenterTest.kt +++ b/app/src/test/kotlin/fr/free/nrw/commons/upload/UploadMediaPresenterTest.kt @@ -82,9 +82,9 @@ class UploadMediaPresenterTest { */ @Test fun verifyImageQualityTest() { - Mockito.`when`(repository?.getImageQuality(ArgumentMatchers.any(UploadModel.UploadItem::class.java), ArgumentMatchers.any(Boolean::class.java))).thenReturn(testSingleImageResult) + Mockito.`when`(repository?.getImageQuality(ArgumentMatchers.any(UploadModel.UploadItem::class.java))).thenReturn(testSingleImageResult) Mockito.`when`(uploadItem?.imageQuality).thenReturn(ArgumentMatchers.anyInt()) - uploadMediaPresenter?.verifyImageQuality(uploadItem, true) + uploadMediaPresenter?.verifyImageQuality(uploadItem) verify(view)?.showProgress(true) testScheduler?.triggerActions() verify(view)?.showProgress(false) @@ -185,4 +185,4 @@ class UploadMediaPresenterTest { verify(repository)?.updateUploadItem(0,uploadItem) } -} \ No newline at end of file +} 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 index c6f8518897..764c0a447f 100644 --- a/app/src/test/kotlin/fr/free/nrw/commons/upload/UploadModelTest.kt +++ b/app/src/test/kotlin/fr/free/nrw/commons/upload/UploadModelTest.kt @@ -65,7 +65,7 @@ class UploadModelTest { .thenReturn(mock(FileInputStream::class.java)) `when`(fileUtilsWrapper!!.getGeolocationOfFile(anyString())) .thenReturn("") - `when`(imageProcessingService!!.validateImage(any(UploadModel.UploadItem::class.java), anyBoolean())) + `when`(imageProcessingService!!.validateImage(any(UploadModel.UploadItem::class.java))) .thenReturn(Single.just(IMAGE_OK)) } @@ -129,4 +129,4 @@ class UploadModelTest { @Test fun buildContributions() { } -} \ No newline at end of file +}