Arch components lifecycle support #71

ZacSweers · 2017-05-24T18:43:16Z

This is a first pass of support for Android Architecture Components' Lifecycle support. This works fairly similarly to ViewScopeProvider from a structural standpoint. LifecycleOwner or Lifecycle can be passed in, an inner observer is passed in and removed upon disposal.

Thoughts for discussion:

Name is up for grabs. This one's a bit long :)
Should it be its own artifact? I'm leaning toward this, then we could also do an alpha release of it separately.
Things get a bit convoluted for me around State vs. Event. I'm not sure which one we should watch for, but all the querying APIs revolve around State so I went with that.
Does it matter if it's on the main thread?

If this looks good - I'll take a pass at tests.

I think this would also resolve #36 and #37 by just supporting them (in the support lib anyway)

ZacSweers · 2017-05-24T18:45:25Z

autodispose-android/src/main/java/com/uber/autodispose/android/LifecycleStatesObservable.java

+
+    @OnLifecycleEvent(Lifecycle.Event.ON_ANY) void onStateChange() {
+      if (!isDisposed()) {
+        observer.onNext(lifecycle.getCurrentState());


I'm not sure if this could result in duplicate state emissions since this fires on events. Will find out

tonycosentini · 2017-05-24T20:21:30Z

autodispose-android/build.gradle

@@ -49,6 +49,7 @@ dependencies {
  compile deps.rx.java
  compile deps.rx.android
  compile deps.support.annotations
+  compile "android.arch.lifecycle:extensions:1.0.0-alpha1"


+1 for a separate module approach

+1. at least until we call it beta (which means stable api)

yup moved it

btw, you don't need extensions, you should only need lifecycle:common

cool, I'll remove it

Actually I remember now why I added it, I need LifecycleRegistry for the TestAndroidLifecycleScopeProvider implementation. Added back in 1c06bef

tonycosentini · 2017-05-24T20:27:38Z

autodispose-android/src/main/java/com/uber/autodispose/android/LifecycleOwnerScopeProvider.java

+          switch (lastEvent) {
+            case INITIALIZED:
+              throw new LifecycleNotStartedException();
+            case CREATED:


Is the lifecycle intentionally not 1-1? It seems very strange to have multiple start events map to the same stop event.

There's only one stop event, not sure why

I think you are confusing states vs events.
https://developer.android.com/topic/libraries/architecture/lifecycle.html

e.g. both onStart and onPause brings you to the STARTED state. I'm not sure where you are using these but this seems wrong.

jaychang0917 · 2017-08-05T11:59:22Z

I think Event is more suitable since you can't manage to cancel subscription in corresponding lifecycle event (e.g. onResume -> onPause) if you use State.

ZacSweers · 2017-08-06T08:11:24Z

Yep event is the right way to go. Sorry for the lag on this, kind of mentally put it on the backburner since it was still in alpha. Will jump on it this coming week

ZacSweers · 2017-08-20T10:56:17Z

So I'm a little stuck on this right now. I've switched the implementation to events, but I'm not sure how to test this. I tried writing this as an espresso test (similar to how the -android artifact is), but lifecycle events when manipulating activities in espresso seem to fire events in random order.

yigit · 2017-08-29T16:10:01Z

fyi, you don't need to keep this as a separate artifact, Lifecycle's core libs will be a dependency of support Fragments so all apps will have them.

ZacSweers · 2017-08-30T21:12:06Z

yeah I was just making it separate for while it was in alpha, but maybe you'll beat me to it :)

ZacSweers · 2017-08-30T22:30:00Z

Will handle rebasing before merging later. Going to figure out testing for this now

ZacSweers · 2017-08-31T02:46:11Z

Ok, things should be ready to go here. Going to do a rebase

ZacSweers · 2017-08-31T02:53:23Z

Rebased. Requesting review from @tonycosentini and @jbarr21, also welcome feedback from anyone else on the thread :)

tonycosentini · 2017-09-04T04:38:59Z

...Test/java/com/uber/autodispose/android/archcomponents/AndroidLifecycleScopeProviderTest.java

+    subject.onNext(2);
+    o.assertNoMoreEvents();
+
+    d.dispose();


Dumb question - why is this needed?

I don’t remember (this test is largely a copy of the viewscopeprovidertest), I think it’s not and I just did it out of habit or something. Will try removing

tonycosentini · 2017-09-04T04:40:47Z

...nts/src/main/java/com/uber/autodispose/android/archcomponents/LifecycleEventsObservable.java

+
+  @SuppressWarnings("CheckReturnValue") LifecycleEventsObservable(Lifecycle lifecycle) {
+    this.lifecycle = lifecycle;
+    // Backfill if already created for boundary checking


nit: doesn't seem like it matters with today's implementation - but would it ever be beneficial to replay all the previous events?

I'm not sure what you mean. Like having that be a use case of the library?

tonycosentini · 2017-09-04T04:41:25Z

...nts/src/main/java/com/uber/autodispose/android/archcomponents/LifecycleEventsObservable.java

+  @Override protected void subscribeActual(Observer<? super Event> observer) {
+    if (!isMainThread()) {
+      observer.onError(
+          new IllegalStateException("Lifecycles can only be bound to on the main thread!"));


Is this a strict requirement with the new architecture library?

Not necessarily but it's always emitted on the main thread and you could have a race condition if you don't.

Checked with @yigit, answer is yes otherwise no guarantees

This is a first pass of support for Android Architecture Components' `Lifecycle` support. This works fairly similarly to `ViewScopeProvider` from a structural standpoint. `LifecycleOwner` or `Lifecycle` can be passed in, an inner observer is passed in and removed upon disposal. Thoughts for discussion: - Name is up for grabs. This one's a bit long :) - Should it be its own artifact? I'm leaning toward this, then we could also do an alpha release of it separately. - Things get a bit convoluted for me around `State` vs. `Event`. I'm not sure which one we should watch for, but all the querying APIs revolve around `State` so I went with that. - Does it matter if it's on the main thread? If this looks good - I'll take a pass at tests.

WIP still as I don't really know how to test this nicely

DESTROY is _before_ INITIALIZED apparently

ZacSweers · 2017-09-10T22:40:54Z

Rebased and ran UI tests locally

ZacSweers added Android Enhancement labels May 24, 2017

ZacSweers self-assigned this May 24, 2017

ZacSweers requested review from tonycosentini and jbarr21 May 24, 2017 18:43

ZacSweers commented May 24, 2017

View reviewed changes

tonycosentini reviewed May 24, 2017

View reviewed changes

ZacSweers mentioned this pull request Aug 21, 2017

Discussion/proposal: Allow for consumer code gen to generate first party APIs #75

Closed

ZacSweers force-pushed the z/archLifecycle branch from 2d5bfc2 to c98b639 Compare August 31, 2017 02:52

tonycosentini reviewed Sep 4, 2017

View reviewed changes

tonycosentini approved these changes Sep 7, 2017

View reviewed changes

ZacSweers mentioned this pull request Sep 7, 2017

Android emulators are broken on trusty env update because avdmanager is not on PATH travis-ci/travis-ci#8360

Closed

ZacSweers added 10 commits September 10, 2017 15:30

Move to separate artifact

c13ca56

Refactor RecordingObserver to internal test-utils library for reuse

161ab6d

fixup

df1bb07

Move archcomponents to dependencies.gradle + updated version

0aba54a

Add WIP TestLifecycleOwnerScopeProvider for tests

8afc1de

Switch to event-based implementation

5f37f39

WIP still as I don't really know how to test this nicely

Remove extensions and runtime, just depend on common

3b6703e

Fix backfilling logic

65a7e2f

DESTROY is _before_ INITIALIZED apparently

Style nitpick

3418af5

ZacSweers added 8 commits September 10, 2017 15:30

Correctly pass null in TestLifecycleOwnerScopeProvider

8aa5cd4

Backfill state based on event in test helper too

0c72080

Add back extensions for test helper

5eafbc3

Flesh out docs in test helper

cffe472

Add tests!

b49b13a

Rename to AndroidLifecycleScopeProvider

14208b3

Add arch components to travis script

88d3946

Remove dispose

653e1a6

ZacSweers force-pushed the z/archLifecycle branch from d5eff71 to 653e1a6 Compare September 10, 2017 22:31

ZacSweers added 3 commits September 10, 2017 15:32

Update to javaxExtras dep

7bba5cb

Update to new annotation for nullability

ad4d61c

Fix test names to be more reflective of event names

a327ae6

ZacSweers merged commit ca3139f into master Sep 10, 2017

ZacSweers deleted the z/archLifecycle branch September 10, 2017 22:45

ZacSweers modified the milestone: 0.3.0 Sep 10, 2017

ZacSweers mentioned this pull request Sep 10, 2017

Add demo scope provider for Fragment #37

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Arch components lifecycle support #71

Arch components lifecycle support #71

ZacSweers commented May 24, 2017 •

edited

Loading

ZacSweers May 24, 2017

tonycosentini May 24, 2017

yigit May 24, 2017

ZacSweers May 24, 2017

yigit Aug 30, 2017

ZacSweers Aug 30, 2017

ZacSweers Aug 30, 2017

ZacSweers Aug 31, 2017

tonycosentini May 24, 2017

ZacSweers May 24, 2017

yigit May 24, 2017

jaychang0917 commented Aug 5, 2017

ZacSweers commented Aug 6, 2017

ZacSweers commented Aug 20, 2017

yigit commented Aug 29, 2017

ZacSweers commented Aug 30, 2017

ZacSweers commented Aug 30, 2017 •

edited

Loading

ZacSweers commented Aug 31, 2017

ZacSweers commented Aug 31, 2017

tonycosentini Sep 4, 2017

ZacSweers Sep 4, 2017

ZacSweers Sep 7, 2017

tonycosentini Sep 4, 2017

ZacSweers Sep 4, 2017

tonycosentini Sep 4, 2017

ZacSweers Sep 4, 2017

ZacSweers Sep 8, 2017

ZacSweers commented Sep 10, 2017

Arch components lifecycle support #71

Arch components lifecycle support #71

Conversation

ZacSweers commented May 24, 2017 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaychang0917 commented Aug 5, 2017

ZacSweers commented Aug 6, 2017

ZacSweers commented Aug 20, 2017

yigit commented Aug 29, 2017

ZacSweers commented Aug 30, 2017

ZacSweers commented Aug 30, 2017 • edited Loading

ZacSweers commented Aug 31, 2017

ZacSweers commented Aug 31, 2017

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

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

ZacSweers commented Sep 10, 2017

ZacSweers commented May 24, 2017 •

edited

Loading

ZacSweers commented Aug 30, 2017 •

edited

Loading