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

Initialization refactoring & improvements #38

Merged
merged 1 commit into from
May 31, 2013
Merged

Initialization refactoring & improvements #38

merged 1 commit into from
May 31, 2013

Conversation

dimazen
Copy link
Contributor

@dimazen dimazen commented May 31, 2013

Replace initializing code into initWithNibName:bundle:.
Before that if we're using [MMDrawerController new]; - no setup occured and different options stay uninitialized.

Also i move [self.view setBackgroundColor:]; & gesture recognizers setup to viewDidLoad:
Calling self.view in init leads to view load & inefficient resources usage.

@kcharwood
Copy link
Contributor

Thanks for submitting the pull request. I'm curious to hear more about your last point. We're you running into real performance issues, or is this just more of a best practice?

I haven't noticed any issues on my end with that.

@larsacus
Copy link
Contributor

How is this different from calling self = [self init] for the situation you're running into? According to the documentation for initWithNibName:bundle:, init should call initWithNibName:bundle: with nil parameters:

If you subclass UIViewController, you must call the super implementation of this
method, even if you aren't using a NIB. (As a convenience, the default init method will do this for you,
and specify nil for both of this methods arguments.)

@dimazen
Copy link
Contributor Author

dimazen commented May 31, 2013

Well, there is actually no performance problems, you're right :)
It's kind of a best practice in terms of performance in general. Apple recommends to write init methods as efficient as possible. View allocation may take additional time (additional memory allocation, nib instatiation, etc).

@dimazen
Copy link
Contributor Author

dimazen commented May 31, 2013

@larsacus
Sorry, not sure, that i get your question.
Something wrong with my commit or..? :)
Thanks in advance

@larsacus
Copy link
Contributor

I'm just posing this question. We probably need initWithNibName:bundle: to support storyboards, but I'm just trying to figure out why this was necessary for the specific case you're describing.

@kcharwood
Copy link
Contributor

I'm guessing that since initWithNibName:bundle: is the designated initializer for UIViewController, calling [MMDrawerController new] does not go through our designated initializer, which leaves a bunch of properties uninitialized.

@dimazen
Copy link
Contributor Author

dimazen commented May 31, 2013

@larsacus
Ok, let me describe.
I'm instatiating MMDrawerController from code, but central contoller is unknown for this moment - it will be setted later. Therefore i've decided to move some config code intro initWithNibName:bundle: to support alloc/init

Also, if we're instantiating controller from XIB - we also in trouble because of unconfiguret properties

That was the case :)

@kcharwood
Copy link
Contributor

Docs from Apple on a designated initializer:

The designated initializer plays an important role for a class. It ensures that inherited instance variables are initialized by invoking the designated initializer of the superclass.

I think we may actually need to be calling super initWithNibName:bundle:...

@dimazen
Copy link
Contributor Author

dimazen commented May 31, 2013

@kcharwood
it's already done. Probably it require some additional change for storyboard support :^)

@larsacus
Copy link
Contributor

We're still investigating storyboard support in #33, but this looks like a needed change regardless.

Yet another reason to stay away from [UIViewController new], though, IMO...

@kcharwood kcharwood merged commit 1ef85aa into mutualmobile:master May 31, 2013
@kcharwood
Copy link
Contributor

I merged this in with 90582bf, and will get you credit in the changelog in the 0.2.0.

Thanks for the pull!

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.

3 participants