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

Add untilEvent overload for AndroidLifecycleScopeProvider#from #107

Merged
merged 4 commits into from
Oct 10, 2017
Merged

Add untilEvent overload for AndroidLifecycleScopeProvider#from #107

merged 4 commits into from
Oct 10, 2017

Conversation

rubengees
Copy link
Contributor

Description: This adds two overloads to the AndroidLifecycleScopeProvider#from methods, to achieve the same functionality as RxLifecycle's bindUntilEvent.

I have taken the approach of returning the set value in the correspondingEvents function. It works, but I'm not sure if that is the right way.

I also fixed minor issues I found while scanning through the code. I can revert those if not appropriate.

Related issue(s): #72, #104

@CLAassistant
Copy link

CLAassistant commented Oct 9, 2017

CLA assistant check
All committers have signed the CLA.

@ZacSweers
Copy link
Collaborator

I also fixed minor issues I found while scanning through the code.

Could you call those out, perhaps in comments?

/**
* Creates a {@link AndroidLifecycleScopeProvider} for Android LifecycleOwners.
*
* @param owner the owner to scope for
* @param owner the owner to scope for.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very minor: Missing dot.

* Creates a {@link AndroidLifecycleScopeProvider} for Android Lifecycles.
*
* @param lifecycle the lifecycle to scope for
* @param lifecycle the lifecycle to scope for.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above.

@@ -35,7 +37,7 @@

private static final String TAG = "AutoDispose";

@Override protected void onCreate(Bundle savedInstanceState) {
@Override protected void onCreate(@Nullable Bundle savedInstanceState) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added @Nullable annotation as suggested by Android Studio.

@@ -97,6 +99,21 @@
.to(AutoDispose.with(AndroidLifecycleScopeProvider.from(this)).<Long>forObservable())
.subscribe(new Consumer<Long>() {
@Override public void accept(Long num) throws Exception {
Log.i(TAG, "Started in onResume(), running until in onPause(): " + num);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed "...running until in onDestroy" to "...running until in onPause".

@rubengees
Copy link
Contributor Author

I have added comments and signed the cla!

Copy link
Collaborator

@ZacSweers ZacSweers left a comment

Choose a reason for hiding this comment

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

One minor nit. Thanks for this!

}

@Override public Lifecycle.Event peekLifecycle() {
return lifecycleObservable.getValue();
}

private static class UntilEventFunction implements Function<Lifecycle.Event, Lifecycle.Event> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good idea. I'd initially thought about doing it as a filter, but with this you hook in nicely to the api

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I started trying that but could not get it to work, so came up with this approach instead.

* @param untilEvent the event until the scope is valid.
* @return a {@link AndroidLifecycleScopeProvider} against this owner.
*/
public static AndroidLifecycleScopeProvider from(LifecycleOwner owner,
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: put both of these on the next line or at least make until event use a normal trailing indent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@@ -73,8 +73,8 @@ public static AndroidLifecycleScopeProvider from(LifecycleOwner owner) {
* @param untilEvent the event until the scope is valid.
* @return a {@link AndroidLifecycleScopeProvider} against this owner.
*/
public static AndroidLifecycleScopeProvider from(LifecycleOwner owner,
Lifecycle.Event untilEvent) {
public static AndroidLifecycleScopeProvider from(
Copy link
Collaborator

Choose a reason for hiding this comment

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

One more nit - if they start on a new line, let's make each of them a separate line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like this?

public static AndroidLifecycleScopeProvider from(
          LifecycleOwner owner, 
          Lifecycle.Event untilEvent) {

Copy link
Collaborator

Choose a reason for hiding this comment

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

yep!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@ZacSweers ZacSweers merged commit 9b26a0c into uber:master Oct 10, 2017
@ZacSweers
Copy link
Collaborator

Thanks! I'll cut a patch release soon-ish

@ghost
Copy link

ghost commented Oct 12, 2017

@hzsweers Hope it comes quickly

@rubengees rubengees deleted the from-until-event branch October 14, 2017 12:20
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.

3 participants