Added reactive versions of Activities and Fragments #93

dlew · 2014-11-26T21:39:52Z

Subclasses of the common Activity and Fragment classes, extended so you can easily get an Observable<LifecycleEvent>.

I wanted to get the ball rolling on this discussion (since it's the second part of #12, as I viewed it). I am fully aware this may be a controversial move. :P

I tried working up a version using ActivityLifecycleCallbacks, but it felt too weird to have one solution for Activity and another for Fragment. Also, watching #84 has caused me to realize there are multiple places where being able to hook into the Activity or Fragment directly will save a lot of boilerplate.

I purposefully chose to not rename the subclasses. That way, when you extend, say, Activity the package resolution dialog will show this version as well.

One conspicuously missing extension is ActionBarActivity, since it depends on #83.

JakeWharton · 2014-11-26T21:52:11Z

I really don't like the naming here. I would prefix these with "Rx" or something to disambiguate.

mttkay · 2014-11-27T07:26:18Z

Should this be part of the rxandroid-framework module?
On Nov 26, 2014 10:52 PM, "Jake Wharton" notifications@github.com wrote:

I really don't like the naming here. I would prefix these with "Rx" or
something to disambiguate.

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

Dorvaryn · 2014-11-27T10:03:08Z

@dlew I see the rational behind the subclassing of those. As you mentioned it can save some boilerplate code, however I don't think this is the way to go. If we go down that road it is quite restrictive for users and we have to maintain Multiple copies of those (what about FragmentActivity, or other subclasses ?).
I usually prefer to let the users hook into the lifecycle calls by providing hooks entries.
Anyway if we end up going for this I agree with @JakeWharton and @mttkay I would rather see this in the rxandroid-framework module and have an explicit name.

dlew · 2014-11-27T13:55:34Z

I'm fine with renaming the classes if people think that's best; I was just following the pattern of the support lib, which doesn't rename. I find that makes it easier to switch between versions since it's just a package name swap.

I'm also fine with this going into rxandroid-framework, which I didn't know existed (probably because it just appeared a couple days ago, hah).

dlew · 2014-11-27T14:04:52Z

@Dorvaryn

I usually prefer to let the users hook into the lifecycle calls by providing hooks entries.

I've come up with a metric for any given solution to RxAndroid problems, which is "is it better than doing it yourself?"

For example: in the case of the lifecycle, "doing it yourself" would be manually handling a CompositeSubscription. It requires 1 line of code for each member variable, +2 lines of code for each lifecycle event you track (one for init, another for unsubscribe). Then, of course, you need to hook it into each Observable you want auto-unsubscribed.

Measured by this yardstick, there's basically no way of doing better than CompositeSubscription without subclassing:

You need a member variable either way, either as a CompositeSubscription or an Observable. With Observable (or some other supporting class) you can reduce it to one member variable total, instead of one per lifecycle period. That said - subclassing means you can hide the member variable entirely from the API consumer.
You need some way to track when a lifecycle period starts and ends, so there's no getting around those two lines of code either. But subclassing can mean the API consumer doesn't need to track it themselves.
The only thing the API consumer needs to do is hook every Observable into the system, whichever we choose. No amount of subclassing can solve this.

Subclassing seems to be the only solution to reducing boilerplate. Any other solution is going to be just as laborious as CompositeSubscription. Adding hooks means users are doing all the work themselves. It might be wrapped up in a pretty bow, but it's just as much work.

hamidp · 2014-11-27T16:56:06Z

This is straight-up what we use at Trello and it is extremely convenient. Given that, I definitely think it should be called RxActivity. No objections to moving to framework.

Dorvaryn · 2014-11-27T17:20:39Z

@dlew 👍 I don't see any simple way either. Let's get this into framework then.

roman-mazur · 2014-11-28T11:49:02Z

Framework module already has ReactiveDialog class. Prefixes should be consistent, if used.

dlew · 2014-11-28T14:04:31Z

Modified and rebased. Here's what I did:

Moved all classes into rxandroid-framework
Renamed them to Rx*. See point 3 for why.
Made ReactiveDialog extend RxDialogFragment.

It feels a bit awkward having RxDialogFragment next to ReactiveDialog but they serve completely different purposes... one is for basic extensions for reactive coding, the other is a framework for doing dialogs in a completely different manner.

Dorvaryn · 2014-11-28T14:06:57Z

@dlew We should probably rename the ReactiveDialog to something else.

dpsm · 2014-11-28T21:00:08Z

rxandroid-framework/src/main/java/rx/android/app/RxActivity.java

+
+    @Override
+    protected void onPause() {
+        super.onPause();


Shouldn't the super.onXX() be the last call?

dpsm · 2014-11-28T21:03:24Z

@dlew might need rebase

meoyawn · 2014-11-29T00:26:58Z

rxandroid-framework/src/main/java/rx/android/app/RxActivity.java

+    @Override
+    protected void onCreate(Bundle savedInstanceState) {
+        super.onCreate(savedInstanceState);
+        lifecycleSubject.onNext(LifecycleEvent.CREATE);


don't you want to pass savedInstanceState with the create event?

Maybe eventually? The goal here isn't to get rid of having to override onCreate(), but giving a way to detect when onCreate() has been called so other reactive code can... react.

I'm open to explore this more later but I'd rather start minimalist then add as necessary.

@dlew totally makes sense

dlew · 2014-11-29T04:16:11Z

@dpsm Rebased; hopefully #102 will fix the builds.

roman-mazur · 2014-11-29T11:10:57Z

sample-app/src/main/java/rx/android/samples/LifecycleObservableActivity.java

+        super.onStart();
+
+        subscription =
+                LifecycleObservable.bindActivityLifecycle(lifecycle(), ViewObservable.clicks(button))


Do you think click listener observable is a good example here?
Have you experienced a necessity to bind such an observable to the lifecycle in practice?

If no one ever had use for binding a click listener in practice, why do we bother having ViewObservable.clicks()?

I'm not speaking about necessity of click observable. But whether it is
necessary to unsubscribe in this case. Unsubscription in this case leads to
remove of click listener. And I personally rarely need to do it in onStop
or in onDestroy.

On 14:57, Sat, Nov 29, 2014 Daniel Lew notifications@github.com wrote:

In
sample-app/src/main/java/rx/android/samples/LifecycleObservableActivity.java:

@OverRide

protected void onCreate(Bundle savedInstanceState) {

super.onCreate(

savedInstanceState);

button = new Button(this);

button.setText("Click Me!");

setContentView(button);

}

@OverRide

protected void onStart() {

super.onStart();

subscription =

LifecycleObservable.bindActivityLifecycle(lifecycle(), ViewObservable.clicks(button))

If no one ever had use for binding a click listener in practice, why do we
bother having ViewObservable.clicks()?

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

Try removing the unsubscribe. The activity will leak. At least it did last
time I tested the sample.

On Sat, Nov 29, 2014, 10:26 Roman Mazur notifications@github.com wrote:

In
sample-app/src/main/java/rx/android/samples/LifecycleObservableActivity.java:

@OverRide

protected void onCreate(Bundle savedInstanceState) {

super.onCreate(savedInstanceState);

button = new Button(this);

button.setText("Click Me!");

setContentView(button);

}

@OverRide

protected void onStart() {

super.onStart();

subscription =

LifecycleObservable.bindActivityLifecycle(lifecycle(), ViewObservable.clicks(button))

I'm not speaking about necessity of click observable. But whether it is
necessary to unsubscribe in this case. Unsubscription in this case leads to
remove of click listener. And I personally rarely need to do it in onStop
or in onDestroy.
… <#msg-f:1486120348025889653_>
On 14:57, Sat, Nov 29, 2014 Daniel Lew notifications@github.com wrote:
In
sample-app/src/main/java/rx/android/samples/LifecycleObservableActivity.java:

@OverRide https://github.com/Override > + protected void
onCreate(Bundle savedInstanceState) { > + super.onCreate(
savedInstanceState); > + > + button = new Button(this); > +
button.setText("Click Me!"); > + setContentView(button); > + } > + > +
@OverRide https://github.com/Override > + protected void onStart() { >

super.onStart(); > + > + subscription = > +
LifecycleObservable.bindActivityLifecycle(lifecycle(),
ViewObservable.clicks(button)) If no one ever had use for binding a click
listener in practice, why do we bother having ViewObservable.clicks()? —
Reply to this email directly or view it on GitHub <
https://github.com/ReactiveX/RxAndroid/pull/93/files#r21053708>.

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

hamidp · 2014-12-02T15:58:52Z

I read quickly through the PR and I do not see any major objections to this.

Maybe we can merge this after some 👍 ?

mttkay · 2014-12-02T16:56:02Z

I think @dpsm 's remarks need addressing (re. order of method invocation for onPause/onStop/onDestroy etc)

We should inform the subscriber before the fragment view gets destroyed about the fact that it's about to get destroyed.

dlew · 2014-12-02T17:02:40Z

@mttkay I've been torn on that one. People extending on* can still decide when the Observable fires, based on where they decide to call super() in their own overrides.

I'm looking for a strong argument one way or the other; even though you expect it to fire before an event, my intuitive understanding is that it fires after the event. I'm not sure which is clearer.

mttkay · 2014-12-02T17:36:33Z

I would understand these hooks as a chance for the subscriber to perform any clean up prior to the fragment getting destroyed. If that logic depends on the state prior to destruction (after destruction there is no state to rely on after all), then this might break the subscriber because, for instance, getView will come back as null.

dpsm · 2014-12-02T17:37:30Z

@mttkay 👍

dlew · 2014-12-02T17:51:14Z

@mttkay @dpsm Alright, that makes sense. Does that mean that the lifecycle event should fire after each of the creation events, but before any of the destruction events? E.g., in onResume() it's super-then-event; in onPause() it's event-then-super.

mttkay · 2014-12-02T17:53:09Z

Yes, that's exactly what I would vote for.

By the way, Android has a Linter check for exactly that. If you override an Activity/Fragment/Service, and you dispatch e.g. super.onDestroy before your own calls when overriding these hooks, it'll fail the check.

mttkay · 2014-12-02T17:53:25Z

Actually it might have been PMD not Android lint.

Includes both built-in versions + support versions. Minus any sort of weird annotation hacking, this seems to be the "best" method for creating a version of the base UI component classes that we can hook into.

dlew · 2014-12-02T18:01:18Z

SGTM. Fixed ordering and rebased.

Added reactive versions of Activities and Fragments

Alexander-- mentioned this pull request Nov 28, 2014

[RFC] Improve support for auto-unsubscribing observables and newer language/API levels #12

Closed

dlew force-pushed the dlew/reactive-ui branch from eb3ef5e to 38e85aa Compare November 28, 2014 14:02

dpsm reviewed Nov 28, 2014
View reviewed changes

meoyawn reviewed Nov 29, 2014
View reviewed changes

dlew force-pushed the dlew/reactive-ui branch from 38e85aa to 0122aa9 Compare November 29, 2014 04:15

roman-mazur reviewed Nov 29, 2014
View reviewed changes

dlew added 2 commits December 2, 2014 13:00

Added reactive versions of main Activities/Fragments

26b8a37

Includes both built-in versions + support versions. Minus any sort of weird annotation hacking, this seems to be the "best" method for creating a version of the base UI component classes that we can hook into.

Added sample Activity demonstrating LifecycleObservable

c22fc9a

Add Rx utilities to ReactiveDialog

c5504c0

dlew force-pushed the dlew/reactive-ui branch from 0122aa9 to c5504c0 Compare December 2, 2014 18:00

mttkay added a commit that referenced this pull request Dec 3, 2014

Merge pull request #93 from dlew/dlew/reactive-ui

73e6699

Added reactive versions of Activities and Fragments

mttkay merged commit 73e6699 into ReactiveX:0.x Dec 3, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added reactive versions of Activities and Fragments #93

Added reactive versions of Activities and Fragments #93

dlew commented Nov 26, 2014

JakeWharton commented Nov 26, 2014

mttkay commented Nov 27, 2014

Dorvaryn commented Nov 27, 2014

dlew commented Nov 27, 2014

dlew commented Nov 27, 2014

hamidp commented Nov 27, 2014

Dorvaryn commented Nov 27, 2014

roman-mazur commented Nov 28, 2014

dlew commented Nov 28, 2014

Dorvaryn commented Nov 28, 2014

dpsm Nov 28, 2014

dlew Nov 29, 2014

dpsm commented Nov 28, 2014

meoyawn Nov 29, 2014

dlew Nov 29, 2014

meoyawn Nov 29, 2014

dlew commented Nov 29, 2014

roman-mazur Nov 29, 2014

dlew Nov 29, 2014

roman-mazur Nov 29, 2014

dlew Nov 29, 2014

hamidp commented Dec 2, 2014

mttkay commented Dec 2, 2014

dlew commented Dec 2, 2014

mttkay commented Dec 2, 2014

dpsm commented Dec 2, 2014

dlew commented Dec 2, 2014

mttkay commented Dec 2, 2014

mttkay commented Dec 2, 2014

dlew commented Dec 2, 2014

Added reactive versions of Activities and Fragments #93

Added reactive versions of Activities and Fragments #93

Conversation

dlew commented Nov 26, 2014

JakeWharton commented Nov 26, 2014

mttkay commented Nov 27, 2014

Dorvaryn commented Nov 27, 2014

dlew commented Nov 27, 2014

dlew commented Nov 27, 2014

hamidp commented Nov 27, 2014

Dorvaryn commented Nov 27, 2014

roman-mazur commented Nov 28, 2014

dlew commented Nov 28, 2014

Dorvaryn commented Nov 28, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpsm commented Nov 28, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlew commented Nov 29, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hamidp commented Dec 2, 2014

mttkay commented Dec 2, 2014

dlew commented Dec 2, 2014

mttkay commented Dec 2, 2014

dpsm commented Dec 2, 2014

dlew commented Dec 2, 2014

mttkay commented Dec 2, 2014

mttkay commented Dec 2, 2014

dlew commented Dec 2, 2014