From f7302d430153647c18e6457b392df6517901506d Mon Sep 17 00:00:00 2001 From: Dmitry Brant Date: Fri, 22 Mar 2019 23:22:36 -0400 Subject: [PATCH] Fix a couple more Rx/context inconsistencies. (#2715) * Stop storing Context! * Properly contain and dispose of observables. * Add a few more containments of Rx observables. --- .../free/nrw/commons/delete/DeleteTask.java | 2 +- .../notification/NotificationActivity.java | 8 +- .../nrw/commons/review/ReviewActivity.java | 26 +++-- .../nrw/commons/review/ReviewController.java | 104 ++++++++---------- .../commons/review/ReviewImageFragment.java | 24 ++-- .../nrw/commons/upload/UploadPresenter.java | 4 +- 6 files changed, 76 insertions(+), 92 deletions(-) diff --git a/app/src/main/java/fr/free/nrw/commons/delete/DeleteTask.java b/app/src/main/java/fr/free/nrw/commons/delete/DeleteTask.java index 95d4a29ee..7629fb304 100644 --- a/app/src/main/java/fr/free/nrw/commons/delete/DeleteTask.java +++ b/app/src/main/java/fr/free/nrw/commons/delete/DeleteTask.java @@ -236,7 +236,7 @@ public class DeleteTask extends AsyncTask { } } - ((ReviewActivity)context).reviewController.swipeToNext(); + ((ReviewActivity)context).swipeToNext(); ((ReviewActivity)context).runRandomizer(); DeleteTask deleteTask = new DeleteTask(context, media, reason); diff --git a/app/src/main/java/fr/free/nrw/commons/notification/NotificationActivity.java b/app/src/main/java/fr/free/nrw/commons/notification/NotificationActivity.java index f51d32dcb..2389654fe 100644 --- a/app/src/main/java/fr/free/nrw/commons/notification/NotificationActivity.java +++ b/app/src/main/java/fr/free/nrw/commons/notification/NotificationActivity.java @@ -78,7 +78,7 @@ public class NotificationActivity extends NavigationBaseActivity { @SuppressLint("CheckResult") public void removeNotification(Notification notification) { - Observable.fromCallable(() -> controller.markAsRead(notification)) + compositeDisposable.add(Observable.fromCallable(() -> controller.markAsRead(notification)) .subscribeOn(Schedulers.io()) .observeOn(AndroidSchedulers.mainThread()) .subscribe(result -> { @@ -107,7 +107,7 @@ public class NotificationActivity extends NavigationBaseActivity { throwable.printStackTrace(); ViewUtil.showShortSnackbar(relativeLayout, R.string.error_notifications); progressBar.setVisibility(View.GONE); - }); + })); } @@ -140,7 +140,7 @@ public class NotificationActivity extends NavigationBaseActivity { private void addNotifications(boolean archived) { Timber.d("Add notifications"); if (mNotificationWorkerFragment == null) { - Observable.fromCallable(() -> { + compositeDisposable.add(Observable.fromCallable(() -> { progressBar.setVisibility(View.VISIBLE); return controller.getNotifications(archived); @@ -164,7 +164,7 @@ public class NotificationActivity extends NavigationBaseActivity { Timber.e(throwable, "Error occurred while loading notifications"); ViewUtil.showShortSnackbar(relativeLayout, R.string.error_notifications); progressBar.setVisibility(View.GONE); - }); + })); } else { notificationList = mNotificationWorkerFragment.getNotificationList(); setAdapter(notificationList); diff --git a/app/src/main/java/fr/free/nrw/commons/review/ReviewActivity.java b/app/src/main/java/fr/free/nrw/commons/review/ReviewActivity.java index a5ac5dbd0..54d0891bc 100644 --- a/app/src/main/java/fr/free/nrw/commons/review/ReviewActivity.java +++ b/app/src/main/java/fr/free/nrw/commons/review/ReviewActivity.java @@ -18,6 +18,7 @@ import androidx.appcompat.widget.Toolbar; import androidx.drawerlayout.widget.DrawerLayout; import butterknife.BindView; import butterknife.ButterKnife; +import fr.free.nrw.commons.Media; import fr.free.nrw.commons.R; import fr.free.nrw.commons.auth.AuthenticatedActivity; import fr.free.nrw.commons.mwapi.MediaResult; @@ -75,7 +76,7 @@ public class ReviewActivity extends AuthenticatedActivity { ButterKnife.bind(this); initDrawer(); - reviewController = new ReviewController(this); + reviewController = new ReviewController(); reviewPagerAdapter = new ReviewPagerAdapter(getSupportFragmentManager()); reviewPager.setAdapter(reviewPagerAdapter); @@ -97,7 +98,7 @@ public class ReviewActivity extends AuthenticatedActivity { reviewPager.setCurrentItem(0); compositeDisposable.add(reviewHelper.getRandomMedia() - .map(media -> media.getFilename()) + .map(Media::getFilename) .subscribeOn(Schedulers.io()) .observeOn(AndroidSchedulers.mainThread()) .subscribe(this::updateImage)); @@ -110,21 +111,21 @@ public class ReviewActivity extends AuthenticatedActivity { return; } reviewController.onImageRefreshed(fileName); //file name is updated - reviewHelper.getFirstRevisionOfFile("File:" + fileName) + compositeDisposable.add(reviewHelper.getFirstRevisionOfFile("File:" + fileName) .subscribeOn(Schedulers.io()) .observeOn(AndroidSchedulers.mainThread()) .subscribe(revision -> { reviewController.firstRevision = revision; reviewPagerAdapter.updateFileInformation(fileName, revision); - }); + })); reviewPager.setCurrentItem(0); - Observable.fromCallable(() -> { + compositeDisposable.add(Observable.fromCallable(() -> { MediaResult media = mwApi.fetchMediaByFilename("File:" + fileName); return MediaDataExtractorUtil.extractCategories(media.getWikiSource()); }) .subscribeOn(Schedulers.io()) .observeOn(AndroidSchedulers.mainThread()) - .subscribe(this::updateCategories, this::categoryFetchError); + .subscribe(this::updateCategories, this::categoryFetchError)); } @@ -139,12 +140,13 @@ public class ReviewActivity extends AuthenticatedActivity { reviewPagerAdapter.updateCategories(); } - /** - * References ReviewPagerAdapter to null before the activity is destroyed - */ - @Override - public void onDestroy() { - super.onDestroy(); + public void swipeToNext() { + int nextPos = reviewPager.getCurrentItem() + 1; + if (nextPos <= 3) { + reviewPager.setCurrentItem(nextPos); + } else { + runRandomizer(); + } } /** diff --git a/app/src/main/java/fr/free/nrw/commons/review/ReviewController.java b/app/src/main/java/fr/free/nrw/commons/review/ReviewController.java index 36dbbb465..4d236a96c 100644 --- a/app/src/main/java/fr/free/nrw/commons/review/ReviewController.java +++ b/app/src/main/java/fr/free/nrw/commons/review/ReviewController.java @@ -1,6 +1,7 @@ package fr.free.nrw.commons.review; import android.annotation.SuppressLint; +import android.app.Activity; import android.app.NotificationManager; import android.content.Context; import android.view.Gravity; @@ -11,9 +12,9 @@ import java.util.ArrayList; import javax.inject.Inject; import javax.inject.Singleton; +import androidx.annotation.NonNull; import androidx.annotation.Nullable; import androidx.core.app.NotificationCompat; -import androidx.viewpager.widget.ViewPager; import fr.free.nrw.commons.Media; import fr.free.nrw.commons.R; import fr.free.nrw.commons.auth.SessionManager; @@ -43,16 +44,6 @@ public class ReviewController { @Inject SessionManager sessionManager; - private ReviewPagerAdapter reviewPagerAdapter; - private ViewPager viewPager; - private ReviewActivity reviewActivity; - - ReviewController(Context context) { - reviewActivity = (ReviewActivity) context; - reviewPagerAdapter = reviewActivity.reviewPagerAdapter; - viewPager = ((ReviewActivity) context).reviewPager; - } - public void onImageRefreshed(String fileName) { this.fileName = fileName; media = new Media("File:" + fileName); @@ -63,49 +54,41 @@ public class ReviewController { ReviewController.categories = categories; } - public void swipeToNext() { - int nextPos = viewPager.getCurrentItem() + 1; - if (nextPos <= 3) { - viewPager.setCurrentItem(nextPos); - } else { - reviewActivity.runRandomizer(); - } + public void reportSpam(@NonNull Activity activity) { + DeleteTask.askReasonAndExecute(new Media("File:" + fileName), + activity, + activity.getString(R.string.review_spam_report_question), + activity.getString(R.string.review_spam_report_problem)); } - public void reportSpam() { + public void reportPossibleCopyRightViolation(@NonNull Activity activity) { DeleteTask.askReasonAndExecute(new Media("File:" + fileName), - reviewActivity, - reviewActivity.getResources().getString(R.string.review_spam_report_question), - reviewActivity.getResources().getString(R.string.review_spam_report_problem)); - } - - public void reportPossibleCopyRightViolation() { - DeleteTask.askReasonAndExecute(new Media("File:" + fileName), - reviewActivity, - reviewActivity.getResources().getString(R.string.review_c_violation_report_question), - reviewActivity.getResources().getString(R.string.review_c_violation_report_problem)); + activity, + activity.getResources().getString(R.string.review_c_violation_report_question), + activity.getResources().getString(R.string.review_c_violation_report_problem)); } @SuppressLint("CheckResult") - public void reportWrongCategory() { + public void reportWrongCategory(@NonNull Activity activity) { + Context context = activity.getApplicationContext(); ApplicationlessInjection - .getInstance(reviewActivity.getApplicationContext()) + .getInstance(context) .getCommonsApplicationComponent() .inject(this); - notificationManager = (NotificationManager) reviewActivity.getSystemService(Context.NOTIFICATION_SERVICE); - notificationBuilder = new NotificationCompat.Builder(reviewActivity); - Toast toast = new Toast(reviewActivity); + notificationManager = (NotificationManager) context.getSystemService(Context.NOTIFICATION_SERVICE); + notificationBuilder = new NotificationCompat.Builder(context); + Toast toast = new Toast(context); toast.setGravity(Gravity.CENTER, 0, 0); - toast = Toast.makeText(reviewActivity, reviewActivity.getString(R.string.check_category_toast, media.getDisplayTitle()), Toast.LENGTH_SHORT); + toast = Toast.makeText(context, context.getString(R.string.check_category_toast, media.getDisplayTitle()), Toast.LENGTH_SHORT); toast.show(); Observable.fromCallable(() -> { - publishProgress(0); + publishProgress(context, 0); String editToken; String authCookie; - String summary = reviewActivity.getString(R.string.check_category_edit_summary); + String summary = context.getString(R.string.check_category_edit_summary); authCookie = sessionManager.getAuthCookie(); mwApi.setAuthCookie(authCookie); @@ -115,10 +98,10 @@ public class ReviewController { if (editToken.equals("+\\")) { return false; } - publishProgress(1); + publishProgress(context, 1); mwApi.appendEdit(editToken, "\n{{subst:chc}}\n", media.getFilename(), summary); - publishProgress(2); + publishProgress(context, 2); } catch (Exception e) { Timber.d(e); return false; @@ -132,11 +115,11 @@ public class ReviewController { String title = ""; if (result) { - title = reviewActivity.getString(R.string.check_category_success_title); - message = reviewActivity.getString(R.string.check_category_success_message, media.getDisplayTitle()); + title = context.getString(R.string.check_category_success_title); + message = context.getString(R.string.check_category_success_message, media.getDisplayTitle()); } else { - title = reviewActivity.getString(R.string.check_category_failure_title); - message = reviewActivity.getString(R.string.check_category_failure_message, media.getDisplayTitle()); + title = context.getString(R.string.check_category_failure_title); + message = context.getString(R.string.check_category_failure_message, media.getDisplayTitle()); } notificationBuilder.setDefaults(NotificationCompat.DEFAULT_ALL) @@ -150,17 +133,16 @@ public class ReviewController { notificationManager.notify(NOTIFICATION_CHECK_CATEGORY, notificationBuilder.build()); }, Timber::e); - swipeToNext(); } - private void publishProgress(int i) { + private void publishProgress(@NonNull Context context, int i) { int[] messages = new int[]{R.string.getting_edit_token, R.string.check_category_adding_template}; String message = ""; if (0 < i && i < messages.length) { - message = reviewActivity.getString(messages[i]); + message = context.getString(messages[i]); } - notificationBuilder.setContentTitle(reviewActivity.getString(R.string.check_category_notification_title, media.getDisplayTitle())) + notificationBuilder.setContentTitle(context.getString(R.string.check_category_notification_title, media.getDisplayTitle())) .setStyle(new NotificationCompat.BigTextStyle() .bigText(message)) .setSmallIcon(R.drawable.ic_launcher) @@ -170,20 +152,21 @@ public class ReviewController { } @SuppressLint("CheckResult") - public void sendThanks() { + public void sendThanks(@NonNull Activity activity) { + Context context = activity.getApplicationContext(); ApplicationlessInjection - .getInstance(reviewActivity.getApplicationContext()) + .getInstance(context) .getCommonsApplicationComponent() .inject(this); - notificationManager = (NotificationManager) reviewActivity.getSystemService(Context.NOTIFICATION_SERVICE); - notificationBuilder = new NotificationCompat.Builder(reviewActivity); - Toast toast = new Toast(reviewActivity); + notificationManager = (NotificationManager) context.getSystemService(Context.NOTIFICATION_SERVICE); + notificationBuilder = new NotificationCompat.Builder(context); + Toast toast = new Toast(context); toast.setGravity(Gravity.CENTER, 0, 0); - toast = Toast.makeText(reviewActivity, reviewActivity.getString(R.string.send_thank_toast, media.getDisplayTitle()), Toast.LENGTH_SHORT); + toast = Toast.makeText(context, context.getString(R.string.send_thank_toast, media.getDisplayTitle()), Toast.LENGTH_SHORT); toast.show(); Observable.fromCallable(() -> { - publishProgress(0); + publishProgress(context, 0); String editToken; String authCookie; @@ -195,10 +178,10 @@ public class ReviewController { if (editToken.equals("+\\")) { return false; } - publishProgress(1); + publishProgress(context, 1); assert firstRevision != null; mwApi.thank(editToken, firstRevision.getRevid()); - publishProgress(2); + publishProgress(context, 2); } catch (Exception e) { Timber.d(e); return false; @@ -211,11 +194,11 @@ public class ReviewController { String message = ""; String title = ""; if (result) { - title = reviewActivity.getString(R.string.send_thank_success_title); - message = reviewActivity.getString(R.string.send_thank_success_message, media.getDisplayTitle()); + title = context.getString(R.string.send_thank_success_title); + message = context.getString(R.string.send_thank_success_message, media.getDisplayTitle()); } else { - title = reviewActivity.getString(R.string.send_thank_failure_title); - message = reviewActivity.getString(R.string.send_thank_failure_message, media.getDisplayTitle()); + title = context.getString(R.string.send_thank_failure_title); + message = context.getString(R.string.send_thank_failure_message, media.getDisplayTitle()); } notificationBuilder.setDefaults(NotificationCompat.DEFAULT_ALL) @@ -229,6 +212,5 @@ public class ReviewController { notificationManager.notify(NOTIFICATION_SEND_THANK, notificationBuilder.build()); }, Timber::e); - swipeToNext(); } } diff --git a/app/src/main/java/fr/free/nrw/commons/review/ReviewImageFragment.java b/app/src/main/java/fr/free/nrw/commons/review/ReviewImageFragment.java index 47268a07c..31814a2e1 100644 --- a/app/src/main/java/fr/free/nrw/commons/review/ReviewImageFragment.java +++ b/app/src/main/java/fr/free/nrw/commons/review/ReviewImageFragment.java @@ -94,9 +94,7 @@ public class ReviewImageFragment extends CommonsDaggerSupportFragment { explanation = getString(R.string.review_copyright_explanation); yesButtonText = getString(R.string.review_copyright_yes_button_text); noButtonText = getString(R.string.review_copyright_no_button_text); - yesButton.setOnClickListener(view -> { - ((ReviewActivity) getActivity()).reviewController.reportPossibleCopyRightViolation(); - }); + yesButton.setOnClickListener(view -> getReviewActivity().reviewController.reportPossibleCopyRightViolation(requireActivity())); break; case CATEGORY: question = getString(R.string.review_category); @@ -104,7 +102,8 @@ public class ReviewImageFragment extends CommonsDaggerSupportFragment { yesButtonText = getString(R.string.review_category_yes_button_text); noButtonText = getString(R.string.review_category_no_button_text); yesButton.setOnClickListener(view -> { - ((ReviewActivity) getActivity()).reviewController.reportWrongCategory(); + getReviewActivity().reviewController.reportWrongCategory(requireActivity()); + getReviewActivity().swipeToNext(); }); break; case SPAM: @@ -112,19 +111,18 @@ public class ReviewImageFragment extends CommonsDaggerSupportFragment { explanation = getString(R.string.review_spam_explanation); yesButtonText = getString(R.string.review_spam_yes_button_text); noButtonText = getString(R.string.review_spam_no_button_text); - yesButton.setOnClickListener(view -> { - ((ReviewActivity) getActivity()).reviewController.reportSpam(); - }); + yesButton.setOnClickListener(view -> getReviewActivity().reviewController.reportSpam(requireActivity())); break; case THANKS: question = getString(R.string.review_thanks); - explanation = getString(R.string.review_thanks_explanation, ((ReviewActivity) getActivity()).reviewController.firstRevision.getUser()); + explanation = getString(R.string.review_thanks_explanation, getReviewActivity().reviewController.firstRevision.getUser()); yesButtonText = getString(R.string.review_thanks_yes_button_text); noButtonText = getString(R.string.review_thanks_no_button_text); yesButton.setTextColor(Color.parseColor("#228b22")); noButton.setTextColor(Color.parseColor("#116aaa")); yesButton.setOnClickListener(view -> { - ((ReviewActivity) getActivity()).reviewController.sendThanks(); + getReviewActivity().reviewController.sendThanks(getReviewActivity()); + getReviewActivity().swipeToNext(); }); break; default : @@ -134,9 +132,7 @@ public class ReviewImageFragment extends CommonsDaggerSupportFragment { noButtonText = "no"; } - noButton.setOnClickListener(view -> { - ((ReviewActivity) getActivity()).reviewController.swipeToNext(); - }); + noButton.setOnClickListener(view -> getReviewActivity().swipeToNext()); ((TextView) textViewQuestion).setText(question); ((TextView) textViewQuestionContext).setText(explanation); @@ -156,6 +152,10 @@ public class ReviewImageFragment extends CommonsDaggerSupportFragment { return layoutView; } + private ReviewActivity getReviewActivity() { + return (ReviewActivity) requireActivity(); + } + private void fillImageCaption() { if (imageCaption != null && fileName != null && revision != null) { ((TextView) imageCaption).setText(fileName + " is uploaded by: " + revision.getUser()); diff --git a/app/src/main/java/fr/free/nrw/commons/upload/UploadPresenter.java b/app/src/main/java/fr/free/nrw/commons/upload/UploadPresenter.java index 36348e7e6..ed47e92e2 100644 --- a/app/src/main/java/fr/free/nrw/commons/upload/UploadPresenter.java +++ b/app/src/main/java/fr/free/nrw/commons/upload/UploadPresenter.java @@ -115,11 +115,11 @@ public class UploadPresenter { List descriptions) { Timber.e("Inside handleNext"); view.showProgressDialog(); - uploadModel.getImageQuality(uploadModel.getCurrentItem(), true) + compositeDisposable.add(uploadModel.getImageQuality(uploadModel.getCurrentItem(), true) .subscribeOn(Schedulers.io()) .observeOn(AndroidSchedulers.mainThread()) .subscribe(imageResult -> handleImage(title, descriptions, imageResult), - throwable -> Timber.e(throwable, "Error occurred while handling image")); + throwable -> Timber.e(throwable, "Error occurred while handling image"))); } private void handleImage(Title title, List descriptions, Integer imageResult) {