Skip to content

Commit

Permalink
Fixes commons-app#4704: Remove 'Please Wait' dialog and do task in ba…
Browse files Browse the repository at this point in the history 
…ckground (commons-app#5570)

* Initial changes to the flow, merged conflicts

* Major changes to flow and logic

* Final major changes to the flow and merged conflicts

* Minor changes to thumbnail flow and merge conflicts

* Fixed ImageProcessingServiceTest

* Removed unnecessary file

* Some code cleanup and fixed UploadRepositoryUnitTest

* Minor javadoc changes and null checks

* Fixed UMDFragmentUnitTest

* Fixed and added new tests in UploadMediaPresenterTest

* Optimised code for no connection cases and minor code cleanup

* Minor bug fix

* Fixed minor bug

* Fixed a failing unit test

* Removed values-yue-hant

* Update UploadRepository.java

---------

Co-authored-by: Nicolas Raoul <[email protected]>
  • Loading branch information
@ShashwatKedia @nicolas-raoul
ShashwatKedia and nicolas-raoul authored Mar 27, 2024
1 parent 16960a3 commit 4264164
Show file tree
Hide file tree
Showing 13 changed files with 586 additions and 264 deletions.
14 changes: 12 additions & 2 deletions app/src/main/java/fr/free/nrw/commons/repository/UploadRepository.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -196,13 +196,23 @@ public Observable<UploadItem> preProcessImage(UploadableFile uploadableFile, Pla
/**
* query the RemoteDataSource for image quality
*
* @param uploadItem
* @return
* @param uploadItem UploadItem whose caption is to be checked
* @return Quality of UploadItem
*/
public Single<Integer> getImageQuality(UploadItem uploadItem, LatLng location) {
return uploadModel.getImageQuality(uploadItem, location);
}

/**
* query the RemoteDataSource for caption quality
*
* @param uploadItem UploadItem whose caption is to be checked
* @return Quality of caption of the UploadItem
*/
public Single<Integer> getCaptionQuality(UploadItem uploadItem) {
return uploadModel.getCaptionQuality(uploadItem);
}

/**
* asks the LocalDataSource to delete the file with the given file path
*
Expand Down
45 changes: 33 additions & 12 deletions app/src/main/java/fr/free/nrw/commons/upload/ImageProcessingService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,14 @@

import static fr.free.nrw.commons.utils.ImageUtils.EMPTY_CAPTION;
import static fr.free.nrw.commons.utils.ImageUtils.FILE_NAME_EXISTS;
import static fr.free.nrw.commons.utils.ImageUtils.IMAGE_DUPLICATE;
import static fr.free.nrw.commons.utils.ImageUtils.IMAGE_KEEP;
import static fr.free.nrw.commons.utils.ImageUtils.IMAGE_OK;

import android.content.Context;
import fr.free.nrw.commons.location.LatLng;
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;
Expand Down Expand Up @@ -45,13 +46,17 @@ public ImageProcessingService(FileUtilsWrapper fileUtilsWrapper,

/**
* Check image quality before upload - checks duplicate image - checks dark image - checks
* geolocation for image - check for valid title
* geolocation for image
*
* @param uploadItem UploadItem whose quality is to be checked
* @param inAppPictureLocation In app picture location (if any)
* @return Quality of UploadItem
*/
Single<Integer> validateImage(UploadItem uploadItem, LatLng inAppPictureLocation) {
int currentImageQuality = uploadItem.getImageQuality();
Timber.d("Current image quality is %d", currentImageQuality);
if (currentImageQuality == ImageUtils.IMAGE_KEEP) {
return Single.just(ImageUtils.IMAGE_OK);
if (currentImageQuality == IMAGE_KEEP || currentImageQuality == IMAGE_OK) {
return Single.just(IMAGE_OK);
}
Timber.d("Checking the validity of image");
String filePath = uploadItem.getMediaUri().getPath();
Expand All @@ -60,18 +65,34 @@ Single<Integer> validateImage(UploadItem uploadItem, LatLng inAppPictureLocation
checkDuplicateImage(filePath),
checkImageGeoLocation(uploadItem.getPlace(), filePath, inAppPictureLocation),
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:"
(duplicateImage, wrongGeoLocation, darkImage, fbmd, exif) -> {
Timber.d("duplicate: %d, geo: %d, dark: %d" + "fbmd:" + fbmd + "exif:"
+ exif,
duplicateImage, wrongGeoLocation, darkImage, itemTitle);
return duplicateImage | wrongGeoLocation | darkImage | itemTitle | fbmd | exif;
duplicateImage, wrongGeoLocation, darkImage);
return duplicateImage | wrongGeoLocation | darkImage | fbmd | exif;
}
);
}

/**
* Checks caption of the given UploadItem
*
* @param uploadItem UploadItem whose caption is to be verified
* @return Quality of caption of the UploadItem
*/
Single<Integer> validateCaption(UploadItem uploadItem) {
int currentImageQuality = uploadItem.getImageQuality();
Timber.d("Current image quality is %d", currentImageQuality);
if (currentImageQuality == IMAGE_KEEP) {
return Single.just(IMAGE_OK);
}
Timber.d("Checking the validity of caption");

return validateItemTitle(uploadItem);
}

/**
* 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
Expand Down Expand Up @@ -126,7 +147,7 @@ private Single<Integer> checkDuplicateImage(String filePath) {
.flatMap(mediaClient::checkFileExistsUsingSha)
.map(b -> {
Timber.d("Result for duplicate image %s", b);
return b ? ImageUtils.IMAGE_DUPLICATE : ImageUtils.IMAGE_OK;
return b ? IMAGE_DUPLICATE : IMAGE_OK;
})
.subscribeOn(Schedulers.io());
}
Expand All @@ -152,13 +173,13 @@ private Single<Integer> checkDarkImage(String filePath) {
private Single<Integer> checkImageGeoLocation(Place place, String filePath, LatLng inAppPictureLocation) {
Timber.d("Checking for image geolocation %s", filePath);
if (place == null || StringUtils.isBlank(place.getWikiDataEntityId())) {
return Single.just(ImageUtils.IMAGE_OK);
return Single.just(IMAGE_OK);
}
return Single.fromCallable(() -> filePath)
.flatMap(path -> Single.just(fileUtilsWrapper.getGeolocationOfFile(path, inAppPictureLocation)))
.flatMap(geoLocation -> {
if (StringUtils.isBlank(geoLocation)) {
return Single.just(ImageUtils.IMAGE_OK);
return Single.just(IMAGE_OK);
}
return imageUtilsWrapper
.checkImageGeolocationIsDifferent(geoLocation, place.getLocation());
Expand Down
38 changes: 34 additions & 4 deletions app/src/main/java/fr/free/nrw/commons/upload/UploadActivity.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
import fr.free.nrw.commons.upload.license.MediaLicenseFragment;
import fr.free.nrw.commons.upload.mediaDetails.UploadMediaDetailFragment;
import fr.free.nrw.commons.upload.mediaDetails.UploadMediaDetailFragment.UploadMediaDetailFragmentCallback;
import fr.free.nrw.commons.upload.mediaDetails.UploadMediaPresenter;
import fr.free.nrw.commons.upload.worker.WorkRequestHelper;
import fr.free.nrw.commons.utils.DialogUtil;
import fr.free.nrw.commons.utils.PermissionUtils;
Expand Down Expand Up @@ -96,7 +97,7 @@ public class UploadActivity extends BaseActivity implements UploadContract.View,
private DepictsFragment depictsFragment;
private MediaLicenseFragment mediaLicenseFragment;
private ThumbnailsAdapter thumbnailsAdapter;

BasicKvStore store;
private Place place;
private LatLng prevLocation;
private LatLng currLocation;
Expand Down Expand Up @@ -139,6 +140,9 @@ public class UploadActivity extends BaseActivity implements UploadContract.View,
* Whether fragments have been saved.
*/
private boolean isFragmentsSaved = false;

public static final String keyForCurrentUploadImagesSize = "CurrentUploadImagesSize";
public static final String storeNameForCurrentUploadImagesSize = "CurrentUploadImageQualities";

private ActivityUploadBinding binding;

Expand Down Expand Up @@ -179,7 +183,10 @@ protected void onCreate(Bundle savedInstanceState) {
}
locationManager.requestLocationUpdatesFromProvider(LocationManager.GPS_PROVIDER);
locationManager.requestLocationUpdatesFromProvider(LocationManager.NETWORK_PROVIDER);
store = new BasicKvStore(this, storeNameForCurrentUploadImagesSize);
store.clearAll();
checkStoragePermissions();

}

private void init() {
Expand Down Expand Up @@ -470,6 +477,8 @@ private void receiveSharedItems() {
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) {
if (uploadableFiles.size() > 3
&& !defaultKvStore.getBoolean("hasAlreadyLaunchedBigMultiupload")) {
// When battery-optimisation dialog is shown don't show the image quality dialog
UploadMediaPresenter.isBatteryDialogShowing = true;
DialogUtil.showAlertDialog(
this,
getString(R.string.unrestricted_battery_mode),
Expand All @@ -490,8 +499,15 @@ private void receiveSharedItems() {
Intent batteryOptimisationSettingsIntent = new Intent(
Settings.ACTION_IGNORE_BATTERY_OPTIMIZATION_SETTINGS);
startActivity(batteryOptimisationSettingsIntent);
// calling checkImageQuality after battery dialog is interacted with
// so that 2 dialogs do not pop up simultaneously
presenter.checkImageQuality(0);
UploadMediaPresenter.isBatteryDialogShowing = false;
},
() -> {}
() -> {
presenter.checkImageQuality(0);
UploadMediaPresenter.isBatteryDialogShowing = false;
}
);
defaultKvStore.putBoolean("hasAlreadyLaunchedBigMultiupload", true);
}
Expand Down Expand Up @@ -525,6 +541,8 @@ private void receiveSharedItems() {
UploadMediaDetailFragmentCallback uploadMediaDetailFragmentCallback = new UploadMediaDetailFragmentCallback() {
@Override
public void deletePictureAtIndex(int index) {
store.putInt(keyForCurrentUploadImagesSize,
(store.getInt(keyForCurrentUploadImagesSize) - 1));
presenter.deletePictureAtIndex(index);
}

Expand Down Expand Up @@ -668,6 +686,8 @@ public boolean isWLMUpload() {
binding.vpUpload.setOffscreenPageLimit(fragments.size());

}
// Saving size of uploadableFiles
store.putInt(keyForCurrentUploadImagesSize, uploadableFiles.size());
}

/**
Expand Down Expand Up @@ -787,7 +807,11 @@ public void onNextButtonClicked(int index) {
binding.vpUpload.setCurrentItem(index + 1, false);
fragments.get(index + 1).onBecameVisible();
((LinearLayoutManager) binding.rvThumbnails.getLayoutManager())
.scrollToPositionWithOffset((index > 0) ? index-1 : 0, 0);
.scrollToPositionWithOffset((index > 0) ? index - 1 : 0, 0);
if (index < fragments.size() - 4) {
// check image quality if next image exists
presenter.checkImageQuality(index + 1);
}
} else {
presenter.handleSubmit();
}
Expand All @@ -800,7 +824,11 @@ public void onPreviousButtonClicked(int index) {
fragments.get(index - 1).onBecameVisible();
((LinearLayoutManager) binding.rvThumbnails.getLayoutManager())
.scrollToPositionWithOffset((index > 3) ? index-2 : 0, 0);
binding.llContainerTopCard.setVisibility(View.VISIBLE);
if ((index != 1) && ((index - 1) < uploadableFiles.size())) {
// Shows the top card if it was hidden because of the last image being deleted and
// now the user has hit previous button to go back to the media details
showHideTopCard(true);
}
}
}

Expand Down Expand Up @@ -854,6 +882,8 @@ public void onRlContainerTitleClicked() {
@Override
protected void onDestroy() {
super.onDestroy();
// Resetting all values in store by clearing them
store.clearAll();
presenter.onDetachView();
compositeDisposable.clear();
fragments = null;
Expand Down
7 changes: 7 additions & 0 deletions app/src/main/java/fr/free/nrw/commons/upload/UploadContract.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,5 +64,12 @@ public interface UserActionListener extends BasePresenter<View> {
void handleSubmit();

void deletePictureAtIndex(int index);

/**
* Calls checkImageQuality of UploadMediaPresenter to check image quality of next image
*
* @param uploadItemIndex Index of next image, whose quality is to be checked
*/
void checkImageQuality(int uploadItemIndex);
}
}
17 changes: 17 additions & 0 deletions app/src/main/java/fr/free/nrw/commons/upload/UploadModel.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,10 +92,27 @@ public Observable<UploadItem> preProcessImage(final UploadableFile uploadableFil
createAndAddUploadItem(uploadableFile, place, similarImageInterface, inAppPictureLocation));
}

/**
* Calls validateImage() of ImageProcessingService to check quality of image
*
* @param uploadItem UploadItem whose quality is to be checked
* @param inAppPictureLocation In app picture location (if any)
* @return Quality of UploadItem
*/
public Single<Integer> getImageQuality(final UploadItem uploadItem, LatLng inAppPictureLocation) {
return imageProcessingService.validateImage(uploadItem, inAppPictureLocation);
}

/**
* Calls validateCaption() of ImageProcessingService to check caption of image
*
* @param uploadItem UploadItem whose caption is to be checked
* @return Quality of caption of the UploadItem
*/
public Single<Integer> getCaptionQuality(final UploadItem uploadItem) {
return imageProcessingService.validateCaption(uploadItem);
}

private UploadItem createAndAddUploadItem(final UploadableFile uploadableFile,
final Place place,
final SimilarImageInterface similarImageInterface,
Expand Down
28 changes: 28 additions & 0 deletions app/src/main/java/fr/free/nrw/commons/upload/UploadPresenter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import fr.free.nrw.commons.filepicker.UploadableFile;
import fr.free.nrw.commons.kvstore.JsonKvStore;
import fr.free.nrw.commons.repository.UploadRepository;
import fr.free.nrw.commons.upload.mediaDetails.UploadMediaDetailsContract;
import io.reactivex.Observer;
import io.reactivex.disposables.CompositeDisposable;
import io.reactivex.disposables.Disposable;
Expand All @@ -30,6 +31,8 @@ public class UploadPresenter implements UploadContract.UserActionListener {
private final UploadRepository repository;
private final JsonKvStore defaultKvStore;
private UploadContract.View view = DUMMY;
@Inject
UploadMediaDetailsContract.UserActionListener presenter;

private CompositeDisposable compositeDisposable;
public static final String COUNTER_OF_CONSECUTIVE_UPLOADS_WITHOUT_COORDINATES
Expand Down Expand Up @@ -135,17 +138,42 @@ public void onComplete() {
}
}

/**
* Calls checkImageQuality of UploadMediaPresenter to check image quality of next image
*
* @param uploadItemIndex Index of next image, whose quality is to be checked
*/
@Override
public void checkImageQuality(int uploadItemIndex) {
UploadItem uploadItem = repository.getUploadItem(uploadItemIndex);
presenter.checkImageQuality(uploadItem, uploadItemIndex);
}


@Override
public void deletePictureAtIndex(int index) {
List<UploadableFile> uploadableFiles = view.getUploadableFiles();
if (index == uploadableFiles.size() - 1) {
// If the next fragment to be shown is not one of the MediaDetailsFragment
// lets hide the top card so that it doesn't appear on the other fragments
view.showHideTopCard(false);
}
view.setImageCancelled(true);
repository.deletePicture(uploadableFiles.get(index).getFilePath());
if (uploadableFiles.size() == 1) {
view.showMessage(R.string.upload_cancelled);
view.finish();
return;
} else {
if (presenter != null) {
presenter.updateImageQualitiesJSON(uploadableFiles.size(), index);
}
view.onUploadMediaDeleted(index);
if (!(index == uploadableFiles.size()) && index != 0) {
// if the deleted image was not the last item to be uploaded, check quality of next
UploadItem uploadItem = repository.getUploadItem(index);
presenter.checkImageQuality(uploadItem, index);
}
}
if (uploadableFiles.size() < 2) {
view.showHideTopCard(false);
Expand Down
Loading

0 comments on commit 4264164

Please sign in to comment.