apps-android-commons
apps-android-commons copied to clipboard
commons-app
Fixes Location related flow of the app #5256 , #5461, #5490
Description (required) This PR aims to fix the location-related flow of the app's various fragments and activities which use the map. I apologise for combining 3 issues into a single PR, but all three were somewhat related and had to be fixed together. Fixing #5461 and #5490 required the location-related flow to be fixed since most of the problems were happening because of the migration from MapBox to OSMDroid.
Fixes #5256, #5461, #5490
What changes did you make and why? I changed the location flow of the fragments and activities, which were using maps. I added the dialogs and intents to the LocationPermissionHelper to remove redundant code. Also, fixed the incorrect behaviour of the target icon.
Tests performed (required)
Tested prodDebug on OnePlusNord CE 2 Lite with API level 31.
Screenshots (for UI changes only)
I tried the app using the changes in this PR. When the app doesn't have the storage permission, it seems to show the "Media location access denied" dialog but it didn't trigger the storage permission request the first time I hit "Ok". Could you cross-check this?
Also, as noted in #5276, the string we show in that "Media location access denied" dialog is misleading since the description mentions nothing about us going to request permissions. Further, we only seem to be requesting the external storage permission in reality. Are you observing a similar behaviour? If so, could you possibly improve the experience of that dialog?
For the note, I tested this on a OnePlus Nord running Android 12
I further noticed the "in-app camera can't record location" toast appearing for a completely unrelated screen. It also seems not to be appearing for the in-app camera flow which is strange.
Check out the following recording to understand what I'm conveying more clearly: https://drive.google.com/file/d/1lvNDtu9YljMhKDioJIr6LYq9WhXlDp7u/view?usp=sharing
Faced yet another strange scenario in the Nearby screen. where the app doesn't seem to be asking for the location permission even when the permission level is set to "Ask every time". Check out this recording: https://drive.google.com/file/d/17YKFpUajv0twsLqelL_x7MpeB747aGVx/view?usp=sharing
The only way to grant location permission seems to be to set the level to "Allow only while using the app". This shouldn't be the case, though.
I tried the app using the changes in this PR. When the app doesn't have the storage permission, it seems to show the "Media location access denied" dialog but it didn't trigger the storage permission request the first time I hit "Ok". Could you cross-check this?
Interesting, because this seems to work perfectly on my device, OnePlus Nord CE 2 Lite, running Android 12 as well. Here's the link: https://drive.google.com/file/d/1fMKSbSQwgxTMJuYM2TlEVgyhz_nnzKsB/view?usp=share_link
Also, as noted in #5276, the string we show in that "Media location access denied" dialog is misleading since the description mentions nothing about us going to request permissions. Further, we only seem to be requesting the external storage permission in reality. Are you observing a similar behaviour? If so, could you possibly improve the experience of that dialog?
I agree the permissions dialog is misleading. I'll try to improve the dialog and tweak the code. Also, I think the toast displayed when user clicks on cancel, vaguely says "Permissions are required for functionality", which can be improved. Should I make seperate PR for these, since they seem to fall out of scope of this PR, which is to fix location-related flows?
I further noticed the "in-app camera can't record location" toast appearing for a completely unrelated screen.
There seems to be a mismatch in the text passed to that particular Toast, I'll change it, my apologies for that
It also seems not to be appearing for the in-app camera flow which is strange.
While testing initially, the app did not show any Toast, so I assumed that is the expected behaviour. Nevertheless, I'll add a Toast there :)
Faced yet another strange scenario in the Nearby screen. where the app doesn't seem to be asking for the location permission even when the permission level is set to "Ask every time". Check out this recording: https://drive.google.com/file/d/17YKFpUajv0twsLqelL_x7MpeB747aGVx/view?usp=sharing
Thanks for noticing this bug; I have fixed it now. While testing for this bug, I found something interesting, that if initially, the user gives permission as 'Only this time' then closes the app and reopens it, then, PermissionUtils.hasPermission(activity, new String[]{Manifest.permission.ACCESS_FINE_LOCATION}) is returning true. I wonder how/why this is happening as clicking 'Only this time' means that the app has permission till it is not destroyed, but here it has permission even after removing it from recent apps.
On this branch, the Requesting location permission dialog keeps popping up even after I tap on "Don't ask again" [Screencast]. I know the two dialogs are different, but if users do not want to share their location, continuously asking for it whenever they switch to Nearby tab can be annoying, plus it gives an impression that the functionality won't work at all without giving location access, though that's not the case. We can still manually choose a location and upload (the coordinates can be misleading, but that is currently supported by the app). Even if we don't allow such uploads, users should be permitted to use the map in Explore, at least. How about adding a toast for it?
I tried to reproduce this issue on devices running API level 31 and API 27, but it never occured. The dialog requesting user to give permission by going to settings, pops up only when the target icon is clicked, not when switched to the Nearby tab. Here's the recording of the device running API 27: https://drive.google.com/file/d/1w9l4ePTxnn7q-3q758QfID8Y87aFHAkA/view?usp=sharing
Also, the dialog to show nearby places when a user wants to use the in-app camera is a bit unrelated [Screencast]. Could you please check this as well?
Um, I realised that I missed fixing the flow of the in-app camera location permission. For the time being, I have fixed this particular issue, but the code already present has some redundancy, which should be fixed. If you agree, I will fix the in-app camera location flow in a separate PR, and we'll close #5256 only after that PR is merged since this PR is already handling 3 issues.
For me, it's asking for location access even when I use the Uploaded via mobile tab [Screencast]
If I am not wrong, your device also runs API 27 right?
Yes, it's API level 27 only.
Hmm, then how can it happen that it's occurring for your device but I am not able to reproduce it. Not even this issue:
For me, it's asking for location access even when I use the Uploaded via mobile tab [Screencast]
@RitikaPahwa4444 I did a rigorous dry run of my code and tested on a variety of devices with different API versions, but couldn't reproduce the bug you pointed out, here are the recordings of some:
https://drive.google.com/file/d/1_o34tWpNcpxXpgq0bCaYAt5AWz285gGH/view?usp=sharing
https://drive.google.com/file/d/1ahquM-jjyWZU6ZVZR09WS8xHqdkhqjpp/view?usp=sharing
https://drive.google.com/file/d/1LgZPrQJPSppFcAnatOaHe8U8HW0EgImN/view?usp=sharing
What should be my next steps to solve this?
I'm sorry for my limited availability on weekdays. I tried resetting the app but I'm still facing the same issue. I'll try reinstalling the app tomorrow and share my observations.
Tried again. Happening on a fresh installation as well. I didn't even open any activity having a map :( Explore > Featured and then Uploaded via Mobile.
Tried again. Happening on a fresh installation as well. I didn't even open any activity having a map :(
Explore > Featuredand thenUploaded via Mobile.
I have tested on devices running API Levels 26 - 31, but I couldn't reproduce the popping up of that dialog whenever that fragment is visible. Here's the screencast of a device running API 26, which I tried just now. Can it be because of some device-specific issue? Could you try running the app on any emulator, or some other device?
And with respect to the 'Upload via Mobile', I saw that this was there in previous versions also. Though I couldn't reproduce the popping up of this dialog, but on a fresh installation, if you open the 'Upload via Mobile' tab (without opening any other maps), you will be prompted for location permission, even though that fragment has got nothing to do with location permissions. Here's the recording of the app downloaded from the Play Store. I'll check why this behaviour is happening.
Hey @ShashwatKedia! Thanks for the changes. I could confirm the subtle issues I mentioned are not seen with the latest changes in this PR. I noticed one awkward thing so far, when the app doesn't have files/media access, it show the "Media location access denied pop-up" on opn. I dismissed the pop-up and granted the permission. Now when I try to upload an image, the app is showing the request for "Requesting external storage" pop-up. IIUC, this dialog shouldn't show-up as the already has that permission. Could you check this?
Also, tweaking the text of the "Media location access denied" separately makes sense. Feel free to raise a separate PR for the same 🙂
Regarding the issue of the "location permission being requested on opening Nearby". I too don't observe this behaviour. I get the pop-up only when tapping on the target icon. Not sure why that's behaving differently for Ritika. Could you clarify if you denied the permission by choosing "Do not ask again" Ritika? May be that could trigger some different behaviour? 🤔
About the permission popping up when the "Upload via mobile" screen is opened, there's a chance for this to happen because Android could try to initialize the "Map" screen of Explore when the user is in the "Upload via mobile" screen presuming that the user could potentially switch to the "Map" screen soon. This is a kind of optimization that Android does to ensure the view renders quickly. It's not clear why the pop-up is triggered, though.
Could you clarify if you denied the permission by choosing "Do not ask again" Ritika?
Yes, I checked "Do not ask again", so the system dialog does not appear but the custom one requesting for it keeps appearing.
there's a chance for this to happen because Android could try to initialize the "Map" screen of Explore when the user is in the "Upload via mobile" screen presuming that the user could potentially switch to the "Map" screen soon. This is a kind of optimization that Android does to ensure the view renders quickly.
Interesting optimisation. Android is unpredictable. On one hand it calls location as a sensitive permission, and then presumably introduces an optimisation that leaves users perplexed. Since it is not related to this branch, we can take this up as a separate issue.
Hey @ShashwatKedia! Thanks for the changes. I could confirm the subtle issues I mentioned are not seen with the latest changes in this PR. I noticed one awkward thing so far, when the app doesn't have files/media access, it show the "Media location access denied pop-up" on opn. I dismissed the pop-up and granted the permission. Now when I try to upload an image, the app is showing the request for "Requesting external storage" pop-up. IIUC, this dialog shouldn't show-up as the already has that permission. Could you check this?
Sure, I initially fixed only the location-related permissions, but I guess I will fix the storage-related flow in this PR only :)
Could you clarify if you denied the permission by choosing "Do not ask again" Ritika? May be that could trigger some different behaviour? 🤔
Um, but I tried to reproduce the issue by following the same steps as Ritika showed in her screencast (which does include the "Do not ask again" check), but I couldn't reproduce this issue. I tried on a variety of devices with API levels ranging from 26-31 :(
About the permission popping up when the "Upload via mobile" screen is opened, there's a chance for this to happen because Android could try to initialize the "Map" screen of Explore when the user is in the "Upload via mobile" screen presuming that the user could potentially switch to the "Map" screen soon. This is a kind of optimization that Android does to ensure the view renders quickly. It's not clear why the pop-up is triggered, though.
Yeah, I tried debugging this and saw that when the "Uploaded via Mobile" is opened after the "Featured" tab, then it requests for location permissions (I added logs in the onViewCreated & onCreateView of ExploreMapFragment and found that, indeed, these were getting called instead of those in ExploreListRootFragment). But this doesn't happen if we select the "Uploaded via Mobile" right after opening the "Map" screen.
Could you clarify if you denied the permission by choosing "Do not ask again" Ritika?
Yes, I checked "Do not ask again", so the system dialog does not appear but the custom one requesting for it keeps appearing.
Got it. That might have something to do with the issue. I'll see if I could reproduce this on a different device.
Sure, I initially fixed only the location-related permissions, but I guess I will fix the storage-related flow in this PR only :)
I was of the presumption that this was somehow due to the changes in this PR. I don't observe this in the 4.2.1 available in play store. If this is not related to your changes, we could take it up separately.
Um, but I tried to reproduce the issue by following the same steps as Ritika showed in her screencast (which does include the "Do not ask again" check), but I couldn't reproduce this issue. I tried on a variety of devices with API levels ranging from 26-31 :(
Thanks for your effort in trying to dig into this! I could understand the difficulty with identifying the root cause when an issue isn't reproducible to us. Unfortunately, Android development is sprinkled with oddities / outliers here and there. We'll see if we could figure this out to some extent. Otherwise, we could handle it separately.
But this doesn't happen if we select the "Uploaded via Mobile" right after opening the "Map" screen.
That's understandable given the fragment has already been created. I think in the ideal world, we should request permission when the fragment is visible not immediately upon creation.
I was of the presumption that this was somehow due to the changes in this PR. I don't observe this in the 4.2.1 available in play store. If this is not related to your changes, we could take it up separately.
I'm believe these issues are not related to changes I made, as I can reproduce these on the main branch [Screencast], but I thought you wanted me to fix these as well. I think taking it up separately would be better since it would help to keep this PR within the scope of the issues mentioned and will help in the future if any issues related to these changes pop up. @sivaraam Should I create a new issue for this?
Thanks for your effort in trying to dig into this! I could understand the difficulty with identifying the root cause when an issue isn't reproducible to us. Unfortunately, Android development is sprinkled with oddities / outliers here and there. We'll see if we could figure this out to some extent. Otherwise, we could handle it separately.
Thanks for your kind words. Indeed, I am learning a lot about Android while contributing in this project, things which can never be taught in any course for Android dev :)
I'm believe these issues are not related to changes I made, as I can reproduce these on the main branch [Screencast], but I thought you wanted me to fix these as well. I think taking it up separately would be better since it would help to keep this PR within the scope of the issues mentioned and will help in the future if any issues related to these changes pop up. @sivaraam Should I create a new issue for this?
Sure. Feel free to do so 🙂
Thanks for your kind words. Indeed, I am learning a lot about Android while contributing in this project, things which can never be taught in any course for Android dev :)
Indeed. As they say "Experience is the best teacher"
In the in-app camera flow, after showing the consent dialog the app would usually show the prompt asking to grant location permission and turn on GPS (if it's turned off). I'm not seeing the location permission request when app has been previously denied permission. I also don't see the dialog prompting users to turn on GPS if its turned off.
Could you check this?
In the in-app camera flow, after showing the consent dialog the app would usually show the prompt asking to grant location permission and turn on GPS (if it's turned off). I'm not seeing the location permission request when app has been previously denied permission.
Um, most probably, you would have denied location permission twice before testing the in-app camera, as Android allows the app to request any permission only twice if the user denies it. If I deny the permission only once, then I can see a location permission prompt Screencast.
I also don't see the dialog prompting users to turn on GPS if its turned off.
On the main branch, the app doesn't ask to turn on GPS if it's off, even after location permission is given. I thought this was intended not to disturb the user every time their GPS is off. On the contrary, I noticed that if I deny the permission once, and hit "OK" in the next rationale dialog, it pops up a dialog to turn on GPS, even though location permission is not there with the app. This Screencast shows both behaviours I'm referring to. I'll fix this incorrect behaviour and add the location off dialog :)
@sivaraam I have added the required changes and fixed the in-app camera location flow as well :)
If I deny the permission only once, then I can see a location permission prompt Screencast.
Got it. I think I was misremembering that we would show a pop-up like Nearby requesting to turn-on location permission if it's missing (the pop-up that we show when user denied twice). I think we were only showing the toast.
On the main branch, the app doesn't ask to turn on GPS if it's off, even after location permission is given.
This is what I find odd. It seems like we're not asking for it after obtaining the location permission for the first time. I was supposing we did this in 4.2.1 release. But that doesn't seem to be the case. Do you remeber what was our decision about this @RitikaPahwa4444 ? 🤔
This is what I find odd. It seems like we're not asking for it after obtaining the location permission for the first time. I was supposing we did this in 4.2.1 release. But that doesn't seem to be the case. Do you remeber what was our decision about this @RitikaPahwa4444 ? 🤔
I could see that there was a dialog to ask user to turn on GPS, but it was placed incorrectly to run when user denies permission and clicks okay in the rationale. I think there might have been some confusion previously, anyways, I've already fixed it in my latest commit, now the app asks the user to turn on GPS, if permission is already given.
This is what I find odd. It seems like we're not asking for it after obtaining the location permission for the first time. I was supposing we did this in 4.2.1 release. But that doesn't seem to be the case. Do you remeber what was our decision about this @RitikaPahwa4444 ? 🤔
Thanks Shashwat for spotting this! Sivaraam, this was the last discussion I remember about these two popups. I did not check much thereafter, and just realised things on the production app are very different from what I was expecting. I don't even see any toasts now.