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

Extensions for NSControl and UIControl #2182

Closed
jspahrsummers opened this Issue Jul 18, 2015 · 18 comments

Comments

Projects
None yet
@jspahrsummers
Member

jspahrsummers commented Jul 18, 2015

These should be ported from the Objective-C API, so we can create well-typed versions in Swift.

The README should also be updated when this happens.

@patr1ck

This comment has been minimized.

Show comment
Hide comment
@patr1ck

patr1ck Jul 21, 2015

FWIW I've been using the extensions provided by @ColinEberhardt's Swift/RAC sample projects:
https://github.com/ColinEberhardt/ReactiveTwitterSearch/blob/82ab9d2595b07cbefd4c917ae643b568dd858119/ReactiveTwitterSearch/Util/UIKitExtensions.swift

It would be cool if the official extensions similarly exposed not Signals or SignalProducers, but MutablePropertys where possible. Maybe that's already the plan though. 😄

patr1ck commented Jul 21, 2015

FWIW I've been using the extensions provided by @ColinEberhardt's Swift/RAC sample projects:
https://github.com/ColinEberhardt/ReactiveTwitterSearch/blob/82ab9d2595b07cbefd4c917ae643b568dd858119/ReactiveTwitterSearch/Util/UIKitExtensions.swift

It would be cool if the official extensions similarly exposed not Signals or SignalProducers, but MutablePropertys where possible. Maybe that's already the plan though. 😄

@liscio

This comment has been minimized.

Show comment
Hide comment
@liscio

liscio Aug 18, 2015

Contributor

@patr1ck The above extensions are great, however I think that the implementation can be dangerous and/or confusing in the wrong hands, and when used in conjunction with UITableViewCells.

I would recommend that if used, the approach be modified slightly so that a new MutableProperty is vended for each exposed property. The trouble arises when the repeated reuse of a cell, combined with code along these lines while binding your ViewModel to a table view cell:

imageView.rac_image <~ viewModel.fetchingSignalProducer
textView.rac_text <~ viewModel.otherSlowTextSignalProducer

will lead to an explosion of long-lived signals as the user scrolls. If your otherSlowTextSignalProducer is attached to a large list of ViewModels, neither the MutableProperty vended by rac_text nor the SignalProducer created implicitly here will terminate quickly enough.

Instead, if we replace the MutableProperty with a new one each time it is "bound" to a Signal or SignalProducer then we can guarantee that termination will occur each time.

I am also aware of the approach to monitor rac_prepareForReuseSignal to cancel out any of these longer-running operations so they don't stick around too long, but then I feel that it is adding unnecessary complexity to the API's usability and forces the user of the underlying ViewModel to know an awful lot about the Signal{Producer,}s they are binding to, and that they won't change in the future.

Frankly, I feel it is much less confusing if the "binding operator" as applied above replaces an existing binding rather than allowing many signals to target the same property by accident.

Spending some time with Instruments last night / this morning I have also found that the rac_prepareForReuseSignal approach is also less performant than my "blow away the MutableProperty every time" approach. This was with the swift2 branch version of RAC built with all the optimizations on, and while my memory is fuzzy I want to say it was on the order of half the number of samples during profiling.

Just throwing my $0.02 on the pile…

/cc @ColinEberhardt

Contributor

liscio commented Aug 18, 2015

@patr1ck The above extensions are great, however I think that the implementation can be dangerous and/or confusing in the wrong hands, and when used in conjunction with UITableViewCells.

I would recommend that if used, the approach be modified slightly so that a new MutableProperty is vended for each exposed property. The trouble arises when the repeated reuse of a cell, combined with code along these lines while binding your ViewModel to a table view cell:

imageView.rac_image <~ viewModel.fetchingSignalProducer
textView.rac_text <~ viewModel.otherSlowTextSignalProducer

will lead to an explosion of long-lived signals as the user scrolls. If your otherSlowTextSignalProducer is attached to a large list of ViewModels, neither the MutableProperty vended by rac_text nor the SignalProducer created implicitly here will terminate quickly enough.

Instead, if we replace the MutableProperty with a new one each time it is "bound" to a Signal or SignalProducer then we can guarantee that termination will occur each time.

I am also aware of the approach to monitor rac_prepareForReuseSignal to cancel out any of these longer-running operations so they don't stick around too long, but then I feel that it is adding unnecessary complexity to the API's usability and forces the user of the underlying ViewModel to know an awful lot about the Signal{Producer,}s they are binding to, and that they won't change in the future.

Frankly, I feel it is much less confusing if the "binding operator" as applied above replaces an existing binding rather than allowing many signals to target the same property by accident.

