Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
RitikaPahwa4444 opened this issue Jul 7, 2023 · 22 comments
Assignees
Labels

Comments

@RitikaPahwa4444
Copy link
Collaborator

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
Copy link
Member

sivaraam commented Jul 8, 2023

A PR comment related to this issue.

@paco-arana
Copy link
Contributor

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

@nicolas-raoul
Copy link
Member

@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
Copy link
Contributor

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
Copy link
Member

sivaraam commented Jan 10, 2024

@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
Copy link
Contributor

@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
Copy link
Contributor

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

@ShashwatKedia
Copy link
Contributor

ShashwatKedia commented Jan 10, 2024

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
Copy link
Contributor

@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
Copy link
Contributor

ShashwatKedia commented Jan 10, 2024

@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
Copy link
Member

sivaraam commented Jan 11, 2024

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
Copy link
Contributor

rohit9625 commented Jan 14, 2024

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.

WhatsApp.Video.2024-01-14.at.21.42.11_4aa5ed18.mp4

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
Copy link
Member

nicolas-raoul commented Jan 15, 2024

@ShashwatKedia
Copy link
Contributor

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

@rohit9625
Copy link
Contributor

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

@rohit9625
Copy link
Contributor

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
mobizen_20240123_165603.-.Trim.mp4
  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.
mobizen_20240123_165804.-.Trim.mp4

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:

mobizen_20240123_171837.mp4

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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

@rohit9625
Copy link
Contributor

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
Copy link
Collaborator Author

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants