From fec6dba341257998d65f7e217c06e608b547d451 Mon Sep 17 00:00:00 2001 From: Shashank Kumar <126143257+shashankiitbhu@users.noreply.github.com> Date: Sun, 17 Mar 2024 19:16:13 +0530 Subject: [PATCH] Fix Crash in LocationPickerActivity when device configuration is changed (#5500) * Fix Crash in LocationPickerActivity when device configuration is changed (UI Modes) * clean up * fix * fix * fix * fix * removed faulty test * cleanup * fix and tests added --- .../LocationPicker/LocationPicker.java | 12 +++ .../LocationPickerActivity.java | 90 +++++++++++++++++-- .../LocationPickerConstants.java | 3 + .../commons/media/MediaDetailFragment.java | 57 ++---------- .../LocationPickerActivityUnitTests.kt | 9 +- 5 files changed, 111 insertions(+), 60 deletions(-) diff --git a/app/src/main/java/fr/free/nrw/commons/LocationPicker/LocationPicker.java b/app/src/main/java/fr/free/nrw/commons/LocationPicker/LocationPicker.java index 696dc9810..baa322180 100644 --- a/app/src/main/java/fr/free/nrw/commons/LocationPicker/LocationPicker.java +++ b/app/src/main/java/fr/free/nrw/commons/LocationPicker/LocationPicker.java @@ -3,6 +3,7 @@ package fr.free.nrw.commons.LocationPicker; import android.app.Activity; import android.content.Intent; import com.mapbox.mapboxsdk.camera.CameraPosition; +import fr.free.nrw.commons.Media; /** * Helper class for starting the activity @@ -52,6 +53,17 @@ public final class LocationPicker { return this; } + /** + * Gets and puts media in intent + * @param media Media + * @return LocationPicker.IntentBuilder + */ + public LocationPicker.IntentBuilder media( + final Media media) { + intent.putExtra(LocationPickerConstants.MEDIA, media); + return this; + } + /** * Gets and sets the activity * @param activity Activity diff --git a/app/src/main/java/fr/free/nrw/commons/LocationPicker/LocationPickerActivity.java b/app/src/main/java/fr/free/nrw/commons/LocationPicker/LocationPickerActivity.java index ef4b31a36..5b1e781d7 100644 --- a/app/src/main/java/fr/free/nrw/commons/LocationPicker/LocationPickerActivity.java +++ b/app/src/main/java/fr/free/nrw/commons/LocationPicker/LocationPickerActivity.java @@ -31,8 +31,10 @@ import androidx.core.content.ContextCompat; import com.google.android.material.floatingactionbutton.FloatingActionButton; import com.mapbox.mapboxsdk.camera.CameraPosition; import com.mapbox.mapboxsdk.geometry.LatLng; +import fr.free.nrw.commons.Media; import fr.free.nrw.commons.R; import fr.free.nrw.commons.Utils; +import fr.free.nrw.commons.coordinates.CoordinateEditHelper; import fr.free.nrw.commons.filepicker.Constants; import fr.free.nrw.commons.kvstore.JsonKvStore; import fr.free.nrw.commons.location.LocationPermissionsHelper; @@ -41,6 +43,9 @@ import fr.free.nrw.commons.location.LocationPermissionsHelper.LocationPermission import fr.free.nrw.commons.location.LocationServiceManager; import fr.free.nrw.commons.theme.BaseActivity; import fr.free.nrw.commons.utils.SystemThemeUtils; +import fr.free.nrw.commons.utils.ViewUtilWrapper; +import io.reactivex.android.schedulers.AndroidSchedulers; +import io.reactivex.schedulers.Schedulers; import java.util.List; import javax.inject.Inject; import javax.inject.Named; @@ -52,13 +57,22 @@ import org.osmdroid.views.overlay.Marker; import org.osmdroid.views.overlay.Overlay; import org.osmdroid.views.overlay.ScaleDiskOverlay; import org.osmdroid.views.overlay.TilesOverlay; +import timber.log.Timber; /** * Helps to pick location and return the result with an intent */ public class LocationPickerActivity extends BaseActivity implements LocationPermissionCallback { - + /** + * coordinateEditHelper: helps to edit coordinates + */ + @Inject + CoordinateEditHelper coordinateEditHelper; + /** + * media : Media object + */ + private Media media; /** * cameraPosition : position of picker */ @@ -125,6 +139,13 @@ public class LocationPickerActivity extends BaseActivity implements @Inject LocationServiceManager locationManager; + /** + * Constants + */ + private static final String CAMERA_POS = "cameraPosition"; + private static final String ACTIVITY = "activity"; + + @SuppressLint("ClickableViewAccessibility") @Override protected void onCreate(@Nullable final Bundle savedInstanceState) { @@ -145,8 +166,12 @@ public class LocationPickerActivity extends BaseActivity implements cameraPosition = getIntent() .getParcelableExtra(LocationPickerConstants.MAP_CAMERA_POSITION); activity = getIntent().getStringExtra(LocationPickerConstants.ACTIVITY_KEY); + media = getIntent().getParcelableExtra(LocationPickerConstants.MEDIA); + }else{ + cameraPosition = savedInstanceState.getParcelable(CAMERA_POS); + activity = savedInstanceState.getString(ACTIVITY); + media = savedInstanceState.getParcelable("sMedia"); } - bindViews(); addBackButtonListener(); addPlaceSelectedButton(); @@ -220,7 +245,10 @@ public class LocationPickerActivity extends BaseActivity implements */ private void addBackButtonListener() { final ImageView backButton = findViewById(R.id.maplibre_place_picker_toolbar_back_button); - backButton.setOnClickListener(view -> finish()); + backButton.setOnClickListener(v -> { + finish(); + }); + } /** @@ -311,14 +339,42 @@ public class LocationPickerActivity extends BaseActivity implements + mapView.getMapCenter().getLongitude()); applicationKvStore.putString(LAST_ZOOM, mapView.getZoomLevel() + ""); } - final Intent returningIntent = new Intent(); - returningIntent.putExtra(LocationPickerConstants.MAP_CAMERA_POSITION, - new CameraPosition(new LatLng(mapView.getMapCenter().getLatitude(), - mapView.getMapCenter().getLongitude()), 14f, 0, 0)); - setResult(AppCompatActivity.RESULT_OK, returningIntent); + + if (media == null) { + final Intent returningIntent = new Intent(); + returningIntent.putExtra(LocationPickerConstants.MAP_CAMERA_POSITION, + new CameraPosition(new LatLng(mapView.getMapCenter().getLatitude(), + mapView.getMapCenter().getLongitude()), 14f, 0, 0)); + setResult(AppCompatActivity.RESULT_OK, returningIntent); + } else { + updateCoordinates(String.valueOf(mapView.getMapCenter().getLatitude()), + String.valueOf(mapView.getMapCenter().getLongitude()), + String.valueOf(0.0f)); + } + finish(); } + /** + * Fetched coordinates are replaced with existing coordinates by a POST API call. + * @param Latitude to be added + * @param Longitude to be added + * @param Accuracy to be added + */ + public void updateCoordinates(final String Latitude, final String Longitude, + final String Accuracy) { + if (media == null) { + return; + } + compositeDisposable.add(coordinateEditHelper.makeCoordinatesEdit(getApplicationContext(),media, + Latitude, Longitude, Accuracy) + .subscribeOn(Schedulers.io()) + .observeOn(AndroidSchedulers.mainThread()) + .subscribe(s -> { + Timber.d("Coordinates are added."); + })); + } + /** * Center the camera on the last saved location */ @@ -458,4 +514,22 @@ public class LocationPickerActivity extends BaseActivity implements mapView.getOverlays().add(startMarker); } + /** + * Saves the state of the activity + * @param outState Bundle + */ + @Override + public void onSaveInstanceState(@NonNull final Bundle outState) { + super.onSaveInstanceState(outState); + if(cameraPosition!=null){ + outState.putParcelable(CAMERA_POS, cameraPosition); + } + if(activity!=null){ + outState.putString(ACTIVITY, activity); + } + + if(media!=null){ + outState.putParcelable("sMedia", media); + } + } } diff --git a/app/src/main/java/fr/free/nrw/commons/LocationPicker/LocationPickerConstants.java b/app/src/main/java/fr/free/nrw/commons/LocationPicker/LocationPickerConstants.java index eb27e496c..060a15c88 100644 --- a/app/src/main/java/fr/free/nrw/commons/LocationPicker/LocationPickerConstants.java +++ b/app/src/main/java/fr/free/nrw/commons/LocationPicker/LocationPickerConstants.java @@ -11,6 +11,9 @@ public final class LocationPickerConstants { public static final String MAP_CAMERA_POSITION = "location.picker.cameraPosition"; + public static final String MEDIA + = "location.picker.media"; + private LocationPickerConstants() { } diff --git a/app/src/main/java/fr/free/nrw/commons/media/MediaDetailFragment.java b/app/src/main/java/fr/free/nrw/commons/media/MediaDetailFragment.java index bb2da3fe2..52eaed9c0 100644 --- a/app/src/main/java/fr/free/nrw/commons/media/MediaDetailFragment.java +++ b/app/src/main/java/fr/free/nrw/commons/media/MediaDetailFragment.java @@ -938,12 +938,14 @@ public class MediaDetailFragment extends CommonsDaggerSupportFragment implements } } - startActivityForResult(new LocationPicker.IntentBuilder() + + startActivity(new LocationPicker.IntentBuilder() .defaultLocation(new CameraPosition.Builder() .target(new LatLng(defaultLatitude, defaultLongitude)) .zoom(16).build()) .activityKey("MediaActivity") - .build(getActivity()), REQUEST_CODE); + .media(media) + .build(getActivity())); } @OnClick(R.id.description_edit) @@ -1121,32 +1123,7 @@ public class MediaDetailFragment extends CommonsDaggerSupportFragment implements @Nullable final Intent data) { super.onActivityResult(requestCode, resultCode, data); - if (requestCode == REQUEST_CODE && resultCode == RESULT_OK) { - - assert data != null; - final CameraPosition cameraPosition = LocationPicker.getCameraPosition(data); - - if (cameraPosition != null) { - - final String latitude = String.valueOf(cameraPosition.target.getLatitude()); - final String longitude = String.valueOf(cameraPosition.target.getLongitude()); - final String accuracy = String.valueOf(cameraPosition.target.getAltitude()); - String currentLatitude = null; - String currentLongitude = null; - - if (media.getCoordinates() != null) { - currentLatitude = String.valueOf(media.getCoordinates().getLatitude()); - currentLongitude = String.valueOf(media.getCoordinates().getLongitude()); - } - - if (!latitude.equals(currentLatitude) || !longitude.equals(currentLongitude)) { - updateCoordinates(latitude, longitude, accuracy); - } else if (media.getCoordinates() == null) { - updateCoordinates(latitude, longitude, accuracy); - } - } - - } else if (requestCode == REQUEST_CODE_EDIT_DESCRIPTION && resultCode == RESULT_OK) { + if (requestCode == REQUEST_CODE_EDIT_DESCRIPTION && resultCode == RESULT_OK) { final String updatedWikiText = data.getStringExtra(UPDATED_WIKITEXT); compositeDisposable.add(descriptionEditHelper.addDescription(getContext(), media, updatedWikiText) @@ -1174,11 +1151,6 @@ public class MediaDetailFragment extends CommonsDaggerSupportFragment implements progressBarEditDescription.setVisibility(GONE); editDescription.setVisibility(VISIBLE); - } else if (requestCode == REQUEST_CODE && resultCode == RESULT_CANCELED) { - viewUtil.showShortToast(getContext(), - requireContext() - .getString(R.string.coordinates_picking_unsuccessful)); - } else if (requestCode == REQUEST_CODE_EDIT_DESCRIPTION && resultCode == RESULT_CANCELED) { progressBarEditDescription.setVisibility(GONE); editDescription.setVisibility(VISIBLE); @@ -1196,24 +1168,6 @@ public class MediaDetailFragment extends CommonsDaggerSupportFragment implements media.setCaptions(updatedCaptions); } - /** - * Fetched coordinates are replaced with existing coordinates by a POST API call. - * @param Latitude to be added - * @param Longitude to be added - * @param Accuracy to be added - */ - public void updateCoordinates(final String Latitude, final String Longitude, - final String Accuracy) { - compositeDisposable.add(coordinateEditHelper.makeCoordinatesEdit(getContext(), media, - Latitude, Longitude, Accuracy) - .subscribeOn(Schedulers.io()) - .observeOn(AndroidSchedulers.mainThread()) - .subscribe(s -> { - Timber.d("Coordinates are added."); - coordinates.setText(prettyCoordinates(media)); - })); - } - @SuppressLint("StringFormatInvalid") @OnClick(R.id.nominateDeletion) public void onDeleteButtonClicked(){ @@ -1598,4 +1552,5 @@ public class MediaDetailFragment extends CommonsDaggerSupportFragment implements SharedPreferences imageBackgroundColorPref = this.getImageBackgroundColorPref(); return imageBackgroundColorPref.getInt(IMAGE_BACKGROUND_COLOR, DEFAULT_IMAGE_BACKGROUND_COLOR); } + } diff --git a/app/src/test/kotlin/fr/free/nrw/commons/locationpicker/LocationPickerActivityUnitTests.kt b/app/src/test/kotlin/fr/free/nrw/commons/locationpicker/LocationPickerActivityUnitTests.kt index ca1826c95..dcbb9ddd0 100644 --- a/app/src/test/kotlin/fr/free/nrw/commons/locationpicker/LocationPickerActivityUnitTests.kt +++ b/app/src/test/kotlin/fr/free/nrw/commons/locationpicker/LocationPickerActivityUnitTests.kt @@ -2,6 +2,7 @@ package fr.free.nrw.commons.locationpicker import android.content.Context import android.os.Looper +import android.util.Log import android.view.View import android.widget.Button import android.widget.ImageView @@ -20,10 +21,14 @@ import com.nhaarman.mockitokotlin2.times import com.nhaarman.mockitokotlin2.verify import com.nhaarman.mockitokotlin2.whenever import fr.free.nrw.commons.LocationPicker.LocationPickerActivity +import fr.free.nrw.commons.Media import fr.free.nrw.commons.TestCommonsApplication +import fr.free.nrw.commons.coordinates.CoordinateEditHelper import fr.free.nrw.commons.kvstore.JsonKvStore import fr.free.nrw.commons.upload.mediaDetails.UploadMediaDetailFragment.LAST_LOCATION import fr.free.nrw.commons.upload.mediaDetails.UploadMediaDetailFragment.LAST_ZOOM +import io.reactivex.android.plugins.RxAndroidPlugins +import io.reactivex.schedulers.Schedulers import org.junit.Assert import org.junit.Assert.* import org.junit.Before @@ -93,6 +98,7 @@ class LocationPickerActivityUnitTests { MockitoAnnotations.initMocks(this) context = RuntimeEnvironment.getApplication().applicationContext activity = Robolectric.buildActivity(LocationPickerActivity::class.java).get() + RxAndroidPlugins.setInitMainThreadSchedulerHandler { Schedulers.trampoline() } Whitebox.setInternalState(activity, "mapView", mapView) Whitebox.setInternalState(activity, "applicationKvStore", applicationKvStore) @@ -165,4 +171,5 @@ class LocationPickerActivityUnitTests { } -} \ No newline at end of file + +}