bluelinelabs / Conductor

A small, yet full-featured framework that allows building View-based Android applications
Apache License 2.0
3.9k stars 343 forks source link

Problem with creating multiple routers in Activity#onCreate(Bundle) #299

Closed bejibx closed 7 years ago

bejibx commented 7 years ago

Hi! I was playing with Conductor in small test project and notice strange behaviour when adding multiple routers in Activity#onCreate(Bundle) method:

public class MainActivity extends AppCompatActivity {

    private Router router1;
    private Router router2;

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_main);

        router1 = Conductor.attachRouter(this, (ViewGroup) findViewById(R.id.timer1),
                savedInstanceState);
        if (!router1.hasRootController()) {
            router1.setRoot(RouterTransaction.with(TimerController.create("Controller1")));
        }

        router2 = Conductor.attachRouter(this, (ViewGroup) findViewById(R.id.timer2),
                savedInstanceState);
        if (!router2.hasRootController()) {
            router2.setRoot(RouterTransaction.with(TimerController.create("Controller2")));
        }
    }

    @Override
    public void onBackPressed() {
        if (!router1.handleBack() && !router2.handleBack()) {
            super.onBackPressed();
        }
    }
}

And here is log on first screen rotate:

D/Controller1: onSaveViewState(outState: Bundle[{Controller.viewState.bundle=Bundle[{}], Controller.viewState.hierarchy={2131427425=android.view.AbsSavedState$1@be95d5f}}])
D/Controller1: onSaveInstanceState(outState: Bundle[{}])
D/Controller2: onSaveViewState(outState: Bundle[{Controller.viewState.bundle=Bundle[{}], Controller.viewState.hierarchy={2131427425=android.view.AbsSavedState$1@be95d5f}}])
D/Controller2: onSaveInstanceState(outState: Bundle[{}])
D/Controller1: preDetach(...)
D/Controller1: postDetach(...)
D/Controller2: preDetach(...)
D/Controller2: postDetach(...)
D/Controller1: preDestroyView(...)
D/Controller1: postDestroyView(...)
D/Controller2: preDestroyView(...)
D/Controller2: postDestroyView(...)
D/Controller1: NewInstance(args: Bundle[{TAG=Controller1}])
D/Controller1: onRestoreInstanceState(savedInstanceState: Bundle[{MoxyDelegateBundle=Bundle[{com.arellomobile.mvp.MvpDelegate.KEY_TAG=TimerController$MvpDelegate@8a79176}]}])
D/Controller1: preCreateView(...)
D/Controller1: postCreateView(...)
D/Controller1: onRestoreViewState(savedViewState: Bundle[{Controller.viewState.bundle=Bundle[{}], Controller.viewState.hierarchy={2131427425=android.view.AbsSavedState$1@be95d5f}}])
D/Controller1: onChangeStart(...)
D/Controller2: preCreateView(...)
D/Controller2: postCreateView(...)
D/Controller2: onRestoreViewState(savedViewState: Bundle[{Controller.viewState.bundle=Bundle[{}], Controller.viewState.hierarchy={2131427425=android.view.AbsSavedState$1@be95d5f}}])
D/Controller2: onChangeStart(...)
D/Controller1: onChangeEnd(...)
D/Controller1: preAttach(...)
D/Controller2: onChangeEnd(...)
D/Controller2: preAttach(...)
D/Controller1: postAttach(...)
D/Controller2: postAttach(...)

As you can see, first controller is created again. This happens because the way LifecycleListener gets installed into Activity:

@NonNull
public static LifecycleHandler install(@NonNull Activity activity) {
    LifecycleHandler lifecycleHandler = findInActivity(activity);
    if (lifecycleHandler == null) {
        lifecycleHandler = new LifecycleHandler();
        activity.getFragmentManager().beginTransaction().add(lifecycleHandler, FRAGMENT_TAG).commit();
    }
    lifecycleHandler.registerActivityListener(activity);
    return lifecycleHandler;
}

FragmentTransaction#commit() is asynchronous operation, so during second router attach it's not yet added to Activity which causes multiple LifecycleListener instances being created.

For now to workaround this I'm calling FragmentManager#executePendingTransactions() after first router attach:

@Override
protected void onCreate(Bundle savedInstanceState) {
    super.onCreate(savedInstanceState);
    setContentView(R.layout.activity_main);

    router1 = Conductor.attachRouter(this, (ViewGroup) findViewById(R.id.timer1),
            savedInstanceState);
    if (!router1.hasRootController()) {
        router1.setRoot(RouterTransaction.with(TimerController.create("Controller1")));
    }

    getFragmentManager().executePendingTransactions();

    router2 = Conductor.attachRouter(this, (ViewGroup) findViewById(R.id.timer2),
            savedInstanceState);
    if (!router2.hasRootController()) {
        router2.setRoot(RouterTransaction.with(TimerController.create("Controller2")));
    }
}

But I think this method should be called inside LifecycleListener#install(Activity).

EricKuck commented 7 years ago

Great find! Unfortunately calling executePendingTransactions() here may have unintended side-effects for developers who use Fragments for other things, as it would execute all pending transactions, not just this one (ugh, fragments...). The fix for this will be a little more involved, but it should be doable.

bejibx commented 7 years ago

Unfortunately calling executePendingTransactions() here may have unintended side-effects for developers who use Fragments for other things

I'm agree with that, thats just the only reliable way which came to my mind. Curious how are you planning to solve this without using executePendingTransactions()?

EricKuck commented 7 years ago

I'm not sure yet. If nothing else we can keep a Map<Activity, LifecycleHandler> that removes entries when activities get destroyed to prevent memory leaks.

EricKuck commented 7 years ago

I ultimately settled on using the Map idea, which isn't awesome, but seemed to be the only thing that worked without interfering with other things the developer may be doing. Should be fixed in the latest snapshot.