Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not forward appearance events to central VC when drawer is offscreen #271

Merged
merged 1 commit into from
Aug 25, 2014
Merged

Do not forward appearance events to central VC when drawer is offscreen #271

merged 1 commit into from
Aug 25, 2014

Conversation

pronebird
Copy link
Contributor

The idea is to avoid sending appearance events when setting new central controller before drawer put into view hierarchy (or offscreen). This PR fixes issue #261 related to state restoration. Related issue #268.

This patch also adds some minor optimization for side controller setter method to avoid reseting the same view controller. This does make sense with state restoration because at the moment all controllers will be set twice, first time when your code actually instantiates other controllers from storyboard and second time while state restoration decoding.

I tested this patch in my app with and without state restoration and everything works. I haven't tested hot-swap of central controller, but I believe it should work as before.

The good thing, it seems state restoration is smart enough and grabs the last instance of sidebar/central controller instantiated using -[UIStoryboard instantiateViewControllerWithIdentifier:]. If you create controllers manually, then you end up with two different sets of controllers I assume.

If you ask me, I do not encode child VCs for my controllers, because I create them in code anyway. Instead, I save properties that will help me to restore the state of those controllers. From my observations, saving child controller is only makes sense when you use embed segue in storyboard, then you leverage Storyboard to save/restore such controller. But essentially it always creates this controller for you, so you never even decode it, you only encode it.

@ronak2121
Copy link

+1

@vanderneut
Copy link

I'm surprised to see that this fix has not been merged in yet. The double triggering of viewWillAppear is causing bugs for me as well.

I'll look at the code changes here and try them out on my end and see if that fixes things.

Erik

@pronebird
Copy link
Contributor Author

Is @kcharwood alive?

@vanderneut
Copy link

I'm hoping he's just busy enjoying a nice long Summer vacation ;-)

In my app I ended up coding a workaround so I'm not getting hurt by the double triggering of viewWillAppear. Still hoping that at some point your fix will be pulled in with an MMDrawerController pod update.

Erik

@pronebird
Copy link
Contributor Author

@vanderneut I hope so too. I use my fork at the moment and planning to switch pod to official pod once it's merged.

@ronak2121
Copy link

Same thing here. We've coded workarounds for this for now.

@x2on
Copy link

x2on commented Aug 11, 2014

👍 we also use a fork at the moment.

@kcharwood
Copy link
Contributor

@vanderneut I'm alive!!!

Been a very busy summer. Carving out some time this morning to see what I can do here.

@kcharwood kcharwood added the bug label Aug 25, 2014
@kcharwood kcharwood added this to the 0.5.7 milestone Aug 25, 2014
kcharwood added a commit that referenced this pull request Aug 25, 2014
Do not forward appearance events to central VC when drawer is offscreen
@kcharwood kcharwood merged commit fa2f7bc into mutualmobile:master Aug 25, 2014
@pronebird pronebird deleted the appearance-patch branch August 25, 2014 13:08
@kcharwood
Copy link
Contributor

Sorry for the big delay on this one.

Thanks for the patch!!

🍻

@pronebird
Copy link
Contributor Author

No worries @kcharwood, cheers 🍺

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants