thunderbird / thunderbird-android

Thunderbird for Android – Open Source Email App for Android (fka K-9 Mail)
https://thunderbird.net/mobile
Apache License 2.0
11.05k stars 2.51k forks source link

Fix state update when one of the remembered values changes #8500

Closed wmontwe closed 2 weeks ago

cketti commented 3 weeks ago

I don't really understand this. Can you explain what problem this is solving?

wmontwe commented 3 weeks ago

A crash occurs in the SpecialFoldersContent composable when attempting to display the error state using rememberContentLoadingErrorViewState. This happens when the error state is triggered even when the state.error is already null again.

The state was not updated as the value is based on a calculation. The derivedStateOf ensures that the value is calculated again if any of the remember keys got changed.

cketti commented 3 weeks ago

I can reliably reproduce the crash by adding the following test to SpecialFoldersScreenKtTest.

@Test
fun `switch from error to loading state`() {
    val initialState = State(
        isLoading = false,
        error = SpecialFoldersContract.Failure.LoadFoldersFailed("irrelevant")
    )
    val viewModel = FakeSpecialFoldersViewModel(initialState)

    setContentWithTheme {
        SpecialFoldersScreen(
            onNext = { },
            onBack = { },
            viewModel = viewModel,
            brandNameProvider = FakeBrandNameProvider,
        )
    }

    viewModel.updateStateForTest {
        it.copy(
            isLoading = true,
            error = null
        )
    }

    composeTestRule.mainClock.advanceTimeBy(500)
}

FakeSpecialFoldersViewModel.updateStateForTest() is a helper to make BaseViewModel.updateState() accessible.

Unfortunately, the change in this PR doesn't appear to fix the crash.

I believe the problem is that the composable functions called inside of AnimatedContent use state other than targetState. This will lead to the error/loading/content functions being called with the wrong state when creating the content to animate in or out.

I think our ContentLoadingErrorView needs to be changed to accommodate for this.