From 5cee6b7a7128aaeea61b6feade0d75ea00d459a3 Mon Sep 17 00:00:00 2001 From: Artem Smirnov Date: Sun, 4 Oct 2026 16:51:11 +0300 Subject: [PATCH] Keep a shared photo or link in the draft when adding people to the conversation --- ...nversationDraftsRepositoryMoveDraftTest.kt | 325 ++++++++++++++++++ .../AddParticipantsViewModelTest.kt | 180 +++++++++- .../BaseConversationDraftDelegateTest.kt | 17 + ...nversationDraftDelegateSimSelectionTest.kt | 4 + .../ConversationDraftDelegateTransferTest.kt | 140 ++++++++ .../ConversationDraftsRepository.kt | 93 +++++ .../store/ConversationDraftStore.kt | 47 +++ .../data/extension/DatabaseTransactions.kt | 8 + .../conversation/ConversationLaunchModule.kt | 8 + .../AddParticipantsViewModel.kt | 68 +++- .../delegate/ConversationDraftDelegate.kt | 37 ++ .../ConversationDraftEditorDelegate.kt | 11 + .../delegate/ConversationDraftTransfers.kt | 61 ++++ 13 files changed, 990 insertions(+), 9 deletions(-) create mode 100644 app/src/test/kotlin/com/android/messaging/data/conversation/repository/ConversationDraftsRepositoryMoveDraftTest.kt create mode 100644 app/src/test/kotlin/com/android/messaging/ui/conversation/composer/delegate/draft/ConversationDraftDelegateTransferTest.kt create mode 100644 src/com/android/messaging/data/extension/DatabaseTransactions.kt create mode 100644 src/com/android/messaging/ui/conversation/composer/delegate/ConversationDraftTransfers.kt diff --git a/app/src/test/kotlin/com/android/messaging/data/conversation/repository/ConversationDraftsRepositoryMoveDraftTest.kt b/app/src/test/kotlin/com/android/messaging/data/conversation/repository/ConversationDraftsRepositoryMoveDraftTest.kt new file mode 100644 index 000000000..195a81619 --- /dev/null +++ b/app/src/test/kotlin/com/android/messaging/data/conversation/repository/ConversationDraftsRepositoryMoveDraftTest.kt @@ -0,0 +1,325 @@ +package com.android.messaging.data.conversation.repository + +import android.content.Context +import androidx.core.content.contentValuesOf +import androidx.core.net.toUri +import com.android.messaging.FactoryTestAccess +import com.android.messaging.data.conversation.mapper.ConversationDraftMessageDataMapperImpl +import com.android.messaging.data.conversation.mapper.ConversationMessageDataDraftMapperImpl +import com.android.messaging.data.conversation.model.ConversationId +import com.android.messaging.data.conversation.model.draft.ConversationDraft +import com.android.messaging.data.conversation.model.draft.ConversationDraftAttachment +import com.android.messaging.data.conversation.store.ConversationDraftStore +import com.android.messaging.data.conversation.store.ConversationDraftStoreImpl +import com.android.messaging.datamodel.DataModel +import com.android.messaging.datamodel.DatabaseHelper +import com.android.messaging.datamodel.DatabaseHelper.ConversationColumns +import com.android.messaging.datamodel.DatabaseHelper.ParticipantColumns +import com.android.messaging.datamodel.DatabaseWrapper +import com.android.messaging.datamodel.MediaScratchFileProvider +import com.android.messaging.datamodel.createInMemoryActionSyncTestDatabase +import com.android.messaging.datamodel.data.MessageData +import com.android.messaging.datamodel.data.ParticipantData +import com.android.messaging.testutil.MainDispatcherRule +import com.android.messaging.testutil.installTestFactory +import io.mockk.every +import io.mockk.mockk +import io.mockk.unmockkAll +import kotlinx.collections.immutable.persistentListOf +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.test.runTest +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.Robolectric +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment + +/** + * Adding people to a conversation opens a different (group) conversation, so whatever was being + * composed (typically a shared photo or link) has to travel with it. These run the real draft SQL + * so the scratch files backing the moved attachments are covered too. + */ +@RunWith(RobolectricTestRunner::class) +class ConversationDraftsRepositoryMoveDraftTest { + + @get:Rule + val mainDispatcherRule = MainDispatcherRule() + + private lateinit var context: Context + private lateinit var database: DatabaseWrapper + private lateinit var repository: ConversationDraftsRepository + private var sourceConversationId = ConversationId(value = "") + private var targetConversationId = ConversationId(value = "") + private lateinit var sharedImageUri: String + private lateinit var targetImageUri: String + + @Before + fun setUp() { + context = RuntimeEnvironment.getApplication().applicationContext + + val dataModel = mockk(relaxed = true) + installTestFactory(context = context, dataModel = dataModel) + database = createInMemoryActionSyncTestDatabase(context) + every { dataModel.database } returns database + + // Draft loading drops attachments whose file can't be opened, so back them with real files + Robolectric.setupContentProvider( + MediaScratchFileProvider::class.java, + MediaScratchFileProvider.AUTHORITY, + ) + sharedImageUri = MediaScratchFileProvider.buildMediaScratchSpaceUri("png").toString() + targetImageUri = MediaScratchFileProvider.buildMediaScratchSpaceUri("png").toString() + + val selfParticipantId = insertSelfParticipant() + sourceConversationId = insertConversation(selfParticipantId = selfParticipantId) + targetConversationId = insertConversation(selfParticipantId = selfParticipantId) + + repository = createRepository(conversationDraftStore = ConversationDraftStoreImpl()) + } + + @After + fun tearDown() { + unmockkAll() + FactoryTestAccess.reset() + } + + @Test + fun moveDraft_movesSharedAttachmentToTargetConversation() { + runTest(context = mainDispatcherRule.testDispatcher) { + repository.saveDraft( + conversationId = sourceConversationId, + draft = draft(messageText = "look", attachmentUri = sharedImageUri), + ) + + repository.moveDraft( + fromConversationId = sourceConversationId, + toConversationId = targetConversationId, + ) + + val targetDraft = loadDraft(conversationId = targetConversationId) + assertEquals("look", targetDraft.messageText) + assertEquals(listOf(sharedImageUri), targetDraft.attachments.map { it.contentUri }) + assertEquals("1", readConversationColumn(targetConversationId, SHOW_DRAFT)) + assertEquals( + sharedImageUri, + readConversationColumn(targetConversationId, DRAFT_PREVIEW_URI), + ) + } + } + + @Test + fun moveDraft_clearsSourceConversationWithoutDeletingMovedAttachmentFile() { + runTest(context = mainDispatcherRule.testDispatcher) { + repository.saveDraft( + conversationId = sourceConversationId, + draft = draft(messageText = "look", attachmentUri = sharedImageUri), + ) + + repository.moveDraft( + fromConversationId = sourceConversationId, + toConversationId = targetConversationId, + ) + + assertFalse(loadDraft(conversationId = sourceConversationId).hasContent) + assertEquals("0", readConversationColumn(sourceConversationId, SHOW_DRAFT)) + assertEquals("", readConversationColumn(sourceConversationId, DRAFT_PREVIEW_URI)) + assertTrue(scratchFileExists(uri = sharedImageUri)) + } + } + + @Test + fun moveDraft_keepsDraftAlreadyInTargetConversation() { + runTest(context = mainDispatcherRule.testDispatcher) { + repository.saveDraft( + conversationId = targetConversationId, + draft = draft(messageText = "hi all", attachmentUri = targetImageUri), + ) + repository.saveDraft( + conversationId = sourceConversationId, + draft = draft(messageText = "look", attachmentUri = sharedImageUri), + ) + + repository.moveDraft( + fromConversationId = sourceConversationId, + toConversationId = targetConversationId, + ) + + val targetDraft = loadDraft(conversationId = targetConversationId) + assertEquals("hi all\nlook", targetDraft.messageText) + assertEquals( + listOf(targetImageUri, sharedImageUri), + targetDraft.attachments.map { it.contentUri }, + ) + } + } + + @Test + fun moveDraft_toSameConversation_keepsDraftUnchanged() { + runTest(context = mainDispatcherRule.testDispatcher) { + repository.saveDraft( + conversationId = sourceConversationId, + draft = draft(messageText = "look", attachmentUri = sharedImageUri), + ) + + repository.moveDraft( + fromConversationId = sourceConversationId, + toConversationId = sourceConversationId, + ) + + val sourceDraft = loadDraft(conversationId = sourceConversationId) + assertEquals("look", sourceDraft.messageText) + assertEquals(listOf(sharedImageUri), sourceDraft.attachments.map { it.contentUri }) + } + } + + @Test + fun moveDraft_keepsBothSubjectsWhenTheyDiffer() { + runTest(context = mainDispatcherRule.testDispatcher) { + repository.saveDraft( + conversationId = targetConversationId, + draft = ConversationDraft(subjectText = "Dinner"), + ) + repository.saveDraft( + conversationId = sourceConversationId, + draft = ConversationDraft(subjectText = "Photos"), + ) + + repository.moveDraft( + fromConversationId = sourceConversationId, + toConversationId = targetConversationId, + ) + + assertEquals( + "Dinner Photos", + loadDraft(conversationId = targetConversationId).subjectText, + ) + } + } + + @Test + fun moveDraft_whenDraftIsNotWritten_failsAndKeepsSourceDraft() { + runTest(context = mainDispatcherRule.testDispatcher) { + repository.saveDraft( + conversationId = sourceConversationId, + draft = draft(messageText = "look", attachmentUri = sharedImageUri), + ) + val conversationDraftStore = ConversationDraftStoreImpl() + // The target conversation disappears after the drafts are read, right before the write + val targetDeletingStore = object : ConversationDraftStore by conversationDraftStore { + override fun moveDraftMessage( + fromConversationId: ConversationId, + toConversationId: ConversationId, + message: MessageData, + ) { + database.delete( + DatabaseHelper.CONVERSATIONS_TABLE, + "${ConversationColumns._ID}=?", + arrayOf(toConversationId.value), + ) + conversationDraftStore.moveDraftMessage( + fromConversationId = fromConversationId, + toConversationId = toConversationId, + message = message, + ) + } + } + + val failure = runCatching { + createRepository(conversationDraftStore = targetDeletingStore).moveDraft( + fromConversationId = sourceConversationId, + toConversationId = targetConversationId, + ) + }.exceptionOrNull() + + assertTrue(failure is IllegalStateException) + val sourceDraft = loadDraft(conversationId = sourceConversationId) + assertEquals("look", sourceDraft.messageText) + assertEquals(listOf(sharedImageUri), sourceDraft.attachments.map { it.contentUri }) + } + } + + private fun createRepository( + conversationDraftStore: ConversationDraftStore, + ): ConversationDraftsRepository { + return ConversationDraftsRepositoryImpl( + contentResolver = context.contentResolver, + conversationDraftMessageDataMapper = ConversationDraftMessageDataMapperImpl(), + conversationMessageDataDraftMapper = ConversationMessageDataDraftMapperImpl(), + conversationDraftStore = conversationDraftStore, + defaultDispatcher = mainDispatcherRule.testDispatcher, + ioDispatcher = mainDispatcherRule.testDispatcher, + messagingDbDispatcher = mainDispatcherRule.testDispatcher, + ) + } + + private suspend fun loadDraft(conversationId: ConversationId): ConversationDraft { + return repository.observeConversationDraft(conversationId = conversationId).first() + } + + private fun scratchFileExists(uri: String): Boolean { + return MediaScratchFileProvider.getFileFromUri(uri.toUri()).exists() + } + + private fun draft(messageText: String, attachmentUri: String): ConversationDraft { + return ConversationDraft( + messageText = messageText, + attachments = persistentListOf( + ConversationDraftAttachment( + contentType = "image/png", + contentUri = attachmentUri, + ), + ), + ) + } + + private fun insertSelfParticipant(): Long { + return database.insert( + DatabaseHelper.PARTICIPANTS_TABLE, + null, + contentValuesOf( + ParticipantColumns.SUB_ID to ParticipantData.DEFAULT_SELF_SUB_ID, + ParticipantColumns.NORMALIZED_DESTINATION to "self", + ParticipantColumns.SEND_DESTINATION to "self", + ), + ) + } + + private fun insertConversation(selfParticipantId: Long): ConversationId { + val conversationId = database.insert( + DatabaseHelper.CONVERSATIONS_TABLE, + null, + contentValuesOf( + ConversationColumns.CURRENT_SELF_ID to selfParticipantId, + ConversationColumns.PARTICIPANT_COUNT to 1, + ), + ) + + return ConversationId(conversationId.toString()) + } + + private fun readConversationColumn(conversationId: ConversationId, column: String): String { + return database.query( + DatabaseHelper.CONVERSATIONS_TABLE, + arrayOf(column), + "${ConversationColumns._ID}=?", + arrayOf(conversationId.value), + null, + null, + null, + ).use { cursor -> + cursor.moveToFirst() + cursor.getString(0) + } + } + + private companion object { + private const val SHOW_DRAFT = ConversationColumns.SHOW_DRAFT + private const val DRAFT_PREVIEW_URI = ConversationColumns.DRAFT_PREVIEW_URI + } +} diff --git a/app/src/test/kotlin/com/android/messaging/ui/conversation/addparticipants/AddParticipantsViewModelTest.kt b/app/src/test/kotlin/com/android/messaging/ui/conversation/addparticipants/AddParticipantsViewModelTest.kt index d40ec0f4a..5a25611fa 100644 --- a/app/src/test/kotlin/com/android/messaging/ui/conversation/addparticipants/AddParticipantsViewModelTest.kt +++ b/app/src/test/kotlin/com/android/messaging/ui/conversation/addparticipants/AddParticipantsViewModelTest.kt @@ -7,12 +7,14 @@ import com.android.messaging.data.contact.formatter.ContactDestinationFormatter import com.android.messaging.data.conversation.model.ConversationId import com.android.messaging.data.conversation.model.ParticipantId import com.android.messaging.data.conversation.model.recipient.ConversationRecipient +import com.android.messaging.data.conversation.repository.ConversationDraftsRepository import com.android.messaging.data.conversation.repository.ConversationParticipantsRepository import com.android.messaging.testutil.MainDispatcherRule import com.android.messaging.testutil.TEST_CONVERSATION_ID as CONVERSATION_ID import com.android.messaging.testutil.assertThat import com.android.messaging.ui.conversation.addparticipants.model.AddParticipantsEffect import com.android.messaging.ui.conversation.addparticipants.model.AddParticipantsNavEvent +import com.android.messaging.ui.conversation.composer.delegate.ConversationDraftTransfersImpl import com.android.messaging.ui.conversation.recipientpicker.delegate.ConversationResolutionDelegate import com.android.messaging.ui.conversation.recipientpicker.delegate.SelectedRecipientsDelegate import com.android.messaging.ui.conversation.recipientpicker.model.picker.ConversationResolutionOutcome @@ -21,19 +23,26 @@ import com.android.messaging.ui.conversation.recipientpicker.model.picker.Recipi import com.android.messaging.ui.recipientselection.delegate.RecipientPickerDelegate import com.android.messaging.ui.recipientselection.model.picker.RecipientPickerUiState import com.android.messaging.ui.recipientselection.model.picker.SelectedRecipient +import com.android.messaging.util.LogUtil +import io.mockk.coEvery +import io.mockk.coVerify import io.mockk.every import io.mockk.just import io.mockk.mockk +import io.mockk.mockkStatic import io.mockk.runs +import io.mockk.unmockkAll import io.mockk.verify import kotlinx.collections.immutable.ImmutableList import kotlinx.collections.immutable.persistentListOf +import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.flow.MutableSharedFlow import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.test.advanceUntilIdle import kotlinx.coroutines.test.runTest +import org.junit.After import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse import org.junit.Assert.assertTrue @@ -46,6 +55,11 @@ class AddParticipantsViewModelTest { @get:Rule val mainDispatcherRule = MainDispatcherRule() + @After + fun tearDown() { + unmockkAll() + } + @Test fun init_bindsDelegates() { runTest(context = mainDispatcherRule.testDispatcher) { @@ -209,7 +223,7 @@ class AddParticipantsViewModelTest { assertThat(awaitItem()).isEqualTo( AddParticipantsNavEvent.OpenConversation( conversationId = ConversationId("conversation-2"), - ) + ), ) verify(exactly = 1) { selectedRecipientsDelegate.clear() @@ -219,8 +233,161 @@ class AddParticipantsViewModelTest { } } + @Test + fun resolvedOutcome_movesDraftToResolvedConversationBeforeNavigating() { + runTest(context = mainDispatcherRule.testDispatcher) { + val moveDraftGate = CompletableDeferred() + val conversationDraftsRepository = createDraftsRepository() + coEvery { + conversationDraftsRepository.moveDraft( + fromConversationId = any(), + toConversationId = any(), + ) + } coAnswers { + moveDraftGate.await() + } + val resolutionDelegate = createResolutionDelegate() + val viewModel = createViewModel( + conversationDraftsRepository = conversationDraftsRepository, + conversationResolutionDelegate = resolutionDelegate.mock, + ) + viewModel.onConversationIdChanged(conversationId = CONVERSATION_ID) + + viewModel.navigationEvents.test { + advanceUntilIdle() + resolutionDelegate.outcomesSource.emit( + ConversationResolutionOutcome.Resolved( + conversationId = RESOLVED_CONVERSATION_ID, + ), + ) + advanceUntilIdle() + + coVerify(exactly = 1) { + conversationDraftsRepository.moveDraft( + fromConversationId = CONVERSATION_ID, + toConversationId = RESOLVED_CONVERSATION_ID, + ) + } + expectNoEvents() + + moveDraftGate.complete(Unit) + + assertThat(awaitItem()).isEqualTo( + AddParticipantsNavEvent.OpenConversation( + conversationId = RESOLVED_CONVERSATION_ID, + ), + ) + cancelAndIgnoreRemainingEvents() + } + } + } + + @Test + fun resolvedOutcome_staysOnScreenWhenMovingDraftFails() { + runTest(context = mainDispatcherRule.testDispatcher) { + mockkStatic(LogUtil::class) + every { LogUtil.e(any(), any(), any()) } just runs + val conversationDraftsRepository = createDraftsRepository() + coEvery { + conversationDraftsRepository.moveDraft( + fromConversationId = any(), + toConversationId = any(), + ) + } throws IllegalStateException("Conversation conversation-2 no longer exists") + val selectedRecipientsDelegate = createSelectedRecipientsDelegate( + selectedRecipients = persistentListOf( + selectedRecipient(destination = "+1 555 0101"), + ), + ) + val resolutionDelegate = createResolutionDelegate() + val viewModel = createViewModel( + conversationDraftsRepository = conversationDraftsRepository, + selectedRecipientsDelegate = selectedRecipientsDelegate, + conversationResolutionDelegate = resolutionDelegate.mock, + ) + viewModel.onConversationIdChanged(conversationId = CONVERSATION_ID) + + viewModel.effects.test { + advanceUntilIdle() + viewModel.onConfirmClick() + resolutionDelegate.outcomesSource.emit( + ConversationResolutionOutcome.Resolved( + conversationId = RESOLVED_CONVERSATION_ID, + ), + ) + + assertEquals( + AddParticipantsEffect.ShowMessage( + messageResId = R.string.conversation_creation_failure, + ), + awaitItem(), + ) + cancelAndIgnoreRemainingEvents() + } + viewModel.navigationEvents.test { + expectNoEvents() + } + verify(exactly = 0) { + selectedRecipientsDelegate.clear() + } + viewModel.onConfirmClick() + assertEquals(2, resolutionDelegate.resolvedDestinations.size) + } + } + + @Test + fun resolvedOutcome_ignoresClicksUntilDraftMoveFinishes() { + runTest(context = mainDispatcherRule.testDispatcher) { + val moveGate = CompletableDeferred() + val conversationDraftsRepository = createDraftsRepository() + coEvery { + conversationDraftsRepository.moveDraft( + fromConversationId = any(), + toConversationId = any(), + ) + } coAnswers { + moveGate.await() + } + val selectedRecipientsDelegate = createSelectedRecipientsDelegate( + selectedRecipients = persistentListOf( + selectedRecipient(destination = "+1 555 0101"), + ), + ) + val resolutionDelegate = createResolutionDelegate() + val viewModel = createViewModel( + conversationDraftsRepository = conversationDraftsRepository, + selectedRecipientsDelegate = selectedRecipientsDelegate, + conversationResolutionDelegate = resolutionDelegate.mock, + ) + viewModel.onConversationIdChanged(conversationId = CONVERSATION_ID) + advanceUntilIdle() + viewModel.onConfirmClick() + // The resolution is already idle again while the draft is still moving + resolutionDelegate.outcomesSource.emit( + ConversationResolutionOutcome.Resolved( + conversationId = RESOLVED_CONVERSATION_ID, + ), + ) + advanceUntilIdle() + + viewModel.onRecipientClicked(recipient = selectedRecipient(destination = "+1 555 0102")) + viewModel.onConfirmClick() + moveGate.complete(Unit) + advanceUntilIdle() + + assertEquals(1, resolutionDelegate.resolvedDestinations.size) + verify(exactly = 0) { + selectedRecipientsDelegate.toggle( + recipient = any(), + canAdd = any(), + ) + } + } + } + private fun createViewModel( contactDestinationFormatter: ContactDestinationFormatter = createFormatter(), + conversationDraftsRepository: ConversationDraftsRepository = createDraftsRepository(), conversationParticipantsRepository: ConversationParticipantsRepository = createParticipantsRepository(), isRecipientLimitExceeded: Boolean = false, @@ -231,6 +398,9 @@ class AddParticipantsViewModelTest { ): AddParticipantsViewModel { return AddParticipantsViewModel( contactDestinationFormatter = contactDestinationFormatter, + conversationDraftTransfers = ConversationDraftTransfersImpl( + conversationDraftsRepository = conversationDraftsRepository, + ), conversationParticipantsRepository = conversationParticipantsRepository, isConversationRecipientLimitExceeded = { isRecipientLimitExceeded @@ -251,6 +421,10 @@ class AddParticipantsViewModelTest { return formatter } + private fun createDraftsRepository(): ConversationDraftsRepository { + return mockk(relaxed = true) + } + private fun createParticipantsRepository( participants: ImmutableList = persistentListOf(), ): ConversationParticipantsRepository { @@ -340,4 +514,8 @@ class AddParticipantsViewModelTest { val outcomesSource: MutableSharedFlow, val resolvedDestinations: MutableList>, ) + + private companion object { + private val RESOLVED_CONVERSATION_ID = ConversationId("conversation-2") + } } diff --git a/app/src/test/kotlin/com/android/messaging/ui/conversation/composer/delegate/draft/BaseConversationDraftDelegateTest.kt b/app/src/test/kotlin/com/android/messaging/ui/conversation/composer/delegate/draft/BaseConversationDraftDelegateTest.kt index 94020f042..5a1475ee0 100644 --- a/app/src/test/kotlin/com/android/messaging/ui/conversation/composer/delegate/draft/BaseConversationDraftDelegateTest.kt +++ b/app/src/test/kotlin/com/android/messaging/ui/conversation/composer/delegate/draft/BaseConversationDraftDelegateTest.kt @@ -17,6 +17,8 @@ import com.android.messaging.testutil.MainDispatcherRule import com.android.messaging.testutil.TEST_CONVERSATION_ID as CONVERSATION_ID import com.android.messaging.ui.conversation.composer.delegate.ConversationDraftDelegateImpl import com.android.messaging.ui.conversation.composer.delegate.ConversationDraftEditorDelegateImpl +import com.android.messaging.ui.conversation.composer.delegate.ConversationDraftTransfers +import com.android.messaging.ui.conversation.composer.delegate.ConversationDraftTransfersImpl import io.mockk.coEvery import io.mockk.every import io.mockk.just @@ -43,12 +45,14 @@ internal abstract class BaseConversationDraftDelegateTest { actionRequirements: CheckConversationActionRequirements = createActionRequirementsMock(), conversationsRepository: ConversationsRepository = createConversationsRepositoryMock(), getDraftSendProtocol: GetConversationDraftSendProtocol = createGetDraftSendProtocolMock(), + conversationDraftTransfers: ConversationDraftTransfers? = null, ): DelegateHarness { val harness = createHarness( sendConversationDraft = sendConversationDraft, actionRequirements = actionRequirements, conversationsRepository = conversationsRepository, getDraftSendProtocol = getDraftSendProtocol, + conversationDraftTransfers = conversationDraftTransfers, ) harness.conversationIdFlow.value = CONVERSATION_ID harness.emitDraft( @@ -66,6 +70,7 @@ internal abstract class BaseConversationDraftDelegateTest { conversationsRepository: ConversationsRepository = createConversationsRepositoryMock(), getDraftSendProtocol: GetConversationDraftSendProtocol = createGetDraftSendProtocolMock(), observeFailure: Exception? = null, + conversationDraftTransfers: ConversationDraftTransfers? = null, ): DelegateHarness { val dispatcher = mainDispatcherRule.testDispatcher val applicationScope = TestScope(dispatcher) @@ -75,6 +80,9 @@ internal abstract class BaseConversationDraftDelegateTest { draftFlows = draftFlows, observeFailure = observeFailure, ) + val draftTransfers = conversationDraftTransfers ?: ConversationDraftTransfersImpl( + conversationDraftsRepository = conversationDraftsRepository, + ) val subscriptionsRepository = createSubscriptionsRepositoryMock() val conversationDraftEditorDelegate = ConversationDraftEditorDelegateImpl( subscriptionsRepository = subscriptionsRepository, @@ -91,6 +99,7 @@ internal abstract class BaseConversationDraftDelegateTest { checkConversationActionRequirements = actionRequirements, conversationDraftsRepository = conversationDraftsRepository, conversationDraftEditorDelegate = conversationDraftEditorDelegate, + conversationDraftTransfers = draftTransfers, sendConversationDraft = sendConversationDraft, defaultDispatcher = dispatcher, ) @@ -104,6 +113,7 @@ internal abstract class BaseConversationDraftDelegateTest { return DelegateHarness( delegate = delegate, conversationDraftsRepository = conversationDraftsRepository, + conversationDraftTransfers = draftTransfers, draftFlows = draftFlows, conversationIdFlow = conversationIdFlow, delegateScope = delegateScope, @@ -150,6 +160,12 @@ internal abstract class BaseConversationDraftDelegateTest { draft = any(), ) } just runs + coEvery { + repository.moveDraft( + fromConversationId = any(), + toConversationId = any(), + ) + } just runs return repository } @@ -208,6 +224,7 @@ internal abstract class BaseConversationDraftDelegateTest { protected data class DelegateHarness( val delegate: ConversationDraftDelegateImpl, val conversationDraftsRepository: ConversationDraftsRepository, + val conversationDraftTransfers: ConversationDraftTransfers, val draftFlows: MutableMap>, val conversationIdFlow: MutableStateFlow, val delegateScope: TestScope, diff --git a/app/src/test/kotlin/com/android/messaging/ui/conversation/composer/delegate/draft/ConversationDraftDelegateSimSelectionTest.kt b/app/src/test/kotlin/com/android/messaging/ui/conversation/composer/delegate/draft/ConversationDraftDelegateSimSelectionTest.kt index 12da812e5..94c92428d 100644 --- a/app/src/test/kotlin/com/android/messaging/ui/conversation/composer/delegate/draft/ConversationDraftDelegateSimSelectionTest.kt +++ b/app/src/test/kotlin/com/android/messaging/ui/conversation/composer/delegate/draft/ConversationDraftDelegateSimSelectionTest.kt @@ -14,6 +14,7 @@ import com.android.messaging.domain.conversation.usecase.draft.model.Conversatio import com.android.messaging.testutil.assertThat import com.android.messaging.ui.conversation.composer.delegate.ConversationDraftDelegateImpl import com.android.messaging.ui.conversation.composer.delegate.ConversationDraftEditorDelegateImpl +import com.android.messaging.ui.conversation.composer.delegate.ConversationDraftTransfersImpl import io.mockk.coEvery import io.mockk.every import io.mockk.mockk @@ -159,6 +160,9 @@ internal class ConversationDraftDelegateSimSelectionTest { checkConversationActionRequirements = checkConversationActionRequirements, conversationDraftsRepository = conversationDraftsRepository, conversationDraftEditorDelegate = conversationDraftEditorDelegate, + conversationDraftTransfers = ConversationDraftTransfersImpl( + conversationDraftsRepository = conversationDraftsRepository, + ), sendConversationDraft = sendConversationDraft, defaultDispatcher = StandardTestDispatcher(scheduler = testScheduler), ) diff --git a/app/src/test/kotlin/com/android/messaging/ui/conversation/composer/delegate/draft/ConversationDraftDelegateTransferTest.kt b/app/src/test/kotlin/com/android/messaging/ui/conversation/composer/delegate/draft/ConversationDraftDelegateTransferTest.kt new file mode 100644 index 000000000..7a64ae4ab --- /dev/null +++ b/app/src/test/kotlin/com/android/messaging/ui/conversation/composer/delegate/draft/ConversationDraftDelegateTransferTest.kt @@ -0,0 +1,140 @@ +package com.android.messaging.ui.conversation.composer.delegate.draft + +import com.android.messaging.data.conversation.model.draft.ConversationDraft +import com.android.messaging.testutil.TEST_CONVERSATION_ID as CONVERSATION_ID +import com.android.messaging.testutil.TEST_RESOLVED_CONVERSATION_ID as TARGET_CONVERSATION_ID +import io.mockk.coVerify +import io.mockk.coVerifyOrder +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.launch +import kotlinx.coroutines.test.advanceUntilIdle +import kotlinx.coroutines.test.runTest +import org.junit.Assert.assertTrue +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +/** + * Adding people opens a different (group) conversation, so the composer has to save what it has + * not saved yet before the draft moves there. + */ +@OptIn(ExperimentalCoroutinesApi::class) +@RunWith(RobolectricTestRunner::class) +internal class ConversationDraftDelegateTransferTest : BaseConversationDraftDelegateTest() { + + @Test + fun transferDraft_savesUnsavedEditsBeforeMovingDraft() { + runTest(context = mainDispatcherRule.testDispatcher) { + val harness = createBoundLoadedDelegateHarness() + + try { + harness.delegate.onMessageTextChanged(messageText = "latest link") + // Queued behind the transfer, so it must not save the moved edits again + harness.delegate.persistDraft() + + harness.conversationDraftTransfers.transferDraft( + fromConversationId = CONVERSATION_ID, + toConversationId = TARGET_CONVERSATION_ID, + ) + advanceUntilIdle() + + coVerifyOrder { + harness.conversationDraftsRepository.saveDraft( + conversationId = CONVERSATION_ID, + draft = match { draft -> draft.messageText == "latest link" }, + ) + harness.conversationDraftsRepository.moveDraft( + fromConversationId = CONVERSATION_ID, + toConversationId = TARGET_CONVERSATION_ID, + ) + } + coVerify(exactly = 1) { + harness.conversationDraftsRepository.saveDraft( + conversationId = any(), + draft = any(), + ) + } + } finally { + harness.cancel() + } + } + } + + @Test + fun transferDraft_waitsForDraftToLoadBeforeSavingEdits() { + runTest(context = mainDispatcherRule.testDispatcher) { + val harness = createHarness() + + try { + harness.conversationIdFlow.value = CONVERSATION_ID + advanceUntilIdle() + harness.delegate.seedDraft( + conversationId = CONVERSATION_ID, + draft = ConversationDraft(messageText = "latest link"), + ) + + val transfer = launch(mainDispatcherRule.testDispatcher) { + harness.conversationDraftTransfers.transferDraft( + fromConversationId = CONVERSATION_ID, + toConversationId = TARGET_CONVERSATION_ID, + ) + } + advanceUntilIdle() + assertTrue(transfer.isActive) + + harness.emitDraft( + conversationId = CONVERSATION_ID, + draft = ConversationDraft(messageText = "old link"), + ) + advanceUntilIdle() + + coVerifyOrder { + harness.conversationDraftsRepository.saveDraft( + conversationId = CONVERSATION_ID, + draft = match { draft -> draft.messageText == "latest link" }, + ) + harness.conversationDraftsRepository.moveDraft( + fromConversationId = CONVERSATION_ID, + toConversationId = TARGET_CONVERSATION_ID, + ) + } + } finally { + harness.cancel() + } + } + } + + @Test + fun transferDraft_savesEditsOfEveryComposerOnTheConversation() { + runTest(context = mainDispatcherRule.testDispatcher) { + val editingHarness = createBoundLoadedDelegateHarness() + val laterHarness = createBoundLoadedDelegateHarness( + conversationDraftTransfers = editingHarness.conversationDraftTransfers, + ) + + try { + editingHarness.delegate.onMessageTextChanged(messageText = "latest link") + + editingHarness.conversationDraftTransfers.transferDraft( + fromConversationId = CONVERSATION_ID, + toConversationId = TARGET_CONVERSATION_ID, + ) + advanceUntilIdle() + + coVerifyOrder { + editingHarness.conversationDraftsRepository.saveDraft( + conversationId = CONVERSATION_ID, + draft = match { draft -> draft.messageText == "latest link" }, + ) + editingHarness.conversationDraftsRepository.moveDraft( + fromConversationId = CONVERSATION_ID, + toConversationId = TARGET_CONVERSATION_ID, + ) + } + } finally { + editingHarness.cancel() + laterHarness.cancel() + } + } + } +} diff --git a/src/com/android/messaging/data/conversation/repository/ConversationDraftsRepository.kt b/src/com/android/messaging/data/conversation/repository/ConversationDraftsRepository.kt index 012e2dcf9..f2a2dd441 100644 --- a/src/com/android/messaging/data/conversation/repository/ConversationDraftsRepository.kt +++ b/src/com/android/messaging/data/conversation/repository/ConversationDraftsRepository.kt @@ -10,7 +10,9 @@ import com.android.messaging.data.conversation.mapper.ConversationMessageDataDra import com.android.messaging.data.conversation.model.ConversationId import com.android.messaging.data.conversation.model.ParticipantId import com.android.messaging.data.conversation.model.draft.ConversationDraft +import com.android.messaging.data.conversation.model.draft.ConversationDraftAttachment import com.android.messaging.data.conversation.store.ConversationDraftStore +import com.android.messaging.data.extension.runInTransaction import com.android.messaging.datamodel.MessagingContentProvider import com.android.messaging.datamodel.data.MessageData import com.android.messaging.di.core.DefaultDispatcher @@ -38,6 +40,11 @@ internal interface ConversationDraftsRepository { conversationId: ConversationId, draft: ConversationDraft, ) + + suspend fun moveDraft( + fromConversationId: ConversationId, + toConversationId: ConversationId, + ) } internal class ConversationDraftsRepositoryImpl @Inject constructor( @@ -101,6 +108,27 @@ internal class ConversationDraftsRepositoryImpl @Inject constructor( } } + override suspend fun moveDraft( + fromConversationId: ConversationId, + toConversationId: ConversationId, + ) { + if (fromConversationId == toConversationId) { + return + } + + withContext(context = messagingDbDispatcher) { + runInTransaction { + moveDraftInTransaction( + fromConversationId = fromConversationId, + toConversationId = toConversationId, + ) + } + + notifyConversationMetadataChanged(conversationId = fromConversationId) + notifyConversationMetadataChanged(conversationId = toConversationId) + } + } + private fun observeDraftChanges(uri: Uri): Flow { return callbackFlow { val observer = object : ContentObserver(null) { @@ -138,6 +166,71 @@ internal class ConversationDraftsRepositoryImpl @Inject constructor( ) } + private fun moveDraftInTransaction( + fromConversationId: ConversationId, + toConversationId: ConversationId, + ) { + val movedDraft = loadConversationDraft(conversationId = fromConversationId) + if (!movedDraft.hasContent) { + return + } + + val message = conversationDraftMessageDataMapper.map( + conversationId = toConversationId, + draft = mergeDrafts( + targetDraft = loadConversationDraft(conversationId = toConversationId), + movedDraft = movedDraft, + ), + ) + val boundMessage = checkNotNull( + bindDraftParticipantsIfNeeded( + conversationId = toConversationId, + message = message, + ), + ) { + "Conversation ${toConversationId.value} no longer exists" + } + + conversationDraftStore.moveDraftMessage( + fromConversationId = fromConversationId, + toConversationId = toConversationId, + message = boundMessage, + ) + } + + private fun mergeDrafts( + targetDraft: ConversationDraft, + movedDraft: ConversationDraft, + ): ConversationDraft { + val attachments = (targetDraft.attachments + movedDraft.attachments) + .distinctBy(ConversationDraftAttachment::contentUri) + + return targetDraft.copy( + messageText = mergeDraftText( + targetText = targetDraft.messageText, + movedText = movedDraft.messageText, + separator = "\n", + ), + subjectText = mergeDraftText( + targetText = targetDraft.subjectText, + movedText = movedDraft.subjectText, + separator = " ", + ), + attachments = attachments.toImmutableList(), + ) + } + + private fun mergeDraftText( + targetText: String, + movedText: String, + separator: String, + ): String { + return listOf(targetText, movedText) + .filter { it.isNotBlank() } + .distinct() + .joinToString(separator = separator) + } + private fun createConversationDraft( selfParticipantId: ParticipantId, draftMessage: MessageData?, diff --git a/src/com/android/messaging/data/conversation/store/ConversationDraftStore.kt b/src/com/android/messaging/data/conversation/store/ConversationDraftStore.kt index ee187f81f..0d9a43492 100644 --- a/src/com/android/messaging/data/conversation/store/ConversationDraftStore.kt +++ b/src/com/android/messaging/data/conversation/store/ConversationDraftStore.kt @@ -4,8 +4,11 @@ import com.android.messaging.data.conversation.model.ConversationId import com.android.messaging.data.conversation.model.ParticipantId import com.android.messaging.datamodel.BugleDatabaseOperations import com.android.messaging.datamodel.DataModel +import com.android.messaging.datamodel.DatabaseHelper +import com.android.messaging.datamodel.DatabaseHelper.MessageColumns import com.android.messaging.datamodel.data.ConversationListItemData import com.android.messaging.datamodel.data.MessageData +import com.android.messaging.util.db.ext.withTransaction import javax.inject.Inject internal interface ConversationDraftStore { @@ -20,6 +23,12 @@ internal interface ConversationDraftStore { conversationId: ConversationId, message: MessageData, ) + + fun moveDraftMessage( + fromConversationId: ConversationId, + toConversationId: ConversationId, + message: MessageData, + ) } internal class ConversationDraftStoreImpl @Inject constructor() : ConversationDraftStore { @@ -55,4 +64,42 @@ internal class ConversationDraftStoreImpl @Inject constructor() : ConversationDr BugleDatabaseOperations.UPDATE_MODE_ADD_DRAFT, ) } + + override fun moveDraftMessage( + fromConversationId: ConversationId, + toConversationId: ConversationId, + message: MessageData, + ) { + val database = DataModel.get().database + + database.withTransaction { + checkNotNull( + BugleDatabaseOperations.updateDraftMessageData( + database, + toConversationId.value, + message, + BugleDatabaseOperations.UPDATE_MODE_ADD_DRAFT, + ), + ) { + "Draft was not written to conversation ${toConversationId.value}" + } + + // Clearing through updateDraftMessageData would delete the scratch files of the + // moved attachments, so drop the source draft row first and only reset its snippet + database.delete( + DatabaseHelper.MESSAGES_TABLE, + "${MessageColumns.STATUS}=? AND ${MessageColumns.CONVERSATION_ID}=?", + arrayOf( + MessageData.BUGLE_STATUS_OUTGOING_DRAFT.toString(), + fromConversationId.value, + ), + ) + BugleDatabaseOperations.updateDraftMessageData( + database, + fromConversationId.value, + null, + BugleDatabaseOperations.UPDATE_MODE_CLEAR_DRAFT, + ) + } + } } diff --git a/src/com/android/messaging/data/extension/DatabaseTransactions.kt b/src/com/android/messaging/data/extension/DatabaseTransactions.kt new file mode 100644 index 000000000..f29466a5f --- /dev/null +++ b/src/com/android/messaging/data/extension/DatabaseTransactions.kt @@ -0,0 +1,8 @@ +package com.android.messaging.data.extension + +import com.android.messaging.datamodel.DataModel +import com.android.messaging.util.db.ext.withTransaction + +internal fun runInTransaction(block: () -> Unit) { + DataModel.get().database.withTransaction(block) +} diff --git a/src/com/android/messaging/di/conversation/ConversationLaunchModule.kt b/src/com/android/messaging/di/conversation/ConversationLaunchModule.kt index ebe797f0c..c0a92f268 100644 --- a/src/com/android/messaging/di/conversation/ConversationLaunchModule.kt +++ b/src/com/android/messaging/di/conversation/ConversationLaunchModule.kt @@ -4,6 +4,8 @@ import com.android.messaging.data.conversation.event.ConversationArchiveEvents import com.android.messaging.data.conversation.event.ConversationArchiveEventsImpl import com.android.messaging.domain.conversation.usecase.action.ArchiveConversation import com.android.messaging.domain.conversation.usecase.action.ArchiveConversationImpl +import com.android.messaging.ui.conversation.composer.delegate.ConversationDraftTransfers +import com.android.messaging.ui.conversation.composer.delegate.ConversationDraftTransfersImpl import com.android.messaging.ui.conversation.entry.ConversationLaunchStore import com.android.messaging.ui.conversation.entry.ConversationLaunchStoreImpl import com.android.messaging.ui.conversation.navigation.ConversationDraftLauncher @@ -39,4 +41,10 @@ internal abstract class ConversationLaunchModule { abstract fun bindConversationDraftLauncher( impl: ConversationLaunchStoreImpl, ): ConversationDraftLauncher + + @Binds + @ActivityRetainedScoped + abstract fun bindConversationDraftTransfers( + impl: ConversationDraftTransfersImpl, + ): ConversationDraftTransfers } diff --git a/src/com/android/messaging/ui/conversation/addparticipants/AddParticipantsViewModel.kt b/src/com/android/messaging/ui/conversation/addparticipants/AddParticipantsViewModel.kt index 212d373a2..cfe57be67 100644 --- a/src/com/android/messaging/ui/conversation/addparticipants/AddParticipantsViewModel.kt +++ b/src/com/android/messaging/ui/conversation/addparticipants/AddParticipantsViewModel.kt @@ -13,6 +13,7 @@ import com.android.messaging.domain.conversation.usecase.participant.IsConversat import com.android.messaging.ui.conversation.addparticipants.model.AddParticipantsEffect import com.android.messaging.ui.conversation.addparticipants.model.AddParticipantsNavEvent import com.android.messaging.ui.conversation.addparticipants.model.AddParticipantsUiState +import com.android.messaging.ui.conversation.composer.delegate.ConversationDraftTransfers import com.android.messaging.ui.conversation.recipientpicker.delegate.ConversationResolutionDelegate import com.android.messaging.ui.conversation.recipientpicker.delegate.SelectedRecipientsDelegate import com.android.messaging.ui.conversation.recipientpicker.model.picker.ConversationResolutionOutcome @@ -22,6 +23,7 @@ import com.android.messaging.ui.recipientselection.delegate.RecipientPickerDeleg import com.android.messaging.ui.recipientselection.model.picker.RecipientPickerUiState import com.android.messaging.ui.recipientselection.model.picker.SelectedRecipient import com.android.messaging.ui.recipientselection.model.picker.sanitizedOrNull +import com.android.messaging.util.LogUtil import dagger.hilt.android.lifecycle.HiltViewModel import javax.inject.Inject import kotlinx.collections.immutable.ImmutableList @@ -30,6 +32,7 @@ import kotlinx.collections.immutable.PersistentList import kotlinx.collections.immutable.persistentListOf import kotlinx.collections.immutable.persistentSetOf import kotlinx.collections.immutable.toImmutableSet +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.channels.Channel import kotlinx.coroutines.flow.Flow @@ -57,6 +60,7 @@ internal interface AddParticipantsScreenModel { @HiltViewModel internal class AddParticipantsViewModel @Inject constructor( private val contactDestinationFormatter: ContactDestinationFormatter, + private val conversationDraftTransfers: ConversationDraftTransfers, private val conversationParticipantsRepository: ConversationParticipantsRepository, private val isConversationRecipientLimitExceeded: IsConversationRecipientLimitExceeded, private val recipientPickerDelegate: RecipientPickerDelegate, @@ -127,6 +131,7 @@ internal class AddParticipantsViewModel @Inject constructor( existingParticipants = persistentEmptyParticipants(), existingParticipantCanonicalDestinations = persistentSetOf(), isLoadingConversationParticipants = conversationId != null, + isOpeningConversation = false, ) recipientPickerDelegate.onExcludedDestinationsChanged( destinations = emptySet(), @@ -227,6 +232,8 @@ internal class AddParticipantsViewModel @Inject constructor( return } + // Busy from here until the conversation opens, so picks made meanwhile cannot be lost + setOpeningConversation(isOpeningConversation = true) conversationResolutionDelegate.resolve(destinations = allDestinations) } @@ -239,7 +246,8 @@ internal class AddParticipantsViewModel @Inject constructor( return AddParticipantsUiState( existingParticipants = localState.existingParticipants, isLoadingConversationParticipants = localState.isLoadingConversationParticipants, - isResolvingConversation = resolutionState is ConversationResolutionState.Resolving, + isResolvingConversation = localState.isOpeningConversation || + resolutionState is ConversationResolutionState.Resolving, recipientPickerUiState = recipientPickerUiState, selectedRecipients = selectedRecipients, ) @@ -252,15 +260,11 @@ internal class AddParticipantsViewModel @Inject constructor( .collect { outcome -> when (outcome) { is ConversationResolutionOutcome.Resolved -> { - selectedRecipientsDelegate.clear() - navigationEventsChannel.trySend( - AddParticipantsNavEvent.OpenConversation( - conversationId = outcome.conversationId, - ), - ) + openConversationWithDraft(conversationId = outcome.conversationId) } ConversationResolutionOutcome.Failed -> { + setOpeningConversation(isOpeningConversation = false) showMessage(messageResId = R.string.conversation_creation_failure) } } @@ -268,8 +272,54 @@ internal class AddParticipantsViewModel @Inject constructor( } } + private suspend fun openConversationWithDraft(conversationId: ConversationId) { + when (transferDraftToConversation(conversationId = conversationId)) { + true -> { + selectedRecipientsDelegate.clear() + navigationEventsChannel.trySend( + AddParticipantsNavEvent.OpenConversation(conversationId = conversationId), + ) + } + + // Stay here with the recipients still picked so confirming again retries the move + false -> { + setOpeningConversation(isOpeningConversation = false) + showMessage(messageResId = R.string.conversation_creation_failure) + } + } + } + + @Suppress("TooGenericExceptionCaught") + private suspend fun transferDraftToConversation(conversationId: ConversationId): Boolean { + val currentConversationId = conversationIdFlow.value + + return try { + currentConversationId?.let { fromConversationId -> + conversationDraftTransfers.transferDraft( + fromConversationId = fromConversationId, + toConversationId = conversationId, + ) + } + true + } catch (exception: CancellationException) { + throw exception + } catch (exception: Exception) { + LogUtil.e( + LOG_TAG, + "Failed to move draft to conversation ${conversationId.value}", + exception, + ) + false + } + } + private fun isResolvingConversation(): Boolean { - return conversationResolutionDelegate.state.value is ConversationResolutionState.Resolving + return localUiState.value.isOpeningConversation || + conversationResolutionDelegate.state.value is ConversationResolutionState.Resolving + } + + private fun setOpeningConversation(isOpeningConversation: Boolean) { + localUiState.value = localUiState.value.copy(isOpeningConversation = isOpeningConversation) } private fun showMessage(messageResId: Int) { @@ -294,10 +344,12 @@ internal class AddParticipantsViewModel @Inject constructor( val existingParticipants: ImmutableList = persistentListOf(), val existingParticipantCanonicalDestinations: ImmutableSet = persistentSetOf(), val isLoadingConversationParticipants: Boolean = true, + val isOpeningConversation: Boolean = false, ) private companion object { private const val CONVERSATION_ID_KEY = "conversation_id" + private const val LOG_TAG = "AddParticipantsViewModel" private const val STATEFLOW_STOP_TIMEOUT_MILLIS = 5_000L } } diff --git a/src/com/android/messaging/ui/conversation/composer/delegate/ConversationDraftDelegate.kt b/src/com/android/messaging/ui/conversation/composer/delegate/ConversationDraftDelegate.kt index d7906be52..0d8ea7747 100644 --- a/src/com/android/messaging/ui/conversation/composer/delegate/ConversationDraftDelegate.kt +++ b/src/com/android/messaging/ui/conversation/composer/delegate/ConversationDraftDelegate.kt @@ -40,6 +40,7 @@ import kotlinx.coroutines.flow.asSharedFlow import kotlinx.coroutines.flow.asStateFlow import kotlinx.coroutines.flow.catch import kotlinx.coroutines.flow.collect +import kotlinx.coroutines.flow.collectLatest import kotlinx.coroutines.flow.debounce import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.emitAll @@ -121,6 +122,7 @@ internal class ConversationDraftDelegateImpl @Inject constructor( private val checkConversationActionRequirements: CheckConversationActionRequirements, private val conversationDraftsRepository: ConversationDraftsRepository, private val conversationDraftEditorDelegate: ConversationDraftEditorDelegate, + private val conversationDraftTransfers: ConversationDraftTransfers, private val sendConversationDraft: SendConversationDraft, @param:DefaultDispatcher private val defaultDispatcher: CoroutineDispatcher, @@ -161,6 +163,10 @@ internal class ConversationDraftDelegateImpl @Inject constructor( ) bindDraftAutosave(scope = scope) bindDraftSendProtocol(scope = scope) + bindDraftTransfers( + scope = scope, + conversationIdFlow = conversationIdFlow, + ) } override fun onMessageTextChanged(messageText: String) { @@ -365,6 +371,22 @@ internal class ConversationDraftDelegateImpl @Inject constructor( } } + private suspend fun commitUnsavedDraft(conversationId: ConversationId) { + // Edits made before the stored draft loads are only saved once it has loaded. + // Under the save lock the commit follows any save already in progress, and marking the + // edits persisted stops a later save from writing them back after the draft moves away + conversationDraftEditorDelegate.awaitDraftLoaded(conversationId = conversationId) + draftSaveMutex.withLock { + conversationDraftEditorDelegate.currentSaveRequest?.let { saveRequest -> + conversationDraftsRepository.saveDraft( + conversationId = saveRequest.conversationId, + draft = saveRequest.draft, + ) + conversationDraftEditorDelegate.applyPersistedSaveResult(saveRequest = saveRequest) + } + } + } + private fun bindConversationDraftObservation( scope: CoroutineScope, conversationIdFlow: StateFlow, @@ -400,6 +422,21 @@ internal class ConversationDraftDelegateImpl @Inject constructor( } } + private fun bindDraftTransfers( + scope: CoroutineScope, + conversationIdFlow: StateFlow, + ) { + scope.launch(defaultDispatcher) { + conversationIdFlow.collectLatest { conversationId -> + conversationId?.let { boundConversationId -> + conversationDraftTransfers.commitDraftWhileActive(boundConversationId) { + commitUnsavedDraft(conversationId = boundConversationId) + } + } + } + } + } + private suspend fun resetDraftEditorState(conversationId: ConversationId?) { pendingMessageLimitSendRequest = null _attachmentLimitWarning.value = null diff --git a/src/com/android/messaging/ui/conversation/composer/delegate/ConversationDraftEditorDelegate.kt b/src/com/android/messaging/ui/conversation/composer/delegate/ConversationDraftEditorDelegate.kt index 0137ac62c..cc37e01d4 100644 --- a/src/com/android/messaging/ui/conversation/composer/delegate/ConversationDraftEditorDelegate.kt +++ b/src/com/android/messaging/ui/conversation/composer/delegate/ConversationDraftEditorDelegate.kt @@ -20,6 +20,7 @@ import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow import kotlinx.coroutines.flow.debounce import kotlinx.coroutines.flow.distinctUntilChanged +import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.mapLatest import kotlinx.coroutines.flow.update @@ -74,6 +75,9 @@ internal interface ConversationDraftEditorDelegate { fun applyPersistedSaveResult(saveRequest: DraftSaveRequest) + /** Returns once the stored draft of [conversationId] has loaded or the editor moved on. */ + suspend fun awaitDraftLoaded(conversationId: ConversationId) + fun applySendProtocol(sendProtocol: ConversationDraftSendProtocol) fun createSendRequestOrNull(): DraftSendRequest? @@ -291,6 +295,13 @@ internal class ConversationDraftEditorDelegateImpl @Inject constructor( } } + override suspend fun awaitDraftLoaded(conversationId: ConversationId) { + draftEditorState.first { currentDraftEditorState -> + currentDraftEditorState.conversationId != conversationId || + currentDraftEditorState.isLoaded + } + } + override fun applySendProtocol(sendProtocol: ConversationDraftSendProtocol) { _state.update { currentState -> currentState.copy( diff --git a/src/com/android/messaging/ui/conversation/composer/delegate/ConversationDraftTransfers.kt b/src/com/android/messaging/ui/conversation/composer/delegate/ConversationDraftTransfers.kt new file mode 100644 index 000000000..1d4769fd2 --- /dev/null +++ b/src/com/android/messaging/ui/conversation/composer/delegate/ConversationDraftTransfers.kt @@ -0,0 +1,61 @@ +package com.android.messaging.ui.conversation.composer.delegate + +import com.android.messaging.data.conversation.model.ConversationId +import com.android.messaging.data.conversation.repository.ConversationDraftsRepository +import java.util.concurrent.ConcurrentHashMap +import javax.inject.Inject +import kotlinx.coroutines.awaitCancellation + +internal interface ConversationDraftTransfers { + suspend fun commitDraftWhileActive( + conversationId: ConversationId, + commitDraft: suspend () -> Unit, + ): Nothing + + suspend fun transferDraft( + fromConversationId: ConversationId, + toConversationId: ConversationId, + ) +} + +internal class ConversationDraftTransfersImpl @Inject constructor( + private val conversationDraftsRepository: ConversationDraftsRepository, +) : ConversationDraftTransfers { + + private val draftCommitters = ConcurrentHashMap Unit>>() + + override suspend fun commitDraftWhileActive( + conversationId: ConversationId, + commitDraft: suspend () -> Unit, + ): Nothing { + draftCommitters.merge(conversationId, listOf(commitDraft)) { current, added -> + current + added + } + try { + awaitCancellation() + } finally { + draftCommitters.computeIfPresent(conversationId) { _, current -> + (current - commitDraft).ifEmpty { null } + } + } + } + + override suspend fun transferDraft( + fromConversationId: ConversationId, + toConversationId: ConversationId, + ) { + when (fromConversationId) { + toConversationId -> Unit + + else -> { + draftCommitters[fromConversationId].orEmpty().forEach { commitDraft -> + commitDraft() + } + conversationDraftsRepository.moveDraft( + fromConversationId = fromConversationId, + toConversationId = toConversationId, + ) + } + } + } +}