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

Extensions for NSControl and UIControl #2182

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

Extensions for NSControl and UIControl #2182

jspahrsummers opened this issue Jul 18, 2015 · 18 comments

Comments

@jspahrsummers
Copy link
Member

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
Copy link

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
Copy link
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
Copy link
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
Copy link
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
Copy link
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
Copy link
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
Copy link
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
Copy link
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
Copy link
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
Copy link

@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
Copy link
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
Copy link

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
Copy link

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

@neilpa
Copy link
Member

neilpa commented Nov 4, 2015

@ivan-ushakov Sorry, but not anytime soon

@mortyccp
Copy link

Any update on the progress?

@NachoSoto
Copy link
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

@mpurland
Copy link

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
Copy link
Member

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

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

No branches or pull requests