5195: Fix crash on setting location for pictures with no EXIF location #5205
Conversation
| * Ask for location access in case it is unavailable | ||
| */ | ||
| if(locationComponent.getLastKnownLocation() == null){ | ||
| showLocationOffDialog(); |
There was a problem hiding this comment.
IIUC, this particular invocation is getting invoked at all as the if statement above ...
if (PermissionsManager.areLocationPermissionsGranted(this))... would be evaluated to false when location is not enabled. You should look into handling this differently. Further, I'm not sure enableLocationComponent is the correct method to place this check since it is invoked during the initialization of the activity. This results in the location request dialog being show even before the user hits the "target" icon. It would be better if we only request for the permission when the user actually hits the "target" icon.
There was a problem hiding this comment.
It is getting invoked even when the if statement above checks it beforehand. I had added it for the case where we don't have the last known location. I'd used enableLocationComponent since most of the code was redundant. It indeed shows up before tapping the target icon and shows the message accordingly. I'll make the changes you've suggested and request for the permission only on tapping the target icon
There was a problem hiding this comment.
Are you getting prompted with the location request dialog when the app does not have the location permission? I tested with this MR and I'm not shown the dialog when hitting the target icon.
To me, this seems to be due to what I've mentioned just above.
There was a problem hiding this comment.
Yes, I am getting the dialog again in case I deny in the beginning and use the target button later:
location.dialog.mp4
It doesn't ask for it again in case I allow in the beginning itself.
There was a problem hiding this comment.
@sivaraam, I have defined a separate function for this purpose and removed the dialog in the beginning. Now it asks for permission only on tapping the target icon.
There was a problem hiding this comment.
I think the ideal flow should be such that the permission should be requested and then once it has been granted the app should use should automatically navigate the pin to the current location. I suppose the current code might need some tweaks in order to get that flow. Kindly check the same.
| } else if (locationManager.isGPSProviderEnabled() || | ||
| locationManager.isNetworkProviderEnabled()) { | ||
| locationManager.requestLocationUpdatesFromProvider(LocationManager.NETWORK_PROVIDER); | ||
| locationManager.requestLocationUpdatesFromProvider(LocationManager.GPS_PROVIDER); |
There was a problem hiding this comment.
The location does not seem updated based on the code in this else block which is confusing. Could you kindly elaborate what are you trying to achieve here? 🤔
There was a problem hiding this comment.
I took three cases: one where the last location is known already(recently updated case), another where we are able to request for updates from the GPS/Network Provider and the third where we don't have location access at all. I was trying to replicate the behaviour exhibited in the Nearby fragment since it does not crash on tapping the target icon and the code seemed promising, so that all the map fragments have the same behaviour for this icon. However, I just noticed nearby does not follow the flow you have described above. In fact, it does not ask for the location access at all and simply points to a predefined location:
nearby.mp4
The map in the Explore tab seems to work as intended, though:
explore.mp4
I think I need to understand a bit more about the different map fragments; the requirements aren't as clear and simple as I had assumed to be. Investigating it now.
|
Hi Ritika! I think the current code does not take into account the case where the app does not have the location permission. Could you kindly adjust the code to take that into account? Also, a kind request. It would be nice if you could let us know about when you could address this issue. That would help to decide whether to include this PR in the upcoming release or not :-) PS: There seem to be multiple crashes as a consequence of the bug that this PR fixes. So, it would be great to have this in the release. |
|
I'm sorry, I started working on the GSoC tasks after my exams and couldn't work on this one.
I think merging with the main branch should work. I'll try merging and testing it. |
There was a problem hiding this comment.
It works great!
Tested with https://commons.m.wikimedia.org/wiki/File%3AGrey_vessel_across_Reykjavik_port_2.jpg
Description (required)
Fixes #5195
What changes did you make and why?
The app crashes on tapping the "My Location" icon in the upload wizard and also in the media details editor when pictures with no EXIF location are uploaded. Used user's current location instead of EXIF coordinates to fix the crash.
Tests performed (required)
Tested prodDebug on Redmi 5A with API level 27.