Spending some time with Instruments last night / this morning I have also found that the rac_prepareForReuseSignal approach is also less performant than my "blow away the MutableProperty every time" approach. This was with the swift2 branch version of RAC built with all the optimizations on, and while my memory is fuzzy I want to say it was on the order of half the number of samples during profiling.

Just throwing my $0.02 on the pile…

/cc @ColinEberhardt

@neilpa

This comment has been minimized.

Show comment
Hide comment
@neilpa

neilpa Aug 18, 2015

Member

Generally, you should avoid re-binding, especially in your table view cells. Instead, I would recommend exposing a MutableProperty<MyViewModel?> on your cell and internally binding to the views once during load.

Member

neilpa commented Aug 18, 2015

Generally, you should avoid re-binding, especially in your table view cells. Instead, I would recommend exposing a MutableProperty<MyViewModel?> on your cell and internally binding to the views once during load.

@liscio

This comment has been minimized.

Show comment
Hide comment
@liscio

liscio Aug 18, 2015

Contributor

@neilpa I like the idea, but how would this look in practice?

When asked to configure a UITableViewCell from the UITableViewDataSource, I suspect that this is where we'd set theCell.myViewModelProperty.value = viewModelForThisIndexPath, correct? Is there a mechanism to grab SignalProducers or {Mutable,Constant}Property instances for the MutableProperty<MyViewModel?>'s children?

Because RAC3 (and the swift2 branch) are still so new, there is a dearth of "best practices" to follow for this stuff, so further pointers/guidance are highly appreciated.

Contributor

liscio commented Aug 18, 2015

@neilpa I like the idea, but how would this look in practice?

When asked to configure a UITableViewCell from the UITableViewDataSource, I suspect that this is where we'd set theCell.myViewModelProperty.value = viewModelForThisIndexPath, correct? Is there a mechanism to grab SignalProducers or {Mutable,Constant}Property instances for the MutableProperty<MyViewModel?>'s children?

Because RAC3 (and the swift2 branch) are still so new, there is a dearth of "best practices" to follow for this stuff, so further pointers/guidance are highly appreciated.

@neilpa

This comment has been minimized.

Show comment
Hide comment
@neilpa

neilpa Aug 19, 2015

Member

I suspect that this is where we'd set theCell.myViewModelProperty.value = viewModelForThisIndexPath, correct?

Exactly

Is there a mechanism to grab SignalProducers or {Mutable,Constant}Property instances for the MutableProperty<MyViewModel?>'s children?

Yea, if the view model provides producers or properties for these then you'll want to use flatten(.Latest) (or flatMap) on the producer for the cell's vm property to grab the desired value.

// N.B. github comment box compiled
public let viewModel: MutableProperty<MyViewModel?> = MutableProperty(nil)
private var imageView: UIImageView!
private var textView: UITextField!

func viewDidLoad() {
    imageView.rac_image <~ viewModel.producer
        // Every time the viewModel property changes you can re-fetch the new image.
        .flatMap(.Latest) { vm in
            return vm?.fetchImage() ?? SignalProducer(value: nil)
            // or if it exposes an image property instead of a producer
            // vm?.imageProperty.producer ?? SignalProducer(value: nil)
        }
        // may also need to flatMapError (previously catch) if the above producer
        // returns something other than NoError

    textView.rac_text <~ viewModel.producer.map { $0?.someText ?? "" }
}

I've found this pattern to work well for most views, not just cells.

Member

neilpa commented Aug 19, 2015

I suspect that this is where we'd set theCell.myViewModelProperty.value = viewModelForThisIndexPath, correct?

Exactly

Is there a mechanism to grab SignalProducers or {Mutable,Constant}Property instances for the MutableProperty<MyViewModel?>'s children?

Yea, if the view model provides producers or properties for these then you'll want to use flatten(.Latest) (or flatMap) on the producer for the cell's vm property to grab the desired value.

// N.B. github comment box compiled
public let viewModel: MutableProperty<MyViewModel?> = MutableProperty(nil)
private var imageView: UIImageView!
private var textView: UITextField!

func viewDidLoad() {
    imageView.rac_image <~ viewModel.producer
        // Every time the viewModel property changes you can re-fetch the new image.
        .flatMap(.Latest) { vm in
            return vm?.fetchImage() ?? SignalProducer(value: nil)
            // or if it exposes an image property instead of a producer
            // vm?.imageProperty.producer ?? SignalProducer(value: nil)
        }
        // may also need to flatMapError (previously catch) if the above producer
        // returns something other than NoError

    textView.rac_text <~ viewModel.producer.map { $0?.someText ?? "" }
}

