Multiple Controllers getting Attached On Orientation Change

[mmangliers]

I probably just messed something up in my migration to v2, but this is the behavior I'm noticing:

With Controller A on screen, turn the screen off and then back on.

Navigate to Controller B. Change orientation of the device.

After the orientation change, views for both A and B were attached to the screen. This can be repeated, adding another controller with each cycle.

I've been having trouble figuring out what's going on here. - specifically what's triggering the controller which was on screen during the screen on/off to be reattached on the orientation change. Not sure if it's a problem with the library or my implementation. Any thoughts?

[marukami]

I just had something similar happen. For me, I was not checking if there was already a router attached to the container after an orientation change.

if it's the same issue, something like this should work.

if (!router.hasRootController()) {
    router.setRoot(RouterTransaction.with(new HomeController()));
}

[mmangliers]

Already doing that unfortunately; I handle the activity identically to the demo. Plus if that were the issue I would expect the home controller to be the one getting duplicated, but it can be any controller I turn the screen on and off at.

[mmangliers]

I've narrowed it down to something to do with stopping and resuming, but not destroying, the activity. My best guess is that the needsAttach flag somehow gets set to true in the old controller. In the process of stopping, so that it gets reattached erroneously on activity recreation. Can't find anything that would cause that though.

Here's what I'm seeing, after putting in some logs:

Activity pauses Controller saves state Activity stops

Curiously I'm not seeing any log in my activity's onSaveInstanceState, so as far as I can tell that's not being called - so I'm not sure what's causing the controller to be saved there. I'm also not getting a callback to onRestoreInstanceState in my controller (bug?), even after an orientation change, so I can't be sure what's going on there.

What I think is happening is that when we return from the stop, we restore the state from the bundle - this sets needsAttach to true. But it's already attached, so we don't attach it when we resume, leaving the needsAttach flag to be hit when we recreate the controller on orientation change.

Update: Confirmed that to be the case. Stopping and resuming a controller incorrectly flags it as needing to be attached, which then causes it to be attached after an orientation change even if it shouldn't be. Am I the only person with this issue? There shouldn't be anything in my code mucking with the lifecycle.

[sockeqwe]

I see the same issue, but I have to investigate what exactly the problem is and when it occurs. At first glance it seems that it affects ChildControllers and has is occurs after you did an orientation change. But as already said, I have to investigate further on this ...

[leonardo2204]

@mmangliers I may be mistaken here, but isn't it right that Conductor does not call onRestoreInstaceState after a config change ? If you check the code, the InternalLifecycle(Fragment) calls setRetainInstace(true), so it shouldn't call it, only when Android is about to destroy your app.

[mmangliers]

Ah, you're correct; rest should still be relevant though. In the controller class, restoreInstanceState is being called after stop/resume, resetting the needsAttach flag. Fix should be as simple as something changing how needsAttach is saved or adding an else to activityResumed that sets the flag back to false. When I have time I'll download a local copy and play with some tweaks.

[EricKuck]

Thanks for all the productive discussion on this. It was super helpful. I've pushed a change to the latest snapshot build that fixes all instances of this issue I could reproduce. I think it's likely fixed for good, but it sounds like maybe different people might have different root issues. If anyone still has issues on the latest snapshot, please let me know!

[sockeqwe]

Hi, I use 2.0.1-SNAPSHOT in a tiny sample project. It is (will become) a TODO list app. In the sample (see video attached) I do the following:

1. Start Activity with TasksController (should display a list of all tasks, not implemented yet)
2. You click on the FAB, this will start CreateTaskController and put it also on the backstack

3. You can add a Person from your contacts to a Task. So you click on the + icon. This will add ContactsPickerController as child controller to CreateTaskController. The code looks like this (kotlin):

   addPersonButton.setOnClickListener {
    val childRouter = getChildRouter(personPickerContainer, null)
    if (!childRouter.hasRootController()) {
      childRouter.setRoot(RouterTransaction.with(ContactsPickerController())
          .popChangeHandler(ContactsPickerChaneHandler())
      )
      childRouter.setPopsLastView(true)
    }
   }

4. By clicking on the back button ContactsPickerController will be removed as expected from backstack.
5. Clicking on the back button again will pop CreateTaskController from backstack as expected so that TasksController will be displayed.

So far so good everything works as expected. However, as shown in the second part of the attached video screen orientation changes causes an issue of having a child controller attached twice (or have childcontroller twice on the backstack). As the video shows we continue like this:

1. Click on FAB --> CreateTaskController will be displayed.
2. Click on + icon --> ContactsPickerController will be added as child controller to CreateTaskController.
3. Configuration change: from portrait to landscape.
4. Everything seems to be ok (on the UI). BUT pressing android's back button doesn't remove ContactsPickerController. It seems that there is a second instance of ContactsPickerController on the backstack.
5. Clicking on android back button again (second time) will pop ContactsPickerController from backstack.
6. Click back button again and ContactsPickerController will be removed from backstac as expected and TasksController will be displayed (as expected).

So the critical point is 3., 4. and 5..

Video link: https://youtu.be/OSq-gH3rXL4

Source code: https://github.com/sockeqwe/mosby-conductor/tree/master/app

Maybe also related to #87

[EricKuck]

I just ran your demo app and it actually worked as expected for me, even after rotations. Is it possible that an old snapshot got cached on your machine?

[sockeqwe]

Hm, ok it seems that I was using an older 2.0.1-SNAPSHOT before (I don't know which 2.0.1 snapshot version I was using before)

With this version the bug is not reproducable as shown in the previous video anymore.

Download https://oss.sonatype.org/content/repositories/snapshots/com/bluelinelabs/conductor/2.0.1-SNAPSHOT/conductor-2.0.1-20160708.133245-6.aar

However, I see another (similar) bug from time to time:

1. Open the CreateTaskController
2. Click + icon to show ContactsPickerController
3. Click back button --> ContactsPickerController removed
4. Click + icon to show ContactsPickerController
5. Rotate device from Portrait to Landscape
6. Click back button --> ContactsPickerController removed
7. Click + icon to show ContactsPickerController
8. Click back button --> The whole CreateTaskController will be removed and not just ContactsPickerController (child controller).

So at 8. the child controller should be removed, but the parent controller will be removed from backstack.

Maybe this is another issue and not related to the original one reported here.

[sockeqwe]

Here is a video showing the issue described above: https://youtu.be/c_Y3GVkAqVE

As you can see in the last screen few seconds of the video (phone in landscape): The ContactsPickerController is visible on the screen, but pressing android's back button doesn't pop the ContactsPickerController but rather pops his parent controller (CreateTaskController) from navigation stack.

[mmangliers]

Snapshot fixed this for me; thanks!

[EricKuck]

Thanks for all the details, @sockeqwe! It would have taken me forever to figure that out. It's now fixed in the latest snapshot. Assuming there are no more issues, I'll be pushing another release in the next few days.

[sockeqwe]

Thanks for all your hard work! It's fixed now for me.

[mmangliers]

Oh no, I knew that if-else was going to be too simple. We caused a new problem. Under certain circumstances, child routers will not properly rebind because we've now flagged them as NOT needing to be attached. Here's the play by play:

Controller A has child controllers in the support view pager. Navigate one down into the stack to Controller B. At Controller B, perform any action which causes a stop - orientation change or screen shut off. Press back to return to controller A. When retrieving the child controllers from the router, their needsAttach flag is false, so they are not reattached to the screen.

The short of it is that we can't blindly flag everything that wasn't attached in onResume to not needsAttach. Here's my initial proposal - add a wasAttached flag that we save in addition to needsAttach so that we only set needsAttach to false in the cases we covered earlier (haven't gotten to test it yet):

       if (!attached && view != null && viewIsAttached) {
            attach(view);
        } else if(wasAttached) {
            needsAttach = false;
        }

        onActivityResumed(activity);

[EricKuck]

Update: Reproduced using another method. Ignore this.

@cordeveloper Unfortunately I'm not able to reproduce the scenario you've laid out. I think I did everything you said, but everything's still reattaching properly. What I did was modify the demo app's ChildController to have this:

    @OnClick(R.id.tv_title) void onTitleClick() {
        getParentController().getRouter().pushController(RouterTransaction.with(new TransitionDemoController(0))
                .pushChangeHandler(new FadeChangeHandler())
                .popChangeHandler(new FadeChangeHandler()));
    }

which makes it so that when you tap on the title of a page in the ViewPager demo, it pushes another controller. Once I'm on that other controller, I turn the screen off and on (or rotate), then hit the back button. When I come back, everything looks like I'd expect it to. Any idea what I might be missing?

[EricKuck]

Alright, this should all be fixed up now. Writing tests around all the different cases I can think of now.

[EricKuck]

Released a proper v2.0.1 with all these fixes just now for anyone who was running into issues.