Rightpoint / RZDataBinding

Lightweight KVO-based data binding options.
Other
543 stars 57 forks source link

Parameter to trigger initial function call for rz_bind:toKeyPath:ofObject:withFunction #25

Closed gekitz closed 9 years ago

gekitz commented 9 years ago

I can see that it makes sense to call the function immediately in the setup process, but I ran into some cases where it would make sense to define wether the function should be called during the setup process or if the first function call should be defered until the real first change was triggered by KVO.

Any thoughts on that?

jvisenti commented 9 years ago

The word "function" in the method name is a bit misleading, and should really be called "transform" (this will probably happen in 2.0). This parameter is meant to be nothing more than an id -> id transform on the value that was set to the bound key path. It gives you an opportunity to, say, map NO -> YES or 0xFFFFFFFF -> [UIColor whiteColor].

The reason that the bind: methods are called immediately is so that the bound key value and the key path value are guaranteed to initially match. It sounds like for your use case it may make more sense to use rz_addTarget:action:forKeyPathChange:, which doesn't call the action initially. In the callback method, you can then do more complex things than a simple value transform.

If you still think bind:...withFunction: should take a callImmediately flag, perhaps another solution would be for the bind: methods to check the existing bound key value and only call setValue:forKey: if it is not already isEqual: to the new value? This would then be covered by #24

gekitz commented 9 years ago

I agree that "transform" would be a better name :+1:

I mean, maybe is my use case so special that it makes no sense to make this change in the API. If you're interested, I would like to explain it a little bit. Maybe we can find more cases which would justify an api change. In my opinion all cases which consider some sort of validation would benefit from it.

Consider the following view model:

@interface TRCreateAccountModel : NSObject
@property (nonatomic, strong) NSString *name;
@property (nonatomic, readonly) BOOL nameValid;
- (void)validateModel;
@end

@implementation TRCreateAccountModel

- (void)validateModel {

    [self willChangeValueForKey:@"nameValid"];
    self.nameValid = self.name.length > 0;
    [self didChangeValueForKey:@"nameValid"];
}
@end

And a corresponding controller which has a UIButton and a UITexfield in it's view. The UIButton would trigger the validation. You bind the textfield.text to model.name and the model.nameValid to the textField.textColor. During the setup of the bind the transform function would be called and the textField.textColor would change to the "invalid color" because there is no text in it.

This could be solved in 3 ways:

Unfortunately this won't be solved by #34. I would be happy to help you out with this and create PR if you think that's something which makes sense.

jvisenti commented 9 years ago

Thanks for the detailed feedback and example! A few comments:

1) If there's no text in the text field, then it shouldn't matter what the text color is because it won't be visible ;)

2) You shouldn't need to manually call willChange/didChange in validateModel, and doing so will trigger a double change event unless you return NO from +automaticallyNotifiesObserversOfNameValid.

3) I think in your case establishing a binding is not strictly correct since under certain circumstances you don't want the binding to apply. For example, before the user has entered any name or perhaps while the user is in the process of entering a name. It may make more sense to have something like:

...
// assuming self.viewModel is an object of type TRCreateAccountModel
[self rz_addTarget:self action:@selector(accountNameValidityChanged:) forKeyPathChange:RZDB_KP_SELF(viewModel.nameValid)];
...

- (void)accountNameValidityChanged:(NSDictionary *)change
{
    BOOL valid = [change[kRZDBChangeKeyObject] boolValue];

    // you might have additional logic to determine which color to show
    self.nameField.textColor = (valid || self.nameField.isEditing) ? kValidColor : kInvalidColor;
}

From an API standpoint, the bind: methods are intended to establish value equality as soon as possible, and always enforce it. This functionality is actually a subset of the functionality that can be achieved with the rz_addTarget:action: methods, which are more general.

gekitz commented 9 years ago

okay, that sounds like I want to do something with the API which it wasn't intended to be used for. Thanks for clarifying things.

@ 1) I was actually changing the placeholder color of the UITextField, which is only available in my subclass and I wanted to simply the case, but you're right, it doesn't make sense the way I described it, but it makes more sense if you consider the placeholder's color to be changed based on the valid value.

@ 2) I think I have to do it, bc as far as I know KVO only works on objects automatically and my BOOL in that case is no object and I would have to trigger it manually.

I'll close the issue and again thanks for clarifying.

jvisenti commented 9 years ago

Sure thing. I recommend testing the assumption that KVO only works automatically for objects. If you take a look at testMultiRegistration in RZDBTests you'll see an example of getting a KVO callback on a primitive-type property automatically. If you find that your BOOL is, in fact, not notifying observers automatically, then I'd like to know! Good luck.