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

A very bad idea in the UINavigation category !!!! #38

Closed
nverinaud opened this issue Dec 28, 2012 · 14 comments
Closed

A very bad idea in the UINavigation category !!!! #38

nverinaud opened this issue Dec 28, 2012 · 14 comments
Labels

Comments

@nverinaud
Copy link

This lib completely destroy the way navigation controllers work.

You are redefining UINavigationController appearance callbacks which is a really bad thing.
When you redefine a method in a category you completely replace this method with your own, it's not like inheritance where you have access to the super implementation.

To illustrate, consider the following code from UINavigationController+MFSideMenu.m:

- (void) viewWillAppear:(BOOL)animated {
    [super viewWillAppear:animated];

    [self.sideMenu performSelector:@selector(navigationControllerWillAppear)];
}

Calling [super viewWillAppear:animated] here will call -[UIViewController viewWillAppear:] and not -[UINavigationController viewWillAppear:].
I think many issues, particularly in iOS 5, are due to this bad design.

An option would be to create a UINavigationController subclass or use delegation.

@fabiosoft
Copy link

have you some alternative? I mean...what other control con you advice to developer who want to add a side menu to their app? I tried MFSideMenu and it works pretty well...you know the very "best" control i can use in alternative?

@mikefrederick
Copy link
Owner

@nverinaud you're right, that was a poor design, I don't know how I missed that. I will refactor it when I get a chance.

@nverinaud
Copy link
Author

@fabiosoft I suggest you my solution available here : http://www.cocoacontrols.com/platforms/ios/controls/nvslidemenucontroller. Or you can try John Luch's one available here : http://www.cocoacontrols.com/platforms/ios/controls/swrevealviewcontroller. The two requires iOS 5+ and mine is not ARC compatible.

@mikefrederick
Copy link
Owner

I have already implemented a fix for this, but it hasn't been pushed to the repo yet, so don't let this chase you away.

@chilloutman
Copy link

Will you push it soon? The category breaks all viewWillAppear and similar events for all UINavigationControllers in my app. The events don't propagate to the rootViewController of the navigation controller and causes all sorts of issues. I can't use it like this.

@chilloutman
Copy link

Any news on when the fix will be made available?

@fabiosoft
Copy link

an update was released less than a month ago.... i use and i love this control! :)

@chilloutman
Copy link

The file has not been changed in the last 3 months and I can see the override is still inside the UINavigationController-category. The problem is still there.
https://github.com/mikefrederick/MFSideMenu/blob/master/MFSideMenuDemo/MFSideMenu/UINavigationController%2BMFSideMenu.m

@nverinaud
Copy link
Author

Poke @mikefrederick to release his update...

@mikefrederick
Copy link
Owner

Sorry guys, I am trying to figure out the best solution for the issue, I will have a fix soon.

@inPhilly
Copy link

Has this been fixed yet?

gonecoding added a commit to gonecoding/MFSideMenu that referenced this issue Mar 20, 2013
@gonecoding
Copy link

I used namespaced versions of the overwritten methods in the UINavigationController category and swizzled the implementations in the designated initializer. Seems to work fine for me after a quick check. The -viewDidAppear on one of my view controllers is now being called again. Have not thoroughly tested this though.

@mikefrederick
Copy link
Owner

I considered doing a swizzle as a temporary fix. Really not a fan of method swizzling but since I haven't had a chance to implement a permanent fix yet, @gonecoding will you submit a pull request?

@gonecoding
Copy link

Yeah, not a big fan of swizzling myself. Makes code harder to maintain. Another possibility to approach this would be to create an MFNavigationController class that users would have to subclass.

I'll submit a pull request as soon as I find out how to do that while including only one commit... :)

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 a pull request may close this issue.

6 participants