Show Error Play Services Dialog when appropriate

[Dvik]

#132 A request to location or map is now done only after checking if the play services version on the user's phone is available and is compatible with the play services version of the app.

Generalized the code keeping in mind DRY(Don't Repeat Yourself) by creating a PlayServicesUtils class with a static method which is called whenever needed.

API 16 without Google Play Services

[Dvik]

@lognaturel @yanokwa Please review and suggest changes

[lognaturel]

Nice! When I run this with API 22 and play services disabled, I get a nice popup with an 'enable' button which takes me to the settings. But when I enable and come back to Collect, I see a white screen. I have to go back and then launch the geo activity again to make progress. Ideally either the geo activity would be displayed or I'd be back at the widget screen with the button to launch it.

I like that you introduced a class with static method to share behavior. I don't like the structure of the checkPlayServices method though because its name suggests its a simple test but really it possibly shows a notification to the user. I believe you should be able to split it up so you can do something like if !playServicesAvailable alertUser else .

If I missed something and you can't split it up I would recommend a name like checkPlayServicesAndAlertIfMissing to make it clear the method has side effects. This is less important and more of a taste thing but with a method like that I wouldn't directly call it from a conditional. I'd instead save its result with something like boolean canUsePlayServices = checkPlayServicesAndAlertIfMissing(). It's more wordy but I think it really helps someone reading the code later.

[lognaturel]

If my first paragraph doesn't quite make sense you may want to try it for yourself by having Play Services installed, disabling it through the apps setting and then trying to launch a geo activity.

[Dvik]

Yeah, the first paragraph makes sense. I think i should send the user to the screen from where he can launch GeoActivity again or look into displaying the GeoActivity right away.

I agree with the naming convention to make the code more readable. Will split the method into two: one that shows the dialog and other that checks for play services.

[Dvik]

Now, the user would be redirected to the calling activity after they have enabled play services.

Also, made the necessary change regarding splitting of checkPlayServices() and introduced a new method: requestPlayServicesErrorDialog() in PlayServicesUtils.

[lognaturel]

Just one tiny style change. This is great, I really like the structure you ended up with!

If you are interested, one thing you could do that would be very helpful is propose a new hierarchy for the geo activities so that there can be more code sharing between them. You could think about it and then file an issue so we can come to agreement. For a good example, check out @dcbriccetti's #480 where he pulled out common behavior between activities that display lists. He also organized some of the code into smaller methods that make the code easier to read, made variables local when they could be and things like that.

[lognaturel]

You can just return resultCode == ConnectionResult.SUCCESS here! If it's true, true is returned. If it's false, false is returned. "Boolean zen" 😊

[Dvik]

Ohh, nice catch. Write less, Do more. 👍

[lognaturel]

Thanks, @Dvik! I recommend that you start creating branches for each project/feature/fix you work on. That way you can more easily work with a couple of different projects at a time if you want. It also makes it easier for other people to check out your code.

[Dvik]

Yeah,sure. Branches do help.

Had a look at @dcbriccetti 's changes in the PR. Yes, we have to do something to better organize the Geo** activities. Will look into it and file a refactoring approach.