-
Notifications
You must be signed in to change notification settings - Fork 638
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
Added support for android-lifecycle #216
Conversation
|
||
import static com.trello.rxlifecycle2.RxLifecycle.bind; | ||
|
||
public class RxLifecycleAndroidLifecycle { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't think of a better name ¯_(ツ)_/¯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's a bit rough here isn't it...
Minor: I'd make this class final
. (I'm noticing this mistake in some classes I wrote before, too, will have to go correct that later.)
import io.reactivex.subjects.PublishSubject; | ||
|
||
@RunWith(RobolectricTestRunner.class) | ||
@Config(manifest = Config.NONE, sdk = 17) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test fails on sdk = 16
, I'm not sure why.
rxlifecycle-kotlin/build.gradle
Outdated
} | ||
|
||
dependencies { | ||
compile kotlinStdlib | ||
compile project(':rxlifecycle-android') | ||
provided project(':rxlifecycle-android-lifecycle') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wasn't sure if I should create a seperate kotlin module for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although laborious, I think that's the right way to go. Otherwise people end up pulling in more than they bargained for when they pull in this project.
import io.reactivex.Observable; | ||
import io.reactivex.subjects.BehaviorSubject; | ||
|
||
public class RxLifecycleObserver implements LifecycleProvider<Lifecycle.Event>, LifecycleObserver { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Open to a better name. This wraps a LifecycleOwner
and provides a LifecycleProvider
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the name is okay. I'd make it final
.
|
||
private static final Function<Lifecycle.Event, Lifecycle.Event> LIFECYCLE = new Function<Lifecycle.Event, Lifecycle.Event>() { | ||
@Override | ||
public Lifecycle.Event apply(Lifecycle.Event lastEvent) throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose to go with the provided events instead of the rxlifecycle-android
ones. They are a bit more limited on fragments so I think this is less surprizing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable to me.
|
||
// Easier than making everyone create their own shadows | ||
private ActivityController<FragmentActivity> startFragment(Fragment fragment) { | ||
ActivityController<FragmentActivity> controller = Robolectric.buildActivity(FragmentActivity.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lifecycle events won't fire if you call methods on a fragment directly. You need to trigger them through the hosting activity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I hate Robolectric as a result of this project. I'm this close to just making them run on an emulator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks good - just some changes I've noted.
If you could also add some explanatory documentation above the two new classes that'd be swell - want to make sure people know what they are for. :)
build.gradle
Outdated
@@ -38,6 +38,8 @@ ext { | |||
rxJava = 'io.reactivex.rxjava2:rxjava:2.0.6' | |||
rxAndroid = 'io.reactivex.rxjava2:rxandroid:2.0.1' | |||
navi = 'com.trello.navi2:navi:2.0' | |||
lifecycle = 'android.arch.lifecycle:runtime:1.0.0-alpha1' | |||
lifecycleProcessor = 'android.arch.lifecycle:compiler:1.0.0-alpha1' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I'd link up these two using $lifecycleVersion
or something.
build.gradle
Outdated
@@ -8,7 +8,7 @@ buildscript { | |||
jcenter() | |||
} | |||
dependencies { | |||
classpath 'com.android.tools.build:gradle:2.3.0-rc1' | |||
classpath 'com.android.tools.build:gradle:2.3.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Might as well bump this to 2.3.2 while we're at it.
|
||
private static final Function<Lifecycle.Event, Lifecycle.Event> LIFECYCLE = new Function<Lifecycle.Event, Lifecycle.Event>() { | ||
@Override | ||
public Lifecycle.Event apply(Lifecycle.Event lastEvent) throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable to me.
@@ -0,0 +1,55 @@ | |||
package com.trellow.relifecycle2.android.lifecycle; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some typos in the package name - should be com.trello.lifecycle2.android.lifecycle
@@ -0,0 +1,45 @@ | |||
package com.trellow.relifecycle2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Package name doesn't match directory structure.
|
||
import static com.trello.rxlifecycle2.RxLifecycle.bind; | ||
|
||
public class RxLifecycleAndroidLifecycle { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's a bit rough here isn't it...
Minor: I'd make this class final
. (I'm noticing this mistake in some classes I wrote before, too, will have to go correct that later.)
import io.reactivex.Observable; | ||
import io.reactivex.subjects.BehaviorSubject; | ||
|
||
public class RxLifecycleObserver implements LifecycleProvider<Lifecycle.Event>, LifecycleObserver { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the name is okay. I'd make it final
.
|
||
// Easier than making everyone create their own shadows | ||
private ActivityController<FragmentActivity> startFragment(Fragment fragment) { | ||
ActivityController<FragmentActivity> controller = Robolectric.buildActivity(FragmentActivity.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I hate Robolectric as a result of this project. I'm this close to just making them run on an emulator.
rxlifecycle-kotlin/build.gradle
Outdated
} | ||
|
||
dependencies { | ||
compile kotlinStdlib | ||
compile project(':rxlifecycle-android') | ||
provided project(':rxlifecycle-android-lifecycle') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although laborious, I think that's the right way to go. Otherwise people end up pulling in more than they bargained for when they pull in this project.
and changed constructor to factory method. This makes it better match the navi provider.
// If you want to use Kotlin syntax | ||
compile 'com.trello.rxlifecycle2:rxlifecycle-kotlin:2.0.1' | ||
|
||
// If you want to use Kotlin syntax with Android Lifecycle | ||
compile 'com.trello.rxlifecycle2:rxlifecycle-android-kotlin:2.0.1' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Speaking from previous experience, we should make a comment here that it's unreleased (or just remove this altogether). Otherwise people will start trying to use it before we release.
Thanks so much for all the work! I'll try to get a release out with this soon. |
#215