I've found this pattern to work well for most views, not just cells.

@neilpa

This comment has been minimized.

Show comment
Hide comment
@neilpa

neilpa Aug 19, 2015

Member

I've even messed around with creating a of ReactiveView<ViewModel> that enforces and simplifies this pattern but in the end it didn't provide any real value. However, it may be worthwhile to outline this general pattern in the docs for building UI code.

Member

neilpa commented Aug 19, 2015

I've even messed around with creating a of ReactiveView<ViewModel> that enforces and simplifies this pattern but in the end it didn't provide any real value. However, it may be worthwhile to outline this general pattern in the docs for building UI code.

@liscio

This comment has been minimized.

Show comment
Hide comment
@liscio

liscio Aug 19, 2015

Contributor

This is great, Neil!

So with all this said, will changes to the underlying viewModel cause pending signals to the rac_image to be shut down immediately, or will they still linger if they are long-running operations?

Contributor

liscio commented Aug 19, 2015

This is great, Neil!

So with all this said, will changes to the underlying viewModel cause pending signals to the rac_image to be shut down immediately, or will they still linger if they are long-running operations?

@neilpa

This comment has been minimized.

Show comment
Hide comment
@neilpa

neilpa Aug 19, 2015

Member

@liscio Assuming the returned producer from fetchImage is properly behaved and cancels outstanding work on disposal then it shouldn't "hang around". If you're really curious you can take a look at the implementation of switchToLatest which is what implements flatten(.Latest). Specifically, this line will dispose of the previous inner producer (e.g. image fetch) when a new outer producer (e.g. view model) is sent.

Member

neilpa commented Aug 19, 2015

@liscio Assuming the returned producer from fetchImage is properly behaved and cancels outstanding work on disposal then it shouldn't "hang around". If you're really curious you can take a look at the implementation of switchToLatest which is what implements flatten(.Latest). Specifically, this line will dispose of the previous inner producer (e.g. image fetch) when a new outer producer (e.g. view model) is sent.

@liscio

This comment has been minimized.

Show comment
Hide comment
@liscio

liscio Aug 19, 2015

Contributor

@neilpa That was helpful, thanks.

A preliminary test with the changes proposed above made a significant improvement to both the code and performance. If you're looking for another set of eyes on whatever you come up with for #2269, I'd be happy to help.

Contributor

liscio commented Aug 19, 2015

@neilpa That was helpful, thanks.

A preliminary test with the changes proposed above made a significant improvement to both the code and performance. If you're looking for another set of eyes on whatever you come up with for #2269, I'd be happy to help.

@pteasima

This comment has been minimized.

Show comment
Hide comment
@pteasima

pteasima Aug 20, 2015

@neilpa Does this approach also work on RC1 or is it only for the swift2 version?

When I do viewModel.producer |> map { $0.someText }, $0 is of type MyViewModel? so it doesnt compile. I could add an |> ignoreNil, but I am not sure I want that.
How do you handle cases when viewModel.value == nil? Does the view just stay bound to the previous viewModel or is there a way to reset the UI to an "empty" state?

pteasima commented Aug 20, 2015

@neilpa Does this approach also work on RC1 or is it only for the swift2 version?

When I do viewModel.producer |> map { $0.someText }, $0 is of type MyViewModel? so it doesnt compile. I could add an |> ignoreNil, but I am not sure I want that.
How do you handle cases when viewModel.value == nil? Does the view just stay bound to the previous viewModel or is there a way to reset the UI to an "empty" state?

@neilpa

This comment has been minimized.

Show comment
Hide comment
@neilpa

neilpa Aug 20, 2015

Member

It should work for 1.2 as well.

When I do viewModel.producer |> map { $0.someText }, $0 is of type MyViewModel? so it doesnt compile

I typed this up quickly in the browser without testing it. That should be $0?.someText ?? "".

How do you handle cases when viewModel.value == nil?

In whatever way makes the most since for your UI. Just need to return a "reasonable" value when the view model is nil. For text views that's often empty strings.

Member

neilpa commented Aug 20, 2015

It should work for 1.2 as well.

When I do viewModel.producer |> map { $0.someText }, $0 is of type MyViewModel? so it doesnt compile

I typed this up quickly in the browser without testing it. That should be $0?.someText ?? "".

How do you handle cases when viewModel.value == nil?

In whatever way makes the most since for your UI. Just need to return a "reasonable" value when the view model is nil. For text views that's often empty strings.

@pteasima

This comment has been minimized.

