#3732 - Nearby Tab Accessible Without Location Permission #4259
Conversation
There was a problem hiding this comment.
Nice, it works well!
But one major thing: I think the app should remember the user's choice. Consider this scenario:
- User opens the app
- User refuses to give location permission
- User closes the app
- Next day, user opens the app again
- User is asked for permission again
I think that step 5 makes the app too insisting, users will complain that the app creepily tries to get location permission every time despite the user refusing each time.
Would you mind persisting the user's choice? Sorry for the additional work.
|
@nicolas-raoul I have made the changes now it will remember the user choice, and the user can enable its location by clicking the recenter button. |
| } | ||
| presenter.onMapReady(); | ||
| registerUnregisterLocationListener(false); | ||
| if(!applicationKvStore.getBoolean("NotDisplayLocationPermissionAlertBox", false) || |
There was a problem hiding this comment.
Would you mind calling it DoNotAskForLocationPermission? If that sounds OK to you.
|
It works great! Now actually the main activity's insistence on getting that permission becomes apparent... Thanks a lot! |
|
Okay! I will make the changes in this PR only. |
There was a problem hiding this comment.
Thanks!
I just tried your PR.
Permission is only asked when opening the Nearby tab for the first time. The "near place" notification in Contributions only appears after Nearby has been accessed at least once. It is a bit different from the previous behavior, but I think it makes sense, and avoids asking for permission the first time the user opens the app. It also makes it more likely that the user will accept to give that permission, as it is asked when the user opens the "Nearby" tab.
Everyone: A last thing that I am not sure about is the case convention for new kvstore keys, should it be DoNotAskForLocationPermission or do_not_ask_for_location_permission or something else, and should the string be store somewhere, such as CommonsApplication.java?
| @@ -451,6 +451,7 @@ private void checkPermissionsAndShowNearbyCardView() { | |||
| onLocationPermissionGranted(); | |||
| } else if (shouldShowRequestPermissionRationale(Manifest.permission.ACCESS_FINE_LOCATION) | |||
| && store.getBoolean("displayLocationPermissionForCardView", true) | |||
| && !store.getBoolean("DoNotAskForLocationPermission", false) | |||
There was a problem hiding this comment.
I checked our repository and seems like we don't have an accurate convention for naming stored variables. I think it should be in lower camel case (doNotAskForLocationPermission). The only exception in Google's Java style guide seems like CONSTANT_CASE to use underscore, which is not our case.
There was a problem hiding this comment.
Understood Nes, thanks so much! :-)
@nicolas-raoul we never store these strings in somewhere, I think we should keep to do same: not store, just hardcode it for now. However, I think it could be nice to have such an organization for future. |
| @@ -138,7 +138,8 @@ public void onPermissionRationaleShouldBeShown(PermissionRequest permission, | |||
| activity.getString(rationaleMessage), | |||
| activity.getString(android.R.string.ok), | |||
| activity.getString(android.R.string.cancel), | |||
| token::continuePermissionRequest, token::cancelPermissionRequest); | |||
| token::continuePermissionRequest, token::cancelPermissionRequest, | |||
| null,false); | |||
There was a problem hiding this comment.
Very minor: space after ,
Or how about this?
token::continuePermissionRequest,
token::cancelPermissionRequest,
null,
false
There was a problem hiding this comment.
Yes, that looks better, I have made the changes!
|
Thanks a lot @Pratham2305 ! |
Description (required)
Fixes #3732
What changes did you make and why?
Summary :
Nearby tab initially show a zoom-out map, which will be zoomed to the user's location if permission granted, and if not then still a zoom-out map will be shown with "Search this Area" enabled.
Major Changes:
Added a function that will set the Tower of London as the camera position on completely zoomed-out map, instead of going back to the contribution tab, if the user denies the location permission and also enabled the "Search this Area" button.
Changed the functionality of recenter button when permission is denied, as it is of no use if the user's location is unknown. and also disable the disappearing of the permission alertbox by clicking outside the box which is causing the empty map problem.
Tests performed (required)
Tested betaDebug on Pixel 3 with API level 29.