commons-app
[Bug]: App prompts for user location in Explore's uploaded via mobile screen
Summary
The app seems to be prompting for user location in the Explore's "Uploaded via mobile" screen. This is incorrect as that screen doesn't need location permission at all.
Steps to reproduce
- Freshly install the app
- Open Explore
- Tap on the "Uploaded via mobile" tab
Expected behaviour
The app shows images in "Uploaded via mobile" screen without any prompt.
Actual behaviour
The app requests for location permission. The same is necessary only for the "Map" screen of Explore. So, prompt being show in "Uploaded via mobile" screen is incorrect.
Device name
OnePlus Nord
Android version
Android 12
Commons app version
main branch
Device logs
No response
Screen-shots
Screencast: https://drive.google.com/file/d/1_RTNcnU7ludiNFEmd2ZeDMcDcdQ8zZCZ/view?usp=sharing
Would you like to work on the issue?
None
Isn't this activity centered around the user, though?
I sometimes use this activity to see whether the area I am in in already well covered, or has some gaps in coverage.
@sivaraam I can reproduce this issue on main branch as well, so could you update the description of this issue to display "Commons version" as main, this will help others to directly use the main branch to work on this issue :)
Isn't this activity centered around the user, though?
I sometimes use this activity to see whether the area I am in in already well covered, or has some gaps in coverage.
I think the "Map" tab is user centric, and you would use that tab to see whether the are you are in is well covered or not. But what Sivaraam wants to say here is that, even without opening the "Map" tab and just opening the "Uploaded via mobile" tab prompts for location permission, which it shouldn't do, as it doesn't require that permission to display the images :-)
@sivaraam I can reproduce this issue on main branch as well, so could you update the description of this issue to display "Commons version" as main, this will help others to directly use the main branch to work on this issue :)
Tweaked the description. Thanks for the info 🙂
@sivaraam This is similar to issue #5551 , we can club both of these issues together (update the description of this issue to address both) so that I can close #5551, I was just working on exploring so I think I got the idea of what is causing both of these issues (there might be others too), so I think it will be better if we can handle all of these at once.
@nicolas-raoul If clubbing both these issues in this one sounds good then can you assign this to me? as this task can get a bit complex
@sivaraam This is similar to issue #5551 , we can club both of these issues together (update the description of this issue to address both) so that I can close #5551, I was just working on exploring so I think I got the idea of what is causing both of these issues (there might be others too), so I think it will be better if we can handle all of these at once.
Actually, both of these issues are being caused because of the same reason, as correctly pointed out by Sivaraam here, which is that Android is initializing the 'Map' tab automatically, in anticipation that user might switch to it soon. So, even further issues related to this will have been caused by the same reason.
@nicolas-raoul If clubbing both these issues in this one sounds good then can you assign this to me? as this task can get a bit complex
I would suggest you should wait for #5494 to be merged because it brings significant changes to the flow and code of all the fragments which use maps. Otherwise, you will have to rewrite the code as there will be merge conflicts :)
@ShashwatKedia I do understand the reason as I have worked recently on this, we can always handle merge conflicts so I don't think that is an issue , I am good with what @nicolas-raoul , @sivaraam suggests here. waiting is not an issue too, and @ShashwatKedia let me know if you need help anywhere here so that it can merged sooner , let this not be a block for too long, thanks
@sivaraam This is similar to issue #5551 , we can club both of these issues together (update the description of this issue to address both) so that I can close #5551
As per Shashwat's linked comment, I think it's already clear that one part is handled by hisPR/#5494. The other part is already captured in this issue. So, I'm not sure why the description needs an edit. Let me know if I missed something.
Also, could you give that PR #5494 a try and see if it indeed fixes the issue for you. If so, we could close the other issue. Otherwise, we may need to explore the root cause of the behaviour that you observe.
Hey @nicolas-raoul I am intrested and i would like to work on this issue , Can you assign this issue to me :) ?
