Skip to content

Prevent additional possible crash(es) from improperly disposed observables. #2670

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Prevent additional possible crash(es) from improperly disposed observables. #2670

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions app/src/main/java/fr/free/nrw/commons/upload/FileProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import fr.free.nrw.commons.caching.CacheController;
import fr.free.nrw.commons.kvstore.JsonKvStore;
import fr.free.nrw.commons.mwapi.CategoryApi;
import io.reactivex.disposables.CompositeDisposable;
import io.reactivex.schedulers.Schedulers;
import timber.log.Timber;

Expand All @@ -42,11 +43,16 @@ public class FileProcessor implements SimilarImageDialogFragment.onResponse {
private ExifInterface exifInterface;
private boolean haveCheckedForOtherImages = false;
private GPSExtractor tempImageObj;
private CompositeDisposable compositeDisposable = new CompositeDisposable();

@Inject
FileProcessor() {
}

public void cleanup() {
compositeDisposable.clear();
}

void initFileDetails(@NonNull String filePath, ContentResolver contentResolver) {
this.filePath = filePath;
this.contentResolver = contentResolver;
Expand Down Expand Up @@ -138,7 +144,7 @@ private void useImageCoords() {

// If no categories found in cache, call MediaWiki API to match image coords with nearby Commons categories
if (catListEmpty) {
apiCall.request(decimalCoords)
compositeDisposable.add(apiCall.request(decimalCoords)
.subscribeOn(Schedulers.io())
.observeOn(Schedulers.io())
.subscribe(
Expand All @@ -147,7 +153,7 @@ private void useImageCoords() {
Timber.e(throwable);
gpsCategoryModel.clear();
}
);
));
Timber.d("displayCatList size 0, calling MWAPI %s", displayCatList);
} else {
Timber.d("Cache found, setting categoryList in model to %s", displayCatList);
Expand Down
7 changes: 5 additions & 2 deletions app/src/main/java/fr/free/nrw/commons/upload/ImageProcessingService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,16 +33,19 @@ public class ImageProcessingService {
private final MediaWikiApi mwApi;
private final ReadFBMD readFBMD;
private final EXIFReader EXIFReader;
private final Context context;

@Inject
public ImageProcessingService(FileUtilsWrapper fileUtilsWrapper,
ImageUtilsWrapper imageUtilsWrapper,
MediaWikiApi mwApi, ReadFBMD readFBMD, EXIFReader EXIFReader) {
MediaWikiApi mwApi, ReadFBMD readFBMD, EXIFReader EXIFReader,
Context context) {
this.fileUtilsWrapper = fileUtilsWrapper;
this.imageUtilsWrapper = imageUtilsWrapper;
this.mwApi = mwApi;
this.readFBMD = readFBMD;
this.EXIFReader = EXIFReader;
this.context = context;
}

/**
Expand All @@ -61,13 +64,13 @@ Single<Integer> validateImage(UploadModel.UploadItem uploadItem, boolean checkTi
Timber.d("Checking the validity of image");
String filePath = uploadItem.getMediaUri().getPath();
Uri contentUri=uploadItem.getContentUri();
Context context=uploadItem.getContext();
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(context,contentUri);
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);
Expand Down
24 changes: 12 additions & 12 deletions app/src/main/java/fr/free/nrw/commons/upload/UploadModel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
import io.reactivex.Observable;
import io.reactivex.Single;
import io.reactivex.android.schedulers.AndroidSchedulers;
import io.reactivex.disposables.Disposable;
import io.reactivex.disposables.CompositeDisposable;
import io.reactivex.functions.Consumer;
import io.reactivex.schedulers.Schedulers;
import io.reactivex.subjects.BehaviorSubject;
Expand All @@ -45,15 +45,15 @@ public class UploadModel {
};
private final JsonKvStore store;
private final List<String> licenses;
private final Context context;
private String license;
private final Map<String, String> licensesByName;
private List<UploadItem> items = new ArrayList<>();
private boolean topCardState = true;
private boolean bottomCardState = true;
private boolean rightCardState = true;
private int currentStepIndex = 0;
public static Context context;
private Disposable badImageSubscription;
private CompositeDisposable compositeDisposable = new CompositeDisposable();

private SessionManager sessionManager;
private FileProcessor fileProcessor;
Expand All @@ -77,6 +77,11 @@ public class UploadModel {
this.imageProcessingService = imageProcessingService;
}

void cleanup() {
compositeDisposable.clear();
fileProcessor.cleanup();
}

@SuppressLint("CheckResult")
Observable<UploadItem> preProcessImages(List<UploadableFile> uploadableFiles,
Place place,
Expand Down Expand Up @@ -219,8 +224,7 @@ private void setCurrentUploadDescriptions(List<Description> descriptions) {
}

public void previous() {
if (badImageSubscription != null)
badImageSubscription.dispose();
cleanup();
markCurrentUploadVisited();
if (currentStepIndex > 0) {
currentStepIndex--;
Expand Down Expand Up @@ -305,16 +309,16 @@ void keepPicture() {
}

void deletePicture() {
badImageSubscription.dispose();
cleanup();
updateItemState();
}

void subscribeBadPicture(Consumer<Integer> consumer, boolean checkTitle) {
if (isShowingItem()) {
badImageSubscription = getImageQuality(getCurrentItem(), checkTitle)
compositeDisposable.add(getImageQuality(getCurrentItem(), checkTitle)
.subscribeOn(Schedulers.io())
.observeOn(AndroidSchedulers.mainThread())
.subscribe(consumer, Timber::e);
.subscribe(consumer, Timber::e));
}
}

Expand Down Expand Up @@ -432,10 +436,6 @@ public Place getPlace() {
public Uri getContentUri() {
return originalContentUri;
}

public Context getContext(){
return UploadModel.context;
}
}

}
12 changes: 8 additions & 4 deletions app/src/main/java/fr/free/nrw/commons/upload/UploadPresenter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import fr.free.nrw.commons.utils.StringUtils;
import io.reactivex.Observable;
import io.reactivex.android.schedulers.AndroidSchedulers;
import io.reactivex.disposables.CompositeDisposable;
import io.reactivex.schedulers.Schedulers;
import timber.log.Timber;

Expand Down Expand Up @@ -52,6 +53,7 @@ public class UploadPresenter {
private final UploadController uploadController;
private final Context context;
private final JsonKvStore directKvStore;
private CompositeDisposable compositeDisposable = new CompositeDisposable();

@Inject
UploadPresenter(UploadModel uploadModel,
Expand All @@ -77,12 +79,12 @@ void receive(List<UploadableFile> media,
Observable<UploadItem> uploadItemObservable = uploadModel
.preProcessImages(media, place, source, similarImageInterface);

uploadItemObservable
compositeDisposable.add(uploadItemObservable
.toList()
.subscribeOn(Schedulers.io())
.observeOn(AndroidSchedulers.mainThread())
.subscribe(uploadItems -> onImagesProcessed(uploadItems, place),
throwable -> Timber.e(throwable, "Error occurred in processing images"));
throwable -> Timber.e(throwable, "Error occurred in processing images")));
}

private void onImagesProcessed(List<UploadItem> uploadItems, Place place) {
Expand Down Expand Up @@ -211,9 +213,9 @@ void thumbnailClicked(UploadItem item) {
@SuppressLint("CheckResult")
void handleSubmit(CategoriesModel categoriesModel) {
if (view.checkIfLoggedIn())
uploadModel.buildContributions(categoriesModel.getCategoryStringList())
compositeDisposable.add(uploadModel.buildContributions(categoriesModel.getCategoryStringList())
.observeOn(Schedulers.io())
.subscribe(uploadController::startUpload);
.subscribe(uploadController::startUpload));
}

/**
Expand Down Expand Up @@ -286,6 +288,8 @@ public void init() {
}

void cleanup() {
compositeDisposable.clear();
uploadModel.cleanup();
uploadController.cleanup();
}

Expand Down