Added LifecycleObservable binds

[dlew]

This is the first half of the puzzle that is #12. It provides two main facilities:

• LifecycleObservable.bindUntilLifecycleEvent() lets you specify an exact point in a lifecycle to unsubscribe at.
• LifecycleObservable.bindActivityLifecycle() and bindFragmentLifecycle() which make a best effort guess at when you want a subscription to end, based on the lifecycle state.

Implementation detail: OperatorSubscribeUntil acts like takeUntil(), except that it unsubscribes instead of calling onComplete() when the conditional Observable emits an item.

If this works then we can later figure out how to create these Observable<LifecycleEvent> streams.

[mr-archano]

Put this as default case, so you can save the check at line 130 altogether

[dlew]

The way it's setup now allows me to write one exception for both null-check and default. If I put it in the default case then I have to write the exception in two different places.

[dlew]

I'll grant that perhaps we should throw different messages based on null vs. default. I felt that both null and DESTROY mean the same thing here (either too early or too late to be in the lifecycle).

[mr-archano]

I just commented upon the code above: there is no distinction between null and ATTACH, they both end up throwing an exception. If you want to highlight what is supported and what is not you might do as follow:

   switch (lastEvent) {
       case CREATE:
           return LifecycleEvent.DESTROY;
       case START:
           return LifecycleEvent.STOP;
       case RESUME:
           return LifecycleEvent.PAUSE;
       case PAUSE:
           return LifecycleEvent.STOP;
       case STOP:
           return LifecycleEvent.DESTROY;
       case ATTACH:
       case CREATE_VIEW:
       case DESTROY_VIEW: 
       case DESTROY:
       case DETACH:
           throw new IllegalStateException("Cannot bind to " + lastEvent + " for an Activity.");
       default:
           throw new IllegalStateException("Cannot bind to Activity lifecycle when outside of it.");
   }

[dlew]

You get a NullPointerException if you put a null inside of a switch statement, so you'd still need to do a null-check before then.

Regardless, I see value in us being very explicit about what is/is not allowed here, so I'll make the changes.

[dlew]

I've added some fixup commits based on suggestions from @mr-archano.

Not sure what the merge flow is here, but we should probably rebase/squash before the final merge.

[mr-archano]

Sorry @dlew but I meant that this test suite is not complete given there is no tests around:

• onNext() makes parent to unsubscribe
• onError() forwards the error to parent
• onCompleted() makes parent to unsubscribe

[dlew]

Ah, I see, I can flesh it out.

[mttkay]

I was wondering actually, should we use @Nullable/ @NotNull in this library? If so, which implementation? We use the JetBrains one at SoundCloud, because I believe at some point we found that the javax annotations crashed certain Samsung devices. I would just prefer to document expectations around nullables as part of the signature, not hidden away with a runtime error.

[roman-mazur]

Since it's an Android library, we can use Android support annotations.

[mttkay]

That sounds sensible +1

@dlew do you wanna take a stab at this as part of this PR, otherwise we can
do it in a separate step

On Mon, Nov 24, 2014 at 11:16 AM, Roman Mazur notifications@github.com
wrote:

In rxandroid/src/main/java/rx/android/lifecycle/LifecycleObservable.java:

•    throw new AssertionError("LifeCycleObservable cannot be instantiated.");
  

• }

• /**

• \* Binds the given source to a lifecycle.
  

• \* <p/>
  

• \* When the lifecycle event occurs, the source will cease to receive any notifications.
  

• *
  

• \* @param lifecycle the lifecycle sequence
  

• \* @param source    the source sequence
  

• \* @param event     the event which should conclude notifications to the source
  

• */
  

• public static Observable bindUntilLifecycleEvent(Observable lifecycle,

•                                                        Observable<T> source,
  

•                                                        final LifecycleEvent event) {
  

•    if (lifecycle == null || source == null) {
  

Since it's an Android library, we can use Android support annotations
http://tools.android.com/tech-docs/support-annotations.

—
Reply to this email directly or view it on GitHub
https://github.com/ReactiveX/RxAndroid/pull/75/files#r20781487.

Matthias Käppler

Engineer

Twitter: https://twitter.com/mttkay

Skype: matthias-sc

SoundCloud Ltd. | Rheinsberger Str. 76/77, 10115 Berlin, Germany | +49
(0)172 2345679

Managing Director: Alexander Ljung | Incorporated in England & Wales
with Company
No. 6343600 | Local Branch Office | AG Charlottenburg | HRB 110657B

Capture and share your music & audio on SoundCloud
http://soundcloud.com/creators

[dlew]

Agreed that @Nullable and @NotNull would be great.

I think it'd make more sense in another PR. In this PR it would either be too limited in scope (to just LifecycleObservable) or it would make the PR too broad (if I fixed everything).

[mttkay]

Pretty neat solution! 👍

[roman-mazur]

To be honest I do not like the idea. Activity/Fragment lifecycle is a rather complicated thing. And I do not like when a new level of abstraction appears on top of it in any library or tool.

Also you try to generalize activity/fragment lifecycle (use same states for both of them). They are different though...
ATTACH event is ambiguous for me. Although we almost always think about fragments when attach verb is used, Activity has onAttachedToWindow callback (similar to the View class method). If you proceed with such a generalization, you can end up with using these events for View lifecycle as well, which is ugly, in my opinion.

As for me it's better to make a nice wiki page with a few recipes how to manage subscriptions in activities/fragments with subjects, operators, whatever.

[mr-archano]

I put some comment about this PR on the main discussion running in #12, but I will reiterate my main concerns with this solution:

• there's a lot of checking around what can and cannot be used to bind the unsubscribing trigger, mainly because the events allowed are coming from the same enum for both fragments and activities, while something more elegant can be done with inheritance and generics as drafted here
• LifecycleObservable.bindActivityLifecycle() and bindFragmentLifecycle() are adding too much behaviour and look a bit convoluted to me. I don't think we should have such opinionated solutions as part of the library, let's keep it nuts and bolts.

[dlew]

We could use two separate enums for fragment lifecycle vs. activity lifecycle. (As an aside, using strings as keys instead is not a solution IMO because of lack of type checking.) I don't have a strong opinion on it so I'd like a maintainer to weigh in.

@roman-mazur: I don't view ATTACH as ambiguous. If you wanted to track onAttachedToWindow() it would be named ATTACHED_TO_WINDOW. Regardless, I don't consider either (in View or Activity) to be part of the lifecycle in the first place. Its place is not well-defined in terms of the rest of the lifecycle.

@mr-archano I feel like I ought to go through my entire thought process here for why I came up with this solution, because the same thoughts I had are coming up now. This is the third iteration of the solution I've come up with:

1. First solution used CompositeSubscription + bindLifecycle(). The issue here is that while it's very explicit, it's not very reactive in that it works with Subscriptions. That's why I wanted to switch to a takeUntil()-based solution, so that the bind method would return an Observable instead. In addition, it required a lot of special care taken in each on* method (to know exactly what to do).
2. The second solution used takeUntil() but had a separate LifecycleManager object (which was similar to @mr-archano's EventDrivenUnsubscriber). When I sat around long enough thinking about it, storing extra data (like subjects or listeners) didn't make any sense. All I really cared about was a series of events, and the timing of binding/unbinding.
3. ...Which led me to solution presented here. This is only presented as half the solution, because it cleanly separates out concerns. Here is a way to handle a series of events for automatic binding/unbinding. Next would be a provider of these events. When providing them, it's a simple matter of creating an Observable to emit items in sequence. No special logic is required because it's all contained here.

---

OVERALL - This is a problem that a lot of developers need to solve. I can't imagine many people using RxAndroid and not needing it. Everyone I've talked to either asks how to solve the lifecycle problem, or talks about the solution they came up with for it. I know that it's not incredibly pure in the same way that observing a View click is pure, but the need is there, and the problem is more complex than most.

There was discussion a while ago about having core + a UX extension. If it ultimately makes more sense there in a UX extension, then I'm fine with that.

[hamidp]

This looks very similar to what we have at Trello, and it has worked very well.

I think @roman-mazur brings up a good point, re: documentation. We should have a wiki page with super scary links that make people read them. In there we can say use LifecycleObservable OR do all these complicated things.

A few things we should figure out:

• How does this play with bindActivity and bindFragment ?
• Do we want to add in error detection, so that we if someone forgets to push a lifecycle event we crash? For example, bind until STOP but we get a series of events that is START -> DETACH -> DESTROY
• Sample on how to use it.
• Provide convenience RxActivity, RxFragment, and RxSupportFragment that do all the right binding things.

[mttkay]

I personally am not a fan of bindFragment and bindActivity, they were short
sighted solutions and patch the symptoms, not the problem. On the other
hand, they’re more universally applicable, but are more a replacement for
what you’d otherwise had done with WeakReferences/nulling out context and
manual checks for isAttached.

So I don’t see them being in conflict with this, although we might find
that at some point they will be superseded by a combination of other
solutions.

On Mon, Nov 24, 2014 at 3:49 PM, Hamid notifications@github.com wrote:

This looks very similar to what we have at Trello, and it has worked very
well.

I think @roman-mazur https://github.com/roman-mazur brings up a good
point, re: documentation. We should have a wiki page with super scary links
that make people read them. In there we can say use LifecycleObservable
OR do all these complicated things.

A few things we should figure out:

• How does this play with bindActivity and bindFragment ?
• Do we want to add in error detection, so that we if someone forgets
  to push a lifecycle event we crash? For example, bind until STOP but
  we get a series of events that is START -> DETACH -> DESTROY
• Sample on how to use it.
• Provide convenience RxActivity, RxFragment, and RxSupportFragment
  that do all the right binding things.

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

Matthias Käppler

Engineer

Twitter: https://twitter.com/mttkay

Skype: matthias-sc

SoundCloud Ltd. | Rheinsberger Str. 76/77, 10115 Berlin, Germany | +49
(0)172 2345679

Managing Director: Alexander Ljung | Incorporated in England & Wales
with Company
No. 6343600 | Local Branch Office | AG Charlottenburg | HRB 110657B

Capture and share your music & audio on SoundCloud
http://soundcloud.com/creators

[mr-archano]

@dlew I started following the discussion in #12 from the very beginning, so I'm aware of all three steps, but thanks for making this crystal clear for anyone else too.
I see issues in this solution, and I pointed them out in my previous comment. They are still valid points to me and as a user I really would like to see them addressed. Nevertheless I'm just another commenter so... ubi maior minor cessat

[dlew]

@mr-archano I've added my thoughts to your gist.

If the main hang-up people have is using a single enum for both lifecycles, we can split it into two enums.

[hamidp]

@mttkay So it sounds like it would be prudent to get consensus on this PR, merge it in, then figure out what to do with bindActivity and bindFragment

[hamidp]

It would also be prudent to more fully state the objections to this PR. The ones that I saw that were not a "I do not like it" were:

• Lifecycle is a hot observable
• ATTACH is ambigious

Others please chime in.

I consider this to be a core feature of RxAndroid since it's a fundamental building block. We should get something in, then build upon it. The pattern seems sound to me.

[mttkay]

I agree. We don't need to drive things to perfection at this point in time, I'd rather have something decent to work with and then take it from there. This looks like a decent solution to me, and it's self-contained, so if people prefer to solve the problem in other ways, they can still do that.

I'll merge this.