Skip to content

Alter viewmodel-sample to show how to use a not always hot StateFlow #274

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.

Sign up for GitHub

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

Open
StylianosGakis wants to merge 6 commits into
base: trunk
Choose a base branch
from
Open

Alter viewmodel-sample to show how to use a not always hot StateFlow #274

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4e80877
Alter viewmodel-sample to show how to use a not always hot StateFlow
StylianosGakis Jul 24, 2023
e9dcfda
Potential fixes after PR feedback
StylianosGakis Jul 24, 2023
1ec6561
Add initialState and presenter Composable function to MoleculeViewMod…
tunjid Jul 25, 2023
8c1621b
Merge pull request #2 from tunjid/tj/viewmodel-sample-cold-stateflow
StylianosGakis Jul 25, 2023
395125d
Merge pull request #1 from StylianosGakis/stylianos-tunji-pr-feedback
StylianosGakis Jul 25, 2023
19bd668
Fix tests
StylianosGakis Jul 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions gradle/libs.versions.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ android-plugin = { module = "com.android.tools.build:gradle", version = "8.0.2"
androidx-core = { module = "androidx.core:core-ktx", version = "1.10.1" }
androidx-test-runner = { module = "androidx.test:runner", version = "1.5.2" }
androidx-activity-compose = { module = "androidx.activity:activity-compose", version = "1.7.2" }
androidx-lifecycle-compose = { module = "androidx.lifecycle:lifecycle-runtime-compose", version = "2.6.1" }

androidx-compose-bom = { module = "androidx.compose:compose-bom", version = "2023.06.01" }
androidx-compose-material3 = { module = "androidx.compose.material3:material3" }
Expand Down
1 change: 1 addition & 0 deletions sample-viewmodel/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ apply plugin: 'app.cash.molecule'
dependencies {
implementation platform(libs.androidx.compose.bom)
implementation libs.androidx.activity.compose
implementation libs.androidx.lifecycle.compose
implementation libs.androidx.compose.material3
implementation libs.coil.compose
implementation libs.squareup.retrofit.client
Expand Down
4 changes: 2 additions & 2 deletions sample-viewmodel/src/main/java/com/example/molecule/viewmodel/MainActivity.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,10 @@ import androidx.compose.material3.darkColorScheme
import androidx.compose.material3.lightColorScheme
import androidx.compose.runtime.Composable
import androidx.compose.runtime.SideEffect
import androidx.compose.runtime.collectAsState
import androidx.compose.runtime.getValue
import androidx.compose.ui.platform.LocalView
import androidx.core.view.WindowCompat
import androidx.lifecycle.compose.collectAsStateWithLifecycle

class MainActivity : ComponentActivity() {
override fun onCreate(savedInstanceState: Bundle?) {
Expand All @@ -39,7 +39,7 @@ class MainActivity : ComponentActivity() {
val viewModel by viewModels<PupperPicsViewModel>()
setContent {
RootContainer {
val model by viewModel.models.collectAsState()
val model by viewModel.models.collectAsStateWithLifecycle()
PupperPicsScreen(model) { event -> viewModel.take(event) }
}
}
Expand Down
11 changes: 11 additions & 0 deletions sample-viewmodel/src/main/java/com/example/molecule/viewmodel/MoleculePresenter.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package com.example.molecule.viewmodel

import androidx.compose.runtime.Composable
import kotlinx.coroutines.flow.Flow

interface MoleculePresenter<Event, Model> {
val seed: Model

@Composable
fun present(events: Flow<Event>): Model
}
42 changes: 34 additions & 8 deletions sample-viewmodel/src/main/java/com/example/molecule/viewmodel/MoleculeViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,20 @@
*/
package com.example.molecule.viewmodel

import androidx.compose.runtime.Composable
import androidx.compose.runtime.remember
import androidx.compose.ui.platform.AndroidUiDispatcher
import androidx.lifecycle.ViewModel
import androidx.lifecycle.viewModelScope
import app.cash.molecule.RecompositionMode.ContextClock
import app.cash.molecule.launchMolecule
import app.cash.molecule.moleculeFlow
import kotlin.time.Duration.Companion.seconds
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.MutableSharedFlow
import kotlinx.coroutines.flow.SharingStarted
import kotlinx.coroutines.flow.StateFlow
import kotlinx.coroutines.flow.WhileSubscribed
import kotlinx.coroutines.flow.onEach
import kotlinx.coroutines.flow.stateIn

abstract class MoleculeViewModel<Event, Model> : ViewModel() {
private val scope = CoroutineScope(viewModelScope.coroutineContext + AndroidUiDispatcher.Main)
Expand All @@ -34,9 +38,16 @@ abstract class MoleculeViewModel<Event, Model> : ViewModel() {
private val events = MutableSharedFlow<Event>(extraBufferCapacity = 20)

val models: StateFlow<Model> by lazy(LazyThreadSafetyMode.NONE) {
scope.launchMolecule(mode = ContextClock) {
models(events)
}
moleculeFlow(mode = ContextClock) {
val presenter = remember { presenterFactory() }
presenter.present(events)
}.onEach {
seed = it
}.stateIn(
scope = scope,
started = SharingStarted.WhileSubscribed(5.seconds),
initialValue = seed,

@jingibus jingibus Jul 25, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The need for this initialValue does make the case that having a started parameter on launchMolecule is maybe not the worst idea in the world. Hooking it up with the existing API surface is uhh waves hands possible, for a certain definition of possible, but not one useful to someone who wants to do what MoleculeViewModel is doing

Comment on lines +48 to +55

@JakeWharton JakeWharton Jul 29, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole pattern really feels like it should be encapsulated in something.

I could see it being one (or more) of the following:

  • A valve. These existed for RxJava and basically let you control the upstream subscription based on another stream. In this case, the lifecycle would be filtered and mapped to a true/false stream based on whether the activity was resumed and that would control the valve on the upstream collection.
  • Replaying share. Another RxJava thing, where an observable instance would cache the latest seen value, replay it to new subscribers, and unsubscribe upstream when the subscriber count dropped to 0.
  • A custom frame clock (or frame clock wrapper) which only ticks when the lifecycle is resumed. The flow collector would always be present, but instead of using active collection to control whether recomposition occurs we use the frame clock.

I legitimately cannot figure out what to do with this PR. The use case is certainly valid. But the amount of code required to accomplish it as-is seems like it would overwhelm someone trying to learn how to use Molecule with AndroidX View Model rather than guide them.

@JakeWharton JakeWharton Jul 29, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This is not really a reflection of your PR, more of the current library design, the design of View Model, and the general sadness of lifecycle.)

@StylianosGakis StylianosGakis Jul 29, 2023 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not really a reflection of your PR , more of the current library design, the design of View Model, and the general sadness of lifecycle.

Yes I understand and I agree. It really shouldn't be this way, but it is the current reality unfortunately.

The use case is certainly valid. But the amount of code required to accomplish it as-is seems like it would overwhelm someone trying to learn how to use Molecule with AndroidX View Model rather than guide them.

So my entire reasoning behind starting this PR came down to the steps we took to adopt molecule in the app I work for:

We were looking into adopting Molecule for our codebase for some time now.

  1. A long time ago there was no viewmodel sample, and I looked briefly into it and I though "eh, it's probably gonna be hell to do in a ViewModel" so I dropped the idea.
  2. Fast forward some time, the sample is there, and I look into it again. It all works using this tiny bit of glue code so I'm thinking let's go ahead and use it in this way exactly as it seems to work well.
  3. I realize that the state flow is always hot, keeping potentially expensive flows hot on the backstack for all entries in the backstack which starts becoming a big deal. I pause this idea briefly
  4. Then I tried to look for a way to make these two worlds play well together, hence this PR was made, where Tunji was very kind to help me through understanding how to work around ViewModel to make it happen.

Basically then, I fear that people who use AAC ViewModel will go up to step # 2 here, and then stop there without realizing there's more to be done. Then eventually when they figure out what happens, they'll blame Molecule instead of ViewModel since that's when this change would have been introduced to them.
So I thought if the sample played well with the lifecycle in the first place, all the people who would come here can see the easy and straightforward sample from the sample module here. And if they want a proper integration with ViewModel, they can look into the more involved sample-viewmodel module in this repo.

If we can do something to make this PR even better and make the code more understandable, perhaps from the points you suggest I'd love for that to happen of course. Other than that I personally still feel like it's a net positive if the sample which a lot of people are probably going to follow blindly is actually playing well with AAC ViewModel.

@jingibus jingibus Jul 29, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JakeWharton, if we consider LaunchedEffect this gets even gnarlier. Those are likely the pieces that you'd actually want to shut down — recomposition is designed not to have so many side effects.

On reflection, I think the happy places to land are likely to be:

  • Include rememberSaveable support, then kill everything with a stateIn type solution, without any seed-type mechanism
  • Just keep the molecule running, killing it when the ViewModel dies
  • Include support for a rememberRetained, like Circuit has

The last two are most appealing to me. But even rememberRetained has weird consequences — IIRC one of the points of retention in ViewModel is to allow those long-running processes to keep going. The only approach we have that would achieve that would be to just keep it running.

And is keeping it running really so bad? How many would actually be kept running at one time in the VM framework? Is it just the VMs currently on screen during config change, or have I fallen off the boat here?

@StylianosGakis StylianosGakis Jul 29, 2023

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's all the VMs in screens in the backstack, so potentially many of them.

@tunjid tunjid Jul 31, 2023 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's worth noting that a rememberSaveable solution would still be supsceptible to the overwrite on restart issue if the value to be restored is not parcelable.

The recommended period for state production to be halted in the absence observers with ViewModels is 5 seconds. If a screen is in the back stack for 5 seconds, it should still have access to it's last produced non parcelable state when resumed IMO. Otherwise, leaving a screen for > 5 seconds and returning to it will have the same behavior as the app cold starting on that screen.

In the case of a screen with a paginated list for example, this may mean a progress bar till the list is refreshed and the scroll position properly restored.

@jingibus jingibus Jul 31, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's worth noting that a rememberSaveable solution would still be supsceptible to the overwrite on restart issue if the value to be restored is not parcelable.

Yep. launchMolecule should be capable of supporting all of stateIn's parameters, which would fix that issue in a nice way.

)
}

fun take(event: Event) {
Expand All @@ -45,6 +56,21 @@ abstract class MoleculeViewModel<Event, Model> : ViewModel() {
}
}

@Composable
protected abstract fun models(events: Flow<Event>): Model
Comment thread
StylianosGakis marked this conversation as resolved.
/**
* This value serves as the initial value that the uiState [StateFlow] will emit and then as a
* way to cache the last emission.
* When the flow goes from being cold (when in the backstack and it has no observers) to being
* hot again, by default the value cached using [stateIn] will be overwritten by the Presenter's
* first emission. By default the presenter at that point won't have any notion of what that
* cached value was without us providing this seed [Model].
* It's the responsibility of the consumer to actually use this seed value when creating the
* Presenter inside the [presenterFactory].
*/
abstract var seed: Model

/**
* This will be remembered in the context of the moleculeFlow, so that it stays alive for as long
* as the [models] [StateFlow] is still hot (has observers or the timeout hasn't timed out yet).
*/
protected abstract fun presenterFactory(): MoleculePresenter<Event, Model>
}
83 changes: 48 additions & 35 deletions sample-viewmodel/src/main/java/com/example/molecule/viewmodel/presentationLogic.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,51 +31,64 @@ sealed interface Event {
data class Model(
val loading: Boolean,
val breeds: List<String>,
val dropdownText: String,
val currentBreed: String?,
val currentUrl: String?,
)
) {
val dropdownText: String = currentBreed ?: "Select breed"
}

class PupperPicsViewModel : MoleculeViewModel<Event, Model>() {
@Composable
override fun models(events: Flow<Event>): Model {
return PupperPicsPresenter(events, PupperPicsService())
override var seed: Model = Model(
loading = false,
breeds = emptyList(),
currentBreed = null,
currentUrl = null,
)

override fun presenterFactory(): MoleculePresenter<Event, Model> {
return PupperPicsPresenter(seed, PupperPicsService())
}
}

@Composable
fun PupperPicsPresenter(events: Flow<Event>, service: PupperPicsService): Model {
var breeds: List<String> by remember { mutableStateOf(emptyList()) }
var currentBreed: String? by remember { mutableStateOf(null) }
var currentUrl: String? by remember { mutableStateOf(null) }
var fetchId: Int by remember { mutableStateOf(0) }
class PupperPicsPresenter(
override val seed: Model,
private val service: PupperPicsService,
) : MoleculePresenter<Event, Model> {
@Composable
override fun present(events: Flow<Event>): Model {
var breeds: List<String> by remember { mutableStateOf(seed.breeds) }
var currentBreed: String? by remember { mutableStateOf(seed.currentBreed) }
var currentUrl: String? by remember { mutableStateOf(seed.currentUrl) }
var fetchId: Int by remember { mutableStateOf(0) }

// Grab the list of breeds and sets the current selection to the first in the list.
// Errors are ignored in this sample.
LaunchedEffect(Unit) {
breeds = service.listBreeds()
currentBreed = breeds.first()
}
// Grab the list of breeds and sets the current selection to the first in the list.
// Errors are ignored in this sample.
LaunchedEffect(Unit) {
breeds = service.listBreeds()
currentBreed = breeds.first()
}

// Load a random URL for the current breed whenever it changes, or the fetchId changes.
LaunchedEffect(currentBreed, fetchId) {
currentUrl = null
currentUrl = currentBreed?.let { service.randomImageUrlFor(it) }
}
// Load a random URL for the current breed whenever it changes, or the fetchId changes.
LaunchedEffect(currentBreed, fetchId) {
currentUrl = null
currentUrl = currentBreed?.let { service.randomImageUrlFor(it) }
}

// Handle UI events.
LaunchedEffect(Unit) {
events.collect { event ->
when (event) {
is Event.SelectBreed -> currentBreed = event.breed
Event.FetchAgain -> fetchId++ // Incrementing fetchId will load another random image URL.
// Handle UI events.
LaunchedEffect(Unit) {
events.collect { event ->
when (event) {
is Event.SelectBreed -> currentBreed = event.breed
Event.FetchAgain -> fetchId++ // Incrementing fetchId will load another random image URL.
}
}
}
}

return Model(
loading = currentBreed == null,
breeds = breeds,
dropdownText = currentBreed ?: "Select breed",
currentUrl = currentUrl,
)
return Model(
loading = currentBreed == null,
breeds = breeds,
currentBreed = currentBreed,
currentUrl = currentUrl,
)
}
}
38 changes: 22 additions & 16 deletions sample-viewmodel/src/test/java/com/example/molecule/viewmodel/PupperPicsPresenterTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,19 +30,23 @@ import org.junit.Assert.assertEquals
import org.junit.Test

class PupperPicsPresenterTest {

private val seed: Model = Model(
loading = true,
breeds = emptyList(),
currentBreed = null,
currentUrl = null,
)

@Test
fun `on launch, breeds are loaded followed by an image url`() = runBlocking {
val picsService = FakePicsService()
val presenter = PupperPicsPresenter(seed, picsService)
moleculeFlow(mode = RecompositionMode.Immediate) {
PupperPicsPresenter(emptyFlow(), picsService)
presenter.present(emptyFlow())
}.distinctUntilChanged().test {
assertEquals(
Model(
loading = true,
breeds = emptyList(),
dropdownText = "Select breed",
currentUrl = null,
),
seed,
awaitItem(),
)

Expand All @@ -51,7 +55,7 @@ class PupperPicsPresenterTest {
Model(
loading = false,
breeds = listOf("akita", "boxer", "corgi"),
dropdownText = "akita",
currentBreed = "akita",
currentUrl = null,
),
awaitItem(),
Expand All @@ -65,7 +69,7 @@ class PupperPicsPresenterTest {
Model(
loading = false,
breeds = listOf("akita", "boxer", "corgi"),
dropdownText = "akita",
currentBreed = "akita",
currentUrl = "akita.jpg",
),
awaitItem(),
Expand All @@ -77,8 +81,9 @@ class PupperPicsPresenterTest {
fun `selecting breed updates dropdown text and fetches new image`() = runBlocking {
val picsService = FakePicsService()
val events = Channel<Event>()
val presenter = PupperPicsPresenter(seed, picsService)
moleculeFlow(mode = RecompositionMode.Immediate) {
PupperPicsPresenter(events.receiveAsFlow(), picsService)
presenter.present(events.receiveAsFlow())
}.distinctUntilChanged().test {
picsService.breeds.add(listOf("akita", "boxer", "corgi"))
picsService.urls.add("akita.jpg")
Expand All @@ -92,7 +97,7 @@ class PupperPicsPresenterTest {
Model(
loading = false,
breeds = listOf("akita", "boxer", "corgi"),
dropdownText = "boxer",
currentBreed = "boxer",
currentUrl = "akita.jpg",
),
awaitItem(),
Expand All @@ -101,7 +106,7 @@ class PupperPicsPresenterTest {
Model(
loading = false,
breeds = listOf("akita", "boxer", "corgi"),
dropdownText = "boxer",
currentBreed = "boxer",
currentUrl = null,
),
awaitItem(),
Expand All @@ -114,7 +119,7 @@ class PupperPicsPresenterTest {
Model(
loading = false,
breeds = listOf("akita", "boxer", "corgi"),
dropdownText = "boxer",
currentBreed = "boxer",
currentUrl = "boxer.jpg",
),
awaitItem(),
Expand All @@ -126,8 +131,9 @@ class PupperPicsPresenterTest {
fun `fetching again requests a new image`() = runBlocking {
val picsService = FakePicsService()
val events = Channel<Event>()
val presenter = PupperPicsPresenter(seed, picsService)
moleculeFlow(mode = RecompositionMode.Immediate) {
PupperPicsPresenter(events.receiveAsFlow(), picsService)
presenter.present(events.receiveAsFlow())
}.distinctUntilChanged().test {
picsService.breeds.add(listOf("akita", "boxer", "corgi"))
assertThat(picsService.urlRequestArgs.awaitItem()).isEqualTo("akita")
Expand All @@ -139,7 +145,7 @@ class PupperPicsPresenterTest {
Model(
loading = false,
breeds = listOf("akita", "boxer", "corgi"),
dropdownText = "akita",
currentBreed = "akita",
currentUrl = null,
),
awaitItem(),
Expand All @@ -151,7 +157,7 @@ class PupperPicsPresenterTest {
Model(
loading = false,
breeds = listOf("akita", "boxer", "corgi"),
dropdownText = "akita",
currentBreed = "akita",
currentUrl = "akita2.jpg",
),
awaitItem(),
Expand Down