commons-app / apps-android-commons

The Wikimedia Commons Android app allows users to upload pictures from their Android phone/tablet to Wikimedia Commons
https://commons-app.github.io/
Apache License 2.0
1.03k stars 1.24k forks source link

Requesting location permission popup opens app's settings instead of location settings #5255

Closed RitikaPahwa4444 closed 10 months ago

RitikaPahwa4444 commented 1 year ago

Summary

A "Requesting location permission" popup is used at various places in the app (like Explore fragment map and also in PR #5249) that asks users to turn location on for all apps in the Settings. The Settings button, however, takes the user to the app's settings instead of location settings where users can turn their GPS on.

Steps to reproduce

  1. Open the map in the Explore fragment
  2. Tap on the target icon
  3. A popup asking for location access is shown. Allow the app to access location
  4. Another popup titled "Requesting location permission" is displayed in case GPS is turned off on the device
  5. Tap on Settings

Expected behaviour

Tapping on Settings should take the users to location settings page where they can enable GPS.

Actual behaviour

The app's settings page is displayed instead of location settings. Since the user has already allowed location access in step 3, displaying this page isn't helpful.

Side note: The same button opens location settings page correctly on some devices.

Device name

Moto g62

Android version

12

Commons app version

master and prodDebug

Device logs

No response

Screen-shots

No response

Would you like to work on the issue?

None

sivaraam commented 1 year ago

A PR comment related to this issue.

paco-arana commented 1 year ago

Is this issue still open? I'd like to be assigned to work on it.

nicolas-raoul commented 1 year ago

@paco-arana You are very welcome to submit a pull request. :-) For this particular issue, please be aware that our GSoC student Ritika may choose to fix it without prior warning if she so desires, but if you are able to fix it promptly that should be fine! In any case, please share your findings regularly, even sharing what does not work :-) Thanks!

rohit9625 commented 10 months ago

I am getting first popup for sure but instead of a popup again, a Toast is shown like this :- Screenshot_20240110_021912_Commons.jpg

sivaraam commented 10 months ago

@rohit9625 That may be due to some recent changes related to migrating to OSM maps. Would you be able to analyse the rrot cause for the same?

ShashwatKedia commented 10 months ago

@sivaraam @rohit9625 for me the toast displays a different message. But when I click the target icon again (after giving permission) it correctly prompts me to turn on the location and take me to the location settings only.

Screenshot_2024-01-10-10-02-15-38_062c6066f629c24413cc8efde3bd9f38.jpg

rohit9625 commented 10 months ago

@ShashwatKedia you are right. The same worked for me after giving permission. So issue seems to be resolved I guess.

ShashwatKedia commented 10 months ago

So issue seems to be resolved I guess.

@rohit9625 I think it's not resolved because, ideally, on giving permission for location, the app should automatically ask to turn on location (if not on) and show the map accordingly. This is not being implemented, as the user has to click the target button again for the app to ask to turn on the location.

rohit9625 commented 10 months ago

@ShashwatKedia you are right. It should work like as you told. So may I work on this issue or someone is already on it?

ShashwatKedia commented 10 months ago

@rohit9625 I found that inside the locationPermissionGranted() function, if the lastKnownLocation is null, then we only display a toast to user, instead of showing dialog to turn on the location. This can be implemented by calling showLocationOffDialog() when location is null.

Here:

Screenshot 2024-01-10 at 10 56 37 AM
sivaraam commented 10 months ago

So may I work on this issue or someone is already on it?

No one else is working on this issue. You could feel free to work on it 🙂

rohit9625 commented 10 months ago

Hey guys, Sorry for being inactive on this issue. I was reproducing the issue again and I faced this error: Error fetching nearby placed Attempt to read from null... See this video.

https://github.com/commons-app/apps-android-commons/assets/101377978/c09bc766-b61f-429e-a4ad-a351237bca41

Also, on the logcat an error returned as a response from Wikimedia API. Saying this { "error": { "code": "invalid-coord", "info": "Invalid coordinate provided", "docref": "See https://commons.wikimedia.beta.wmflabs.org/w/api.php for API usage. Subscribe to the mediawiki-api-announce mailing list at <https://lists.wikimedia.org/postorius/lists/mediawiki-api-announce.lists.wikimedia.org/> for notice of API deprecations and breaking changes." }, "servedby": "deployment-mediawiki11" }

Does anyone know about the code from where this request was sent?

nicolas-raoul commented 10 months ago

@rohit9625 Please use the app's prodDebug flavor. See https://github.com/commons-app/commons-app-documentation/blob/master/android/build-variants/Build-Variants.md

ShashwatKedia commented 10 months ago

@rohit9625, were you able to reproduce the issue, or are you working on this?

rohit9625 commented 10 months ago

Yes I had reproduced the issue and currently working on it.

rohit9625 commented 10 months ago

Adding the showDialogue method is just not enough. Although, it does solve the problem but results in one other problem that is shown in the 2nd video.

  1. Location permission dialogue is shown successfully

https://github.com/commons-app/apps-android-commons/assets/101377978/5fad4da8-f0c4-4be9-be70-322a1fa5f820

  1. But adding that function results in this behavior. Which is when the target button is clicked then two dialogues pop up one after another.

https://github.com/commons-app/apps-android-commons/assets/101377978/76747325-1ff6-4d08-9d33-e8d7d9db8c79

Important I noticed some other issues related to location permissions:-

  1. When location permission is not allowed then, switch to map fragment in the Explore tab and the app doesn't ask for location permission again as it does for the Nearby Tab.

  2. But when you allow the location permission in the Nearby tab, it results in this behavior:

https://github.com/commons-app/apps-android-commons/assets/101377978/fc475e85-0682-4abe-bbd7-71af9388d2dc

The app crashes when tapped on the target button if location services are off. I guess this issue is already mentioned in here

I want some assistance in resolving the double permission popup problem. @ShashwatKedia @kanahia1

ShashwatKedia commented 10 months ago

The app crashes when tapped on the target button if location services are off. I guess this issue is already mentioned in here

This issue is already been solved by this PR #5418, so that is no longer an issue.

Another issue I notice is that even after getting the permission for location, the Nearby fragment doesn't ask to turn on the location, unless tapped on the target icon (which is also the case in Explore fragment, being discussed in this issue).

I want some assistance in resolving the double permission popup problem.

Let me test and check the code once, I'll get back to you in some time (either here or on Zulip)

ShashwatKedia commented 10 months ago

But adding that function results in this behavior. Which is when the target button is clicked then two dialogues pop up one after another.

After a lot of debugging, I found that this was because recenterMap() was calling the showLocationOffDialog() (which is supposed to happen on target icon click), but the ActivityResultLauncher was also calling showLocationOffDialog(), which shouldn't have happened, as ActivityResultLauncher itself shouldn't be called after permission is provided. So possible solutions are removing the call to showLocationOffDialog() from recenterMap(), but it poses it's own problems.

Popping up of 2 dialogs is also caused just when permission is given in the Explore tab itself, not just when the target icon is clicked.

I also found new issues while debugging such as Permission being requested twice simultaneously in the Explore tab (you can reproduce by denying permission once in the Explore tab, to see it pop up again) and some others (and their solutions).

I suggest closing this issue, as we have come too far away from the main agenda of this issue, which was the opening of the app's setting instead of location settings (which seems to be already fixed). I can open a new issue to resolve all possible bugs and incorrect behaviour in the nearby and explore tabs, to clear these issues once and for all. @nicolas-raoul Are we allowed to open such an issue, which requests to solve all possible bugs in an fragment

kanahia1 commented 10 months ago

Hey @rohit9625, I can surely assist you with it, can you please ping me on Zulip

rohit9625 commented 10 months ago

But adding that function results in this behavior. Which is when the target button is clicked then two dialogues pop up one after another.

After a lot of debugging, I found that this was because recenterMap() was calling the showLocationOffDialog() (which is supposed to happen on target icon click), but the ActivityResultLauncher was also calling showLocationOffDialog(), which shouldn't have happened, as ActivityResultLauncher itself shouldn't be called after permission is provided. So possible solutions are removing the call to showLocationOffDialog() from recenterMap(), but it poses it's own problems.

Popping up of 2 dialogs is also caused just when permission is given in the Explore tab itself, not just when the target icon is clicked.

I also found new issues while debugging such as Permission being requested twice simultaneously in the Explore tab (you can reproduce by denying permission once in the Explore tab, to see it pop up again) and some others (and their solutions).

I suggest closing this issue, as we have come too far away from the main agenda of this issue, which was the opening of the app's setting instead of location settings (which seems to be already fixed). I can open a new issue to resolve all possible bugs and incorrect behaviour in the nearby and explore tabs, to clear these issues once and for all. @nicolas-raoul Are we allowed to open such an issue, which requests to solve all possible bugs in an fragment

I had noticed all the stuff you mention. And I agree with you that this issue is about something else which is resolved already. It should be closed now :)

RitikaPahwa4444 commented 10 months ago

I can open a new issue to resolve all possible bugs and incorrect behaviour in the nearby and explore tabs, to clear these issues once and for all.

There are many more, actually. Upload wizard has a map, again some bugs appear there. If you're planning to fix all the issues, #5256 might be worth having a look. All fragments behave differently, you'll have to intensively test all of them unless we write more maintainable code.

As far as this issue is concerned, I no longer have access to that device, so can't confirm. Not reproducible on an emulator, at least. Sivaraam faced the same issue, but I think this got resolved after bumping the targetSdk. Since this is no longer reproducible, I'll close it for now. Anyone who faces this issue in future, please feel free to open it again. Thanks!😄

sivaraam commented 9 months ago

Sivaraam faced the same issue, but I think this got resolved after bumping the targetSdk.

Yes. I could confirm I too don't face this issue anymore. I'm properly taken to the Location setting of the device now.