square/flow

Move Flow configuration/installation out of getBaseContext?

Closed this issue · 11 comments

dcow commented

I just finished migrating a Flow 0.12+Mortar+Dagger codebase to Flow 1.0.0-alpha2 (+Mortar+Dagger). It took about 5 days of work and will require a lot of validation and review. After polling various sources to piece together an idea of what the delta looked like, things went relatively smoothly. The new structure feels much better. The role Flow plays in the project is more intelligible.

That said, the biggest quirk in my opinion was the fact that Flow needs to be installed before onCreate and can only be used onPostCreate. Previously, we didn't know our first state before onCreate (it came in via an intent). My solution is to init flow with an empty state that gets updated in onPostCreate. This is because my keychanger expects keys to be in annotated with Layout and I don't want to special case the empty state so I made an empty key and layout. Not a huge deal but it feels unnecessary. I'd rather be able to configure flow when it's attached to the base context and wait until onCreate to set the dispatcher (when my activity is actually live) and services factory (maybe I have services that aren't available until later like a binding to an android service or something). I've been able to move all my DI graph and mortar scope creation to happen when the base context is attached, but writing some lazy binding thing for async-bound services feels weird too. Just some thoughts.

dcow commented

Additionally, it's not completely clear how the new default history is handled. We previously would call the onCreate hook in the flow delegate with a history provided. And flow would handle making sure to only update the history if needed. I'm guessing I should now check the flow history onCreate and only update it if it's "empty"? Basically, if I don't know my initial state until I've parsed my intent onCreate, how do I tell flow to "set the history only if this is a new creation not if we're simply performing a configuration change"? Do I just have to build my own thing with the isChangingConfiguration() value or check for the presence of a savedInstanceState bundle?

@dcow The Flow installation happens like so:

  • you override attachBaseContext() in Activity to which you define an initial history
  • this Installer thing creates a new base context which will apply Application.ActivityLifecycleCallbacks to the application, in which onActivityCreated() { if(activity == a) { ...} checks if the current activity is being created, and adds the InternalLifecycleIntegration retained fragment to it if it does not exist yet
  • The InternalLifecycleIntegration adds the Flow instance to the Activity in onActivityCreated() callback

In onActivityCreated(), the InternalLifecycleIntegration has access to savedInstanceState, therefore selectHistory selects whether the retained history is empty, available in saved instance state, or available in an intent key, I think the intent key wins, then saved instance, then initial history keys.

how do I tell flow to "set the history only if this is a new creation not if we're simply performing a configuration change"?

Actually that's a bug in Flow and it's a nuisance, onResume() calls the bootstrap traversal even though the view hierarchy exists, which is great /s because Flow also doesn't expose an onSaveInstanceState() callback with which you can directly save the active view hierarchy state, meaning it gets lost when you put the app in background and bring it foreground.

This is exactly why I started making zhuinden/flowless (a fork of Flow 1.0-alpha) - along with the fact that I found TreeKeys and the flow services a bit too confusing for my liking. If you're curious, I made a more complex example with it here.

I don't know my initial state until I've parsed my intent onCreate,

This is what Flow.addHistoryToIntent() call is for, but in the square/flow repository it's completely bugged and onNewIntent() call checks against the wrong constant and also the intent isn't actually always obtained apparently ( see my comment in #180 (comment) )

I've been able to move all my DI graph and mortar scope creation to happen when the base context is attached, but writing some lazy binding thing for async-bound services feels weird too. Just some thoughts.

Technically "the right way to do it" would be to create an annotation called @ServiceFactory(SomeServiceFactory.class) which instantiates the service factory and its binder methods are called based on whichever key you receive as you can see here.

I realized this while writing this code and then I realized this could be a lot more sophisticated if it happened like how Flow originally does it, but alas, I didn't want key reference counting in my KeyManager, so I didn't add it back.


If you find that the new lifecycle integration of Flow 1.0-alpha is a hindrance, then I advise you to look at my new library simple-stack: it adheres to Flow's tests, and I think migration from Flow 1.0-alpha is fairly straight-forward:

  • Dispatcher => StateChanger
  • Traversal => StateChange
  • Traversal.origin => StateChange.getPreviousState()
  • Traversal.destination => StateChange.getNewState()
  • Flow.Installer => BackstackDelegate
  • Flow => Backstack
  • Flow.set => Backstack.goTo
  • KeyManager => BackstackDelegate
  • InternalLifecycleIntegration => BackstackDelegate
  • State => SavedState
  • Flow.Services => not added, but such logic can be moved to StateChanger
  • Direction => constants inside StateChange (REPLACE, FORWARD, BACKWARD)
  • History => ArrayList<Parcelable> ~ please note that History.reverseIterator is equal to List.iterator
  • History.newBuilder() => HistoryBuilder.newBuilder()
  • historyBuilder.push() => historyBuilder.add()
  • historyBuilder.pop(1) => historyBuilder.removeLast()
  • historyBuilder.popUntil() => historyBuilder.removeUntil()
  • History.buildUpon() => HistoryBuilder.from()
  • Flow.onNewIntent() => totally up to you, just create an ArrayList<Parcelable> and use Backstack.setHistory
  • Flow.get(Context) => totally up to you, but basically I assume you are able to write 3 lines that override getSystemService(), it's in the changelog and the first few examples

The BackstackDelegate is very similar to FlowDelegate except it doesn't have onNewIntent() and getSystemService().

I wrote simple-stack because it bothered me that Flow only exists during onPostCreate(). My only assumption is that your keys are immutable with equals/hashCode and are Parcelable.

dcow commented

Wow, thank you. That's very interesting/helpful. If I run into any more issues then I may take a serious look (just saw your blog post too).

Technically "the right way to do it" would be to create an annotation called @servicefactory(SomeServiceFactory.class) which instantiates the service factory and its binder methods are called based on whichever key you receive as you can see here.

I am using the Services api. My qualm was more with the fact that for android services onServiceConnected is an async thing. So it would be nice if I didn't have to build some: if (bound) return serviceConnection logic and instead be able to create a services binder with the android service provided to the constructor and then attach that at a later time. not the end of the world, minor nit.

Actually that's a bug in Flow and it's a nuisance, onResume() calls the bootstrap traversal even though the view hierarchy exists, which is great /s because Flow also doesn't expose an onSaveInstanceState() callback with which you can directly save the active view hierarchy state, meaning it gets lost when you put the app in background and bring it foreground.

I would take that to mean the idea is that since your States are serialized executing that bootstrap traversal constitutes a restore when the activity is first loaded since you do need to restore state to new views even if for simple configuration changes.

Yes, but unfortunately it also happens in Flow if you just put it in background and bring it foreground.

@dcow I have replaced the services api (that I fixed up here previously, but I found it to be extremely limited for my needs) with service-tree which is integrated with simple-stack in this example.

@dcow I have released Navigator which has the lifecycle listener as a fragment, but it gets installed in onCreate().

The services factory isn't included, but there is a very similar setup in the nestedstack sample that would help figure it out.

dcow commented

@Zhuinden it's awesome to see you so active with Android navigation! What was an optimistic OP turned into a nightmare since flow services handles things differently than flow-path did, and I don't think it's documented anywhere. Mortar scope creation and destruction is such a subtle memory management characteristic that it flew under the radar until way too late. I spent two weeks cleaning everything up most because I had to work around all the bugs you've called out. I should have listened to you more seriously d= We are definitely not going to keep using Flow beyond our recent upgrade. I think we all like the pattern though so I am sure we will look at simple-stack, navigator, flowless, etc.

@dcow I marked flowless as stagnant, because navigator is pretty much the same thing, except it doesn't have the attachBaseContext() quirk.

I didn't include services in navigator yet, because despite my initial versioning on service-tree, I do not consider its API finalized. But scoped services can be set up and all Bundleable services can even have their states persisted and restored, the code needed for it can be found in my nestedstack sample.

For more info on Navigator, I wrote a medium article which explains it; it should seem familiar though considering it's very similar to Flow, just with different names and less black magic.

.... And I moved navigator into simple-stack 1.5.0

Looks like this is resolved? We may look at moving out of attachBaseContext, primarily if we abandon Context as the hook for getting access to Flow. In the meantime, it allows the lifecycle integration that we want to have.

@loganj in my solution, I obtain my backstack with Navigator.getBackstack(context) by finding the activity in the base context chain, getting its fragment manager, and getting the backstack from the retained fragment.

This way I could move initialization to onCreate().

It seems to be working even without an additional context wrapper.

Also, onCreate() is where Conductor initializes itself, too.