Fixes Location related flow of the app #5256 , #5461, #5490 #5494
Conversation
ShashwatKedia
changed the title
ShashwatKedia
changed the title
|
@nicolas-raoul now you can review this PR :) |
|
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: |
|
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. |
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
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? |
There seems to be a mismatch in the text passed to that particular Toast, I'll change it, my apologies for that
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 :) |
Thanks for noticing this bug; I have fixed it now. |
There was a problem hiding this comment.
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?
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?
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:
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:
|
|
@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? |
|
@nicolas-raoul I have resolved the merged conflicts, and I see two tests are failing. But I can successfully reproduce the failure of these two tests on the main branch as well. |
|
@sivaraam I have done the necessary changes :) |
|
Testing now :-) |
app/src/main/java/fr/free/nrw/commons/nearby/fragments/NearbyParentFragment.java
Show resolved
Hide resolved
|
When you have the opportunity could you please rebase from main? |
|
@nicolas-raoul @sivaraam is there anything else which needs to be changed in this PR? |
|
I have been testing this branch for a few days and location-related features seem to be working fine. One minor thing: I sometimes see two blue points on the map, not sure whether that happens on the main branch or not:
This can be filed as a separate issue. |
|
There was a potential suspect of this kind of behaviour with the code change in this MR. There's is a possibility that the last few changes may have fixed this issue. If you're not already doing so, could you try pulling in the recent changes and checking if it fixes the issue? |
|
The screenshot above was taken with the latest commit of this branch, 99290df. |
Strange, this shouldn't happen after the recent changes to the MR. I'll try to look into it, but I am a bit caught up now, so if there's an urgency in merging this PR, we can open a new issue about it and merge this one :) |
|
Yes it is very minor so I would agree with merging and creating a separate issue. :-) |
|
Sure, Nicolas. We can merge this. I'll open an issue for that double marker issue. 🙂 |
|
Thank you very much for your contribution and patience @ShashwatKedia ! 🙂 |
It's my pleasure @sivaraam :) |
Improve nearby-place search function for a multi-upload Enhance the depiction consistency of a multi-upload by ensuring that it corresponds to a single place Add javadoc Fixes Location related flow of the app commons-app#5256 , commons-app#5461, commons-app#5490 (commons-app#5494) * Resolved merged conflicts * Resolved merge conflicts and updated workflow * Updated Location Flow and merged conflicts * Update flow and merge conflicts * Fixed LocationPicker's location flow * Removed redundant code from LocationPermissionsHelper * Fixed Explore fragment crashing and incorrect behaviour * Updated LocationPicker Flow * Fixed Nearby not working as intended * Final update to location flow of all maps * Added the reqested changes and fixed bugs * Resolved requested change in in-app camera location flow * Fixed In-app camera location flow * Resolved conflicts in ContributionsListFragment * Updated java doc as requested * Resolved nearby card dialog not being shown * Optimised LocationPermissionsHelper javadoc * Made requested changes for preference check * Added javadoc and requested comment for later reference * Implemented requested code changes * Fixed failing test due to changes made during PR * Added string resource for ExploreMapFragment * Changed string resource for rationale dialog * Added standard location flow information in LocationPermissionsHelper * Added javadoc for doNotAskForLocationPermission * Removed unused import * Updated javadoc * Removed values-yue-hant * Fix some merge conflict errors * Fix minor errors due to mergre conflicts * Fix some refactor errors * Fixed minor bug due to merging conflicts * Delete app/src/main/res/values-yue-hant directory * Final changes to NearbyParentFragment * Fixes commons-app#5686 map coordinates set to image coords * Removed some redundant code from recenterMap * Removed one test whose method no longer exists * Removed unused method from contract of nearby * Removed redundant method from NearbyParentFragment * nearby: add a FIXME about the possibly redudant code --------- Co-authored-by: Kaartic Sivaraam <[email protected]> Localisation updates from https://translatewiki.net. Remove the value-yue-hant file The file is not properly recognized by Android and we've actually codemapped it to yue. The translatewiki configuration has been done in the incorrect file. So, it is still being created. The following Gerrit change would correct it for updates after that change is merged. https://gerrit.wikimedia.org/r/c/translatewiki/+/1022508 For the time being remove it for the sake of release.
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)