Compose and Events! I finally got a chance to rea...

# compose

[Colton Idle]

Compose and Events!

I finally got a chance to read the newly released arch docs for android. Lots of good compose related snippets in there.

Probably the biggest piece of help has been event handling and how events should always result in a UI state update. I do have questions though and I suppose either manuel, florina, adam or ian might be best to chime in (among others) since they helped write the docs. https://developer.android.com/jetpack/guide/ui-layer/events?continue=https%3A%2F%2Fdeve[…]eveloper.android.com%2Fjetpack%2Fguide%2Fui-layer%2Fevents

The example being

// composable
    val currentOnUserLogIn by rememberUpdatedState(onUserLogIn)
    LaunchedEffect(viewModel.uiState)  {
        if (viewModel.uiState.isUserLoggedIn) {
            currentOnUserLogIn()
        }
    }

I have a bunch of questions

1. I don't understand the use of val currentOnUserLogIn by rememberUpdatedState(onUserLogIn). Why doesn't this code sample just call onUserLogIn directly?
2. What goes into LaunchedEffect arg exactly? It seems like its easy to mess up what exactly goes in there and therefore shoot yourself in the foot. I can expand on this if that didn't make sense.
3. Why can't I just pass the ref of onUserLogIn event into the ViewModel and onSuccess of a login network call it would just call onUserLogIn in the VM itself?

[Alex Vanyo]

1. onUserLogIn is a plain lambda passed to LandingScreen. Because the LaunchedEffect doesn’t key on the onUserLogIn, the LaunchedEffect wouldn’t restart even if there is a new onUserLogIn that causes LandingScreen to recompose. If we called onUserLogIn directly, we could be calling the stale, old version of it. There’s another example explained in more detail here: https://developer.android.com/jetpack/compose/side-effects#rememberupdatedstate
2. https://developer.android.com/jetpack/compose/side-effects#restarting-effects has details about the general form all of these keys take. In short, you should add the keys that you want to use to restart the Effect. And yes, this can cause some very subtle behavior changes depending on the exact keys and the effect.
3. If onUserLogIn captures an Activity or other UI object (such as to navigate to another activity or navigate via navigation-compose) you can very easily cause a memory leak or get strange behavior because the AAC ViewModel outlives the UI. Let’s say onUserLogIn needs make a network call, which could take a few seconds. In that time, the user could have rotated their device, the activity gets recreated, and your lambda no longer refers to the correct instance of the Activity. It’s a bit more subtle, but giving a composition-scoped object to something that outlives it is like storing a view reference or activity reference in a ViewModel

[Colton Idle]

@Alex Vanyo

1. & 2. oh boy. this seems super prone to shooting yourself in the foot.

[Colton Idle]

3. So where I got this idea of just passing in the event function was from @Adam Powell here: https://kotlinlang.slack.com/archives/CJLTWPH7S/p1627496295442000?thread_ts=1627406028.253800&cid=CJLTWPH7S

So I'm not sure if Adam knows something I don't know or if I just misinterpreted him. But from what it sounds like @Alex Vanyo, your hint of a memory leak happening is valid, but yeah. adam seemed to reccomend this way?

[Adam Powell]

I was being a little bit facetious in that thread since you had made the claim that an if statement is business logic and you wanted that in your ViewModel. I was implying an implementation of that function that doesn't retain references to those onSuccess/onFailure functions, but that literally just wraps an if. 🙂

fun validateMembership(
  onSuccess: () -> Unit,
  onFailure: () -> Unit
) {
  if (hasValidMembership) onSuccess() else onFailure()
}

[Colton Idle]

Okay. So the tldr is that... for example, on button press when I call vm.doSomething() I should not be passing in any functions from the UI layer.

So like vm.doSomething(showLoginPage)

fun doSomething(showLoginPage: () -> Unit){
//before we do something, check to make sure the user is authenticated, if not then force them to login
if (userManager.isAuth.not()) { 
showLoginPage()
return
}
//do something

}

[Colton Idle]

fack. I've been using this pattern everywhere ever since that convo. lol I've got a lot of updating to do. I was really hoping to get around launchedEffect as it seems too easy to misuse and shoot yourself in the foot.

[Colton Idle]

I don't understand why it's bad necessarily though. Mem leak? I have an activity that overrides all config changes... and my navHost is located there, so how would anything outlive my activity?

[Alex Vanyo]

The general issue at play is having a longer-lived component having a reference to a shorter-lived component, which is something that could happen in a lot of places if you’re not careful.

Consider the following:

@Composable
fun SomeContainer(doSomething: (LazyListState) -> Unit) {
    val lazyListState by rememberLazyListState()
    Button(onClick = { doSomething(lazyListState) }) { /* ... */ }
    // ...
}

