Make Android ViewObservable.input observe TextView instead of String #1545

ronshapiro · 2014-08-04T05:01:25Z

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 · 2014-08-04T05:09:16Z

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

dpsm · 2014-08-04T13:02:54Z

Aside from the public API change LGTM

benjchristensen · 2014-08-04T19:49:42Z

/cc @mttkay @zsxwing

zsxwing · 2014-08-05T01:43:16Z

LGTM since 0.20 will have many breaking changes.

benjchristensen · 2014-08-05T03:27:18Z

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 · 2014-08-05T03:33:23Z

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 · 2014-08-05T03:35:37Z

+1 for text

mttkay · 2014-08-05T07:23:20Z

rxjava-contrib/rxjava-android/src/main/java/rx/android/observables/ViewObservable.java

@@ -30,8 +30,8 @@
        return Observable.create(new OperatorViewClick<T>(view, emitInitialValue));
    }

-    public static Observable<String> input(final EditText input, final boolean emitInitialValue) {
-        return Observable.create(new OperatorEditTextInput(input, emitInitialValue));
+    public static <T extends TextView> Observable<T> input(final T input, final boolean emitInitialValue) {


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

…sion to ViewObservable.text

ronshapiro · 2014-08-05T19:24:17Z

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 · 2014-08-05T19:25:58Z

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

mttkay · 2014-08-05T19:48:02Z

rxjava-contrib/rxjava-android/src/main/java/rx/operators/OperatorTextViewInput.java

+            }
+        };
+
+        final Subscription subscription = AndroidSubscriptions.unsubscribeInUiThread(new Action0() {


@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?

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).

Great explanation.

@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 · 2014-08-08T04:56:44Z

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 · 2014-08-14T14:57:18Z

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

dpsm · 2014-08-14T15:00:21Z

LGTM

cloudbees-pull-request-builder · 2014-08-14T15:37:35Z

RxJava-pull-requests #1489 ABORTED

benjchristensen · 2014-08-14T16:02:06Z

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

mttkay · 2014-08-14T19:59:47Z

👍

benjchristensen · 2014-08-14T21:44:32Z

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

Make Android ViewObservable.input observe TextView instead of String

Make Android ViewObservable.input observe TextView instead of String

0c4dac6

mttkay reviewed Aug 5, 2014
View reviewed changes

Bring back ViewObservable.input(EditText, boolean) and rename new ver…

b338afa

…sion to ViewObservable.text

mttkay reviewed Aug 5, 2014
View reviewed changes

Clarify OperatorTextViewInputTest methods

82bc2de

benjchristensen mentioned this pull request Aug 14, 2014

Add ViewActions utilities for Android #1584

Closed

benjchristensen added a commit that referenced this pull request Aug 14, 2014

Merge pull request #1545 from ronshapiro/master

4cb3e40

Make Android ViewObservable.input observe TextView instead of String

benjchristensen merged commit 4cb3e40 into ReactiveX:master Aug 14, 2014

ronshapiro mentioned this pull request Aug 26, 2014

ViewObservable.text() does not work with Observable.delay() ReactiveX/RxAndroid#7

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Android ViewObservable.input observe TextView instead of String #1545

Make Android ViewObservable.input observe TextView instead of String #1545

ronshapiro commented Aug 4, 2014

cloudbees-pull-request-builder commented Aug 4, 2014

dpsm commented Aug 4, 2014

benjchristensen commented Aug 4, 2014

zsxwing commented Aug 5, 2014

benjchristensen commented Aug 5, 2014

ronshapiro commented Aug 5, 2014

zsxwing commented Aug 5, 2014

mttkay Aug 5, 2014

ronshapiro commented Aug 5, 2014

cloudbees-pull-request-builder commented Aug 5, 2014

mttkay Aug 5, 2014

benjchristensen Aug 14, 2014

zsxwing Aug 15, 2014

dpsm Aug 15, 2014

benjchristensen commented Aug 8, 2014

ronshapiro commented Aug 14, 2014

dpsm commented Aug 14, 2014

cloudbees-pull-request-builder commented Aug 14, 2014

benjchristensen commented Aug 14, 2014

mttkay commented Aug 14, 2014

benjchristensen commented Aug 14, 2014

Make Android ViewObservable.input observe TextView instead of String #1545

Make Android ViewObservable.input observe TextView instead of String #1545

Conversation

ronshapiro commented Aug 4, 2014

cloudbees-pull-request-builder commented Aug 4, 2014

dpsm commented Aug 4, 2014

benjchristensen commented Aug 4, 2014

zsxwing commented Aug 5, 2014

benjchristensen commented Aug 5, 2014

ronshapiro commented Aug 5, 2014

zsxwing commented Aug 5, 2014

mttkay Aug 5, 2014

Choose a reason for hiding this comment

ronshapiro commented Aug 5, 2014

cloudbees-pull-request-builder commented Aug 5, 2014

mttkay Aug 5, 2014

Choose a reason for hiding this comment

benjchristensen Aug 14, 2014

Choose a reason for hiding this comment

zsxwing Aug 15, 2014

Choose a reason for hiding this comment

dpsm Aug 15, 2014

Choose a reason for hiding this comment

benjchristensen commented Aug 8, 2014

ronshapiro commented Aug 14, 2014

dpsm commented Aug 14, 2014

cloudbees-pull-request-builder commented Aug 14, 2014

benjchristensen commented Aug 14, 2014

mttkay commented Aug 14, 2014

benjchristensen commented Aug 14, 2014