Adding UIBarButtonItem RACCommand Support. #406

Merged
merged 3 commits into from Apr 9, 2013

Projects

None yet

3 participants

@KyleLeneau

This commit references issue #403

@joshaber joshaber commented on an outdated diff Mar 28, 2013
...ork/ReactiveCocoa/UIBarButtonItem+RACCommandSupport.m
+#import <ReactiveCocoa/NSObject+RACPropertySubscribing.h>
+
+#import <objc/runtime.h>
+
+static void * UIControlRACCommandKey = &UIControlRACCommandKey;
+
+@implementation UIBarButtonItem (RACCommandSupport)
+
+- (RACCommand *)rac_command {
+ return objc_getAssociatedObject(self, UIControlRACCommandKey);
+}
+
+- (void)setRac_command:(RACCommand *)command {
+ objc_setAssociatedObject(self, UIControlRACCommandKey, command, OBJC_ASSOCIATION_RETAIN_NONATOMIC);
+
+ self.enabled = command != nil ? command.canExecute : YES;
@joshaber
joshaber Mar 28, 2013

Style: surround the ternary expression with parens.

@joshaber joshaber and 2 others commented on an outdated diff Mar 28, 2013
...ork/ReactiveCocoa/UIBarButtonItem+RACCommandSupport.m
+static void * UIControlRACCommandKey = &UIControlRACCommandKey;
+
+@implementation UIBarButtonItem (RACCommandSupport)
+
+- (RACCommand *)rac_command {
+ return objc_getAssociatedObject(self, UIControlRACCommandKey);
+}
+
+- (void)setRac_command:(RACCommand *)command {
+ objc_setAssociatedObject(self, UIControlRACCommandKey, command, OBJC_ASSOCIATION_RETAIN_NONATOMIC);
+
+ self.enabled = command != nil ? command.canExecute : YES;
+
+ if (command == nil) return;
+
+ RAC(self.enabled) = RACAbleWithStart(command, canExecute);
@joshaber
joshaber Mar 28, 2013

The WithStart part of this doesn't seem necessary since you set it directly above.

@jspahrsummers
jspahrsummers Mar 28, 2013

Why not only have this line, instead the one above?

Also, this binding needs to be disposed of if the rac_command is later changed – otherwise, two commands can be bound to this property. So this will need to use -toProperty:onObject: manually, or else subscribeNext:.

@KyleLeneau
KyleLeneau Mar 28, 2013

What do you mean by "Why not only have this line, instead the one above?"?

@jspahrsummers
jspahrsummers Mar 28, 2013

The explicit setting of self.enabled above can be omitted if we just use this binding (except for the command == nil case).

@KyleLeneau
KyleLeneau Mar 28, 2013

Got it. As for the disposing of the signal when setting a new command should I use -toProperty:onObject: to check for an already bound signal and remove it somehow?

@jspahrsummers
jspahrsummers Mar 28, 2013

Well, -toProperty:onObject: behaves like RAC(), except that it returns a disposable representing the binding. We probably need to associate the disposable with the UIBarButtonItem, then dispose of it if the rac_command is changed later.

@joshaber joshaber was assigned Mar 28, 2013
@joshaber
ReactiveCocoa member

@KyleLeneau Thanks! Just a few comments and it'd be great if it could be unit tested. :metal:

@KyleLeneau

@joshaber I agree the testing. I created an issue #405 that would help address the testing. With these UIBarButtonItem changes I can see a couple of scenarios to test for upfront.

@joshaber joshaber commented on an outdated diff Apr 8, 2013
...ork/ReactiveCocoa/UIBarButtonItem+RACCommandSupport.m
+static void * UIControlRACCommandSignalKey = &UIControlRACCommandSignalKey;
+
+@implementation UIBarButtonItem (RACCommandSupport)
+
+- (RACCommand *)rac_command {
+ return objc_getAssociatedObject(self, UIControlRACCommandKey);
+}
+
+- (void)setRac_command:(RACCommand *)command {
+ objc_setAssociatedObject(self, UIControlRACCommandKey, command, OBJC_ASSOCIATION_RETAIN_NONATOMIC);
+
+ if (command == nil) return;
+
+ // Check for stored signal
+ RACDisposable *existingSignal = objc_getAssociatedObject(self, UIControlRACCommandSignalKey);
+ if (existingSignal != nil) {
@joshaber
joshaber Apr 8, 2013

Don't need to bother with the nil check.

@joshaber joshaber and 1 other commented on an outdated diff Apr 8, 2013
...ork/ReactiveCocoa/UIBarButtonItem+RACCommandSupport.m
+ return objc_getAssociatedObject(self, UIControlRACCommandKey);
+}
+
+- (void)setRac_command:(RACCommand *)command {
+ objc_setAssociatedObject(self, UIControlRACCommandKey, command, OBJC_ASSOCIATION_RETAIN_NONATOMIC);
+
+ if (command == nil) return;
+
+ // Check for stored signal
+ RACDisposable *existingSignal = objc_getAssociatedObject(self, UIControlRACCommandSignalKey);
+ if (existingSignal != nil) {
+ // Remove the old signal to add a new one
+ [existingSignal dispose];
+ }
+
+ RACDisposable *newSignal = [RACAbleWithStart(command, canExecute) toProperty:@"enabled" onObject:self];
@joshaber
joshaber Apr 8, 2013

This could use RAC(self.enabled) = ... to avoid having a fragile keypath.

@jspahrsummers
jspahrsummers Apr 8, 2013

The whole reason we're doing this is to get a disposable.

@joshaber
joshaber Apr 8, 2013

Ah, in that case it can use @keypath to get rid of the string keypath.

@joshaber
ReactiveCocoa member

@KyleLeneau Just a few more comments 👍

@KyleLeneau

Thanks for the comments and follow up, I have made the changes and pushed again.

@joshaber
ReactiveCocoa member

Awesome, thanks! :metal:

@joshaber joshaber merged commit daa9f2a into ReactiveCocoa:master Apr 9, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment