playgameservices / android-basic-samples

Google Play game services - Android samples
Apache License 2.0
972 stars 973 forks source link

new gamehelper bug detected in sign in #48

Closed ironiko closed 9 years ago

ironiko commented 10 years ago

onstart the intent to choose the account appears over and over y don't connect never.

the same goes in the method beginUserInitiatedSignIn.

with the previous version did not have this problem.

02-19 02:01:14.591: D/GameHelper(9425): GameHelper: Setup: requested clients: 1 02-19 02:01:14.621: D/GameHelper(9425): GameHelper: onStart 02-19 02:01:14.621: D/GameHelper(9425): GameHelper: Connecting client. 02-19 02:01:15.442: D/GameHelper(9425): GameHelper: onConnectionFailed 02-19 02:01:15.442: D/GameHelper(9425): GameHelper: Connection failure: 02-19 02:01:15.442: D/GameHelper(9425): GameHelper: - code: SIGN_IN_REQUIRED(4) 02-19 02:01:15.442: D/GameHelper(9425): GameHelper: - resolvable: true 02-19 02:01:15.442: D/GameHelper(9425): GameHelper: - details: ConnectionResult{statusCode=SIGN_IN_REQUIRED, resolution=PendingIntent{40515c60: android.os.BinderProxy@40551f90}} 02-19 02:01:15.442: D/GameHelper(9425): GameHelper: onConnectionFailed: WILL resolve because we have below the max# of attempts, 0 < 3 02-19 02:01:15.442: D/GameHelper(9425): GameHelper: onConnectionFailed: resolving problem... 02-19 02:01:15.442: D/GameHelper(9425): GameHelper: resolveConnectionResult: trying to resolve result: ConnectionResult{statusCode=SIGN_IN_REQUIRED, resolution=PendingIntent{40515c60: android.os.BinderProxy@40551f90}} 02-19 02:01:15.442: D/GameHelper(9425): GameHelper: Result has resolution. Starting it. 02-19 02:01:21.108: D/GameHelper(9425): GameHelper: onStop 02-19 02:01:21.108: D/GameHelper(9425): GameHelper: Client already disconnected when we got onStop. 02-19 02:01:46.973: D/GameHelper(9425): GameHelper: onStart

MonsterOfCookie commented 10 years ago

Not sure if it is connected to this issue, but I have found:

.18 boolean mConnectOnStart = true;

In the game helper class. Looking at the code it forces a login on start - Not desired behavior at all.

ianhanniballake commented 10 years ago

@MonsterOfCookie - there's a reason there's a setMaxAutoSignInAttempts method in GameHelper. Given that their best practices video actually showed a huge increase in sign ins if you log in immediately upon start (http://www.youtube.com/watch?v=QrXtqDzUPKk#t=199) I think adding it to GameHelper only made sense.

MonsterOfCookie commented 10 years ago

Yea, just surprising given other platforms like to ask for (prefer it) permissions/login as late possible to reduce user friction.

Am happy with this way of it has been proven, which it looks like it has

AAverin commented 10 years ago

Got a problem with this workflow also. If we auto-signing in onStart and user has multiple G+ accounts on the device - auto-signing fails, firing onSignInFailed() and kinda disrupting my logic. I see that in code it is supposed to resolve this failure by showing a popup, but the whole purpose of onSignInFailed() callback I have overriden in my class is probably to allow me to do something on failure. And when failure isn't really a failure - why should my activity know about it at all?

So my question - is GameHelper really helping? For now it already took me quite some time to add it to a project, and I can't even sign-in properly yet.

Also, noticed a case where you can get onSignInFailed() fired without GoogleApiClient connecting - you just receive a SIGN_IN_FAILED response in onActivityResult after initial invalid signin attempt in onStart had failed.

ianhanniballake commented 10 years ago

@AAverin - note the JavaDoc accompanying GameHelper.GameHelperListener.onSignInFailed:

"Called when sign-in fails. As a result, a "Sign-In" button can be shown to the user; when that button is clicked, call @link{GamesHelper#beginUserInitiatedSignIn}.

Note that not all calls to this method mean an error; it may be a result of the fact that automatic sign-in could not proceed because user interaction was required (consent dialogs). So implementations of this method should NOT display an error message unless a call to @link{GamesHelper#hasSignInError} indicates that an error indeed occurred."

Seems pretty clear why you would want to know that sign in failed (to show a sign in button) and how to check if there was actually an error.

AAverin commented 10 years ago

My problem is that GameHelper that was supposed to simplify my life adding Google Games support isn't really helping=) Sorry for ranting, but I just spent too much time on Games implementation.

It's not clear, because callback method signature makes me think that this is a SignInFailure that I must handle by, for example, allowing user to login without G+ connected and let him do it later from menu. I'd say this stuff should be hidden by GameHelper firing SignInFailure to subscribing activity only if sign-in really failed. Also adding some way to know why it failed would be good - user might have canceled signing process by choosing 'No' and game might have a different workflow for that, because 'No' is also isn't really a sign-in failure.

This might have been working when used from inside the game in a menu, like Clash Of Clans are using it, but when you have login-process tied close to G+ sign-in - it isn't working good.

I will modify GameHelper to my needs, of course, but it looks like some logic refactoring might help to GameHelper.

MatthiasRobbers commented 10 years ago

I ran into the same issue, a loop of sign-in attempts when there are multiple G+ accounts on the device. I have no idea why, but that only occurs on Gingerbread. At least on KitKat, it's fine with multiple accounts.

@AAverin Were you able to fix GameHelper regarding this issue?

AAverin commented 10 years ago

I ended up writing my wrapper around GameHelper.

package com.c8apps.footballcoach.utils;

import android.app.Activity;
import android.content.Intent;
import android.util.Log;

import com.google.example.games.basegameutils.GameHelper;

/**
 * Created by AAverin on 13.03.14.
 */
public class GPGHelper {
    private final static String TAG = "GPGHelper";

    public GameHelper gameHelper;

    public interface GPGListener {
        public void onSignInSuccess(boolean isAutoSignIn);
        public void onSignInFailure(boolean isAutoSignIn);
    }

    private GPGListener gpgListener = null;

    private boolean isAutoSignIn = false;

    public static GPGHelper instance = null;
    private GPGHelper(Activity activity) {

        gameHelper = new GameHelper(activity, GameHelper.CLIENT_GAMES|GameHelper.CLIENT_PLUS);
        GameHelper.GameHelperListener listener = new GameHelper.GameHelperListener() {
            @Override
            public void onSignInSucceeded() {
                Log.d(TAG, "onSignInSucceeded");
                if (gpgListener != null) {
                    gpgListener.onSignInSuccess(isAutoSignIn);
                }
                isAutoSignIn = false;
            }

            @Override
            public void onSignInFailed() {
                Log.d(TAG, "onSignInFailed");
                if (gpgListener != null) {
                    gpgListener.onSignInFailure(isAutoSignIn);
                }
                isAutoSignIn = false;
            }

        };

        gameHelper.setup(listener);

    }

    public static GPGHelper getInstance(Activity activity) {
        if (instance == null) {
            instance = new GPGHelper(activity);
        }
        return instance;
    }

    public void stop() {
        if (gameHelper != null) {
            gameHelper.onStop();
        }
    }

    public void onStart(Activity activity) {
        if (gameHelper != null) {
            isAutoSignIn = true;
            gameHelper.onStart(activity);
        }
    }

    public void onActivityResult(int requestCode, int resultCode, Intent data) {
        if (gameHelper != null) {
            gameHelper.onActivityResult(requestCode, resultCode, data);
        }
    }

    public void setGPGListener(GPGListener listener) {
        this.gpgListener = listener;
    }
}

The idea was that project should have a single instance of GameHelper and single connected instance of GoogleAPI. I moved my GPGHelper to my Application class making it private and obtaining it via :

public GPGHelper getGpgHelper(boolean autoSignIn) {
        if (gpgHelper == null) {
            Log.e(TAG, "gpgHelper == null!");
        }
        if (gpgHelper != null && !autoSignIn) {
            gpgHelper.gameHelper.setConnectOnStart(false);
        }
        return gpgHelper;
    }

It allowed to me to change GameHelper behavior on every screen I wanted to use it. On application start, if user had connected to G+, I try to connect GameHelper with GoogleAPI so my app would have an active and connected GoogleAPI to properly display G+Connected status.

On the screen where user actually connects to G+ I do:

by leveraging isAutoSignin boolean value in my custom wrapper I was able to get rid of mutiple connections situations.

ktrifon commented 10 years ago

I use the Unity 3d plugin and I get into this loop too at devices with multiple accounts... (plugin build 0.9.01)

any hope for fixing it soon?

iuryfranky commented 10 years ago
iuryfranky commented 10 years ago

I found a temporary solution here: https://code.google.com/p/play-games-platform/issues/detail?id=98 only tested on Android 2.3.6

cayrodev commented 10 years ago

Hello I'm a game developer, and I'm very angry, I lose a lot of time with Google bugs. Today I've lost a lot of time because my test mobile android 2.3.5 haven't connected my account because of this bug with multiple accounts. Why Google don't repair it? Multiplayer examples code also have many errors. Why they don't repair it? I'm sick of losing my time with "easy" things... I wait answer or fix that issue for me and for all android players...

mahs18 commented 10 years ago

@iuryfranky , thanks for sharing these link. I've fixed it... I've lost a lot of time because of this annoying bug.

samtstern commented 9 years ago

Closing due to the samples no longer using BaseGameActivity/GameHelper. See this video for more information.