Add ViewActions utilities for Android

[ronshapiro]

No description provided.

[cloudbees-pull-request-builder]

RxJava-pull-requests #1495 SUCCESS
This pull request looks good

[benjchristensen]

Does this replace or add to #1545 ?

Can you provide comments about this pull request such as what it's doing, why it's being proposed, if it affects any existing code, etc?

[ronshapiro]

It's completely separate from that request. These are just common Actions provided as utility for convenience. Shouldn't affect any code - it's all brand new.

One common use case:

Observable<Boolean> isField1Valid = // ...
Observable<Boolean> isField2Valid = // ...

Observable.combineLatest(isField1Valid, isField2Valid)
        .subscribe(setClickable(submitButton));

[benjchristensen]

Okay ... /cc to Android contributors such as @mttkay @zsxwing @dpsm and @Yarikx for review.

[mttkay]

can make these final everywhere?

[ronshapiro]

Good catch - fixed here: https://github.com/ronshapiro/RxJava/commit/5bc9ac13affae0ea919742b41c20903866221b42

[mttkay]

So in general, this is really useful to have and I had started doing something similar in a side project, where I used Rx more in the UI domain. One general thing I don't like about the implementation is that it's based on Action1 which gets translated into a subscriber that only responds to onNext.

This means that whenever your upstream observable fails, that error will get swallowed. I guess alternatively one could do this:

Observable.from(...).doOnNext(setClickable(view)).subscribe(observerThatImplementsOnError);

For some reason I think that a solution using map would be a lot nicer though? i.e., another approach might be to implement these as Func1<Boolean, ViewT>s, where you map an arbitrary sequence to a view, update the view, then emit the view. That way you can rely on OperatorMap to forward errors to subscribers.

Maybe I'm looking at it wrong, just something that occurred to me.

[mttkay]

PS: I know that side effects should usually occur in doOn* implemented as actions, but I find that most of the time it feels more natural to do it as part of a mapping step, since otherwise you end up subscribing a "no op observer", which I always find awkward.

[Yarikx]

LGTM. I also had some part of 'ViewObservers' written just for particular project, so it will be good to have provded ones.

@mttkay I think than using Action1 is Ok, in your provided example

Observable.from(...).doOnNext(setClickable(view)).subscribe(observerThatImplementsOnError);

you can use subscribe method with 2 separated actions for onNext and onError

Observable.from(...).subscribe(setClickable(view), ex -> handleEx(ex));

[dpsm]

LGTM @ronshapiro good job! :)

[ronshapiro]

@mttkay I'm not sure how you would plan on using OperatorMap in that case - what would be the purpose of observing on the view once you've changed the visibility? Plus, you would still have to subscribe, which could be confusing. This wouldn't actually do what you would think it would do:

Observable.from(true, false)
        .map(setVisibility(myView); // this never gets triggered because there's nothing subscribing.

That being said, these are simple classes, and they could easily also implement Func1<Boolean, View> as well and return the view itself if you think there's value to it.

As for error propagation, you can always add an Action1<Throwable> to the .subscribe() call, which will turn both actions into a more complete subscriber. see subscribe(Action1<? super T> onNext, Action1<Throwable> onError, Action0 onComplete). These shouldn't preclude you from observing any errors.

[dpsm]

Just a thought... Given that the Action references a View at the end of the subscription chain for example Observable.from(...).subscribe(setClickable(view); I wonder if these actions shouldn't reference the respective views weakly given the likelihood of upstream async operations?

Unsubscribe should take care of it but given that the process of unsubscribing is manual I wonder if giving this extra "help" to avoid context leaks would be interesting? Thoughts?

[cloudbees-pull-request-builder]

RxJava-pull-requests #1500 SUCCESS
This pull request looks good

[ronshapiro]

@dpsm I'd hope that these Views aren't holding on to an Activity or Service Context, but it may be worthwhile. The view's should already be strongly referenced from their parent/layout, so I don't imagine holding a WeakReference<View> would present any problems. Curious to hear others' thoughts on this. I can try and right a test to make sure that reference is released after unsubscribing though.

[dpsm]

@ronshapiro All Views leak the parent Context one way or another :(

[mttkay]

@ronshapiro fair point, let's use Action1 then

@dpsm has a good point there; all views strongly reference the Activity they're created in (we already had a lot of fun with this when working on bindFragment and its various failed attempts at dealing with this)

I can see this becoming necessary more and more going forward; maybe it would even make sense to provide a wrapper around this, e.g. a WeakView or something analogous to bindFragment that shields it from context leaks?

[dpsm]

@mttkay can you elaborate on "bindFragment and its various failed attempts at dealing with this". I'd love to help fixing that.

In general I like the idea of ViewObservables.weakView(..) or .bindView(..) that trigger a child observable and then holds a weak reference to the View. Did I get your idea right?

[mttkay]

@dpsm The discussion is spread across numerous issue reports and pull requests, but these searches should be a decent starting point:

https://github.com/Netflix/RxJava/search?q=fromFragment&type=Issues
https://github.com/Netflix/RxJava/search?q=bindFragment&type=Issues

(bindFragment used to be called fromFragment and work entirely different)

It's hard or impossible to pro-actively prevent context leaks in subscribers, since Android does not give you enough or even consistent information (hooks) to get notified about context life-cycle events such as activity destruction. WeakReferences usually don't work, since Rx is heavily based on lambdas and short lived objects, so a subscriber that's passed as a closure would get garbage collected immediately.

TLDR is: We must keep strong references to subscribers, but we must not keep strong references to subscribers. :-) Any help welcome in fixing this!