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 all commits
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
30 changes: 22 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 @@ -20,31 +20,45 @@ 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() {
abstract class MoleculeViewModel<Event, Model>(
initialState: Model,
started: SharingStarted = SharingStarted.WhileSubscribed(5.seconds),
presenter: @Composable (seed: Model, events: Flow<Event>) -> Model,
) : ViewModel() {
private val scope = CoroutineScope(viewModelScope.coroutineContext + AndroidUiDispatcher.Main)

// Events have a capacity large enough to handle simultaneous UI events, but
// small enough to surface issues if they get backed up for some reason.
private val events = MutableSharedFlow<Event>(extraBufferCapacity = 20)

private var seed: Model = initialState

val models: StateFlow<Model> by lazy(LazyThreadSafetyMode.NONE) {
scope.launchMolecule(mode = ContextClock) {
models(events)
}
moleculeFlow(mode = ContextClock) {
presenter(seed, events)
}.onEach {
seed = it
}.stateIn(
scope = scope,
started = started,
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) {
if (!events.tryEmit(event)) {
error("Event buffer overflow.")
}
}

@Composable
protected abstract fun models(events: Flow<Event>): Model
Comment thread
StylianosGakis marked this conversation as resolved.
}
44 changes: 31 additions & 13 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,22 +31,40 @@ sealed interface Event {
data class Model(
val loading: Boolean,
val breeds: List<String>,
val dropdownText: String,
val currentBreed: String?,
val currentUrl: String?,
)

class PupperPicsViewModel : MoleculeViewModel<Event, Model>() {
@Composable
override fun models(events: Flow<Event>): Model {
return PupperPicsPresenter(events, PupperPicsService())
}
) {
val dropdownText: String = currentBreed ?: "Select breed"
}

class PupperPicsViewModel(
// This service would typically be injected
service: PupperPicsService = PupperPicsService(),
) : MoleculeViewModel<Event, Model>(
initialState = Model(
loading = false,
breeds = emptyList(),
currentBreed = null,
currentUrl = null,
),
presenter = { seed, events ->
PupperPicsPresenter(
seed = seed,
events = events,
service = service,
)
},
)

@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) }
fun PupperPicsPresenter(
seed: Model,

@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.

This seed thing sure is awkward. There "should" be no need for it — as a practical matter, moleculeFlow will emit immediately. But obviously that is not something a Flow does...

@tunjid tunjid Jul 26, 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.

The need for it mostly stems from restarting. Consider a timer that counts how much time is spent on a screen doing an activity, like the NYT crossword. It has the following presenter Composable:

@Composable
fun TimerPresenter(seed: Long): Long {
    var elapsed by remember { mutableStateOf(seed) }

    LaunchedEffect(Unit) {
        while(true) {
            delay(1.seconds)
            ++elapsed
       } 
    }
}

Without the seed, there is no way to restore elapsed to its last seen state in between presenter stop and restarts save for writing it to a data source. If the data source exposed a cold flow, the last seen value will immediately be overwritten:

@Composable
fun TimerPresenter(dataSource: Flow<Long>): Long {
    // There is no good default to use here upon resumption
    val savedElapsed by dataSource.collectAsState(0)
    // elapsed will start back at 0
    var elapsed by remember { mutableStateOf(savedElapsed) }

    LaunchedEffect(Unit) {
        while(true) {
            delay(1.seconds)
            ++elapsed
       } 
    }

    // Assuming the data source is well behaved and only emits once,
    // add the last time elapsed to the current one
    LaunchedEffect(savedElapsed) {
        elapsed += savedElapsed
    }
}

In the above, the UI will briefly flash 0 when the screen is returned to after state production is resumed after being stopped. The seed provides a way of "freezing" the last seen state for restoration when state production resumes as Compose Presenters unlike Flow require a synchronous (in as much as a Composable function can be deemed synchronous) initial return value.

@tunjid tunjid Jul 26, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This isn't an issue with Flow operators like combine or merge because state won't be updated till all Flows emit in the case of the former, and till independent Flows emit in the latter. The immediate value requirement that Compose imposes seemingly mandates a seed value.

@Sripadmanabans Sripadmanabans Jul 27, 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.

I don't understand the TimerPresenter example. It seems to only emit one value.

LaunchedEffect(Unit) {
    delay(1000L)
    ++elapsed
}

Should it be inside a while (true) loop?

@tunjid tunjid Jul 27, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yes, it should. Thanks for pointing it out!

@Sripadmanabans Sripadmanabans Jul 27, 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.

If we do end up using a while (true) in the presenter and convert the molecule flow with stateIn and SharingStarted.WhileSubscribed(5.seconds) I think the behavior would be wrong. In the sense that we would be tracking an additional 5 seconds after the last collector has been removed.
Would it be better to use molecule to create a StateFlow that is always active and using events to start and stop the timer be a better way to keep track of screen time?

@tunjid tunjid Jul 27, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In the particular case of this example, a no argument SharingStarted.WhileSubscribed() would have been used. The example is a bit trivial however, as other flows may also have been used in the presenter.

The larger point in context being, it would be nice if molecule presenters could be used to create a StateFlow that offers behaviors that match that in all permutations of flow.stateIn(...). Especially in the case where the Flow is stopped and restarted, without overwriting the last emitted value.

events: Flow<Event>,
service: PupperPicsService,
): 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.
Expand Down Expand Up @@ -75,7 +93,7 @@ fun PupperPicsPresenter(events: Flow<Event>, service: PupperPicsService): Model
return Model(
loading = currentBreed == null,
breeds = breeds,
dropdownText = currentBreed ?: "Select breed",
currentBreed = currentBreed,
currentUrl = currentUrl,
)
}
35 changes: 19 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,22 @@ 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()
moleculeFlow(mode = RecompositionMode.Immediate) {
PupperPicsPresenter(emptyFlow(), picsService)
PupperPicsPresenter(seed, emptyFlow(), picsService)
}.distinctUntilChanged().test {
assertEquals(
Model(
loading = true,
breeds = emptyList(),
dropdownText = "Select breed",
currentUrl = null,
),
seed,
awaitItem(),
)

Expand All @@ -51,7 +54,7 @@ class PupperPicsPresenterTest {
Model(
loading = false,
breeds = listOf("akita", "boxer", "corgi"),
dropdownText = "akita",
currentBreed = "akita",
currentUrl = null,
),
awaitItem(),
Expand All @@ -65,7 +68,7 @@ class PupperPicsPresenterTest {
Model(
loading = false,
breeds = listOf("akita", "boxer", "corgi"),
dropdownText = "akita",
currentBreed = "akita",
currentUrl = "akita.jpg",
),
awaitItem(),
Expand All @@ -78,7 +81,7 @@ class PupperPicsPresenterTest {
val picsService = FakePicsService()
val events = Channel<Event>()
moleculeFlow(mode = RecompositionMode.Immediate) {
PupperPicsPresenter(events.receiveAsFlow(), picsService)
PupperPicsPresenter(seed, events.receiveAsFlow(), picsService)
}.distinctUntilChanged().test {
picsService.breeds.add(listOf("akita", "boxer", "corgi"))
picsService.urls.add("akita.jpg")
Expand All @@ -92,7 +95,7 @@ class PupperPicsPresenterTest {
Model(
loading = false,
breeds = listOf("akita", "boxer", "corgi"),
dropdownText = "boxer",
currentBreed = "boxer",
currentUrl = "akita.jpg",
),
awaitItem(),
Expand All @@ -101,7 +104,7 @@ class PupperPicsPresenterTest {
Model(
loading = false,
breeds = listOf("akita", "boxer", "corgi"),
dropdownText = "boxer",
currentBreed = "boxer",
currentUrl = null,
),
awaitItem(),
Expand All @@ -114,7 +117,7 @@ class PupperPicsPresenterTest {
Model(
loading = false,
breeds = listOf("akita", "boxer", "corgi"),
dropdownText = "boxer",
currentBreed = "boxer",
currentUrl = "boxer.jpg",
),
awaitItem(),
Expand All @@ -127,7 +130,7 @@ class PupperPicsPresenterTest {
val picsService = FakePicsService()
val events = Channel<Event>()
moleculeFlow(mode = RecompositionMode.Immediate) {
PupperPicsPresenter(events.receiveAsFlow(), picsService)
PupperPicsPresenter(seed, events.receiveAsFlow(), picsService)
}.distinctUntilChanged().test {
picsService.breeds.add(listOf("akita", "boxer", "corgi"))
assertThat(picsService.urlRequestArgs.awaitItem()).isEqualTo("akita")
Expand All @@ -139,7 +142,7 @@ class PupperPicsPresenterTest {
Model(
loading = false,
breeds = listOf("akita", "boxer", "corgi"),
dropdownText = "akita",
currentBreed = "akita",
currentUrl = null,
),
awaitItem(),
Expand All @@ -151,7 +154,7 @@ class PupperPicsPresenterTest {
Model(
loading = false,
breeds = listOf("akita", "boxer", "corgi"),
dropdownText = "akita",
currentBreed = "akita",
currentUrl = "akita2.jpg",
),
awaitItem(),
Expand Down