-
Notifications
You must be signed in to change notification settings - Fork 83
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 support for suspending observation of flows/state observation while the UI is inactive. #572
base: trunk
Are you sure you want to change the base?
Conversation
Here is a sample app using these primitives: https://github.com/avelicu/ReactiveTest/blob/main/app/src/main/java/net/velicu/reactivetest/MainScreenStateProducer.kt Happy to work to add these into the existing sample apps as well. |
b7f258c
to
0d7dcfa
Compare
…te while the UI is inactive. This change: - Adds a coroutine context element wrapping a stateflow representing whether the molecule is being observed or not. This, for most apps that collect the state via .collectAsStateWithLifecycle, corresponds to whether the UI is active. - Add an extension function CoroutineScope.repeatWhileMoleculeActive, which runs and cancels the inner block as the molecule becomes active and inactive, analogous to androidx.lifecycle.repeatOnLifecycle. - Adds an extension function CoroutineScope.awaitMoleculeActive, which suspends until the molecule is active. This can be used to avoid state generation inside a produceState until the UI becomes active. - Add extension functions on Flow and StateFlow to collectAsStateWhileMoleculeActive, attaching and detaching collectors as the molecule becomes and unbecomes active. This pauses production in Flows and in StateFlows that produce .WhileSubscribed().
0d7dcfa
to
11f768e
Compare
I think I'd like to put my opinion down as against this PR as written for the following reasons:
So while I think the tool isn't without merit, I think it might be better off living in a separate repo. And, failing that, I think the underlying |
That's the good part about this change - the value of the molecule should not change when the stateflow gets rebooted. The entire composition remains active, including the state it internally retains. This is one fundamental difference from the approach in #274 where the entire composition dies (IIUC). You're right that it entirely could be implemented as a wrapper, though. I'm still trying to figure out how to best use Molecule inside a viewmodel and my opinion might change, this PR itself has changed multiple times over the weekend as I figured out the limitations. |
My apologize - I had thought from my reading that this would cancel collection of an underlying If that's the case, though — where's the benefit? Ceasing to drive the state into the Appreciating the opportunity to read your code and think through these issues again! |
Well, it's precisely to help you cut off the expensive background work, for example subscription to database updates, while your app is backgrounded (but the data might still change as a result of syncing with a server, for instance - this happens frequently in our app); at the same time without cutting off the entire state composition so it may be available immediately upon resumption. Imagine that in your state producer you have something like:
and assume that If we Similarly, if you don't already have a DAO that produces this type of stateflow and instead just have an api to read data and get change notifications (like our app currently does..), this type of flow can be implemented inside a composable function with the
where awaitDataChangeNotification is a suspend fun that suspends until a database change occurs. This will:
(I'm typing out pseudocode based on a working local prototype so apologies for mistakes) |
In other words, the .collectAsStateWhileMoleculeActive allows us to bridge the always active composition to stateflows that care about whether or not somebody is subscribed to them, and the awaitMoleculeActive / repeatWhileMoleculeActive APIs allow you to accomplish similar behavior when you're implementing your state producers in molecules. By the way, you talked about |
It's beneficial for the user to control which background work should pause while the UI is away and which background work should continue. For example, mutations should never cancel or pause while the UI is away (more generally, they should not even go away when the viewmodel goes away, they should be ran in a larger scope) but data loads should. And, of course, state should always be retained - otherwise there's not really a point in using a viewmodel over just rememberSaveable state in activity-lifecycled components. |
If this state is intended to be consumed within the Molecule, would it be more idiomatic to expose it as a Finally, if the goal is to provided the programmatic ability to flip tasks on and off within the composition, would it make sense to skip the parameter, and instead provide collection state as an always-on API? (I can't speak for Jake, btw! But: pulling the thread on this idea) |
Right now the state is actually always consumed within coroutines started within the molecule, not within the Composable bits of the molecule, so in that sense a stateflow in a coroutine context is necessary. A previous version of this change also created a compositionlocal that exposed whether the flow is active, but I'm not sure that's necessary anymore. It might allow you to do something like
but it will always restart the launched effect when the composition flips from inactive to active, whereas this variant allows you to eg. only restart parts of your coroutine, for instance restart data collection but not drop cacheable state.
Not sure I follow - what do you mean by an always-on API? |
I think a
I just mean that, if this is worthwhile, there doesn't have to be a flag - always provide this state in a context key, and if you don't use it, you don't use it, no problem. |
Let me look into that and share a diff and we can see how it looks. On your second point: The arg is added to this overload:
I'm not sure if this is actually used in cashapp or other users of the library and is actually intended to be actually public, but if the emitter is specified by the caller like that I don't think we can provide the signal internally. |
This change:
composeOnlyWhileSubscribed
option tolaunchMolecule
which pauses the entire state production while there are no subscribers. I am actually not sure this is necessary anymore with the other 3 changes, and on its own is not really sufficient to avoid extraneous state production, so I am wondering if we should just remove it.CHANGELOG.md
's "Unreleased" section has been updated, if applicable.