Switch from viewModelScope to lifecycleScope for external collections
#3045
Conversation
|
While this is a good start, it breaks the current order of events on app launch. To improve initial loading times and prevent out-of-sync states, the app previously only started listening to updates after the initial data had been loaded, which is no longer true. I think a check should be added Why are you launching 4 coroutine jobs in the home activity, while it could also be 1? Why are you using 4 different functions while it could also be handled from one function exposed from the viewmodel ( |
Interesting I didn't really notice this in my testing but I can see how it can happen :)
ok I will add a check for this in the view model
I think I was trying to mimic the multiple launches I am trying with them combined but it seems that only the state changed events are caught (first in the function) so I believe we still need to keep them split like they were previously as the are |
|
Even when I keep the 4 functions separate in the view model, and keep them under the same |
|
With my last comment I was thinking something like this would be clearer: // HomeActivity.kt
lifecycleScope.launch {
lifecycle.repeatOnLifecycle(Lifecycle.State.RESUMED) {
mainViewModel.getUpdates()
}
}
// MainViewModel.kt
suspend fun getUpdates() {
// if already loaded etc
launch {
// collect entity updates
}
launch {
// collect area updates
}
// ...
}With the latest change that adds a loading check, you now need to call it from the activity after initial loading has finished to get state updates flowing again. |
weird in my testing with the latest change the updates were always flowing again Edit: ok I see what you are saying now, if the screen remained on while its all loading it will not register state updates from there. In my own defense the screen always times out by the time the first call loads lol. I do not see a way to use |
|
I wonder if we really need to check the loading state here because the UI already has its checks in place to show the chips when things are ready. Loading the data early doesn't seem to hurt anything here. |
|
So looks like we do indeed need to keep the methods separated in lifecycle scopes. I don't think we should pass in the To do the loading state check inside the activity also means we have to pass in the At least with the current implementation it solves the issue and the websocket connection is properly closed when we expect and the original loading calls still happen in the background. |
|
I should have tested the sample code before posting it... It could still be simplified somewhat by moving CodelifecycleScope.launch {
lifecycle.repeatOnLifecycle(Lifecycle.State.RESUMED) {
launch { mainViewModel.entityUpdates() }
launch { mainViewModel.areaUpdates() }
launch { mainViewModel.deviceUpdates() }
launch { mainViewModel.entityRegistryUpdates() }
}
}
Not when doing a warm start for me (swipe back until the app closes and then start it again is now significantly faster - though I'm using the emulator). You can see the app isn't fully closed as the websocket connection is restarted but IDs continue from the last time instead of from 1.
Right, it looks like all the async work paid off, I don't see a significant difference anymore either.
This isn't something that has to be done in the activity? Similar to how the activity always calls the init function but that function includes the check |
right this is when compose was able to do some caching which speeds things up :)
Correct but its still updating as we would expect and like :)
indeed it did, the fact that it just works pulling out the code into a different scope lets us know its done really well :)
yes but when a user goes through onboarding we load a new instance of the |
dshokouhi
changed the title
viewModelScope to lifecycleScope for external collections
|
@jpelgrom is there anything still pending here to discuss? |
|
Re. delayed response: I wanted to do some testing for what happens when the app isn't logged in, either first launch or when logged out, and noticed a lot of activity (almost 50 messages) after logging in which I wanted to understand before approving. (It's for all the sensors that are registered which cause entity registry updates so everything is good.)
The And I think this would be nice to avoid if possible, like is done for the other functions, by adding a connected check in the functions in the viewmodel :) |
🙏 thanks for that!! I think the phone app does that too right? Just a bunch of cancellations I think?
Ah I see what you mean now thank you for explaining this further 😃 and yes I do agree we should be avoiding those errors 😬 will update now |
Registering sensors yes, but it isn't listening for entity registry changes (or at least, the 'app' isn't, the frontend is), so it's less noticeable.
No, it actually requests the full registry for each change, as you can see it is very simple logic. Not something introduced here and while this could probably be optimised with the info in the registry change event it shouldn't be done in this PR. |
Thank you for the code reference here, its always better to simplify things I think so I implemented this change as well :) |
Summary
Fixes: #3014 by converting our external collections (flows) from using
viewModelScopetolifecycleScopeand to only run while the state isRESUMEDThese changes allow for initial data to load just like before but updates will only take place while the screen is on and app is in the foreground. When screen is off or app is in background the flows will stop.
Example logs where screen turns on/off and app goes in background/foreground with subscription stopping/starting as expected
Screenshots
Link to pull request in Documentation repository
Documentation: home-assistant/companion.home-assistant#
Any other notes
Removed a few state changes logs to keep github happy in case you wonder about the gaps :)