diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/composable/HomeScreen.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/composable/HomeScreen.kt index 9cf1ec42..6f525349 100644 --- a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/composable/HomeScreen.kt +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/composable/HomeScreen.kt @@ -80,6 +80,7 @@ import mantra.composeapp.generated.resources.messages_need_the_secret_key_this_p import mantra.composeapp.generated.resources.sign_in_with_the_nsec import press.mantra.compose.ui.composable.widgets.LocalCanSign import press.mantra.compose.ui.composable.widgets.ScreenStateTransition +import press.mantra.compose.ui.composable.widgets.ViewModelStoreScopedTo import press.mantra.compose.ui.composable.widgets.buttons.ExtendedFab import press.mantra.compose.ui.theme.ConformancePreviews @@ -311,10 +312,17 @@ fun HomeScreen( message = stringResource(Res.string.pick_a_conversation_to_read_it_here) ) } else { - // Keyed on the room, so switching rooms rebuilds the - // screen's view models rather than feeding a new id to - // ones already subscribed to another room's relay. + // Two things, and the pane needs both. `key` rebuilds + // the composition -- the composer's draft text does not + // follow you into the next room, and the effect that + // loads a room runs again. `ViewModelStoreScopedTo` + // rebuilds the view models, which `key` cannot: they + // hang off the store, and on this screen the store is + // `HomeRoute`'s and never changes, so without it the + // pane kept answering with the first room it was given + // however many were tapped after it. key(chatRoomId) { + ViewModelStoreScopedTo(chatRoomId) { ChatRoomMessagingScreen( activeUserPublicKey = activeUserPublicKey, chatRoomId = chatRoomId, @@ -333,6 +341,7 @@ fun HomeScreen( onNavigateBack = null, ) } + } } } } diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/composable/widgets/ViewModelStoreScopedTo.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/composable/widgets/ViewModelStoreScopedTo.kt new file mode 100644 index 00000000..5577b429 --- /dev/null +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/composable/widgets/ViewModelStoreScopedTo.kt @@ -0,0 +1,51 @@ +package press.mantra.compose.ui.composable.widgets + +import androidx.compose.runtime.Composable +import androidx.compose.runtime.CompositionLocalProvider +import androidx.compose.runtime.DisposableEffect +import androidx.compose.runtime.remember +import androidx.lifecycle.ViewModelStore +import androidx.lifecycle.ViewModelStoreOwner +import androidx.lifecycle.viewmodel.compose.LocalViewModelStoreOwner + +/** + * Gives [content] a `ViewModelStore` of its own, cleared whenever [scopeKey] changes. + * + * **`key(...)` does not do this, which is the whole reason this exists.** A composition + * key replaces a group -- `remember`s are forgotten, `LaunchedEffect`s restart -- but + * `viewModel()` is not composition state. It resolves against + * `LocalViewModelStoreOwner.current` under a key derived from the *class*, so two + * compositions of one screen under one owner get the same instance no matter how they are + * keyed, and the factory is never invoked for the second. Being inside `key(someId)` buys + * nothing: the store is the navigation entry's, and it outlives every group in it. + * + * That is only a problem where one screen is composed for a succession of subjects without + * the navigation entry changing, which in this app means a list-detail pane: the home + * screen stays on `HomeRoute` while the detail pane is asked for one chat room after + * another. A pushed route has no such problem, because each `NavBackStackEntry` is already + * a store of its own -- which is why this only wraps the pane. + * + * The store is cleared on the way out rather than left to the parent, so the room just + * left does not keep its relay subscriptions and database collectors running. That is the + * other half of the job: keying the `viewModel()` calls by room id instead would fix what + * is on screen and quietly accumulate a live view model per room visited. + * + * Nothing in this app puts a `SavedStateHandle` in a view model, so a bare + * `ViewModelStoreOwner` is the whole contract a scope needs here; it deliberately does not + * implement `HasDefaultViewModelProviderFactory`, because every call inside passes its own + * factory. + */ +@Composable +fun ViewModelStoreScopedTo(scopeKey: Any?, content: @Composable () -> Unit) { + val owner = remember(scopeKey) { + object : ViewModelStoreOwner { + override val viewModelStore = ViewModelStore() + } + } + + DisposableEffect(owner) { + onDispose { owner.viewModelStore.clear() } + } + + CompositionLocalProvider(LocalViewModelStoreOwner provides owner, content = content) +} diff --git a/composeApp/src/jvmTest/kotlin/press/mantra/compose/ui/composable/ChatPaneLayoutJvmTest.kt b/composeApp/src/jvmTest/kotlin/press/mantra/compose/ui/composable/ChatPaneLayoutJvmTest.kt index ac4d48b7..937a68aa 100644 --- a/composeApp/src/jvmTest/kotlin/press/mantra/compose/ui/composable/ChatPaneLayoutJvmTest.kt +++ b/composeApp/src/jvmTest/kotlin/press/mantra/compose/ui/composable/ChatPaneLayoutJvmTest.kt @@ -3,12 +3,16 @@ package press.mantra.compose.ui.composable import androidx.compose.ui.test.ExperimentalTestApi import androidx.compose.ui.test.assertIsDisplayed import androidx.compose.ui.test.assertWidthIsEqualTo +import androidx.compose.ui.test.onAllNodesWithText import androidx.compose.ui.test.onNodeWithTag import androidx.compose.ui.test.onNodeWithText +import androidx.compose.ui.test.performClick import androidx.compose.ui.test.runDesktopComposeUiTest import androidx.compose.ui.unit.dp +import com.vitorpamplona.quartz.nip01Core.core.HexKey import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.flowOf +import press.mantra.compose.database.model.ChatRoom import press.mantra.compose.database.model.NostrEvent import press.mantra.compose.database.model.Profile import press.mantra.compose.database.model.intermdiate.LocalChatRoom @@ -21,12 +25,14 @@ import press.mantra.compose.ui.composable.widgets.ProvideSnackbarHost import press.mantra.compose.ui.theme.MantraTheme import press.mantra.compose.ui.view.state.HomeScreenUIState import com.vitorpamplona.quartz.nip10Notes.TextNoteEvent +import androidx.compose.ui.test.ComposeUiTest import kotlin.test.Test +import kotlin.test.assertEquals /** * The chat list-detail split, measured in windows of the widths the phase names. * - * Three claims, and none of them can be read off the source: + * Four claims, and none of them can be read off the source: * * - below expanded the home screen is unchanged. That is the promise that let the split * land as one commit -- the chat room is a navigation destination reached from eleven @@ -35,7 +41,10 @@ import kotlin.test.Test * `calculatePaneScaffoldDirective` gives it, so a hand-built pair measures the same as * a `ListDetailPaneScaffold` would; * - the detail pane says what it is for while nothing is selected, rather than being an - * unexplained empty half of the window. + * unexplained empty half of the window; + * - and a second tap in the list changes what the detail pane shows. That one is not a + * layout claim at all -- it is about view model scope -- but it is only reachable + * through this layout, and it is the way the split visibly failed on a tablet. * * The repositories are the no-op ones with the two reads this screen makes delegated to a * fixed answer. Kotlin's interface delegation makes that ten lines rather than a @@ -47,6 +56,15 @@ class ChatPaneLayoutJvmTest { private val publicKey = "de1a1e64d1c4e0d6bd97b0e73d4dfd0e1ec1cdd1e6d8d0e2b9a7f3c5d0e1a2b3" private val emptyDetail = "Pick a conversation to read it here." + private val roomOne = "Room one" + private val roomTwo = "Room two" + + /** + * Opening a room is a database read on another dispatcher, so the assertions wait + * rather than assume. Generous, because what it is waiting for is a thread hop and the + * cost of being wrong about it is a test that fails on a loaded machine. + */ + private val OpenTimeout = 5_000L @Test fun `a phone window is one pane, as it has always been`() = runDesktopComposeUiTest(400, 900) { @@ -81,8 +99,32 @@ class ChatPaneLayoutJvmTest { onNodeWithTag(ChatListPaneTag).assertWidthIsEqualTo(412.dp) } + @Test + fun `tapping a second room in the list changes what the detail pane shows`() = + runDesktopComposeUiTest(1000, 900) { + setContent { Home(chatRepository = TwoRooms) } + + // Each room's subject is drawn twice once it is the selection -- on its card in + // the list, and as the title of the detail pane's app bar -- so two nodes is + // "this is the room on the right" and one is "this room is merely listed". + onNodeWithText(roomOne).performClick() + waitUntil("room one opens", OpenTimeout) { nodesSaying(roomOne) == 2 } + + onNodeWithText(roomTwo).performClick() + + // Before the pane scoped its view models this timed out: `key(chatRoomId)` was + // rebuilding the composition around a `ChatRoomMessagingViewModel` that + // `viewModel()` kept resolving, by class, out of the home route's store -- so + // the pane answered with room one however many rooms were tapped after it. + waitUntil("room two replaces it", OpenTimeout) { + nodesSaying(roomTwo) == 2 && nodesSaying(roomOne) == 1 + } + assertEquals(2, nodesSaying(roomTwo), "room two is listed and shown") + assertEquals(1, nodesSaying(roomOne), "room one is listed, not shown") + } + @androidx.compose.runtime.Composable - private fun Home() { + private fun Home(chatRepository: ChatRepository = NoRooms) { MantraTheme { // Required, and deliberately so: `LocalSnackbarHostState` throws rather than // defaulting, because a default would make every `notify` on a screen that @@ -95,7 +137,7 @@ class ChatPaneLayoutJvmTest { onNavigateToDirectMessageDetail = {}, onNavigateToChatRoomCreation = {}, nostrRepository = FixedProfile, - chatRepository = NoRooms, + chatRepository = chatRepository, frostSigningRepository = FrostSigningRepository.NO_OP_FROST_SIGNING_REPOSITORY, dkgRepository = DkgRepository.NO_OP_DKG_REPOSITORY, ) @@ -128,9 +170,36 @@ class ChatPaneLayoutJvmTest { } /** The no-op repository, plus the one read the room list makes of it. */ - private val NoRooms = object : ChatRepository by ChatRepository.NO_OP_CHAT_REPOSITORY { - override suspend fun observeChatRoomListByPublicKey( - publicKey: String, - ): Flow> = flowOf(emptyList()) - } + private val NoRooms = rooms() + + /** Two rooms, told apart by their subject, which is what both panes draw as a title. */ + private val TwoRooms = rooms(room("1", roomOne), room("2", roomTwo)) + + /** + * The no-op repository, plus the two reads this pair of panes makes of it: the list + * observes every room, and the detail pane looks one up by id. + */ + private fun rooms(vararg rooms: LocalChatRoom) = + object : ChatRepository by ChatRepository.NO_OP_CHAT_REPOSITORY { + override suspend fun observeChatRoomListByPublicKey( + publicKey: String, + ): Flow> = flowOf(rooms.toList()) + + override suspend fun getChatRoomByIdentifier(id: HexKey): LocalChatRoom? = + rooms.firstOrNull { it.chatRoom.id == id } + } + + private fun room(id: String, subject: String) = LocalChatRoom( + chatRoom = ChatRoom( + id = id, + userPublicKey = publicKey, + subject = subject, + description = null, + initialGiftWrapPayloadId = null, + mlsGroupState = null, + ), + ) + + private fun ComposeUiTest.nodesSaying(text: String) = + onAllNodesWithText(text).fetchSemanticsNodes().size }