The exact behavior depends on what doSomething does, but in general it isn’t safe (and you could classify this as “underhoisting state”)

Suppose doSomething stores the LazyListState in a some var somewhere, and now let’s say that SomeContainer ends up getting removed from composition (there’s some if above it that switches to false ).

That LazyListState will still be in that var, but it’s a stale reference to something that no longer exists in composition. If SomeContainer came back on screen, it’d create a different LazyListState, so now there are two different instances hanging around.

The same sort of issue occurs with the AAC ViewModel, which by construction outlives the UI. As a consequence, any reference to the UI given or passed to a ViewModel (including via lambdas) can be dangerous.

[Alex Vanyo]

Especially in the case where an Activity overrides all config changes, another option is to plain state holders that live within the UI lifetime as opposed to `ViewModel`s that outlive the UI. This section about “State in Compose” goes into a lot more detail about the differences and tradeoffs there: https://developer.android.com/jetpack/compose/state#managing-state

And Manuel’s video here goes into more detail for a specific example of this approach:

https://www.youtube.com/watch?v=rmv2ug-wW4U▾

[Adam Powell]

one way to make this sort of thing easier to reason about is to "simply" not hold long-lived references to any function parameter beyond when that function returns (or throws). Thanks to suspend functions you can use coroutine cancellation to clean up when the caller (that owns those shorter-lived references) cancels

[Colton Idle]

The general issue at play is having a longer-lived component having a reference to a shorter-lived component, which is something that could happen in a lot of places if you’re not careful.

👍

The exact behavior depends on what doSomething does, but in general it isn’t safe (and you could classify this as “underhoisting state”)

Interesting!

The same sort of issue occurs with the AAC ViewModel, which by construction outlives the UI. As a consequence, any reference to the UI given or passed to a ViewModel (including via lambdas) can be dangerous.

Makes sense now. Thanks. I think for some reason since my events were being passed from the root activity (using single activity arch) I thought that the activity essentially outlives just about everything else... especially compose screen VMs. I think Adam explained to me that all VMs technically can/will still outlive the activity because that's what they're meant to do.

Especially in the case where an Activity overrides all config changes, another option is to plain state holders that live within the UI lifetime as opposed to `ViewModel`s that outlive the UI. This section about “State in Compose” goes into a lot more detail about the differences and tradeoffs there

Hm. I wonder if this is basically what I said above. I was (originally) under the impression that my Activity is basically the largest scope and largest lifetime and so thats why its safe to just pass stuff from it, right on through to all of my composable "screen" VMs. I will take a look at those links. Thanks

Re: Adam

one way to make this sort of thing easier to reason about is to "simply" not hold long-lived references to any function parameter beyond when that function returns (or throws). Thanks to suspend functions you can use coroutine cancellation to clean up when the caller (that owns those shorter-lived references) cancels

Hm. I feel like I understood everything up to this point and this sorta went over my head. If you have an example, that would help but no worries. I will re-read after I get some rest. Cheers

[Alex Vanyo]

Here’s an example with alternatives to the logIn example from the documentation.

fun doSomething(onUserLogIn: () -> Unit) {
    viewModelScope.launch {
        if (logIn()) {
            onUserLogIn()
        }
    }
}

suspend fun doSomething(onUserLogIn: () -> Unit) {
    if (logIn()) {
        onUserLogIn()
    }
}

Let’s suppose both of those functions are defined in a ViewModel.

For the first, non-suspending one, doSomething returns immediately, and we start work in the viewModelScope. This scope can outlive the UI, but we have a reference to onUserLogIn (which might call navigate, or have some other reference to the UI). This is a memory leak at least.

For the second, suspending one, doSomething suspends while trying to log in, and this must be done in some scope. If we call this method from a UI scope (say, rememberCoroutineScope() ), then doSomething will be cancelled if the UI is destroyed, and therefore we don’t have a memory leak. We can take advantage of structured concurrency, and use things like try/finally to cleanup resources upon a CancellationException.

The suspending version is better, but still has a subtle problem for a case like logging in. What if doSomething gets cancelled, just after “logging in” but before we have a chance to execute onUserLogIn? To the data layer and the rest of the app, the user is logged in, but we never got a chance to navigate away.

For that reason, we always check the log in state, and do the required navigation away from the login screen in a LaunchedEffect.

[Adam Powell]

Or alternatively, check the login state and just compose either the logged in content or the login flow without trampoline navigation at all, but that gets into a different topic 🙂

[Colton Idle]

Ah thanks. I forgot that coroutine cancellation (when adam mentioned it) technically still leaves me with another avenue to do work. i.e. rememberCoroutineScope, and essentially doing it in the coroutine.

But I also understand the issues you pointed out.
Thanks! I feel much better about this!