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

asObservableFor causes a crash if the property value is nil #19

Closed
serieuxchat opened this issue Mar 4, 2015 · 13 comments
Closed

asObservableFor causes a crash if the property value is nil #19

serieuxchat opened this issue Mar 4, 2015 · 13 comments
Labels

Comments

@serieuxchat
Copy link

I have a class Person with a property commonName. In some cases the value of this property is nil.

If I would like to watch this property, I do the following:

// Suppose that p is of type Person
var commonNameDynamic = Dynamic.asObservableFor(p, keyPath: "commonName")

This causes a crash if the value of commonName is nil.

@srdanrasic
Copy link
Contributor

Hi Sergey,

Can you try specifying the type as optional String and let me know if it helps:

let commonNameDynamic = Dynamic<String?>.asObservableFor(p, keyPath: "commonName")

You'll need to map it to non-optional when binding it to label.

commonNameDynamic.map { $0 ?? "" } ->> commonNameLabel

Also, aim for making your Dynamics as constants (using let keyword) instead of variables.

@serieuxchat
Copy link
Author

Just tried it. Unfortunately, it still crashes...

Thanks for making such an awesome framework, btw! (we'll iron out these small things, I am sure; as for the rest, it just rocks!)

@serieuxchat
Copy link
Author

I guess, the problem is here:
let dynamic = InternalDynamic((object.valueForKeyPath(keyPath) as? T)!, faulty: false)

valueForKeyPath gives back nil, and then there is a forced unwrap, right?

@srdanrasic
Copy link
Contributor

Thank you!

Yeah, that looks like a problem. Working on fix right now...

@serieuxchat
Copy link
Author

Thanks, mate!

@serieuxchat
Copy link
Author

Hi Srđan,
So far I have come up with the following work-around (added a method that also accepts a default value - to be used in case the property is nil)

public class func asObservableFor(object: NSObject, keyPath: String, defaultValue: T) -> Dynamic<T> 
{
    let keyPathValue: AnyObject? = object.valueForKeyPath(keyPath)
    let value: T = (keyPathValue != nil) ? (keyPathValue as? T)! : defaultValue
    let dynamic = InternalDynamic(value, faulty: false)

    let helper = DynamicKVOHelper(keyPath: keyPath, object: object as NSObject) {
        [unowned dynamic] (v: AnyObject) -> Void in

        if (v is NSNull)
        {
            dynamic.value = defaultValue
        }
        else
        {
            dynamic.value = (v as? T)!
        }
    }

    dynamic.retain(helper)
    return dynamic
}

@srdanrasic
Copy link
Contributor

Yeah, it's pain looks like. I don't see a way to do it without defaultValue. If you want, you can do pull request with your update.

@srdanrasic srdanrasic added the Bug label Mar 4, 2015
@serieuxchat
Copy link
Author

I will think a little more about it, but for now, I guess, it would do with the defaultValue approach.
Are you going to keep the second version of the method or always require a default value (if you do a pull request)?

It also looks like it's currently not possible to have an update go in the other direction (with some property of NSObject) I.e. if I want to have person.commonName updated automatically whenever a user types something into some corresponding text field... (In this case the property is not a dynamic and must be updated with setValue:forKeyPath, right?)

@srdanrasic
Copy link
Contributor

Its a good question, I'll give it some more thought tonight.

Two way binding with KVO property should also be doable by having feedback bond like this:

  ...

  let feedbackBond = Bond<T>() { [unowned object] value in
    object.setValue(value, forKey: keyPath)
  }

  dynamic.bindTo(feedbackBond, fire: false, strongly: false)
  dynamic.retain(feedbackBond)

  dynamic.retain(helper)
  return dynamic

But there are some problems with types I need to fix first...

@srdanrasic
Copy link
Contributor

Default value should always be needed to prevent unintentional bugs.

@srdanrasic
Copy link
Contributor

Thanks for your help @serieuxchat . I've fixed this in v3.0.1 with your suggestion.

Additionally, I added another way to create Dynamic representations of K-V observed properties which is to be used for two-way bindings. Checkout README.

@serieuxchat
Copy link
Author

Awesome!
Thanks a lot for reacting so quickly!

I am sure, the framework will get used a lot. You've done a beautiful thing! Thanks again for the effort and the love you've put into this.

Kind regards,
Sergey

On 4 mrt. 2015, at 22:37, Srđan Rašić notifications@github.com wrote:

Thanks for your help @serieuxchat . I've fixed this in v3.0.1 with your suggestion.

Additionally, I added another way to create Dynamic representations of K-V observed properties which is to be used for two-way bindings. Checkout README.


Reply to this email directly or view it on GitHub.

@srdanrasic
Copy link
Contributor

You're welcome! Should you find any more issues, feel free to open another issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants