From 5dc45a53d35baeb93495c3190886ea969fe31351 Mon Sep 17 00:00:00 2001 From: Vivek Maskara Date: Wed, 26 Jun 2019 20:10:49 +0530 Subject: [PATCH 1/3] Fix bugs in peer review flow (#3039) * Fix bugs in peer review flow * Fix tests * Bug fixes * Fix remaining issues with peer review --- .../free/nrw/commons/delete/DeleteHelper.java | 34 +++++-- .../nrw/commons/review/ReviewActivity.java | 18 ++-- .../nrw/commons/review/ReviewController.java | 89 ++++++++--------- .../free/nrw/commons/review/ReviewHelper.java | 6 +- .../commons/review/ReviewImageFragment.java | 96 +++++++++++-------- .../commons/review/ReviewPagerAdapter.java | 15 ++- app/src/main/res/values/strings.xml | 1 + .../nrw/commons/review/ReviewHelperTest.kt | 6 +- 8 files changed, 140 insertions(+), 125 deletions(-) diff --git a/app/src/main/java/fr/free/nrw/commons/delete/DeleteHelper.java b/app/src/main/java/fr/free/nrw/commons/delete/DeleteHelper.java index 1b72833dd..08b13acfa 100644 --- a/app/src/main/java/fr/free/nrw/commons/delete/DeleteHelper.java +++ b/app/src/main/java/fr/free/nrw/commons/delete/DeleteHelper.java @@ -1,9 +1,12 @@ package fr.free.nrw.commons.delete; +import android.annotation.SuppressLint; import android.content.Context; import android.content.Intent; import android.net.Uri; +import androidx.appcompat.app.AlertDialog; + import java.text.SimpleDateFormat; import java.util.ArrayList; import java.util.Calendar; @@ -12,17 +15,16 @@ import java.util.Locale; import javax.inject.Inject; import javax.inject.Singleton; -import androidx.appcompat.app.AlertDialog; import fr.free.nrw.commons.BuildConfig; import fr.free.nrw.commons.Media; import fr.free.nrw.commons.auth.SessionManager; import fr.free.nrw.commons.mwapi.MediaWikiApi; import fr.free.nrw.commons.notification.NotificationHelper; -import fr.free.nrw.commons.review.ReviewActivity; -import fr.free.nrw.commons.utils.ViewUtil; import fr.free.nrw.commons.review.ReviewController; import fr.free.nrw.commons.utils.ViewUtilWrapper; import io.reactivex.Single; +import io.reactivex.android.schedulers.AndroidSchedulers; +import io.reactivex.schedulers.Schedulers; import timber.log.Timber; import static fr.free.nrw.commons.notification.NotificationHelper.NOTIFICATION_DELETE; @@ -57,7 +59,6 @@ public class DeleteHelper { */ public Single makeDeletion(Context context, Media media, String reason) { viewUtil.showShortToast(context, "Trying to nominate " + media.getDisplayTitle() + " for deletion"); - return Single.fromCallable(() -> delete(media, reason)) .flatMap(result -> Single.fromCallable(() -> showDeletionNotification(context, media, result))); @@ -99,7 +100,8 @@ public class DeleteHelper { try { editToken = mwApi.getEditToken(); - if (editToken.equals("+\\")) { + + if(editToken == null) { return false; } @@ -143,7 +145,12 @@ public class DeleteHelper { * @param question * @param problem */ - public void askReasonAndExecute(Media media, Context context, String question, ReviewController.DeleteReason problem) { + @SuppressLint("CheckResult") + public void askReasonAndExecute(Media media, + Context context, + String question, + ReviewController.DeleteReason problem, + ReviewController.ReviewCallback reviewCallback) { AlertDialog.Builder alert = new AlertDialog.Builder(context); alert.setTitle(question); @@ -183,12 +190,19 @@ public class DeleteHelper { } } - ((ReviewActivity) context).reviewController.swipeToNext(); - ((ReviewActivity) context).runRandomizer(); + makeDeletion(context, media, reason) + .subscribeOn(Schedulers.io()) + .observeOn(AndroidSchedulers.mainThread()) + .subscribe(aBoolean -> { + if (aBoolean) { + reviewCallback.onSuccess(); + } else { + reviewCallback.onFailure(); + } + }); - makeDeletion(context, media, reason); }); - alert.setNegativeButton("Cancel", null); + alert.setNegativeButton("Cancel", (dialog, which) -> reviewCallback.onFailure()); AlertDialog d = alert.create(); d.show(); } 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 cbb9848ca..48cde85db 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 @@ -20,8 +20,6 @@ import com.facebook.drawee.view.SimpleDraweeView; import com.google.android.material.navigation.NavigationView; import com.viewpagerindicator.CirclePageIndicator; -import java.util.ArrayList; - import javax.inject.Inject; import butterknife.BindView; @@ -92,6 +90,9 @@ public class ReviewActivity extends AuthenticatedActivity { private CompositeDisposable compositeDisposable = new CompositeDisposable(); + public Media getMedia() { + return media; + } @Override protected void onAuthCookieAcquired(String authCookie) { @@ -163,23 +164,18 @@ public class ReviewActivity extends AuthenticatedActivity { simpleDraweeView.setImageURI(media.getImageUrl()); - reviewController.onImageRefreshed(fileName); //file name is updated + reviewController.onImageRefreshed(media); //file name is updated compositeDisposable.add(reviewHelper.getFirstRevisionOfFile(fileName) .subscribeOn(Schedulers.io()) .observeOn(AndroidSchedulers.mainThread()) .subscribe(revision -> { reviewController.firstRevision = revision; - reviewPagerAdapter.updateFileInformation(fileName); - imageCaption.setText(fileName + " is uploaded by: " + revision.getUser()); + reviewPagerAdapter.updateFileInformation(); + String caption = String.format(getString(R.string.review_is_uploaded_by), fileName, revision.getUser()); + imageCaption.setText(caption); progressBar.setVisibility(View.GONE); })); reviewPager.setCurrentItem(0); - updateCategories(media.getCategories()); - } - - private void updateCategories(ArrayList categories) { - reviewController.onCategoriesRefreshed(categories); - reviewPagerAdapter.updateCategories(); } public void swipeToNext() { 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 b4bf05e2c..c98c7ed3e 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 @@ -7,17 +7,16 @@ import android.content.Context; import android.view.Gravity; import android.widget.Toast; -import org.wikipedia.dataclient.mwapi.MwQueryPage; +import androidx.annotation.NonNull; +import androidx.annotation.Nullable; +import androidx.core.app.NotificationCompat; -import java.util.ArrayList; +import org.wikipedia.dataclient.mwapi.MwQueryPage; 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.CommonsApplication; import fr.free.nrw.commons.Media; import fr.free.nrw.commons.R; import fr.free.nrw.commons.auth.SessionManager; @@ -31,13 +30,11 @@ import timber.log.Timber; @Singleton public class ReviewController { - private String fileName; - public static final int NOTIFICATION_SEND_THANK = 0x102; - protected static ArrayList categories; - public static final int NOTIFICATION_CHECK_CATEGORY = 0x101; + private static final int NOTIFICATION_SEND_THANK = 0x102; + private static final int NOTIFICATION_CHECK_CATEGORY = 0x101; private final DeleteHelper deleteHelper; @Nullable - public MwQueryPage.Revision firstRevision; // TODO: maybe we can expand this class to include fileName + MwQueryPage.Revision firstRevision; // TODO: maybe we can expand this class to include fileName @Inject MediaWikiApi mwApi; @Inject @@ -46,32 +43,19 @@ public class ReviewController { private NotificationCompat.Builder notificationBuilder; private Media media; - private ViewPager viewPager; - private ReviewActivity reviewActivity; - ReviewController(DeleteHelper deleteHelper, Context context) { this.deleteHelper = deleteHelper; - reviewActivity = (ReviewActivity) context; - viewPager = ((ReviewActivity) context).reviewPager; + CommonsApplication.createNotificationChannel(context.getApplicationContext()); + notificationManager = (NotificationManager) context.getSystemService(Context.NOTIFICATION_SERVICE); + notificationBuilder = new NotificationCompat.Builder(context, CommonsApplication.NOTIFICATION_CHANNEL_ID_ALL); } - public void onImageRefreshed(String fileName) { - this.fileName = fileName; - media = new Media("File:" + fileName); - ReviewController.categories = new ArrayList<>(); + void onImageRefreshed(Media media) { + this.media = media; } - public void onCategoriesRefreshed(ArrayList categories) { - ReviewController.categories = categories; - } - - public void swipeToNext() { - int nextPos = viewPager.getCurrentItem() + 1; - if (nextPos <= 3) { - viewPager.setCurrentItem(nextPos); - } else { - reviewActivity.runRandomizer(); - } + public Media getMedia() { + return media; } public enum DeleteReason { @@ -79,30 +63,32 @@ public class ReviewController { COPYRIGHT_VIOLATION } - public void reportSpam(@NonNull Activity activity) { - deleteHelper.askReasonAndExecute(new Media("File:" + fileName), + void reportSpam(@NonNull Activity activity, ReviewCallback reviewCallback) { + Timber.d("Report spam for %s", media.getFilename()); + deleteHelper.askReasonAndExecute(media, activity, activity.getResources().getString(R.string.review_spam_report_question), - DeleteReason.SPAM); + DeleteReason.SPAM, + reviewCallback); } - public void reportPossibleCopyRightViolation(@NonNull Activity activity) { - deleteHelper.askReasonAndExecute(new Media("File:" + fileName), + void reportPossibleCopyRightViolation(@NonNull Activity activity, ReviewCallback reviewCallback) { + Timber.d("Report spam for %s", media.getFilename()); + deleteHelper.askReasonAndExecute(media, activity, activity.getResources().getString(R.string.review_c_violation_report_question), - DeleteReason.COPYRIGHT_VIOLATION); + DeleteReason.COPYRIGHT_VIOLATION, + reviewCallback); } @SuppressLint("CheckResult") - public void reportWrongCategory(@NonNull Activity activity) { + void reportWrongCategory(@NonNull Activity activity, ReviewCallback reviewCallback) { Context context = activity.getApplicationContext(); ApplicationlessInjection .getInstance(context) .getCommonsApplicationComponent() .inject(this); - 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(context, context.getString(R.string.check_category_toast, media.getDisplayTitle()), Toast.LENGTH_SHORT); @@ -120,7 +106,8 @@ public class ReviewController { try { editToken = mwApi.getEditToken(); - if (editToken.equals("+\\")) { + + if (editToken == null) { return false; } publishProgress(context, 1); @@ -136,15 +123,17 @@ public class ReviewController { .subscribeOn(Schedulers.io()) .observeOn(AndroidSchedulers.mainThread()) .subscribe((result) -> { - String message = ""; - String title = ""; + String message; + String title; if (result) { title = context.getString(R.string.check_category_success_title); message = context.getString(R.string.check_category_success_message, media.getDisplayTitle()); + reviewCallback.onSuccess(); } else { title = context.getString(R.string.check_category_failure_title); message = context.getString(R.string.check_category_failure_message, media.getDisplayTitle()); + reviewCallback.onFailure(); } notificationBuilder.setDefaults(NotificationCompat.DEFAULT_ALL) @@ -176,15 +165,13 @@ public class ReviewController { notificationManager.notify(NOTIFICATION_CHECK_CATEGORY, notificationBuilder.build()); } - @SuppressLint("CheckResult") - public void sendThanks(@NonNull Activity activity) { + @SuppressLint({"CheckResult", "StringFormatInvalid"}) + void sendThanks(@NonNull Activity activity) { Context context = activity.getApplicationContext(); ApplicationlessInjection .getInstance(context) .getCommonsApplicationComponent() .inject(this); - 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(context, context.getString(R.string.send_thank_toast, media.getDisplayTitle()), Toast.LENGTH_SHORT); @@ -200,7 +187,7 @@ public class ReviewController { try { editToken = mwApi.getEditToken(); - if (editToken.equals("+\\")) { + if (editToken == null) { return false; } publishProgress(context, 1); @@ -238,4 +225,10 @@ public class ReviewController { }, Timber::e); } + + public interface ReviewCallback { + void onSuccess(); + + void onFailure(); + } } diff --git a/app/src/main/java/fr/free/nrw/commons/review/ReviewHelper.java b/app/src/main/java/fr/free/nrw/commons/review/ReviewHelper.java index 30903a89a..03f67bfcf 100644 --- a/app/src/main/java/fr/free/nrw/commons/review/ReviewHelper.java +++ b/app/src/main/java/fr/free/nrw/commons/review/ReviewHelper.java @@ -6,6 +6,7 @@ import org.wikipedia.dataclient.mwapi.MwQueryPage; import org.wikipedia.dataclient.mwapi.RecentChange; import org.wikipedia.util.DateUtil; +import java.util.Collections; import java.util.Date; import java.util.Random; @@ -53,7 +54,7 @@ public class ReviewHelper { return reviewInterface.getRecentChanges(rcStart) .map(mwQueryResponse -> mwQueryResponse.query().getRecentChanges()) .map(recentChanges -> { - //Collections.shuffle(recentChanges); + Collections.shuffle(recentChanges); return recentChanges; }) .flatMapIterable(changes -> changes) @@ -113,7 +114,8 @@ public class ReviewHelper { * @return */ private boolean isChangeReviewable(RecentChange recentChange) { - if (recentChange.getType().equals("log") && !(recentChange.getOldRevisionId() == 0)) { + if ((recentChange.getType().equals("log") && !(recentChange.getOldRevisionId() == 0)) + || !recentChange.getType().equals("log")) { return false; } 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 5f4e16d29..5952956ed 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 @@ -10,26 +10,26 @@ import android.view.ViewGroup; import android.widget.Button; import android.widget.ProgressBar; import android.widget.TextView; + +import java.util.List; + import butterknife.BindView; import butterknife.ButterKnife; import butterknife.OnClick; +import fr.free.nrw.commons.Media; import fr.free.nrw.commons.R; import fr.free.nrw.commons.di.CommonsDaggerSupportFragment; -import org.wikipedia.dataclient.mwapi.MwQueryPage; public class ReviewImageFragment extends CommonsDaggerSupportFragment { - public static final int SPAM = 0; - public static final int COPYRIGHT = 1; - public static final int CATEGORY = 2; - public static final int THANKS = 3; + static final int CATEGORY = 2; + private static final int SPAM = 0; + private static final int COPYRIGHT = 1; + private static final int THANKS = 3; private int position; - private String fileName; - private String catString; public ProgressBar progressBar; - private MwQueryPage.Revision revision; @BindView(R.id.tv_review_question) TextView textViewQuestion; @@ -40,24 +40,21 @@ public class ReviewImageFragment extends CommonsDaggerSupportFragment { @BindView(R.id.button_no) Button noButton; - - public void update(int position, String fileName) { + public void update(int position) { this.position = position; - this.fileName = fileName; - } - public void updateCategories(Iterable categories) { - if (categories != null && isAdded()) { - catString = TextUtils.join(", ", categories); + private String updateCategoriesQuestion() { + Media media = getReviewActivity().getMedia(); + if (media != null && media.getCategories() != null && isAdded()) { + String catString = TextUtils.join(", ", media.getCategories()); if (catString != null && !catString.equals("") && textViewQuestionContext != null) { catString = "" + catString + ""; String stringToConvertHtml = String.format(getResources().getString(R.string.review_category_explanation), catString); - textViewQuestionContext.setText(Html.fromHtml(stringToConvertHtml)); - } else if (textViewQuestionContext != null) { - textViewQuestionContext.setText(getResources().getString(R.string.review_no_category)); + return Html.fromHtml(stringToConvertHtml).toString(); } } + return getResources().getString(R.string.review_no_category); } @Override @@ -75,29 +72,34 @@ public class ReviewImageFragment extends CommonsDaggerSupportFragment { String question, explanation, yesButtonText, noButtonText; switch (position) { - case COPYRIGHT: - question = getString(R.string.review_copyright); - 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 -> getReviewActivity().reviewController.reportPossibleCopyRightViolation(requireActivity())); - break; - case CATEGORY: - question = getString(R.string.review_category); - explanation = getString(R.string.review_no_category); - yesButtonText = getString(R.string.review_category_yes_button_text); - noButtonText = getString(R.string.review_category_no_button_text); - yesButton.setOnClickListener(view -> { - getReviewActivity().reviewController.reportWrongCategory(requireActivity()); - getReviewActivity().swipeToNext(); - }); - break; case SPAM: question = getString(R.string.review_spam); 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 -> getReviewActivity().reviewController.reportSpam(requireActivity())); + yesButton.setOnClickListener(view -> getReviewActivity() + .reviewController.reportSpam(requireActivity(), getReviewCallback())); + break; + case COPYRIGHT: + question = getString(R.string.review_copyright); + 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 -> getReviewActivity() + .reviewController + .reportPossibleCopyRightViolation(requireActivity(), getReviewCallback())); + break; + case CATEGORY: + question = getString(R.string.review_category); + explanation = updateCategoriesQuestion(); + yesButtonText = getString(R.string.review_category_yes_button_text); + noButtonText = getString(R.string.review_category_no_button_text); + yesButton.setOnClickListener(view -> { + getReviewActivity() + .reviewController + .reportWrongCategory(requireActivity(), getReviewCallback()); + getReviewActivity().swipeToNext(); + }); break; case THANKS: question = getString(R.string.review_thanks); @@ -122,16 +124,26 @@ public class ReviewImageFragment extends CommonsDaggerSupportFragment { textViewQuestionContext.setText(explanation); yesButton.setText(yesButtonText); noButton.setText(noButtonText); - - if (position == CATEGORY) { - updateCategories(ReviewController.categories); - } - return layoutView; } + private ReviewController.ReviewCallback getReviewCallback() { + return new ReviewController + .ReviewCallback() { + @Override + public void onSuccess() { + getReviewActivity().runRandomizer(); + } + + @Override + public void onFailure() { + //do nothing + } + }; + } + @OnClick(R.id.button_no) - public void onNoButtonClicked() { + void onNoButtonClicked() { getReviewActivity().swipeToNext(); } diff --git a/app/src/main/java/fr/free/nrw/commons/review/ReviewPagerAdapter.java b/app/src/main/java/fr/free/nrw/commons/review/ReviewPagerAdapter.java index 98879a332..000fb8374 100644 --- a/app/src/main/java/fr/free/nrw/commons/review/ReviewPagerAdapter.java +++ b/app/src/main/java/fr/free/nrw/commons/review/ReviewPagerAdapter.java @@ -6,11 +6,13 @@ import androidx.fragment.app.Fragment; import androidx.fragment.app.FragmentManager; import androidx.fragment.app.FragmentStatePagerAdapter; +import java.util.List; + public class ReviewPagerAdapter extends FragmentStatePagerAdapter { - ReviewImageFragment[] reviewImageFragments; + private ReviewImageFragment[] reviewImageFragments; - public ReviewPagerAdapter(FragmentManager fm) { + ReviewPagerAdapter(FragmentManager fm) { super(fm); reviewImageFragments = new ReviewImageFragment[]{ new ReviewImageFragment(), @@ -25,18 +27,13 @@ public class ReviewPagerAdapter extends FragmentStatePagerAdapter { return reviewImageFragments.length; } - public void updateFileInformation(String fileName) { + void updateFileInformation() { for (int i = 0; i < getCount(); i++) { ReviewImageFragment fragment = reviewImageFragments[i]; - fragment.update(i, fileName); + fragment.update(i); } } - public void updateCategories() { - ReviewImageFragment categoryFragment = reviewImageFragments[ReviewImageFragment.CATEGORY]; - categoryFragment.updateCategories(ReviewController.categories); - } - @Override public Fragment getItem(int position) { Bundle bundle = new Bundle(); diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 0c6ba28f0..b160e1e8b 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -558,4 +558,5 @@ Upload your first media by tapping on the add button. Cancelled Upload There is no data for previous image\'s title or description Why should %1$s be deleted? + %1$s is uploaded by: %2$s diff --git a/app/src/test/kotlin/fr/free/nrw/commons/review/ReviewHelperTest.kt b/app/src/test/kotlin/fr/free/nrw/commons/review/ReviewHelperTest.kt index d9c808f77..ebd5d4baf 100644 --- a/app/src/test/kotlin/fr/free/nrw/commons/review/ReviewHelperTest.kt +++ b/app/src/test/kotlin/fr/free/nrw/commons/review/ReviewHelperTest.kt @@ -48,9 +48,9 @@ class ReviewHelperTest { `when`(mockRevision.user).thenReturn("TestUser") `when`(mwQueryPage.revisions()).thenReturn(listOf(mockRevision)) - val recentChange = getMockRecentChange("test", "File:Test1.jpeg", 0) - val recentChange1 = getMockRecentChange("test", "File:Test2.png", 0) - val recentChange2 = getMockRecentChange("test", "File:Test3.jpg", 0) + val recentChange = getMockRecentChange("log", "File:Test1.jpeg", 0) + val recentChange1 = getMockRecentChange("log", "File:Test2.png", 0) + val recentChange2 = getMockRecentChange("log", "File:Test3.jpg", 0) val mwQueryResult = mock(MwQueryResult::class.java) `when`(mwQueryResult.recentChanges).thenReturn(listOf(recentChange, recentChange1, recentChange2)) `when`(mwQueryResult.firstPage()).thenReturn(mwQueryPage) From 32715d9cce66eb26e1f13cc63c4818d83bb14610 Mon Sep 17 00:00:00 2001 From: "Vitaly V. Pinchuk" Date: Tue, 16 Jul 2019 12:46:23 +0300 Subject: [PATCH 2/3] Set defaults to ON for all EXIF tags (#3068) --- .../fr/free/nrw/commons/settings/SettingsFragment.java | 7 +++---- .../java/fr/free/nrw/commons/upload/FileProcessor.java | 6 +++--- app/src/main/res/xml/preferences.xml | 1 + 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/app/src/main/java/fr/free/nrw/commons/settings/SettingsFragment.java b/app/src/main/java/fr/free/nrw/commons/settings/SettingsFragment.java index 176697f9a..109dbc178 100644 --- a/app/src/main/java/fr/free/nrw/commons/settings/SettingsFragment.java +++ b/app/src/main/java/fr/free/nrw/commons/settings/SettingsFragment.java @@ -1,15 +1,12 @@ package fr.free.nrw.commons.settings; import android.Manifest; -import android.content.SharedPreferences; import android.net.Uri; import android.os.Bundle; import android.preference.EditTextPreference; -import android.preference.MultiSelectListPreference; import android.preference.ListPreference; import android.preference.Preference; import android.preference.PreferenceFragment; -import android.preference.PreferenceManager; import android.preference.SwitchPreference; import android.text.Editable; import android.text.TextWatcher; @@ -31,6 +28,7 @@ import fr.free.nrw.commons.Utils; import fr.free.nrw.commons.di.ApplicationlessInjection; import fr.free.nrw.commons.kvstore.JsonKvStore; import fr.free.nrw.commons.logging.CommonsLogSender; +import fr.free.nrw.commons.ui.LongTitlePreferences.LongTitleMultiSelectListPreference; import fr.free.nrw.commons.utils.PermissionUtils; import fr.free.nrw.commons.utils.ViewUtil; import fr.free.nrw.commons.upload.Language; @@ -70,8 +68,9 @@ public class SettingsFragment extends PreferenceFragment { return true; }); - MultiSelectListPreference multiSelectListPref = (MultiSelectListPreference) findPreference("manageExifTags"); + LongTitleMultiSelectListPreference multiSelectListPref = (LongTitleMultiSelectListPreference) findPreference("manageExifTags"); if (multiSelectListPref != null) { + defaultKvStore.putJson(Prefs.MANAGED_EXIF_TAGS, multiSelectListPref.getValues()); multiSelectListPref.setOnPreferenceChangeListener((preference, newValue) -> { defaultKvStore.putJson(Prefs.MANAGED_EXIF_TAGS, newValue); return true; diff --git a/app/src/main/java/fr/free/nrw/commons/upload/FileProcessor.java b/app/src/main/java/fr/free/nrw/commons/upload/FileProcessor.java index e9922332c..a287a8bcb 100644 --- a/app/src/main/java/fr/free/nrw/commons/upload/FileProcessor.java +++ b/app/src/main/java/fr/free/nrw/commons/upload/FileProcessor.java @@ -105,13 +105,13 @@ public class FileProcessor implements Callback { */ private Set getExifTagsToRedact(Context context) { Type setType = new TypeToken>() {}.getType(); - Set prefManageEXIFTags = defaultKvStore.getJson(Prefs.MANAGED_EXIF_TAGS, setType); + Set selectedExifTags = defaultKvStore.getJson(Prefs.MANAGED_EXIF_TAGS, setType); Set redactTags = new HashSet<>(Arrays.asList( context.getResources().getStringArray(R.array.pref_exifTag_values))); - Timber.d(redactTags.toString()); - if (prefManageEXIFTags != null) redactTags.removeAll(prefManageEXIFTags); + if (selectedExifTags != null) redactTags.removeAll(selectedExifTags); + else redactTags.clear(); return redactTags; } diff --git a/app/src/main/res/xml/preferences.xml b/app/src/main/res/xml/preferences.xml index cdeaa8366..051cc19ec 100644 --- a/app/src/main/res/xml/preferences.xml +++ b/app/src/main/res/xml/preferences.xml @@ -70,6 +70,7 @@ From 8cd9bd5524248e51ec626aeb2b0a62b3d320fd27 Mon Sep 17 00:00:00 2001 From: Ashish Kumar Date: Sun, 21 Jul 2019 19:20:35 +0530 Subject: [PATCH 3/3] Fix category search bug (#3080) * Closes #3027 * Concat category search with search with image title list and default categories * Fixed unit test CategoriesPresenterTest.searchForCategoriesTest * bug fix [search for categories even when the edit text is empty (perform title based search)] * Category items should show the selected item on top --- .../nrw/commons/category/CategoriesModel.java | 51 ++++++++++++++++++- .../repository/UploadRemoteDataSource.java | 21 ++++++-- .../commons/repository/UploadRepository.java | 16 +++++- .../categories/CategoriesPresenter.java | 3 ++ .../categories/UploadCategoriesFragment.java | 4 +- .../commons/upload/CategoriesPresenterTest.kt | 4 +- 6 files changed, 89 insertions(+), 10 deletions(-) diff --git a/app/src/main/java/fr/free/nrw/commons/category/CategoriesModel.java b/app/src/main/java/fr/free/nrw/commons/category/CategoriesModel.java index 9b084da49..21c39bf97 100644 --- a/app/src/main/java/fr/free/nrw/commons/category/CategoriesModel.java +++ b/app/src/main/java/fr/free/nrw/commons/category/CategoriesModel.java @@ -48,8 +48,21 @@ public class CategoriesModel{ */ public Comparator sortBySimilarity(final String filter) { Comparator stringSimilarityComparator = StringSortingUtils.sortBySimilarity(filter); - return (firstItem, secondItem) -> stringSimilarityComparator - .compare(firstItem.getName(), secondItem.getName()); + return (firstItem, secondItem) -> { + //if the category is selected, it should get precedence + if (null != firstItem && firstItem.isSelected()) { + if (null != secondItem && secondItem.isSelected()) { + return stringSimilarityComparator + .compare(firstItem.getName(), secondItem.getName()); + } + return -1; + } + if (null != secondItem && secondItem.isSelected()) { + return 1; + } + return stringSimilarityComparator + .compare(firstItem.getName(), secondItem.getName()); + }; } /** @@ -255,4 +268,38 @@ public class CategoriesModel{ this.categoriesCache.clear(); this.selectedCategories.clear(); } + + /** + * Search for categories + */ + public Observable searchCategories(String query, List imageTitleList) { + if (TextUtils.isEmpty(query)) { + return gpsCategories() + .concatWith(titleCategories(imageTitleList)) + .concatWith(recentCategories()); + } + + return mwApi + .searchCategories(query, SEARCH_CATS_LIMIT) + .map(s -> new CategoryItem(s, false)); + } + + /** + * Returns default categories + */ + public Observable getDefaultCategories(List titleList) { + Observable directCategories = directCategories(); + if (hasDirectCategories()) { + Timber.d("Image has direct Categories"); + return directCategories + .concatWith(gpsCategories()) + .concatWith(titleCategories(titleList)) + .concatWith(recentCategories()); + } else { + Timber.d("Image has no direct Categories"); + return gpsCategories() + .concatWith(titleCategories(titleList)) + .concatWith(recentCategories()); + } + } } 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 938b6f30d..1ebebb7b7 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 @@ -11,10 +11,8 @@ import fr.free.nrw.commons.upload.UploadModel; import fr.free.nrw.commons.upload.UploadModel.UploadItem; import io.reactivex.Observable; import io.reactivex.Single; - import java.util.Comparator; import java.util.List; - import javax.inject.Inject; import javax.inject.Singleton; @@ -167,7 +165,7 @@ public class UploadRemoteDataSource { } /** - * ask the UplaodModel for the image quality of the UploadItem + * ask the UploadModel for the image quality of the UploadItem * * @param uploadItem * @param shouldValidateTitle @@ -176,4 +174,21 @@ public class UploadRemoteDataSource { public Single getImageQuality(UploadItem uploadItem, boolean shouldValidateTitle) { return uploadModel.getImageQuality(uploadItem, shouldValidateTitle); } + + /** + * Ask the CategoriesModel to search categories + * @param query + * @param imageTitleList + * @return + */ + public Observable searchCategories(String query, List imageTitleList) { + return categoriesModel.searchCategories(query, imageTitleList); + } + + /** + * Ask the CategoriesModel for default categories + */ + public Observable getDefaultCategories(List imageTitleList) { + return categoriesModel.getDefaultCategories(imageTitleList); + } } 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 dbd0f6134..9a157edce 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 @@ -8,10 +8,8 @@ import fr.free.nrw.commons.upload.SimilarImageInterface; import fr.free.nrw.commons.upload.UploadModel.UploadItem; import io.reactivex.Observable; import io.reactivex.Single; - import java.util.Comparator; import java.util.List; - import javax.inject.Inject; import javax.inject.Singleton; @@ -262,4 +260,18 @@ public class UploadRepository { public void setSelectedLicense(String licenseName) { localDataSource.setSelectedLicense(licenseName); } + + /** + * Ask the RemoteDataSource to search for categories + */ + public Observable searchCategories(String query, List imageTitleList) { + return remoteDataSource.searchCategories(query, imageTitleList); + } + + /** + * Ask the RemoteDataSource to get default categories + */ + public Observable getDefaultCategories(List imageTitleList) { + return remoteDataSource.getDefaultCategories(imageTitleList); + } } diff --git a/app/src/main/java/fr/free/nrw/commons/upload/categories/CategoriesPresenter.java b/app/src/main/java/fr/free/nrw/commons/upload/categories/CategoriesPresenter.java index a0a776246..f690d1de6 100644 --- a/app/src/main/java/fr/free/nrw/commons/upload/categories/CategoriesPresenter.java +++ b/app/src/main/java/fr/free/nrw/commons/upload/categories/CategoriesPresenter.java @@ -80,6 +80,9 @@ public class CategoriesPresenter implements CategoriesContract.UserActionListene .observeOn(ioScheduler) .concatWith( repository.searchAll(query, imageTitleList) + .mergeWith(repository.searchCategories(query, imageTitleList)) + .concatWith(TextUtils.isEmpty(query) ? repository + .getDefaultCategories(imageTitleList) : Observable.empty()) ) .filter(categoryItem -> !repository.containsYear(categoryItem.getName())) .distinct(); diff --git a/app/src/main/java/fr/free/nrw/commons/upload/categories/UploadCategoriesFragment.java b/app/src/main/java/fr/free/nrw/commons/upload/categories/UploadCategoriesFragment.java index 48485c17a..77fb6ecb3 100644 --- a/app/src/main/java/fr/free/nrw/commons/upload/categories/UploadCategoriesFragment.java +++ b/app/src/main/java/fr/free/nrw/commons/upload/categories/UploadCategoriesFragment.java @@ -89,7 +89,7 @@ public class UploadCategoriesFragment extends UploadBaseFragment implements Cate @Override public void onResume() { super.onResume(); - if (presenter != null && isVisible && (categories == null || categories.isEmpty())) { + if (presenter != null && isVisible) { presenter.searchForCategories(null); } } @@ -193,7 +193,7 @@ public class UploadCategoriesFragment extends UploadBaseFragment implements Cate super.setUserVisibleHint(isVisibleToUser); isVisible = isVisibleToUser; - if (presenter != null && isResumed() && (categories == null || categories.isEmpty())) { + if (presenter != null && isResumed()) { presenter.searchForCategories(null); } } diff --git a/app/src/test/kotlin/fr/free/nrw/commons/upload/CategoriesPresenterTest.kt b/app/src/test/kotlin/fr/free/nrw/commons/upload/CategoriesPresenterTest.kt index eae8defc5..c54e9c2d9 100644 --- a/app/src/test/kotlin/fr/free/nrw/commons/upload/CategoriesPresenterTest.kt +++ b/app/src/test/kotlin/fr/free/nrw/commons/upload/CategoriesPresenterTest.kt @@ -57,7 +57,9 @@ class CategoriesPresenterTest { fun searchForCategoriesTest() { Mockito.`when`(repository?.sortBySimilarity(ArgumentMatchers.anyString())).thenReturn(Comparator { _, _ -> 1 }) Mockito.`when`(repository?.selectedCategories).thenReturn(categoryItems) - Mockito.`when`(repository?.searchAll(ArgumentMatchers.anyString(), ArgumentMatchers.anyList())).thenReturn(Observable.empty()) + Mockito.`when`(repository?.searchAll(ArgumentMatchers.anyString(), ArgumentMatchers.anyList())).thenReturn(testObservable) + Mockito.`when`(repository?.searchCategories(ArgumentMatchers.anyString(), ArgumentMatchers.anyList())).thenReturn(testObservable) + Mockito.`when`(repository?.getDefaultCategories(ArgumentMatchers.anyList())).thenReturn(testObservable) categoriesPresenter?.searchForCategories("test") verify(view)?.showProgress(true) verify(view)?.showError(null)