Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LifecycleObservable.bindActivityLifecycle without inheritance #152

Closed
wants to merge 8 commits into from

Conversation

nhachicha
Copy link

A companion method to LifecycleObservable.bindActivityLifecycle that uses activity lifecycle callbacks provided in API 14 to avoid inheritance

Nabil HACHICHA added 2 commits April 13, 2015 12:22
…yLifecycleCallbacks to listen for lifecycle events, instead of inheriting from RxJava
* @author Nabil Hachicha.
*/
class LifecycleHelper implements Application.ActivityLifecycleCallbacks {
private final Activity activityToMonitor;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've always thought it would be cool to have the Subject output all events, then use Observable.filter() to limit which ones you care about. Would get rid of all the == checks throughout the rest of this class.

@nhachicha
Copy link
Author

Hi @dlew @JakeWharton
I updated the PR

    @Override
    protected void onResume() {
        subscription =
                LifecycleObservable.bindActivityLifecycle(this,
                        ViewObservable.clicks(button),
                        LifecycleEvent.START)
                        .subscribe(new Action1<OnClickEvent>() {
                            @Override
                            public void call(OnClickEvent onClickEvent) {
                                Toast.makeText(LifecycleObservableActivityComposition.this,
                                        "Clicked button!",
                                        Toast.LENGTH_SHORT)
                                        .show();
                            }
                        });
         // LifecycleCallbacks will catch this event
        super.onResume();
    }

In this case the first event is onResume which is correct (no need to use booleans or BehaviorSubject.hasValue() to adjust the first event). Now imagine this use case

    @Override
    protected void onResume() {
         // LifecycleCallbacks will not catch this event
        super.onResume();
        subscription =
                LifecycleObservable.bindActivityLifecycle(this,
                        ViewObservable.clicks(button),
                        LifecycleEvent.START)
                        .subscribe(new Action1<OnClickEvent>() {
                            @Override
                            public void call(OnClickEvent onClickEvent) {
                                Toast.makeText(LifecycleObservableActivityComposition.this,
                                        "Clicked button!",
                                        Toast.LENGTH_SHORT)
                                        .show();
                            }
                        });
    }

From a LifecycleCallbacks we don't know what was the first event the user used to bind (could be onCreate could be onStart) in order to apply a correction (i.e emitting previous events).

This is why the method accept this now as a parameter from the user (like LifecycleObservable#bindUntilLifecycleEvent()).

  • Add unit tests

Observable
.just(activity)
.filter(sameInstance)
.subscribe(sendCreateEvent);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using an Observable in all these callbacks is wasteful in both allocation and performance. I don't see any advantage over

if (activity == activityToMonitor) {
  subscriber.onNext(LifecycleEvent.CREATE);
}

which is allocation-free.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spot on! changed to bindActivityUntilLifecycle

@nhachicha
Copy link
Author

@JakeWharton updated PR
Cheers,

@nhachicha
Copy link
Author

Ready/Waiting for review @JakeWharton @dlew :)

@JakeWharton
Copy link
Member

As part of #172 LifecycleObservable has been removed from future releases. While something like it may come back (and not favoring inheritance) we are focused on making a 1.0 of a core that's fundamental to all users of RxJava on Android.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants