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

Already on GitHub? Sign in to your account

RBLViewController #46

Merged
merged 12 commits into from Jan 18, 2013

Conversation

Projects
None yet
4 participants
Contributor

Machx commented Nov 1, 2012

This is a pull request for RBLViewController as discussed in Issue #33 to get feedback before merging in. I started here with simply getting much of what Josh did in JAViewController working on Rebel.

I should also note that I had to add a NSObject swizzling methods category that seems like it really belongs somewhere else in foundation level code.

I am open to any changes that should be made or feature additions. I am already starting to use this class in my new project I am working on so I should get more ideas of things to add as time progresses. Thanks

@ghost ghost assigned joshaber Nov 5, 2012

Member

joshaber commented Nov 5, 2012

Hi Colin! Sorry it's taken so long on this—I hope to give it a good review in the next day or two. Because oldjoshcodelolz.

Member

joshaber commented Nov 5, 2012

Ah in the meantime, it looks like this doesn't merge cleanly as-is. Could you merge in upstream?

Contributor

Machx commented Nov 5, 2012

Okay i'll merge in upstream as soon as I can. Im not on the best network for that at the moment, so I'll wait just a little bit till later this afternoon.

Member

joshaber commented Nov 15, 2012

Alright so a couple broad comments first.

  • I think the NSView -setNeedsLayout whatnot can go away since that's in AppKit as of 10.7.
  • All category methods/properties need to be prefixed with rbl_.

I'm inclined to think that the circumstances in which appearance methods should be called be left entirely to the user. In OS X land it seems waaaaaay too hard to know exactly what "appear" means universally.

@jwilling jwilling and 1 other commented on an outdated diff Dec 4, 2012

Rebel/NSView+RBLViewControllerAdditions.m
+
+ if (newViewController) {
+ NSResponder *ownResponder = [self nextResponder];
+ [self setNextResponder:self.rbl_viewController];
+ [self.rbl_viewController setNextResponder:ownResponder];
+ }
+}
+
+#pragma mark - View Controller Methods
+
++(void)loadSupportForRBLViewControllers {
+ static dispatch_once_t onceToken;
+ dispatch_once(&onceToken, ^{
+ //swizzle swizzle...
+ [self rbl_swapMethod:@selector(viewWillMoveToSuperview:) with:@selector(custom_viewWillMoveToSuperview:)];
+ [self rbl_swapMethod:@selector(viewDidMoveToSuperview) with:@selector(custom_viewDidMoveToSuperview)];
@jwilling

jwilling Dec 4, 2012

Contributor

Sorry I don't mean to jump on @joshaber's territory here, but the indentation here looks like it's spaces and not tabs.

@Machx

Machx Dec 4, 2012

Contributor

Yikes, there was a couple lines that had spaces instead of tabs. I don't know how I missed this. I'll fix it quickly. Looks like 4-6 lines that had spaces instead of tabs.

Member

joshaber commented Jan 15, 2013

Oh man I'm really sorry Colin. This totally dropped off my radar. Next time give me a swift kick.

Could you merge master in so this merges cleanly? Then we should be

Contributor

jwilling commented Jan 16, 2013

💥 @Machx Helping Josh out here a bit, looks like you need to update the submodules as well as they seem to be pointing toward old commits.

Contributor

Machx commented Jan 16, 2013

there we go I think I got it all sorted out. let me know if there is anything else I need to do. Thanks!

Member

joshaber commented Jan 18, 2013

💥 👶

@joshaber joshaber added a commit that referenced this pull request Jan 18, 2013

@joshaber joshaber Merge pull request #46 from Machx/master
RBLViewController
ae14d73

@joshaber joshaber merged commit ae14d73 into github:master Jan 18, 2013

Contributor

jwilling commented Jan 18, 2013

👏

One might say this pull is "sizzling" :trollface:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment