Add AndroidObservable#bindView()

[omo]

bindView() is a sister of bindActivity() and bindFragment().
Thanks for View.OnAttachStateChangeListener, it doesn't require
any manual unsubscription. It frees developers from overriding
View#onDetachFromWindow().

[JakeWharton]

Self-closing paragraph tags are not valid in Javadoc's (awful) HTML. Each of these should just be a single <p> declaration on its own line between paragraphs.

[omo]

Thanks for the review folks, could you take another look?

Apparently files like AndroidObservable.java contains some non-conventional code.
I'll submit a cleanup change later.

[mttkay]

@JakeWharton do you think it would make sense to add a checkstyle step to the build? So people can run against it before pushing. I don't feel strong about any particular code style as long as everyone follows it.

[hamidp]

View.OnAttachStateChangeListener here is an implementation detail. I would move it out the way OperatorTextViewInput does it.

[hamidp]

Also the name of the class does not reflect what it really does. It seems like it's more of a OperatorUnsubscribeOnViewDetached -- naming is the hardest thing in the world.

[omo]

@hamidp Could you take another look? I think the last commit addresses your points.

[hamidp]

@omo Thanks! I looked at some other code, and it seems like something like this might work

public static Observable<T> takeUntilViewDetached(Observable<T> observable, final View view) {
        return observable.takeUntil(Observable.create(new Observable.OnSubscribe<Object>() {
            @Override
            public void call(final Subscriber<? super Object> subscriber) {
                view.addOnAttachStateChangeListener(new View.OnAttachStateChangeListener() {
                    @Override
                    public void onViewAttachedToWindow(View v) {

                    }

                    @Override
                    public void onViewDetachedFromWindow(View v) {
                        v.removeOnAttachStateChangeListener(this);

                        if (!subscriber.isUnsubscribed()) {
                            subscriber.onNext(v);
                            subscriber.onCompleted();
                        }
                    }
                });
            }
        }));
    }

I have no strong feelings on it though.

Last thought if we go with your implementation is to rename the class (again sorry) to OperatorTakeUntilViewDetached

[omo]

@hamidp Tried the snippet and found that it is actually better. It is more polite, as it sends onComplete() instead of silently unsubscribing it. Thanks for the suggestion! Pushed another commit.

I'd keep takeUntilViewDetached private and left the bindView() name because

• The name bindXx() aligns the neighbors'
• It does bind the observable to the main thread.

Once we resolve #12 and let other bindXx() behave more like takeUntil(), we could decouple the main thread binding from the stream closing behavior and make takeUntilXx() family public.

[hamidp]

@omo Any reason to keep takeUntilViewDetached around at all?

[omo]

@hamidp It's simply well factored IMHO.

[hamidp]

@omo I can see that -- definitely makes the bindView method shorter and easier to read.

My objection to the helper method being there at all is that AndroidObservables is a factory class and it is called from only one place.

[omo]

That's fair. Probably having a separate class is the "right" way but obviously overkill. Let me just inline it.

[mttkay]

in general, observeOn should appear "on the right" in a sequence of steps, subscribeOn on "the left".

it wouldn't change semantics here, but I think it's better style to invoke observeOn last.

[omo]

@mttkay Thanks for the review! And you're right. Unsubcription handling is nice to have and
it happened to lead the better-looking code as well. I hope the last commit addresses your points.

Note that I did rebase and squash for clarity.

[mttkay]

👍 if someone else can thumb this, we can get it in.

[hamidp]

👍 from me as well.
On Thu, Nov 20, 2014 at 02:04 Matthias Käppler notifications@github.com
wrote:

[image: 👍] if someone else can thumb this, we can get it in.

—
Reply to this email directly or view it on GitHub
#53 (comment).

[mttkay]

#62

should be done as a follow up, as there was an occurrence of this in this PR as well

[omo]

Thank you! I'm preparing the followup.