[WIP] changed confusing message about location permission(#2011) #2305

cypherop · 2019-01-15T16:57:50Z

Fixes #2011 Nearby: confusing message about location permission

What changes did you make and why?
Added a new Alert Dialog when user taps on "Cancel"

Tests performed (required)

Tested on Motorola Moto G5 plus with API level 24

after tapping on cancel

If the user clicks on cancel again then the user is redirected to contributions activity

cypherop · 2019-01-15T17:02:11Z

Sure :)

codecov-io · 2019-01-15T17:25:20Z

Codecov Report

Merging #2305 into master will decrease coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master   #2305      +/-   ##
=========================================
- Coverage    6.42%   6.41%   -0.01%     
=========================================
  Files         236     236              
  Lines       11537   11549      +12     
  Branches     1049    1049              
=========================================
  Hits          741     741              
- Misses      10735   10747      +12     
  Partials       61      61

Impacted Files	Coverage Δ
...ava/fr/free/nrw/commons/nearby/NearbyFragment.java	`0% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e93564f...ff2ac56. Read the comment docs.

sivaraam · 2019-01-15T17:50:09Z

It would be nice if this could also be tested with an Android device running Android 5 or less as the issue #2011 was filed mainly because a confusing message about permissions was shown in a device running an Android version which does not support runtime permissions (yeah, I'm not asked to allow or deny when the requests access to my location etc.). I'm trying to test this as I have a device running Android 5.1.1 but it would be great if someone could test this before me :-)

Also, IIUC from the screen shots posted the when the user visits 'Nearby' the dialog in the first screen shot is shown. If he hits on 'Cancel' in it then pop-up in the second one would be shown. Is that right? If yes, I'm not sure how this improves the status quo. Adding another dialog just seems to be worsening it. I guess an improvement would be to avoid drop the first pop-up in the first one. That would reduce the frustration a little.

Also, I had another doubt. Given that #591 has been closed, does asking for the GPS permission as a mandatory requirement make sens? I think it could be made optional which would make Nearby even more useful.

cypherop · 2019-01-15T18:34:47Z

@maskaravivek Done reusing DialogUtil for displaying this dialog.

cypherop · 2019-01-15T18:39:11Z

@sivaraam Actually the screenshots are of an emulator running android 5.1. I had already tested on this emulater and also on my device.
Also I think the dialog should be there to inform the user that the GPS needs to be enabled in order to use that feature.After cancelling again the user is redirected to the contributions fragment and no further dialog appears

app/src/main/java/fr/free/nrw/commons/nearby/NearbyFragment.java

maskaravivek · 2019-01-16T03:38:46Z

It would be nice if this could also be tested with an Android device running Android 5 or less

The "GPS dialog" suggests that the app has location permissions(on Android 5 it has it by default) but the app wasn't able to fetch the location.

If the app has location permissions it checks if the GPS provider is enabled:

locationManager.isProviderEnabled(LocationManager.GPS_PROVIDER);

If the GPS_PROVIDER isn't enabled, the GPS dialog is shown.

So the dialog is not about location permissions. It is trying to check if you have enabled location services on your device or not.

sivaraam · 2019-01-17T06:15:50Z

Also I think the dialog should be there to inform the user that the GPS needs to be enabled in order to use that feature.

@sp2710 It might be good to have a dialog that informs the user that GPS is required for the feature. I just don't find how the first dialog which plainly states "GPS is disabled" is useful now. It's better not to have subsequent pop-up dialog. In this particular case, the first dialog seems useless, at least to me. So, it would be nice to drop it.

Just to give an idea. If the first dialog is dropped, the flow should be something like:

The user visits 'Nearby'
If GPS disabled, he is shown a dialog stating "GPS is needed to use this feature"
If he hits 'Cancel' he's taken back to 'Contributions'.
If he hits 'Enable GPS', he's taken to 'Nearby'.

Hope that clarifies what I'm trying to say. Let me know if it doesn't.

sivaraam · 2019-01-17T06:25:41Z

So the dialog is not about location permissions. It is trying to check if you have enabled location services on your device or not.

There seems to be some confusion as to what the actual issue in #2011 is. The issue is not about the "GPS is disabled .." dialog that is shown. It is about the "Nearby places cannot be displayed without the location permissions" dialog. I'm not sure when that dialog is shown but the message made me think it is shown incorrectly for device running Android < 6. I suggested to test it in a device running Android < 6 just to ensure the dialog is not shown anymore (I've just tested it and it doesn't seem to be appearing which is fine with my device).

This PR seems to have removed that unnecessary dialog about permissions when it removed the call to showLocationPermissionDeniedErrorDialog(). But it now seems to add another dialog which seems unnecessary (at least to me).

sivaraam · 2019-01-17T06:29:49Z

Actually the screenshots are of an emulator running android 5.1

@sp2710 I asked as the fact that it was also tested with a device running Android 5.1 wasn't mentioned in the Tests performed section of the description.

cypherop · 2019-01-28T14:06:32Z

@maskaravivek I think only one dialog will be sufficient and we should change the message to something like

GPS needs to be enabled to use this feature. Would you like to enable it?

Would you like to give your suggestions?

domdomegg · 2019-03-17T15:19:44Z

@maskaravivek I think only one dialog will be sufficient and we should change the message to something like

GPS needs to be enabled to use this feature. Would you like to enable it?

Would you like to give your suggestions?

I agree with changing the current message. On API 19, this is what happens if GPS is disabled and I open the app:

Immediately get message about permissions (without navigating to nearby) (which doesn't make sense as runtime permissions only added in API 23)
Get message about GPS being off (which is actually correct (well more like location services but it's close) - possibly wording could be improved to "Location services need to be enabled to use nearby. Would you like to enable them?")
Clicking cancel on (2) results in a toast being displayed saying I rejected permissions which is false. I think we don't need a message here at all really as the user will understand that by choosing not to enable location they can't use nearby.

Screenshots:

1	2	3

What I think should happen is:

I open the app, and nothing happens yet as haven't tried to view nearby
I navigate to nearby by clicking the tab at the top
I get the message (2 previously) telling me location services is disabled, and this part of the app needs them enabled
4a. If I enable them, nearby loads correctly
4b. If I keep them disabled, I am returned back to the contributions view

cypherop · 2019-03-17T22:11:13Z

@domdomegg So shall i modify my PR according to it?

domdomegg · 2019-03-17T22:20:05Z

@domdomegg So shall i modify my PR according to it?

I'd be happy to merge it. The one tricky thing might be only requesting location on viewing nearby (step 1), as I think location is used when it tries to get the nearby notification card to display on top of contributions fragment. If the other bits work I'd be happy to merge that and then step 1 can be done separately - whatever is easier.

cypherop · 2019-03-19T14:09:35Z

@domdomegg IMO for contributions fragment we can show dialog with content

Location services need to be enabled to get nearby notification. Would you like to enable them?

And if in nearby we can show this dialog

Location services need to be enabled to use nearby. Would you like to enable them?

domdomegg · 2019-03-19T19:15:35Z

Location services need to be enabled to get nearby notification. Would you like to enable them?

I don't know if this will be clear as users might not know what the nearby notification is referring too, especially as this message is likely to be the first thing the user experiences when opening the app. Maybe just say something like 'location services need to be enabled if you want to know about nearby places of interest that lack media on Commons'

cypherop · 2019-03-19T20:36:05Z

Looks nice.
So shall I update my PR according to it ?

neslihanturan · 2019-04-30T09:12:34Z

@sp2710 please do. After a long discussion, this PR seems like ready to be merged after some string refactors.

maskaravivek · 2019-04-30T19:49:30Z

IMO, this is no longer needed after #2925 is merged. We can probably close this PR

neslihanturan · 2019-04-30T21:31:09Z

Sorry I have missed it @maskaravivek

changed confusing message about location permission(commons-app#2011)

431b626

reused DialogUtil for displaying the dialog

ff2ac56

maskaravivek reviewed Jan 16, 2019

View reviewed changes

app/src/main/java/fr/free/nrw/commons/nearby/NearbyFragment.java Outdated Show resolved Hide resolved

app/src/main/java/fr/free/nrw/commons/nearby/NearbyFragment.java Outdated Show resolved Hide resolved

used lambda

70cc336

domdomegg changed the title ~~changed confusing message about location permission(#2011)~~ [WIP] changed confusing message about location permission(#2011) Mar 17, 2019

neslihanturan closed this Apr 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] changed confusing message about location permission(#2011) #2305

[WIP] changed confusing message about location permission(#2011) #2305

cypherop commented Jan 15, 2019

cypherop commented Jan 15, 2019

codecov-io commented Jan 15, 2019 •

edited

Loading

sivaraam commented Jan 15, 2019

cypherop commented Jan 15, 2019

cypherop commented Jan 15, 2019

maskaravivek commented Jan 16, 2019

sivaraam commented Jan 17, 2019 •

edited

Loading

sivaraam commented Jan 17, 2019 •

edited

Loading

sivaraam commented Jan 17, 2019

cypherop commented Jan 28, 2019 •

edited

Loading

domdomegg commented Mar 17, 2019

cypherop commented Mar 17, 2019

domdomegg commented Mar 17, 2019 •

edited

Loading

cypherop commented Mar 19, 2019

domdomegg commented Mar 19, 2019

cypherop commented Mar 19, 2019

neslihanturan commented Apr 30, 2019

maskaravivek commented Apr 30, 2019

neslihanturan commented Apr 30, 2019

[WIP] changed confusing message about location permission(#2011) #2305

[WIP] changed confusing message about location permission(#2011) #2305

Conversation

cypherop commented Jan 15, 2019

cypherop commented Jan 15, 2019

codecov-io commented Jan 15, 2019 • edited Loading

Codecov Report

sivaraam commented Jan 15, 2019

cypherop commented Jan 15, 2019

cypherop commented Jan 15, 2019

maskaravivek commented Jan 16, 2019

sivaraam commented Jan 17, 2019 • edited Loading

sivaraam commented Jan 17, 2019 • edited Loading

sivaraam commented Jan 17, 2019

cypherop commented Jan 28, 2019 • edited Loading

domdomegg commented Mar 17, 2019

cypherop commented Mar 17, 2019

domdomegg commented Mar 17, 2019 • edited Loading

cypherop commented Mar 19, 2019

domdomegg commented Mar 19, 2019

cypherop commented Mar 19, 2019

neslihanturan commented Apr 30, 2019

maskaravivek commented Apr 30, 2019

neslihanturan commented Apr 30, 2019

codecov-io commented Jan 15, 2019 •

edited

Loading

sivaraam commented Jan 17, 2019 •

edited

Loading

sivaraam commented Jan 17, 2019 •

edited

Loading

cypherop commented Jan 28, 2019 •

edited

Loading

domdomegg commented Mar 17, 2019 •

edited

Loading