Update DefaultLocationProvider.java #37
Conversation
Handle situation when user decide to activate location through status bar while the gps dialog is displayed. Gps provider is checked in every scenario to prevent application from performing unnecessary actions.
Added SourceListener to determine when GooglePlayServiceProvider failed
Passed SourceListener to constructor
Creating fallback when onConnectionSuspended() or onConnectionFailed() happens, firing DefaultProvider
|
1- You are violating separation of concerns by adding SourceListener to DispatcherLocationProvider and sending GooglePlayServices information back. That shouldn't know about those stuff, nor care. Not that i don't wanna do this, but it's quite not easy :) There is also this issue asking for same thing, but even though i tried a few stuff couldn't get a really working solution. With 1 point above, i cannot accept this PR for sure, please take a look at that part again, and remove or modify somehow. |
| @@ -271,19 +271,29 @@ public void runScheduledTask(@NonNull String taskId) { | |||
| } | |||
| } | |||
|
|
|||
| @Override | |||
| @Override | |||
There was a problem hiding this comment.
Please no :) Remove unnecessary space here.
| if (!activityStarted) { | ||
| onLocationFailed(FailType.VIEW_NOT_REQUIRED_TYPE); | ||
| if (isGPSProviderEnabled()) { | ||
| LogUtils.logI("User didn't activate GPS through the dialog, but using status bar"); |
There was a problem hiding this comment.
Please update the text here and below, it doesn't actually necessary to do it via status bar, user can also directly go to settings and enabled it. Just mention as 'but activated manually'
| @Override | ||
| public void onConnectionSuspended(int i) { | ||
| if(activeProvider instanceof GooglePlayServicesLocationProvider){ | ||
| activeProvider.cancel(); |
There was a problem hiding this comment.
And also you don't need to do this stuff manually, it will be done via scheduled task when waiting time is up.
Of course, this would mean quick fallback, agree on that one. But in that case, this is not a concern of GooglePlayServices so using SourceListener which belongs to GooglePlayServicesProvider is not a right tool.
You could create another interface just to notify back when it would fail. And i would do that within another PR, because those 2 different topics now you're trying to cover.
| LogUtils.logI("GoogleApiClient connection is suspended, calling fail..."); | ||
| failed(FailType.GOOGLE_PLAY_SERVICES_CONNECTION_FAIL); | ||
| LogUtils.logI("GoogleApiClient connection is suspended, start DefaultLocation "); | ||
| sourceListener.onConnectionSuspended(i); |
There was a problem hiding this comment.
here now, you are also violating the configuration. See the if block above, it checks whether configuration says it should fail or not, and in this case it should but you're switching back to defaultProviders.
Maybe you can even create another configuration part called "switchToDefaultsOnFail" in GooglePlayServicesConfiguration and implement that way further.
Handle situation when user decide to activate location through status bar while the gps dialog is displayed. Gps provider is checked in every scenario to prevent application from performing unnecessary actions.