Make Android ViewObservable.input observe TextView instead of String

[ronshapiro]

The previous version required ViewObservable.input(TextView, boolean) to emit just Strings of the updated text, but not CharSequences, which is the declared implementation of TextView's mText. This still allows for similar functionality as before via:

ViewObservable.input(myTextView, false).map((textView) -> textView.getText().toString())

It is also flexible like ViewObservable.clicks in that it returns a reference to the View, which should make it more flexible for using it with other reactive methods.

I held off from doing the same to ViewObservable.input(CompoundButton, boolean) but could do so if you think it would be valuable/more consistent.

This would be a breaking change. In order to make it not a breaking change, this method could be renamed and the initial method could map like the above snippet. Let me know if you think this is important and I'd be happy to make this change.

[cloudbees-pull-request-builder]

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

[dpsm]

Aside from the public API change LGTM

[benjchristensen]

/cc @mttkay @zsxwing

[zsxwing]

LGTM since 0.20 will have many breaking changes.

[benjchristensen]

Actually the only breaking API changes I'm aware of are in the Scala adaptor. There are some semantic changes that will affect some use cases, but all backpressure work is additive to APIs.

If this change needs to be breaking, so be it, if everyone is okay with it. Can it be done via deprecation though so the old one is there also in 0.20 and then removed in 1.0?

[ronshapiro]

Definitely can bring back and deprecate the old version for 0.20. Honestly, ViewObservable.input isn't the greatest name to begin with since there could be many input streams associated with text views. Thoughts on ViewObservable.text? Or is there something that comes to mind that's more paradigmatic?

[zsxwing]

+1 for text

[mttkay]

I would vote for having an overload that sets the 2nd argument to false. For simplicity.

[ronshapiro]

Just pushed a commit with the name change and bringing back the old, now deprecated, version.

What are the philosophies for the repo on:

• Nearly identical tests: do you make a helper method or trade DRY for test clarity?
• Rebasing commits for PRs?

[cloudbees-pull-request-builder]

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

[mttkay]

@benjchristensen I could be mistaken, but is this still necessary? I believe OperatorObserveOn schedules the unsubscribe action on the given scheduler already. So this might end up scheduling it twice? I guess it depends on whether we expect the caller to observeOn(mainThread()) explicitly?

Maybe this is part of a larger design question: who should be responsible for making sure that UI related code is executed on the UI thread? The operator? The client?

[benjchristensen]

It depends on where in the chain this operator is lifted. If after observeOn then this code is needed, if before then I don't think it is needed.

There was a very lengthy discussion with @zsxwing at one point about this and it appeared there could be some extreme edge cases that could occur so I think this was done here to conservatively protect against that.

observeOn does schedule the unsubscribe: https://github.com/Netflix/RxJava/blob/master/rxjava-core/src/main/java/rx/internal/operators/OperatorObserveOn.java#L223 However, that is scheduling the unsubscribe for this use case: https://github.com/Netflix/RxJava/blob/master/rxjava-core/src/test/java/rx/internal/operators/OperatorObserveOnTest.java#L374 and it affects the "upstream", not "downstream" where I believe this operator will exist.

Thus, if OperatorTextView is below the observeOn operator and registering an unsubscribe it is outside the control of observeOn. That means it needs to schedule the behavior of input.removeTextChangedListener in case the child Subscriber is unsubscribed on a thread other than the UI thread. If however this operator is above observeOn then it should work fine as observeOn would schedule the unsubscribe on the UI thread.

For example:

// observeOn has no control over the unsubscribe of TestViewInput here
someObservable.observeOn(AndroidScheduler).lift(OperatorTestViewInput).subscribe(unsubscribeAtSomePoint)

// but in this one it would
someObservable.lift(OperatorTestViewInput).observeOn(AndroidScheduler).subscribe(unsubscribeAtSomePoint)

@zsxwing am I correctly explaining the edge case that this is here for?

who should be responsible for making sure that UI related code is executed on the UI thread?

The observeOn operator ensures the on* events correctly get scheduled on the given Scheduler, in this case the AndroidScheduler.

Unsubscribes however are flowing the opposite direction, so any operators or Subscribers below observeOn in the chain would need to handle their unsubscribe scheduling themselves (or use unsubscribeOn).

[zsxwing]

Great explanation.

[dpsm]

@benjchristensen @zsxwing regarding "If however this operator is above observeOn then it should work fine as observeOn would schedule the unsubscribe on the UI thread."

I am not sure it is the case. Looking at https://github.com/Netflix/RxJava/blob/master/rxjava-core/src/main/java/rx/internal/operators/OperatorObserveOn.java#L223 it unsubscribes the worker but looking at https://github.com/Netflix/RxJava/blob/master/rxjava-core/src/main/java/rx/internal/operators/OperatorObserveOn.java#L101 the child unsubscribe() will chain upwards in the same thread instead?

I also am not sure I follow the reason why both https://github.com/Netflix/RxJava/blob/master/rxjava-core/src/main/java/rx/internal/operators/OperatorObserveOn.java#L89 and https://github.com/Netflix/RxJava/blob/master/rxjava-core/src/main/java/rx/internal/operators/OperatorObserveOn.java#L100 exist unsubscribing on 2 threads?

[benjchristensen]

Nearly identical tests: do you make a helper method or trade DRY for test clarity?

I generally prefer clarity if it's a small amount of code. When there is a lot (such as the Schedulers) then I reuse helper methods.

[ronshapiro]

Just pushed a commit to clarify the tests. Anything else that still needs to be addressed?

[dpsm]

LGTM

[cloudbees-pull-request-builder]

RxJava-pull-requests #1489 ABORTED

[benjchristensen]

The aborted build is related to this: #1536 (comment)

[mttkay]

👍

[benjchristensen]

Based on those votes I'll merge and include in next RC.