yayaa / LocationManager

Simplify getting user's location for Android
807 stars 186 forks source link

About "GooglePlayService Dialog cancel / dismiss handling" #110

Closed victor-denisenko closed 4 years ago

victor-denisenko commented 4 years ago

Branch: master Device: Virtual device Pixel 2 XL API 29 without Play Store

Configuration

Default, but askForGooglePlayServices is true

new LocationConfiguration.Builder()
                .askForPermission(new PermissionConfiguration.Builder().rationaleMessage("Gimme the permission!").build())
                .useGooglePlayServices(new GooglePlayServicesConfiguration.Builder()
                        .askForGooglePlayServices(true)
                        .build())
                .useDefaultProviders(new DefaultProviderConfiguration.Builder().gpsMessage("Would you mind to turn GPS on?").build())
                .build();

Execution steps

Start "sample in activity", wait for result

Result

Dialog is open:

After click OK button, empty activity screen is showed, DispatcherLocationProvider#continueWithDefaultProviders() is not called.

Empty screen, cannot get location.

Expected behavior

After click OK button, onCancelListener in DispatcherLocationProvider#resolveGooglePlayServices(int) is called and DispatcherLocationProvider#continueWithDefaultProviders() invoked.

We got location from DefaultLocationProvider.

Logs

I/LocationManager: We got permission!
W/GooglePlayServicesUtil: Google Play Store is missing.
I/DispatcherLocationProvider: GooglePlayServices is NOT available on device.
I/DispatcherLocationProvider: Asking user to handle GooglePlayServices error...
E/GoogleApiAvailability: Google Play services is invalid. Cannot recover.

Conclusions

Maybe this is not library error, but com.google.android.gms implementation issue.

For this error (no Play Service) GoogleApiAvailability#getErrorResolutionIntent(Context, int, String) return null in GoogleApiAvailability#getErrorDialog(Activity, int, int, OnCancelListener), so when DialogInterface.BUTTON_POSITIVE is pressed, the OnClickListener listener do nothing.

We can call Dialog#setOnDismissListener() that catch all close events, but in listener we cannot define what event is happened (issue resolved or not?).

Proposal

Don't call GoogleApiAvailability#getErrorDialog() and implement own dialog, with additional logic in OK button click listener. I looked dialog creation method (GoogleApiAvailability#zaa()), all called methods in code is public.

yayaa commented 4 years ago

Hmm... How about we do another availability check on cancel? If it is available, then it means problem is resolved so we can move on, if it is still not available then it means either user canceled or couldn't resolve the issue, so we continue with default. What do you think?

victor-denisenko commented 4 years ago

For this error (ConnectionResult.SERVICE_INVALID) Cancel listener is never called if user click OK. Dialog closes with OK listener (which do nothing, because Intent is null) and Dismiss listener.

We can listen Dismiss event, but dialog dismiss event is not guaranty that user resolved issue already.

In common cases after user click dialog button, app redirect user to Settings and wait for action (app call activity.startActivityForResult with RequestCode.GOOGLE_PLAY_SERVICES). Answer will be received in DispatcherLocationProvider#onActivityResult(int, int, Intent).

In ConnectionResult.SERVICE_INVALID case OK listener do nothing (Intent is null).

From com.google.android.gms.common.internal.DialogRedirect with my comments:

// OK click listener, we cannot override this with `GoogleApiAvailability.getInstance().getErrorDialog()`
public void onClick(DialogInterface var1, int var2) {
        try {
            /* If Intent not null then call activity.startActivityForResult(), otherwise do nothing.
               Intent is not null only if ConnectionResult equal SERVICE_MISSING, 
               SERVICE_VERSION_UPDATE_REQUIRED or SERVICE_DISABLED 
               (See GoogleApiAvailabilityLight#getErrorResolutionIntent(Context, int, String) ).
           */
            this.redirect();
            return;
        } catch (ActivityNotFoundException var7) {
            Log.e("DialogRedirect", "Failed to start resolution intent", var7);
        } finally {
            // Dismiss dialog, but issue maybe is not resolved yet.
            var1.dismiss();
        }

}

We get expected behavior only if user don't click OK button (User press back button or click outside dialog). Only in this situation Cancel listener is called.

yayaa commented 4 years ago

In ConnectionResult.SERVICE_INVALID case OK listener do nothing (Intent is null).

This is really sad. Have you already create an issue for this?

To be honest, it seems like we could just listen dismiss and have GooglePlayServices check after dismiss. That's what we do in onActivityResult once we get RequestCode.GOOGLE_PLAY_SERVICES anyway. So we could remove that and do it on dismissal, to make sure we cover all the cases.

victor-denisenko commented 4 years ago

This is really sad. Have you already create an issue for this?

Maybe this is right behavior for Play Service logic: no action if error cannot be resolved, only show dialog message. But bad for us, we cannot switch to other provider.

I checked Dismiss listener, this not worked for resolvable error. For example:

For resolvable error (Go to Settings > Applications > All > Google Play Services > Tap Disable > Tap OK to confirm):

  1. getGoogleApiErrorDialog() show dialog
  2. User click OK
  3. App show Settings activity and wait for result
  4. After Settings activity is showed, the dialog is closed (called Dismiss event).
  5. In Dismiss listener we check GooglePlayServices availability. And for this time error maybe not resolved yet. But we call continueWithDefaultProviders().
  6. Some time has passed...
  7. At last user resolve issue and back to app. App get result in onActivityResult(), check GooglePlayServices availability and invoke getLocationFromGooglePlayServices() or continueWithDefaultProviders().

I think changing DispatcherLocationSource#getGoogleApiErrorDialog() and creating own dialog will solve issue for all errors (resolvable and no). I will test this and create PR.

yayaa commented 4 years ago

Ahhh... 5&6&7 is a messy combination. I see. Thanks for the great explanation and testing all these extensively.

I think changing DispatcherLocationSource#getGoogleApiErrorDialog() and creating own dialog will solve issue for all errors (resolvable and no). I will test this and create PR.

I'd prefer to solve specific issue instead of taking all the responsibility to handle everything else, GoogleApiErrorDialog handles a lot of logic under the hood to determine how to solve or what to show use etc.

Let's keep it and try to solve the specific problem related to SERVICE_INVALID and see if that would be sufficient enough. I'd continue with the approach taking in #111