Show comment
Hide comment
@pteasima

pteasima Aug 20, 2015

Edit: I was mixing two things together (map and flatMap).
This comment is for the case that viewModel.someText is a String:
Actually textView.rac_text <~ viewModel.producer.map { $0?.someText } wont work either since you cant do textView.rac_text <~ SignalProducer(nil). What you need is something like textView.rac_text <~ viewModel.producer.map { $0?.someText ?? ""}, which is logically ok, but I think its too verbose. For most cases (e.g. TableViewCells), you don't even need to properly handle the case of a nil viewModel, you just need to get the compiler to stop complaining.

If viewModel.someText is a MutableProperty<String>, I would like to use textView.rac_text <~ viewModel.producer.flatMap (.Latest) { $0?.someText.producer } but that wont compile either, I would have to do textView.rac_text <~ viewModel.producer.flatMap (.Latest) { $0?.someText.producer ?? ConstantProperty("") } which is very verbose as well.

I think the second example is what matters. Is there a better way to do it?

pteasima commented Aug 20, 2015

Edit: I was mixing two things together (map and flatMap).
This comment is for the case that viewModel.someText is a String:
Actually textView.rac_text <~ viewModel.producer.map { $0?.someText } wont work either since you cant do textView.rac_text <~ SignalProducer(nil). What you need is something like textView.rac_text <~ viewModel.producer.map { $0?.someText ?? ""}, which is logically ok, but I think its too verbose. For most cases (e.g. TableViewCells), you don't even need to properly handle the case of a nil viewModel, you just need to get the compiler to stop complaining.

If viewModel.someText is a MutableProperty<String>, I would like to use textView.rac_text <~ viewModel.producer.flatMap (.Latest) { $0?.someText.producer } but that wont compile either, I would have to do textView.rac_text <~ viewModel.producer.flatMap (.Latest) { $0?.someText.producer ?? ConstantProperty("") } which is very verbose as well.

I think the second example is what matters. Is there a better way to do it?

@ivan-ushakov

This comment has been minimized.

Show comment
Hide comment
@ivan-ushakov

ivan-ushakov Nov 4, 2015

@neilpa could you please create simple project with example of your approach for ViewModel and table cell binding?

ivan-ushakov commented Nov 4, 2015

@neilpa could you please create simple project with example of your approach for ViewModel and table cell binding?

@neilpa

This comment has been minimized.

Show comment
Hide comment
@neilpa

neilpa Nov 4, 2015

Member

@ivan-ushakov Sorry, but not anytime soon

Member

neilpa commented Nov 4, 2015

@ivan-ushakov Sorry, but not anytime soon

@mortyccp

This comment has been minimized.

Show comment
Hide comment
@mortyccp

mortyccp Dec 29, 2015

Any update on the progress?

mortyccp commented Dec 29, 2015

Any update on the progress?

@NachoSoto

This comment has been minimized.

Show comment
Hide comment
@NachoSoto

NachoSoto Dec 29, 2015

Member

I'd be in favor of adding this to the next version milestone.

This is what I've been using. It would probably be a good idea for now to implement this in terms of the Objective-C APIs: https://gist.github.com/NachoSoto/25b0003c1b15d7d11a8d

Member

NachoSoto commented Dec 29, 2015

I'd be in favor of adding this to the next version milestone.

This is what I've been using. It would probably be a good idea for now to implement this in terms of the Objective-C APIs: https://gist.github.com/NachoSoto/25b0003c1b15d7d11a8d

@mpurland

This comment has been minimized.

Show comment
Hide comment
@mpurland

mpurland Dec 30, 2015

A few weeks ago I created a library that I've been using to pull in a lot of the work related to binding with UIKit. I would love to get some feedback.

https://github.com/mpurland/ReactiveBind

I've also created a library around MVVM.

https://github.com/mpurland/Morpheus

mpurland commented Dec 30, 2015

A few weeks ago I created a library that I've been using to pull in a lot of the work related to binding with UIKit. I would love to get some feedback.

https://github.com/mpurland/ReactiveBind

I've also created a library around MVVM.

https://github.com/mpurland/Morpheus

@RuiAAPeres

This comment has been minimized.

Show comment
Hide comment
@RuiAAPeres

RuiAAPeres Apr 22, 2016

Member

You can also check Rex and keep an eye on #2790.

Member

RuiAAPeres commented Apr 22, 2016

You can also check Rex and keep an eye on #2790.

@RuiAAPeres RuiAAPeres closed this Apr 22, 2016

@ikesyo ikesyo added the wontfix label Apr 22, 2016

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