From e6c6b7040c9ed6a1cce1f3948c5bf792d2e66c12 Mon Sep 17 00:00:00 2001 From: Artem Smirnov Date: Fri, 28 Aug 2026 11:09:30 +0300 Subject: [PATCH 01/26] Fall back to full name for the snippet sender without first name --- .../datamodel/DatabaseUpgradeHelperTest.kt | 64 +++++++ ...nversationListItemDataSnippetSenderTest.kt | 170 ++++++++++++++++++ res/values/versions.xml | 2 +- .../datamodel/DatabaseUpgradeHelper.java | 9 + .../data/ConversationListItemData.java | 10 ++ 5 files changed, 254 insertions(+), 1 deletion(-) create mode 100644 app/src/test/kotlin/com/android/messaging/datamodel/data/ConversationListItemDataSnippetSenderTest.kt diff --git a/app/src/test/kotlin/com/android/messaging/datamodel/DatabaseUpgradeHelperTest.kt b/app/src/test/kotlin/com/android/messaging/datamodel/DatabaseUpgradeHelperTest.kt index 2251d6c72..266335523 100644 --- a/app/src/test/kotlin/com/android/messaging/datamodel/DatabaseUpgradeHelperTest.kt +++ b/app/src/test/kotlin/com/android/messaging/datamodel/DatabaseUpgradeHelperTest.kt @@ -2,15 +2,72 @@ package com.android.messaging.datamodel import android.database.Cursor import android.database.sqlite.SQLiteDatabase +import androidx.core.content.contentValuesOf +import com.android.messaging.FactoryTestAccess +import com.android.messaging.R import com.android.messaging.datamodel.DatabaseHelper.ConversationColumns +import com.android.messaging.datamodel.data.ConversationListItemData +import com.android.messaging.testutil.installTestFactory +import io.mockk.unmockkAll +import org.junit.After +import org.junit.Assert.assertEquals import org.junit.Assert.assertTrue +import org.junit.Before import org.junit.Test import org.junit.runner.RunWith import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment @RunWith(RobolectricTestRunner::class) class DatabaseUpgradeHelperTest { + @Before + fun setUp() { + installTestFactory(context = RuntimeEnvironment.getApplication().applicationContext) + } + + @After + fun tearDown() { + unmockkAll() + FactoryTestAccess.reset() + } + + /** + * doUpgradeWithExceptions() throws unless a handler carries the version all the way to the + * current one, and doOnUpgrade() answers that by rebuilding every table - which drops all of + * the user's messages. So every bump of R.string.database_version needs its own handler, even + * a handler that changes no tables because only a view changed. + */ + @Test + fun upgradeFromVersion3_keepsExistingDataAndRebuildsViews() { + val context = RuntimeEnvironment.getApplication().applicationContext + val currentVersion = context.getString(R.string.database_version).toInt() + + SQLiteDatabase.create(null).use { db -> + DatabaseHelper.rebuildTables(db) + db.insert( + DatabaseHelper.CONVERSATIONS_TABLE, + null, + contentValuesOf(ConversationColumns.NAME to "Weekend plan"), + ) + + DatabaseUpgradeHelper().doOnUpgrade(db, 3, currentVersion) + + assertEquals( + "upgrade wiped the conversations table", + 1, + db.countRows(DatabaseHelper.CONVERSATIONS_TABLE), + ) + assertTrue( + "conversation_list_view was not rebuilt", + db.hasColumn( + ConversationListItemData.getConversationListView(), + "snippet_sender_full_name", + ), + ) + } + } + @Test fun upgradeToVersion3_createsPinnedColumnAndIndex() { val table = DatabaseHelper.CONVERSATIONS_TABLE @@ -32,6 +89,13 @@ class DatabaseUpgradeHelperTest { } } + private fun SQLiteDatabase.countRows(table: String): Int { + return rawQuery("SELECT COUNT(*) FROM $table", null).use { cursor -> + cursor.moveToFirst() + cursor.getInt(0) + } + } + private fun SQLiteDatabase.hasIndex(name: String): Boolean { return rawQuery( "SELECT name FROM sqlite_master WHERE type='index' AND name=?", diff --git a/app/src/test/kotlin/com/android/messaging/datamodel/data/ConversationListItemDataSnippetSenderTest.kt b/app/src/test/kotlin/com/android/messaging/datamodel/data/ConversationListItemDataSnippetSenderTest.kt new file mode 100644 index 000000000..fedf0ff72 --- /dev/null +++ b/app/src/test/kotlin/com/android/messaging/datamodel/data/ConversationListItemDataSnippetSenderTest.kt @@ -0,0 +1,170 @@ +package com.android.messaging.datamodel.data + +import android.content.Context +import androidx.core.content.contentValuesOf +import com.android.messaging.FactoryTestAccess +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.MessageColumns +import com.android.messaging.datamodel.DatabaseHelper.ParticipantColumns +import com.android.messaging.datamodel.DatabaseWrapper +import com.android.messaging.datamodel.createInMemoryActionSyncTestDatabase +import com.android.messaging.testutil.installTestFactory +import com.android.messaging.util.NotificationChannelUtil +import io.mockk.every +import io.mockk.mockk +import io.mockk.unmockkAll +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import org.robolectric.annotation.Config + +/** + * The conversation list prefixes a snippet with the name of whoever sent the latest message. + * These cover the fallback order that name resolves through, end to end: the real + * `conversation_list_view` SQL, the real projection, and [ConversationListItemData.bind]. + */ +@RunWith(RobolectricTestRunner::class) +@Config(sdk = [36]) +class ConversationListItemDataSnippetSenderTest { + + private lateinit var context: Context + private lateinit var database: DatabaseWrapper + + @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 + + NotificationChannelUtil.onCreate(context) + } + + @After + fun tearDown() { + unmockkAll() + FactoryTestAccess.reset() + } + + @Test + fun snippetSenderName_prefersFirstName() { + seedConversationWithSender( + firstName = "Zoë", + fullName = "Zoë Zimmermann", + displayDestination = DISPLAY_DESTINATION, + ) + + assertEquals("Zoë", loadConversationListItem().snippetSenderName) + } + + /** + * Regression test for BUG-001. The contacts provider yields no given name for CJK names, + * mononyms, and contacts stored with only a structured family name, so `first_name` is empty + * while `full_name` holds the name the rest of the row already displays. Falling straight + * through to the phone number shows a raw number next to the resolved name in the title, and + * makes TalkBack read a number out for the same contact. + */ + @Test + fun snippetSenderName_fallsBackToFullName_whenContactHasNoFirstName() { + seedConversationWithSender( + firstName = "", + fullName = "李明Wang", + displayDestination = DISPLAY_DESTINATION, + ) + + assertEquals("李明Wang", loadConversationListItem().snippetSenderName) + } + + @Test + fun snippetSenderName_fallsBackToDisplayDestination_whenContactHasNoName() { + seedConversationWithSender( + firstName = "", + fullName = "", + displayDestination = DISPLAY_DESTINATION, + ) + + assertEquals(DISPLAY_DESTINATION, loadConversationListItem().snippetSenderName) + } + + private fun seedConversationWithSender( + firstName: String, + fullName: String, + displayDestination: String, + ) { + val participantId = database.insert( + DatabaseHelper.PARTICIPANTS_TABLE, + null, + contentValuesOf( + ParticipantColumns.NORMALIZED_DESTINATION to NORMALIZED_DESTINATION, + ParticipantColumns.SEND_DESTINATION to NORMALIZED_DESTINATION, + ParticipantColumns.DISPLAY_DESTINATION to displayDestination, + ParticipantColumns.FULL_NAME to fullName, + ParticipantColumns.FIRST_NAME to firstName, + ), + ) + assertTrue("participant insert failed", participantId >= 0) + + val conversationId = database.insert( + DatabaseHelper.CONVERSATIONS_TABLE, + null, + contentValuesOf( + ConversationColumns.NAME to CONVERSATION_NAME, + ConversationColumns.PARTICIPANT_COUNT to 2, + ConversationColumns.SNIPPET_TEXT to SNIPPET_TEXT, + ), + ) + assertTrue("conversation insert failed", conversationId >= 0) + + val messageId = database.insert( + DatabaseHelper.MESSAGES_TABLE, + null, + contentValuesOf( + MessageColumns.CONVERSATION_ID to conversationId, + MessageColumns.SENDER_PARTICIPANT_ID to participantId, + MessageColumns.SELF_PARTICIPANT_ID to participantId, + MessageColumns.STATUS to MessageData.BUGLE_STATUS_INCOMING_COMPLETE, + MessageColumns.RECEIVED_TIMESTAMP to RECEIVED_TIMESTAMP_MILLIS, + ), + ) + assertTrue("message insert failed", messageId >= 0) + + database.update( + DatabaseHelper.CONVERSATIONS_TABLE, + contentValuesOf(ConversationColumns.LATEST_MESSAGE_ID to messageId), + "${ConversationColumns._ID}=?", + arrayOf(conversationId.toString()), + ) + } + + private fun loadConversationListItem(): ConversationListItemData { + return database.query( + ConversationListItemData.getConversationListView(), + ConversationListItemData.PROJECTION, + null, + null, + null, + null, + null, + ).use { cursor -> + assertTrue("conversation_list_view returned no rows", cursor.moveToFirst()) + ConversationListItemData().apply { bind(cursor) } + } + } + + private companion object { + private const val NORMALIZED_DESTINATION = "+37255500004" + private const val DISPLAY_DESTINATION = "+372 5550 0004" + private const val CONVERSATION_NAME = "Zoë, 李明Wang, Bob" + private const val SNIPPET_TEXT = "对我来说没问题" + private const val RECEIVED_TIMESTAMP_MILLIS = 1_780_920_000_000L + } +} diff --git a/res/values/versions.xml b/res/values/versions.xml index a02137223..8334be238 100644 --- a/res/values/versions.xml +++ b/res/values/versions.xml @@ -16,7 +16,7 @@ --> - 3 + 4 Enter a contact name or phone number to start a new message + + No contacts found + Block diff --git a/src/com/android/messaging/ui/recipientselection/component/RecipientSelectionContactsContent.kt b/src/com/android/messaging/ui/recipientselection/component/RecipientSelectionContactsContent.kt index 698f8290a..e1ae7bc7f 100644 --- a/src/com/android/messaging/ui/recipientselection/component/RecipientSelectionContactsContent.kt +++ b/src/com/android/messaging/ui/recipientselection/component/RecipientSelectionContactsContent.kt @@ -34,6 +34,7 @@ import androidx.compose.ui.res.stringResource import androidx.compose.ui.text.style.TextAlign import androidx.compose.ui.tooling.preview.PreviewLightDark import androidx.compose.ui.unit.dp +import com.android.messaging.R import com.android.messaging.ui.common.components.PrimaryActionButton import com.android.messaging.ui.common.components.bottomBarInsets import com.android.messaging.ui.common.components.selection.SelectionListContent @@ -142,7 +143,12 @@ private fun LazyListScope.recipientSelectionContactItems( pickerUiState.items.isEmpty() -> { item { - RecipientSelectionEmptyState(text = emptyStateText) + RecipientSelectionEmptyState( + text = when { + pickerUiState.query.isBlank() -> emptyStateText + else -> R.string.recipient_picker_no_results_text + }, + ) } } From ca303dc80fe2aa6b38bbe85c1c7d1095bcc563ef Mon Sep 17 00:00:00 2001 From: Artem Smirnov Date: Sat, 29 Aug 2026 00:42:12 +0300 Subject: [PATCH 09/26] Skip snoozed conversations when posting message notifications The only snooze check guarded the conversation id passed by the caller, so it was bypassed entirely whenever that id was null -- which is what the boot, app-update and redownload paths pass. Every reboot re-notified conversations snoozed "Always". A snoozed conversation could also consume the single notification slot and silence the conversation that had actually received a message. Filter snooze per conversation in the selection stream, where the blocked check already lives, instead of relying on the caller's argument. The guard in update() stays for its cancel() side effect. --- ...gleNotificationsBlockedConversationTest.kt | 7 ++ ...gleNotificationsSnoozedConversationTest.kt | 110 ++++++++++++++++++ .../datamodel/BugleNotifications.java | 2 + 3 files changed, 119 insertions(+) create mode 100644 app/src/test/kotlin/com/android/messaging/datamodel/BugleNotificationsSnoozedConversationTest.kt diff --git a/app/src/test/kotlin/com/android/messaging/datamodel/BugleNotificationsBlockedConversationTest.kt b/app/src/test/kotlin/com/android/messaging/datamodel/BugleNotificationsBlockedConversationTest.kt index 8a784eb23..c83a3716b 100644 --- a/app/src/test/kotlin/com/android/messaging/datamodel/BugleNotificationsBlockedConversationTest.kt +++ b/app/src/test/kotlin/com/android/messaging/datamodel/BugleNotificationsBlockedConversationTest.kt @@ -3,6 +3,7 @@ package com.android.messaging.datamodel import android.media.AudioManager import android.net.Uri import com.android.messaging.FactoryTestAccess +import com.android.messaging.data.conversationsettings.repository.ConversationSnoozeQuery import com.android.messaging.datamodel.data.ConversationListItemData import com.android.messaging.testutil.installTestFactory import com.android.messaging.util.RingtoneUtil @@ -35,6 +36,7 @@ class BugleNotificationsBlockedConversationTest { every { dataModel.getDatabase() } returns database silenceRinger() stubConversationLookup() + stubSnoozeLookup() stubNotificationDelivery() } @@ -120,6 +122,11 @@ class BugleNotificationsBlockedConversationTest { } returns convData } + private fun stubSnoozeLookup() { + mockkStatic(ConversationSnoozeQuery::class) + every { ConversationSnoozeQuery.isConversationSnoozed(any()) } returns false + } + private fun stubNotificationDelivery() { mockkStatic(MessageNotificationState::class) mockkStatic(RingtoneUtil::class) diff --git a/app/src/test/kotlin/com/android/messaging/datamodel/BugleNotificationsSnoozedConversationTest.kt b/app/src/test/kotlin/com/android/messaging/datamodel/BugleNotificationsSnoozedConversationTest.kt new file mode 100644 index 000000000..6a7792672 --- /dev/null +++ b/app/src/test/kotlin/com/android/messaging/datamodel/BugleNotificationsSnoozedConversationTest.kt @@ -0,0 +1,110 @@ +package com.android.messaging.datamodel + +import com.android.messaging.FactoryTestAccess +import com.android.messaging.data.conversationsettings.repository.ConversationSnoozeQuery +import com.android.messaging.datamodel.data.ConversationListItemData +import com.android.messaging.testutil.installTestFactory +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 org.junit.After +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment + +/** + * Regression test for BUG-014. + * + * A snoozed conversation must post no notification, and its unseen messages must not stop another + * conversation from posting one. The only snooze check used to sit in [BugleNotifications.update], + * where it guards the caller's argument -- so it was bypassed entirely whenever the caller passed + * no conversation id, as the boot and app-update paths do. + */ +@RunWith(RobolectricTestRunner::class) +class BugleNotificationsSnoozedConversationTest { + + private val database = mockk(relaxed = true) + private val dataModel = mockk(relaxed = true) + + @Before + fun setUp() { + installTestFactory( + context = RuntimeEnvironment.getApplication().applicationContext, + dataModel = dataModel, + ) + every { dataModel.getDatabase() } returns database + stubConversationLookup() + stubSnoozeLookup() + mockkStatic(MessageNotificationState::class) + mockkStatic(BugleNotifications::class) + every { BugleNotifications.processAndSend(any(), any()) } just runs + } + + @After + fun tearDown() { + unmockkAll() + FactoryTestAccess.reset() + } + + @Test + fun createMessageNotification_whenSnoozedConversationIsNewest_notifiesTheOtherOne() { + // The device repro: the snoozed conversation holds the newest unseen message, so it is the + // one the selection lands on -- and the conversation that actually received a message + // stays silent. + val snoozed = createNotificationConversation(SNOOZED_CONVERSATION_ID) + val allowed = createNotificationConversation(ALLOWED_CONVERSATION_ID) + givenUnseenMessages(snoozed, allowed) + + BugleNotifications.createMessageNotification(ALLOWED_CONVERSATION_ID) + + verify(exactly = 1) { BugleNotifications.processAndSend(any(), allowed) } + verify(exactly = 0) { BugleNotifications.processAndSend(any(), snoozed) } + } + + @Test + fun createMessageNotification_withoutConversationId_skipsSnoozedConversation() { + // Boot and app update pass no conversation id, which bypasses the guard in update(). + val snoozed = createNotificationConversation(SNOOZED_CONVERSATION_ID) + givenUnseenMessages(snoozed) + + BugleNotifications.createMessageNotification(null) + + verify(exactly = 0) { BugleNotifications.processAndSend(any(), any()) } + } + + private fun stubConversationLookup() { + mockkStatic(ConversationListItemData::class) + every { ConversationListItemData.getExistingConversation(database, any()) } returns null + } + + private fun stubSnoozeLookup() { + mockkStatic(ConversationSnoozeQuery::class) + every { ConversationSnoozeQuery.isConversationSnoozed(any()) } returns false + every { + ConversationSnoozeQuery.isConversationSnoozed(SNOOZED_CONVERSATION_ID) + } returns true + } + + private fun givenUnseenMessages( + vararg conversations: MessageNotificationState.Conversation, + ) { + val conversationsList = MessageNotificationState.ConversationsList( + conversations.size, + conversations.toList(), + ) + every { + MessageNotificationState.getNotificationState() + } returns MessageNotificationState(conversationsList) + } + + private companion object { + private const val SNOOZED_CONVERSATION_ID = "194" + private const val ALLOWED_CONVERSATION_ID = "195" + } +} diff --git a/src/com/android/messaging/datamodel/BugleNotifications.java b/src/com/android/messaging/datamodel/BugleNotifications.java index 5dbb77e0d..b49f47999 100644 --- a/src/com/android/messaging/datamodel/BugleNotifications.java +++ b/src/com/android/messaging/datamodel/BugleNotifications.java @@ -174,6 +174,8 @@ static void createMessageNotification(final String conversationId) { // Send per-conversation notifications (if there are multiple conversations). Optional conversation = state.mConversationsList.mConversations.stream() .filter(conv -> !isConversationBlocked(conv.mConversationId)) + .filter(conv -> !ConversationSnoozeQuery.isConversationSnoozed( + conv.mConversationId)) .findFirst(); conversation.ifPresent(conv -> processAndSend(state, conv)); } From 7d522a0a249fc850d51e56eddf6a2f35dcaae5e4 Mon Sep 17 00:00:00 2001 From: Artem Smirnov Date: Sat, 29 Aug 2026 00:42:52 +0300 Subject: [PATCH 10/26] Give each clear-notification PendingIntent a stable identity setIdentifier(currentTimeMillis()) made every clear intent unique, so the platform minted a fresh PendingIntent on each notification update instead of reusing one per conversation. Uniqueness has to come from the intent itself: PendingIntent identity is (requestCode, filterEquals), the request code is constant across conversations, and filterEquals ignores extras -- so the conversation id set, which lives only in an extra, cannot distinguish them. Set the data URI to the conversation's metadata URI instead. Dismissing one conversation's notification then marks only that conversation as seen. --- src/com/android/messaging/ui/UIIntentsImpl.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/com/android/messaging/ui/UIIntentsImpl.java b/src/com/android/messaging/ui/UIIntentsImpl.java index 411eeeb5d..8f6cf3293 100644 --- a/src/com/android/messaging/ui/UIIntentsImpl.java +++ b/src/com/android/messaging/ui/UIIntentsImpl.java @@ -257,11 +257,12 @@ public PendingIntent getPendingIntentForClearingNotifications(final Context cont if (conversationIdSet != null) { intent.putExtra(UI_INTENT_EXTRA_CONVERSATION_ID_SET, conversationIdSet.getDelimitedString()); + // Ensure that the platform doesn't reuse PendingIntents across conversations: the id + // set only lives in an extra, which filterEquals() ignores + intent.setData(MessagingContentProvider.buildConversationMetadataUri( + conversationIdSet.first())); } - // We can have several pending intents for clearing conversations so we need each to be unique - intent.setIdentifier(Long.toString(System.currentTimeMillis())); - return PendingIntent.getBroadcast(context, requestCode, intent, PendingIntent.FLAG_IMMUTABLE); From 8be6546c880395b5089e5690504cce37910eabc4 Mon Sep 17 00:00:00 2001 From: Artem Smirnov Date: Sat, 29 Aug 2026 00:45:39 +0300 Subject: [PATCH 11/26] Notify every conversation with unseen messages, not just the newest The conversations list holds every conversation with unseen incoming messages, ordered newest first. Taking findFirst() from it notified whichever conversation happened to hold the globally-newest unseen message and silently dropped the rest, with no summary notification to surface them -- so a reboot with three unread threads announced exactly one. It also ignored the conversation id the update was triggered for, so an arriving message could be passed over in favour of an unrelated newer thread. Iterate the list instead. The early returns in processAndSend already diff against the active notification, so untouched conversations do not re-alert. --- ...eNotificationsMultipleConversationsTest.kt | 206 ++++++++++++++++++ res/values/strings.xml | 5 + .../datamodel/BugleNotifications.java | 48 +++- 3 files changed, 254 insertions(+), 5 deletions(-) create mode 100644 app/src/test/kotlin/com/android/messaging/datamodel/BugleNotificationsMultipleConversationsTest.kt diff --git a/app/src/test/kotlin/com/android/messaging/datamodel/BugleNotificationsMultipleConversationsTest.kt b/app/src/test/kotlin/com/android/messaging/datamodel/BugleNotificationsMultipleConversationsTest.kt new file mode 100644 index 000000000..015326b39 --- /dev/null +++ b/app/src/test/kotlin/com/android/messaging/datamodel/BugleNotificationsMultipleConversationsTest.kt @@ -0,0 +1,206 @@ +package com.android.messaging.datamodel + +import androidx.core.app.NotificationManagerCompat +import com.android.messaging.FactoryTestAccess +import com.android.messaging.data.conversationsettings.repository.ConversationSnoozeQuery +import com.android.messaging.datamodel.data.ConversationListItemData +import com.android.messaging.testutil.installTestFactory +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 org.junit.After +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment + +@RunWith(RobolectricTestRunner::class) +class BugleNotificationsMultipleConversationsTest { + + private val database = mockk(relaxed = true) + private val dataModel = mockk(relaxed = true) + + @Before + fun setUp() { + installTestFactory( + context = RuntimeEnvironment.getApplication().applicationContext, + dataModel = dataModel, + ) + every { dataModel.getDatabase() } returns database + stubConversationLookup() + stubSnoozeLookup() + mockkStatic(MessageNotificationState::class) + mockkStatic(BugleNotifications::class) + every { BugleNotifications.processAndSend(any(), any()) } just runs + } + + @After + fun tearDown() { + unmockkAll() + FactoryTestAccess.reset() + } + + @Test + fun createMessageNotification_withSeveralUnseenConversations_notifiesEveryConversation() { + val newest = createNotificationConversation("18") + val older = createNotificationConversation("19") + val oldest = createNotificationConversation("23") + givenUnseenMessages(newest, older, oldest) + + BugleNotifications.createMessageNotification(newest.mConversationId) + + verify(exactly = 1) { BugleNotifications.processAndSend(any(), newest) } + verify(exactly = 1) { BugleNotifications.processAndSend(any(), older) } + verify(exactly = 1) { BugleNotifications.processAndSend(any(), oldest) } + } + + @Test + fun createMessageNotification_forOlderConversation_stillNotifiesThatConversation() { + // The MMS-download and delete-conversation paths call update() for a conversation that is + // not the newest unseen one. That conversation is the whole point of the call. + val newest = createNotificationConversation("18") + val downloaded = createNotificationConversation("19") + givenUnseenMessages(newest, downloaded) + + BugleNotifications.createMessageNotification(downloaded.mConversationId) + + verify(exactly = 1) { BugleNotifications.processAndSend(any(), downloaded) } + } + + @Test + fun createMessageNotification_withBlockedConversationAmongUnseen_notifiesTheRest() { + val newest = createNotificationConversation("18") + val blocked = createNotificationConversation(BLOCKED_CONVERSATION_ID) + val oldest = createNotificationConversation("23") + givenUnseenMessages(newest, blocked, oldest) + + BugleNotifications.createMessageNotification(newest.mConversationId) + + verify(exactly = 1) { BugleNotifications.processAndSend(any(), newest) } + verify(exactly = 1) { BugleNotifications.processAndSend(any(), oldest) } + verify(exactly = 0) { BugleNotifications.processAndSend(any(), blocked) } + verify(exactly = 2) { BugleNotifications.processAndSend(any(), any()) } + } + + @Test + fun createMessageNotification_withSnoozedConversationAmongUnseen_notifiesTheRest() { + // update() only rejects a snoozed *argument*; nothing filtered the list itself, so + // iterating it would notify every snoozed conversation on every update (BUG-014). + val newest = createNotificationConversation("18") + val snoozed = createNotificationConversation(SNOOZED_CONVERSATION_ID) + givenUnseenMessages(newest, snoozed) + + BugleNotifications.createMessageNotification(newest.mConversationId) + + verify(exactly = 1) { BugleNotifications.processAndSend(any(), newest) } + verify(exactly = 0) { BugleNotifications.processAndSend(any(), snoozed) } + } + + @Test + fun createMessageNotification_withMoreUnseenConversationsThanTheCap_notifiesTheNewestOnly() { + val conversations = (1..BugleNotifications.MAX_CONVERSATION_NOTIFICATIONS + 3) + .map { createNotificationConversation(it.toString()) } + givenUnseenMessages(*conversations.toTypedArray()) + + BugleNotifications.createMessageNotification(conversations.first().mConversationId) + + verify(exactly = BugleNotifications.MAX_CONVERSATION_NOTIFICATIONS) { + BugleNotifications.processAndSend(any(), any()) + } + conversations.take(BugleNotifications.MAX_CONVERSATION_NOTIFICATIONS).forEach { + verify(exactly = 1) { BugleNotifications.processAndSend(any(), it) } + } + assertTrue( + "the conversations over the cap were dropped without a trace", + activeNotificationTags().any { + it.endsWith(BugleNotifications.SMS_OVERFLOW_NOTIFICATION_TAG) + }, + ) + } + + @Test + fun createMessageNotification_withFewerUnseenConversationsThanTheCap_postsNoOverflow() { + givenUnseenMessages(createNotificationConversation("18")) + + BugleNotifications.createMessageNotification("18") + + assertTrue( + "an overflow notification was posted for a single conversation", + activeNotificationTags().none { + it.endsWith(BugleNotifications.SMS_OVERFLOW_NOTIFICATION_TAG) + }, + ) + } + + @Test + fun createMessageNotification_whenUnseenConversationsFallBackUnderTheCap_cancelsTheOverflow() { + val conversations = (1..BugleNotifications.MAX_CONVERSATION_NOTIFICATIONS + 3) + .map { createNotificationConversation(it.toString()) } + givenUnseenMessages(*conversations.toTypedArray()) + BugleNotifications.createMessageNotification(conversations.first().mConversationId) + + givenUnseenMessages(*conversations.take(2).toTypedArray()) + BugleNotifications.createMessageNotification(conversations.first().mConversationId) + + assertTrue( + "the overflow notification outlived the conversations it stood for", + activeNotificationTags().none { + it.endsWith(BugleNotifications.SMS_OVERFLOW_NOTIFICATION_TAG) + }, + ) + } + + private fun activeNotificationTags(): List { + return NotificationManagerCompat.from(RuntimeEnvironment.getApplication()) + .activeNotifications + .map { it.tag } + } + + private fun stubConversationLookup() { + mockkStatic(ConversationListItemData::class) + every { ConversationListItemData.getExistingConversation(database, any()) } returns null + + val blockedData = mockk(relaxed = true) + every { blockedData.otherParticipantNormalizedDestination } returns BLOCKED_SENDER + every { + ConversationListItemData.getExistingConversation(database, BLOCKED_CONVERSATION_ID) + } returns blockedData + + mockkStatic(BugleDatabaseOperations::class) + every { + BugleDatabaseOperations.isBlockedDestination(database, BLOCKED_SENDER) + } returns true + } + + private fun stubSnoozeLookup() { + mockkStatic(ConversationSnoozeQuery::class) + every { ConversationSnoozeQuery.isConversationSnoozed(any()) } returns false + every { + ConversationSnoozeQuery.isConversationSnoozed(SNOOZED_CONVERSATION_ID) + } returns true + } + + private fun givenUnseenMessages( + vararg conversations: MessageNotificationState.Conversation, + ) { + val conversationsList = MessageNotificationState.ConversationsList( + conversations.size, + conversations.toList(), + ) + every { + MessageNotificationState.getNotificationState() + } returns MessageNotificationState(conversationsList) + } + + private companion object { + private const val BLOCKED_CONVERSATION_ID = "193" + private const val BLOCKED_SENDER = "+15551234567" + private const val SNOOZED_CONVERSATION_ID = "194" + } +} diff --git a/res/values/strings.xml b/res/values/strings.xml index e5053a899..23ad1de30 100644 --- a/res/values/strings.xml +++ b/res/values/strings.xml @@ -255,6 +255,11 @@ Download Reply + + + %d more conversation with new messages + %d more conversations with new messages + %d participant diff --git a/src/com/android/messaging/datamodel/BugleNotifications.java b/src/com/android/messaging/datamodel/BugleNotifications.java index b49f47999..6a5a8afc3 100644 --- a/src/com/android/messaging/datamodel/BugleNotifications.java +++ b/src/com/android/messaging/datamodel/BugleNotifications.java @@ -70,7 +70,6 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Map; -import java.util.Optional; /** * Handle posting, updating and removing all conversation notifications.

@@ -97,6 +96,7 @@ public class BugleNotifications { private static final String SMS_NOTIFICATION_TAG = ":sms:"; private static final String SMS_ERROR_NOTIFICATION_TAG = ":error:"; + public static final String SMS_OVERFLOW_NOTIFICATION_TAG = ":sms:overflow:"; /** * Upper bound on the number of {@link Person} entries attached to a conversation notification. @@ -105,6 +105,9 @@ public class BugleNotifications { @VisibleForTesting public static final int MAX_NOTIFICATION_PEOPLE = 25; + @VisibleForTesting + public static final int MAX_CONVERSATION_NOTIFICATIONS = 40; + /** * This is the volume at which to play the observable-conversation notification sound, * expressed as a fraction of the system notification volume. @@ -168,16 +171,51 @@ static void createMessageNotification(final String conversationId) { final Uri ringtoneUri = getNotificationRingtoneUriForConversationId(conversationId); playObservableConversationNotificationSound(ringtoneUri); } + updateOverflowNotification(0); return; } - // Send per-conversation notifications (if there are multiple conversations). - Optional conversation = state.mConversationsList.mConversations.stream() + // Send per-conversation notifications (if there are multiple conversations). The list + // holds every conversation with unseen messages, so it has to be iterated: picking one + // entry notifies whichever conversation holds the newest unseen message and silently + // drops the rest, and there is no summary notification to surface them. + final List notifiable = state.mConversationsList.mConversations.stream() .filter(conv -> !isConversationBlocked(conv.mConversationId)) .filter(conv -> !ConversationSnoozeQuery.isConversationSnoozed( conv.mConversationId)) - .findFirst(); - conversation.ifPresent(conv -> processAndSend(state, conv)); + .toList(); + + notifiable.stream() + .limit(MAX_CONVERSATION_NOTIFICATIONS) + .forEach(conv -> processAndSend(state, conv)); + + updateOverflowNotification(notifiable.size() - MAX_CONVERSATION_NOTIFICATIONS); + } + + private static void updateOverflowNotification(final int overflowCount) { + final Context context = Factory.get().getApplicationContext(); + final NotificationManagerCompat notificationManager = + NotificationManagerCompat.from(context); + final String tag = buildNotificationTag(SMS_OVERFLOW_NOTIFICATION_TAG, null); + if (overflowCount <= 0) { + notificationManager.cancel(tag, PendingIntentConstants.SMS_NOTIFICATION_ID); + return; + } + + final Notification notification = new NotificationCompat.Builder(context, + NotificationChannelUtil.INCOMING_MESSAGES) + .setContentTitle(context.getResources().getQuantityString( + R.plurals.notification_more_conversations, overflowCount, overflowCount)) + .setSmallIcon(R.drawable.ic_sms_light) + .setContentIntent( + UIIntents.get().getPendingIntentForConversationListActivity(context)) + .setCategory(NotificationCompat.CATEGORY_MESSAGE) + .setVisibility(NotificationCompat.VISIBILITY_PRIVATE) + .setOnlyAlertOnce(true) + .setAutoCancel(true) + .build(); + + notificationManager.notify(tag, PendingIntentConstants.SMS_NOTIFICATION_ID, notification); } /** From 7eb5166d478945c0d3d0aa9f82d8e03e1d9a16db Mon Sep 17 00:00:00 2001 From: Artem Smirnov Date: Sat, 29 Aug 2026 11:14:02 +0300 Subject: [PATCH 12/26] Show the MMS image in its notification --- AndroidManifest.xml | 8 +- .../MessageNotificationAttachmentUriTest.kt | 304 ++++++++++++++++++ .../datamodel/BugleNotifications.java | 199 ++++++++++-- .../datamodel/MessageNotificationState.java | 18 +- .../datamodel/NotificationImageProvider.kt | 99 ++++++ .../datamodel/SharedMemoryImageProvider.kt | 80 ----- 6 files changed, 595 insertions(+), 113 deletions(-) create mode 100644 app/src/androidTest/java/com/android/messaging/datamodel/MessageNotificationAttachmentUriTest.kt create mode 100644 src/com/android/messaging/datamodel/NotificationImageProvider.kt delete mode 100644 src/com/android/messaging/datamodel/SharedMemoryImageProvider.kt diff --git a/AndroidManifest.xml b/AndroidManifest.xml index d9daeacee..7743b6565 100644 --- a/AndroidManifest.xml +++ b/AndroidManifest.xml @@ -235,10 +235,10 @@ android:grantUriPermissions="true" android:exported="false" /> - + + val bitmap = Bitmap.createBitmap(IMAGE_WIDTH, IMAGE_HEIGHT, Bitmap.Config.ARGB_8888) + bitmap.eraseColor(Color.BLUE) + bitmap.compress(Bitmap.CompressFormat.JPEG, 100, checkNotNull(out)) + } + val attachmentFile = checkNotNull(MediaScratchFileProvider.getFileFromUri(attachmentUri)) + ExifInterface(attachmentFile.absolutePath).apply { + setAttribute(ExifInterface.TAG_GPS_LATITUDE, "37/1,25/1,19/1") + setAttribute(ExifInterface.TAG_GPS_LATITUDE_REF, "N") + setAttribute(ExifInterface.TAG_GPS_LONGITUDE, "122/1,5/1,4/1") + setAttribute(ExifInterface.TAG_GPS_LONGITUDE_REF, "W") + setAttribute( + ExifInterface.TAG_ORIENTATION, + ExifInterface.ORIENTATION_ROTATE_90.toString(), + ) + saveAttributes() + } + + context.getSystemService(NotificationManager::class.java).createNotificationChannel( + NotificationChannel( + NotificationChannelUtil.INCOMING_MESSAGES, + "Conversations", + NotificationManager.IMPORTANCE_HIGH, + ), + ) + InstrumentationRegistry.getInstrumentation().uiAutomation.grantRuntimePermission( + context.packageName, + Manifest.permission.POST_NOTIFICATIONS, + ) + } + + @After + fun tearDown() { + context.contentResolver.delete(attachmentUri, null, null) + NotificationImageProvider.listImageFiles().forEach { it.delete() } + NotificationManagerCompat.from(context).cancel(NOTIFICATION_TAG, NOTIFICATION_ID) + } + + @Test + fun createStyledMessage_withImageAttachment_attachesAnOpenableUri() { + val message = messageLineInfo(attachmentUri).createStyledMessage(SENDER) + + val dataUri = message.dataUri + assertNotNull("no image was attached to the notification", dataUri) + + val bytes = try { + context.contentResolver.openInputStream(checkNotNull(dataUri)).use { + checkNotNull(it).readBytes() + } + } catch (e: FileNotFoundException) { + throw AssertionError( + "BUG-015: the notification carries $dataUri, which cannot be opened, so the " + + "image never renders in the shade", + e, + ) + } + + assertTrue("the notification image $dataUri opened but is empty", bytes.isNotEmpty()) + } + + @Test + fun createStyledMessage_withImageAttachment_stripsTheSendersMetadata() { + assertNotNull( + "the attachment was seeded without the gps metadata this test looks for", + exifOf(attachmentUri).getAttribute(ExifInterface.TAG_GPS_LATITUDE), + ) + + val dataUri = + checkNotNull(messageLineInfo(attachmentUri).createStyledMessage(SENDER).dataUri) + + assertNotEquals("the sender's own file was attached", attachmentUri, dataUri) + assertTrue( + "no image was attached, so the metadata check below proves nothing", + boundsOf(dataUri).outWidth > 0, + ) + assertNull( + "the notification image still carries the sender's gps metadata", + exifOf(dataUri).getAttribute(ExifInterface.TAG_GPS_LATITUDE), + ) + } + + @Test + fun createStyledMessage_withRotatedImageAttachment_bakesTheOrientationIn() { + val dataUri = messageLineInfo(attachmentUri).createStyledMessage(SENDER).dataUri + + val bounds = boundsOf(checkNotNull(dataUri)) + + assertEquals("the exif rotation was dropped, not applied", IMAGE_HEIGHT, bounds.outWidth) + assertEquals("the exif rotation was dropped, not applied", IMAGE_WIDTH, bounds.outHeight) + } + + /** + * The platform grant-checks every uri a notification carries and, at targetSdk >= P, rethrows + * the [SecurityException] instead of dropping the uri. An image we attach has to survive that. + */ + @Test + fun postNotification_withImageAttachment_posts() { + val posted = post(messageLineInfo(attachmentUri).createStyledMessage(SENDER)) + + assertTrue("the platform refused to grant access to the attached image", posted) + } + + /** + * ...and an image it will not grant has to be dropped rather than thrown out of `notify()`, + * which would otherwise kill the process. `content://call_log` is read-guarded by a permission + * we never hold, so it stands in for an mms part we can no longer read after losing the sms + * role -- the case [BugleNotifications.updateWithInlineReply] hits when it re-posts a + * notification that has been sitting in the shade since before the role changed hands. + */ + @Test + fun postNotification_withUngrantableAttachment_dropsItInsteadOfThrowing() { + val message = MessagingStyle.Message("Check out this photo!", 1L, SENDER) + .setData(ContentType.IMAGE_JPEG, UNGRANTABLE_URI) + + val posted = post(message) + + assertFalse("$UNGRANTABLE_URI was grantable after all, so this proves nothing", posted) + } + + @Test + fun createStyledMessage_withUndecodableAttachment_leavesNoFileBehind() { + val brokenUri = MediaScratchFileProvider.buildMediaScratchSpaceUri(IMAGE_EXTENSION) + context.contentResolver.openOutputStream(brokenUri).use { + checkNotNull(it).write(NOT_AN_IMAGE.toByteArray()) + } + + val dataUri = messageLineInfo(brokenUri).createStyledMessage(SENDER).dataUri + context.contentResolver.delete(brokenUri, null, null) + + assertNull("an attachment that cannot be decoded was attached anyway", dataUri) + assertEquals( + "the failed encode left its placeholder behind", + 0, + NotificationImageProvider.listImageFiles().size, + ) + } + + @Test + fun createStyledMessage_calledTwice_doesNotReuseTheSameFile() { + val first = messageLineInfo(attachmentUri).createStyledMessage(SENDER).dataUri + val second = messageLineInfo(attachmentUri).createStyledMessage(SENDER).dataUri + + assertNotEquals("the same file was handed to two notifications", first, second) + } + + /** + * The sweep runs after every notification pass, so it has to tell an image the shade is still + * showing from one left behind by a notification that has since been replaced or cancelled. + */ + @Test + fun sweepNotificationImages_keepsPostedImagesAndDeletesOrphans() { + val orphan = imageFileOf(NotificationImageProvider.buildNotificationImageUri()) + val message = messageLineInfo(attachmentUri).createStyledMessage(SENDER) + val posted = imageFileOf(message.dataUri) + assertTrue("the platform refused to grant access to the attached image", post(message)) + awaitPostedNotification() + val earlier = System.currentTimeMillis() - PREVIOUS_PASS_AGE_MILLIS + assertTrue(orphan.setLastModified(earlier)) + assertTrue(posted.setLastModified(earlier)) + + BugleNotifications.sweepNotificationImages(System.currentTimeMillis()) + + assertTrue("the image the shade is showing was swept", posted.exists()) + assertFalse("the orphaned image survived the sweep", orphan.exists()) + } + + @Test + fun sweepNotificationImages_keepsImagesFromTheCurrentPass() { + val passStart = System.currentTimeMillis() + val styled = messageLineInfo(attachmentUri).createStyledMessage(SENDER) + val image = imageFileOf(styled.dataUri) + + BugleNotifications.sweepNotificationImages(passStart) + + assertTrue( + "an image written during the pass was swept before it was posted", + image.exists(), + ) + } + + private fun imageFileOf(uri: Uri?): File { + return checkNotNull(NotificationImageProvider.getFileFromUri(uri)) + } + + private fun awaitPostedNotification() { + val manager = context.getSystemService(NotificationManager::class.java) + val deadline = System.currentTimeMillis() + POST_TIMEOUT_MILLIS + while (System.currentTimeMillis() < deadline) { + if (manager.activeNotifications.any { it.tag == NOTIFICATION_TAG }) { + return + } + Thread.sleep(POST_POLL_MILLIS) + } + throw AssertionError("the notification never reached the shade") + } + + private fun boundsOf(uri: Uri): BitmapFactory.Options { + val bounds = BitmapFactory.Options().apply { inJustDecodeBounds = true } + context.contentResolver.openInputStream(uri).use { + BitmapFactory.decodeStream(it, null, bounds) + } + return bounds + } + + private fun exifOf(uri: Uri): ExifInterface { + return context.contentResolver.openInputStream(uri).use { + ExifInterface(checkNotNull(it)) + } + } + + private fun post(message: MessagingStyle.Message): Boolean { + val style = MessagingStyle(SELF).also { it.addMessage(message) } + val notification = + NotificationCompat.Builder(context, NotificationChannelUtil.INCOMING_MESSAGES) + .setSmallIcon(R.drawable.ic_sms_light) + .setStyle(style) + .build() + + return BugleNotifications.postNotification( + NotificationManagerCompat.from(context), + NOTIFICATION_TAG, + NOTIFICATION_ID, + notification, + ) + } + + private fun messageLineInfo(uri: Uri): MessageNotificationState.MessageLineInfo { + return MessageNotificationState.MessageLineInfo( + // authorId, authorFullName, authorFirstName + "author", + "Sender", + "Sender", + // text, attachmentUri, attachmentType + "Check out this photo!", + uri, + ContentType.IMAGE_JPEG, + // isManualDownloadNeeded, avatarUri, messageId, timestamp, contactUriString + false, + null, + "1", + 1L, + null, + ) + } + + private companion object { + const val IMAGE_WIDTH = 8 + const val IMAGE_HEIGHT = 16 + const val IMAGE_EXTENSION = "jpg" + const val NOT_AN_IMAGE = "this is not an image" + const val PREVIOUS_PASS_AGE_MILLIS = 60_000L + const val POST_TIMEOUT_MILLIS = 5_000L + const val POST_POLL_MILLIS = 50L + const val NOTIFICATION_ID = 0x7103 + const val NOTIFICATION_TAG = "BUG-015" + val UNGRANTABLE_URI: Uri = Uri.parse("content://call_log/calls/1") + val SELF: Person = Person.Builder().setName("Me").build() + val SENDER: Person = Person.Builder().setName("Sender").setKey("author").build() + } +} diff --git a/src/com/android/messaging/datamodel/BugleNotifications.java b/src/com/android/messaging/datamodel/BugleNotifications.java index 6a5a8afc3..e4fb86111 100644 --- a/src/com/android/messaging/datamodel/BugleNotifications.java +++ b/src/com/android/messaging/datamodel/BugleNotifications.java @@ -21,9 +21,12 @@ import android.content.Context; import android.content.Intent; import android.graphics.Bitmap; +import android.graphics.Matrix; import android.media.AudioManager; import android.net.Uri; +import android.service.notification.StatusBarNotification; import android.text.TextUtils; +import android.text.format.DateUtils; import androidx.annotation.VisibleForTesting; import androidx.core.app.NotificationCompat; @@ -50,6 +53,7 @@ import com.android.messaging.datamodel.media.ImageResource; import com.android.messaging.datamodel.media.MediaRequest; import com.android.messaging.datamodel.media.MediaResourceManager; +import com.android.messaging.datamodel.media.UriImageRequestDescriptor; import com.android.messaging.sms.MmsSmsUtils; import com.android.messaging.sms.MmsUtils; import com.android.messaging.ui.UIIntents; @@ -64,12 +68,20 @@ import com.android.messaging.util.RingtoneUtil; import com.android.messaging.util.ThreadUtil; import com.android.messaging.util.UriUtil; +import com.android.messaging.util.exif.ExifInterface; +import java.io.File; +import java.io.FileOutputStream; +import java.io.IOException; +import java.io.OutputStream; import java.util.ArrayList; import java.util.HashMap; +import java.util.HashSet; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.Set; +import java.util.concurrent.atomic.AtomicLong; /** * Handle posting, updating and removing all conversation notifications.

@@ -108,6 +120,15 @@ public class BugleNotifications { @VisibleForTesting public static final int MAX_CONVERSATION_NOTIFICATIONS = 40; + private static final int NOTIFICATION_IMAGE_MAX_SIZE = 1024; + private static final int NOTIFICATION_IMAGE_QUALITY = 85; + private static final long NOTIFICATION_IMAGE_SWEEP_GRACE_MILLIS = + 30 * DateUtils.SECOND_IN_MILLIS; + private static final long NOTIFICATION_IMAGE_SWEEP_INTERVAL_MILLIS = + 5 * DateUtils.MINUTE_IN_MILLIS; + + private static final AtomicLong sLastNotificationImageSweep = new AtomicLong(); + /** * This is the volume at which to play the observable-conversation notification sound, * expressed as a fraction of the system notification volume. @@ -136,24 +157,79 @@ public static void update(final String conversationId, final int coverage) { LogUtil.v(TAG, "Update: conversationId = " + conversationId + " coverage = " + coverage); } - Assert.isNotMainThread(); - - if (!PhoneUtils.getDefault().isDefaultSmsApp()) { - LogUtil.d(TAG, "Skipping notification: not the default SMS app"); - cancel(PendingIntentConstants.SMS_NOTIFICATION_ID); - return; - } - if (conversationId != null - && ConversationSnoozeQuery.isConversationSnoozed(conversationId)) { - LogUtil.d(TAG, "Skipping notification: conversation snoozed, id=" + conversationId); - cancel(PendingIntentConstants.SMS_NOTIFICATION_ID, conversationId); - return; + Assert.isNotMainThread(); + + final long passStart = System.currentTimeMillis(); + try { + if (!PhoneUtils.getDefault().isDefaultSmsApp()) { + LogUtil.d(TAG, "Skipping notification: not the default SMS app"); + cancel(PendingIntentConstants.SMS_NOTIFICATION_ID); + return; + } + if (conversationId != null + && ConversationSnoozeQuery.isConversationSnoozed(conversationId)) { + LogUtil.d(TAG, "Skipping notification: conversation snoozed, id=" + + conversationId); + cancel(PendingIntentConstants.SMS_NOTIFICATION_ID, conversationId); + return; + } + if ((coverage & UPDATE_MESSAGES) != 0) { + createMessageNotification(conversationId); + } + if ((coverage & UPDATE_ERRORS) != 0) { + MessageNotificationState.checkFailedMessages(); + } + } finally { + if (isNotificationImageSweepDue(passStart)) { + sweepNotificationImages(passStart); + } } - if ((coverage & UPDATE_MESSAGES) != 0) { - createMessageNotification(conversationId); + } + + private static boolean isNotificationImageSweepDue(final long passStart) { + final long lastSweep = sLastNotificationImageSweep.get(); + return passStart - lastSweep >= NOTIFICATION_IMAGE_SWEEP_INTERVAL_MILLIS + && sLastNotificationImageSweep.compareAndSet(lastSweep, passStart); + } + + @VisibleForTesting + static void sweepNotificationImages(final long passStart) { + final Set live = new HashSet<>(); + final StatusBarNotification[] activeNotifications = NotificationChannelUtil.INSTANCE + .getNotificationManager() + .getActiveNotifications(); + + for (final StatusBarNotification posted : activeNotifications) { + final MessagingStyle style = MessagingStyle + .extractMessagingStyleFromNotification(posted.getNotification()); + + if (style == null) { + continue; + } + + for (final MessagingStyle.Message message : style.getMessages()) { + final Uri dataUri = message.getDataUri(); + + if (!NotificationImageProvider.isNotificationImageUri(dataUri)) { + continue; + } + + final File liveFile = NotificationImageProvider.getFileFromUri(dataUri); + if (liveFile != null) { + live.add(liveFile.getName()); + } + } } - if ((coverage & UPDATE_ERRORS) != 0) { - MessageNotificationState.checkFailedMessages(); + + final long cutoff = passStart - NOTIFICATION_IMAGE_SWEEP_GRACE_MILLIS; + for (final File file : NotificationImageProvider.listImageFiles()) { + if (file.lastModified() >= cutoff || live.contains(file.getName())) { + continue; + } + if (!file.delete()) { + LogUtil.w(TAG, "Could not delete the orphaned notification image " + + file.getAbsolutePath()); + } } } @@ -215,7 +291,8 @@ private static void updateOverflowNotification(final int overflowCount) { .setAutoCancel(true) .build(); - notificationManager.notify(tag, PendingIntentConstants.SMS_NOTIFICATION_ID, notification); + postNotification(notificationManager, tag, PendingIntentConstants.SMS_NOTIFICATION_ID, + notification); } /** @@ -527,10 +604,22 @@ static void processAndSend(final MessageNotificationState state, final Conversat Notification notification = notifBuilder.build(); notification.flags |= Notification.FLAG_AUTO_CANCEL; - notificationManager.notify(notificationTag, type, notification); + if (postNotification(notificationManager, notificationTag, type, notification)) { + LogUtil.i(TAG, "Notifying for conversation " + conversationId + "; " + + "tag = " + notificationTag + ", type = " + type); + } + } - LogUtil.i(TAG, "Notifying for conversation " + conversationId + "; " - + "tag = " + notificationTag + ", type = " + type); + @VisibleForTesting + static boolean postNotification(final NotificationManagerCompat notificationManager, + final String tag, final int type, final Notification notification) { + try { + notificationManager.notify(tag, type, notification); + return true; + } catch (SecurityException e) { + LogUtil.e(TAG, "Dropping notification: cannot grant access to its attachment", e); + return false; + } } /** @@ -640,6 +729,72 @@ public static Bitmap getAvatarBitmap(Context context, Uri avatarUri) { return null; } + static Uri getNotificationImageUri(final Context context, final Uri imageUri) { + if (imageUri == null) { + return null; + } + + final Uri notificationImageUri = NotificationImageProvider.buildNotificationImageUri(); + if (notificationImageUri == null) { + return null; + } + final File imageFile = NotificationImageProvider.getFileFromUri(notificationImageUri); + if (imageFile == null) { + return null; + } + + final ImageRequestDescriptor descriptor = new UriImageRequestDescriptor( + imageUri, + NOTIFICATION_IMAGE_MAX_SIZE, + NOTIFICATION_IMAGE_MAX_SIZE, + false, + true, + false, + 0, + 0 + ); + final MediaRequest imageRequest = descriptor.buildSyncMediaRequest(context); + final ImageResource image = MediaResourceManager.get().requestMediaResourceSync( + imageRequest); + if (image == null) { + LogUtil.w(TAG, "Could not decode the attachment for its notification"); + imageFile.delete(); + return null; + } + + final File tempFile = new File(imageFile.getPath() + ".tmp"); + boolean written = false; + try (OutputStream out = new FileOutputStream(tempFile)) { + written = orientUpright(image).compress(Bitmap.CompressFormat.JPEG, + NOTIFICATION_IMAGE_QUALITY, out); + } catch (final IOException | RuntimeException | OutOfMemoryError e) { + LogUtil.e(TAG, "Failed to write the notification image", e); + } finally { + image.release(); + } + + if (!written || !tempFile.renameTo(imageFile)) { + tempFile.delete(); + imageFile.delete(); + return null; + } + return notificationImageUri; + } + + private static Bitmap orientUpright(final ImageResource image) { + final Bitmap bitmap = image.getBitmap(); + final ExifInterface.OrientationParams params = + ExifInterface.getOrientationParams(image.getOrientation()); + if (params.rotation == 0 && params.scaleX == 1 && params.scaleY == 1) { + return bitmap; + } + final Matrix matrix = new Matrix(); + matrix.postRotate(params.rotation); + matrix.postScale(params.scaleX, params.scaleY); + return Bitmap.createBitmap(bitmap, 0, 0, bitmap.getWidth(), bitmap.getHeight(), matrix, + false); + } + public static void updateWithInlineReply(final String conversationId, final String message) { Context context = Factory.get().getApplicationContext(); Notification activeNotification = @@ -659,8 +814,8 @@ public static void updateWithInlineReply(final String conversationId, final Stri recoveredBuilder.setOnlyAlertOnce(true); String tag = buildNotificationTag(PendingIntentConstants.SMS_NOTIFICATION_ID, conversationId); - NotificationManagerCompat.from(context) - .notify(tag, PendingIntentConstants.SMS_NOTIFICATION_ID, recoveredBuilder.build()); + postNotification(NotificationManagerCompat.from(context), tag, + PendingIntentConstants.SMS_NOTIFICATION_ID, recoveredBuilder.build()); } } } diff --git a/src/com/android/messaging/datamodel/MessageNotificationState.java b/src/com/android/messaging/datamodel/MessageNotificationState.java index 29f3a6f88..9a33f5d6e 100644 --- a/src/com/android/messaging/datamodel/MessageNotificationState.java +++ b/src/com/android/messaging/datamodel/MessageNotificationState.java @@ -123,9 +123,9 @@ public static class MessageLineInfo { MessageLineInfo(final String authorId, final String authorFullName, final String authorFirstName, final CharSequence text, final Uri attachmentUri, - final String attachmentType, final boolean isManualDownloadNeeded, - final Uri avatarUri, final String messageId, final long timestamp, - final String contactUriString) { + final String attachmentType, + final boolean isManualDownloadNeeded, final Uri avatarUri, final String messageId, + final long timestamp, final String contactUriString) { mAuthorId = authorId; mMessageId = messageId; mName = authorFullName == null ? authorFirstName : authorFullName; @@ -191,8 +191,11 @@ public MessagingStyle.Message createStyledMessage(Person person) { MessagingStyle.Message message = new MessagingStyle.Message(mText, mTimestamp, person); if (mAttachmentUri != null && ContentType.isImageType(mAttachmentType)) { - message.setData(mAttachmentType, - SharedMemoryImageProvider.Companion.buildUri(mAttachmentUri, mAttachmentType)); + final Uri notificationImageUri = BugleNotifications.getNotificationImageUri( + Factory.get().getApplicationContext(), mAttachmentUri); + if (notificationImageUri != null) { + message.setData(ContentType.IMAGE_JPEG, notificationImageUri); + } } return message; } @@ -561,8 +564,9 @@ private static ConversationsList createConversationsList() { conversation.mLineInfos.add(new MessageLineInfo(authorId, authorFullName, authorFirstName, text, - attachmentUri, attachmentType, isManualDownloadNeeded, avatarUri, - messageId, timestamp, contactUriString)); + attachmentUri, attachmentType, + isManualDownloadNeeded, avatarUri, messageId, timestamp, + contactUriString)); messageCount++; conversation.mTotalMessageCount++; } while (convMessageCursor.moveToNext()); diff --git a/src/com/android/messaging/datamodel/NotificationImageProvider.kt b/src/com/android/messaging/datamodel/NotificationImageProvider.kt new file mode 100644 index 000000000..eeb3f283d --- /dev/null +++ b/src/com/android/messaging/datamodel/NotificationImageProvider.kt @@ -0,0 +1,99 @@ +package com.android.messaging.datamodel + +import android.content.ContentResolver +import android.net.Uri +import com.android.messaging.BuildConfig +import com.android.messaging.Factory +import com.android.messaging.util.LogUtil +import java.io.File +import java.io.IOException + +class NotificationImageProvider : FileProvider() { + + override fun getFile(path: String, extension: String?): File? { + return getFileWithExtension(path = path, extension = extension) + } + + companion object { + private const val NOTIFICATION_IMAGE_DIR = "notificationimages" + private const val NOTIFICATION_IMAGE_EXTENSION = "jpg" + private const val AUTHORITY = + "${BuildConfig.APPLICATION_ID}.datamodel.NotificationImageProvider" + + @JvmStatic + fun isNotificationImageUri(uri: Uri?): Boolean { + val segments = uri?.pathSegments ?: return false + + return uri.scheme == ContentResolver.SCHEME_CONTENT && + uri.authority == AUTHORITY && + segments.size == 1 && + isValidFileId(segments[0]) + } + + @JvmStatic + fun buildNotificationImageUri(): Uri? { + val uri = buildFileUri(AUTHORITY, NOTIFICATION_IMAGE_EXTENSION) + val file = getFileFromUri(uri) + + return when { + file == null -> null + ensureFileExists(file) -> uri + else -> { + LogUtil.e( + LogUtil.BUGLE_TAG, + "Failed to create notification image ${file.absolutePath}", + ) + null + } + } + } + + @JvmStatic + fun getFileFromUri(uri: Uri?): File? { + return uri + ?.path + ?.let { path -> + getFileWithExtension( + path = path, + extension = getExtensionFromUri(uri), + ) + } + } + + @JvmStatic + fun listImageFiles(): Array { + return getDirectory().listFiles() ?: emptyArray() + } + + private fun getFileWithExtension(path: String, extension: String?): File? { + val directory = getDirectory() + val fileName = when { + extension.isNullOrEmpty() -> path + else -> "$path.$extension" + } + + val file = File(directory, fileName) + + return try { + when { + file.canonicalPath.startsWith(directory.canonicalPath) -> file + else -> { + LogUtil.e( + LogUtil.BUGLE_TAG, + "getFileWithExtension: path ${file.canonicalPath} " + + "does not start with ${directory.canonicalPath}", + ) + null + } + } + } catch (e: IOException) { + LogUtil.e(LogUtil.BUGLE_TAG, "getFileWithExtension: getCanonicalPath failed ", e) + null + } + } + + private fun getDirectory(): File { + return File(Factory.get().applicationContext.cacheDir, NOTIFICATION_IMAGE_DIR) + } + } +} diff --git a/src/com/android/messaging/datamodel/SharedMemoryImageProvider.kt b/src/com/android/messaging/datamodel/SharedMemoryImageProvider.kt deleted file mode 100644 index a8f5cc5a6..000000000 --- a/src/com/android/messaging/datamodel/SharedMemoryImageProvider.kt +++ /dev/null @@ -1,80 +0,0 @@ -package com.android.messaging.datamodel - -import android.content.ContentResolver -import android.graphics.Bitmap -import android.net.Uri -import android.os.ParcelFileDescriptor -import android.os.SharedMemory -import android.system.OsConstants -import com.android.messaging.BuildConfig -import com.android.messaging.datamodel.media.MediaResourceManager -import com.android.messaging.datamodel.media.UriImageRequestDescriptor -import com.android.messaging.util.ContentType -import com.android.messaging.util.LogUtil -import java.io.ByteArrayOutputStream -import java.io.File -import java.io.FileNotFoundException - -class SharedMemoryImageProvider : FileProvider() { - companion object { - private val TAG = Companion::class.java.simpleName - - const val AUTHORITY: String = - BuildConfig.APPLICATION_ID + ".datamodel.SharedMemoryImageProvider" - - /** - * Returns a uri that can be used to access an image. - * - * @return the URI for an image - */ - fun buildUri(imageRequestUri: Uri, mimeType: String): Uri? { - if (!ContentType.isImageType(mimeType)) { - return null - } - - return Uri.Builder() - .authority(AUTHORITY) - .scheme(ContentResolver.SCHEME_CONTENT) - .appendPath(imageRequestUri.toString()) - .build() - } - } - - override fun getFile(path: String, extension: String): File? = null - - override fun openFile(uri: Uri, fileMode: String): ParcelFileDescriptor? { - val imageRequestUri = uri.path?.replaceFirst("/", "") ?: return null - val imageDescriptor = UriImageRequestDescriptor(Uri.parse(imageRequestUri)) - val imageRequest = imageDescriptor.buildSyncMediaRequest(context) - val imageResource = MediaResourceManager.get().requestMediaResourceSync(imageRequest) - if (imageResource != null) { - try { - val byteStream = ByteArrayOutputStream() - imageResource.bitmap.compress(Bitmap.CompressFormat.PNG, 100, byteStream) - - SharedMemory.create(uri.toString(), byteStream.size()).use { - val byteBuffer = it.mapReadWrite() - byteBuffer.put(byteStream.toByteArray()) - SharedMemory.unmap(byteBuffer) - it.setProtect(OsConstants.PROT_READ) - return it.getParcelFileDescriptor() - } - } catch (e: Exception) { - LogUtil.e(TAG, "Failed to open image", e) - throw FileNotFoundException() - } finally { - imageResource.release() - } - } - return null - } - - override fun getType(uri: Uri): String { - return ContentType.IMAGE_PNG - } -} - -private fun SharedMemory.getParcelFileDescriptor(): ParcelFileDescriptor? { - val getFdDupMethod = SharedMemory::class.java.getMethod("getFdDup") - return getFdDupMethod.invoke(this) as ParcelFileDescriptor? -} From fc2d3ec6d1739e14184ab84017e452ab5904f26c Mon Sep 17 00:00:00 2001 From: Artem Smirnov Date: Sat, 29 Aug 2026 12:58:53 +0300 Subject: [PATCH 13/26] Give each notification button its own pending intent --- .../action/RedownloadMmsPendingIntentTest.kt | 70 +++++++++++++++++++ .../datamodel/BugleNotifications.java | 6 +- .../datamodel/action/ActionService.java | 7 +- .../datamodel/action/ActionServiceImpl.java | 4 +- .../datamodel/action/RedownloadMmsAction.java | 5 +- 5 files changed, 84 insertions(+), 8 deletions(-) create mode 100644 app/src/test/kotlin/com/android/messaging/datamodel/action/RedownloadMmsPendingIntentTest.kt diff --git a/app/src/test/kotlin/com/android/messaging/datamodel/action/RedownloadMmsPendingIntentTest.kt b/app/src/test/kotlin/com/android/messaging/datamodel/action/RedownloadMmsPendingIntentTest.kt new file mode 100644 index 000000000..55ed506e2 --- /dev/null +++ b/app/src/test/kotlin/com/android/messaging/datamodel/action/RedownloadMmsPendingIntentTest.kt @@ -0,0 +1,70 @@ +package com.android.messaging.datamodel.action + +import android.app.PendingIntent +import android.content.Context +import com.android.messaging.FactoryTestAccess +import com.android.messaging.datamodel.BugleNotifications +import com.android.messaging.testutil.installTestFactory +import io.mockk.unmockkAll +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotEquals +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import org.robolectric.Shadows.shadowOf + +@RunWith(RobolectricTestRunner::class) +class RedownloadMmsPendingIntentTest { + + private lateinit var context: Context + + @Before + fun setUp() { + context = RuntimeEnvironment.getApplication().applicationContext + installTestFactory(context = context) + } + + @After + fun tearDown() { + unmockkAll() + FactoryTestAccess.reset() + } + + @Test + fun downloadIntentsForTwoConversationsKeepTheirOwnMessage() { + val first = redownloadPendingIntent("message-1") + val second = redownloadPendingIntent("message-2") + + assertNotEquals(first, second) + assertEquals("message-1", messageIdOf(first)) + assertEquals("message-2", messageIdOf(second)) + } + + @Test + fun downloadIntentForTheSameMessageIsReusedRatherThanDuplicated() { + val first = redownloadPendingIntent("message-1") + val reposted = redownloadPendingIntent("message-1") + + assertEquals(first, reposted) + } + + private fun redownloadPendingIntent(messageId: String): PendingIntent = + RedownloadMmsAction.getPendingIntentForRedownloadMms( + context, + messageId, + BugleNotifications.REQUEST_CODE_REDOWNLOAD_MMS, + ) + + private fun messageIdOf(pendingIntent: PendingIntent): String? { + val actionBundle = shadowOf(pendingIntent).savedIntent + .getBundleExtra(ActionServiceImpl.EXTRA_ACTION_BUNDLE)!! + val action = actionBundle.getParcelable( + ActionServiceImpl.BUNDLE_ACTION, + Action::class.java, + )!! + return action.actionParameters.getString("message_id") + } +} diff --git a/src/com/android/messaging/datamodel/BugleNotifications.java b/src/com/android/messaging/datamodel/BugleNotifications.java index e4fb86111..9e7197682 100644 --- a/src/com/android/messaging/datamodel/BugleNotifications.java +++ b/src/com/android/messaging/datamodel/BugleNotifications.java @@ -100,6 +100,9 @@ public class BugleNotifications { // Logging public static final String TAG = LogUtil.BUGLE_NOTIFICATIONS_TAG; + @VisibleForTesting + public static final int REQUEST_CODE_REDOWNLOAD_MMS = 101; + // Constants to use for update. public static final int UPDATE_NONE = 0; public static final int UPDATE_MESSAGES = 1; @@ -576,7 +579,8 @@ static void processAndSend(final MessageNotificationState state, final Conversat if (conversation.getDoesLatestMessageNeedDownload() && messageId != null && !OsUtil.isSecondaryUser()) { final PendingIntent downloadPendingIntent = - RedownloadMmsAction.getPendingIntentForRedownloadMms(context, messageId); + RedownloadMmsAction.getPendingIntentForRedownloadMms(context, + messageId, REQUEST_CODE_REDOWNLOAD_MMS); final NotificationCompat.Action.Builder actionBuilder = new NotificationCompat.Action.Builder(R.drawable.ic_file_download_light, diff --git a/src/com/android/messaging/datamodel/action/ActionService.java b/src/com/android/messaging/datamodel/action/ActionService.java index 827bbb46c..a1d14cfc9 100644 --- a/src/com/android/messaging/datamodel/action/ActionService.java +++ b/src/com/android/messaging/datamodel/action/ActionService.java @@ -25,9 +25,10 @@ */ public class ActionService { protected static PendingIntent makeStartActionPendingIntent(final Context context, - final Action action, final int requestCode, final boolean launchesAnActivity) { - return ActionServiceImpl.makeStartActionPendingIntent(context, action, requestCode, - launchesAnActivity); + final Action action, final String identifier, final int requestCode, + final boolean launchesAnActivity) { + return ActionServiceImpl.makeStartActionPendingIntent(context, action, identifier, + requestCode, launchesAnActivity); } /** diff --git a/src/com/android/messaging/datamodel/action/ActionServiceImpl.java b/src/com/android/messaging/datamodel/action/ActionServiceImpl.java index e06b6182f..3d6f00e63 100644 --- a/src/com/android/messaging/datamodel/action/ActionServiceImpl.java +++ b/src/com/android/messaging/datamodel/action/ActionServiceImpl.java @@ -213,8 +213,10 @@ public void onReceive(final Context context, final Intent intent) { * triggered */ public static PendingIntent makeStartActionPendingIntent(final Context context, - final Action action, final int requestCode, final boolean launchesAnActivity) { + final Action action, final String identifier, final int requestCode, + final boolean launchesAnActivity) { final Intent intent = PendingActionReceiver.makeIntent(OP_START_ACTION); + intent.setIdentifier(identifier); final Bundle actionBundle = new Bundle(); actionBundle.putParcelable(BUNDLE_ACTION, action); intent.putExtra(EXTRA_ACTION_BUNDLE, actionBundle); diff --git a/src/com/android/messaging/datamodel/action/RedownloadMmsAction.java b/src/com/android/messaging/datamodel/action/RedownloadMmsAction.java index 960ee4f9f..8689082db 100644 --- a/src/com/android/messaging/datamodel/action/RedownloadMmsAction.java +++ b/src/com/android/messaging/datamodel/action/RedownloadMmsAction.java @@ -36,7 +36,6 @@ */ public class RedownloadMmsAction extends Action implements Parcelable { private static final String TAG = LogUtil.BUGLE_DATAMODEL_TAG; - private static final int REQUEST_CODE_PENDING_INTENT = 101; private static final String KEY_SUB_ID = "sub_id"; @@ -52,10 +51,10 @@ public static void redownloadMessage(final String messageId) { * Get a pending intent of for downloading an MMS */ public static PendingIntent getPendingIntentForRedownloadMms( - final Context context, final String messageId) { + final Context context, final String messageId, final int requestCode) { final Action action = new RedownloadMmsAction(messageId); return ActionService.makeStartActionPendingIntent(context, - action, REQUEST_CODE_PENDING_INTENT, false /*launchesAnActivity*/); + action, messageId, requestCode, false /*launchesAnActivity*/); } // Core parameters needed for all types of message From 30b9e06a65ef0edb392823d68a9992a4a306e52e Mon Sep 17 00:00:00 2001 From: Artem Smirnov Date: Sat, 29 Aug 2026 12:59:20 +0300 Subject: [PATCH 14/26] Say which message failed in the failure notification --- ...sageNotificationStateFailedMessagesTest.kt | 158 +++++++++++++++++- res/values/strings.xml | 2 + .../datamodel/MessageNotificationState.java | 74 +++++--- .../datamodel/action/ResendMessageAction.java | 12 ++ 4 files changed, 217 insertions(+), 29 deletions(-) diff --git a/app/src/test/kotlin/com/android/messaging/datamodel/MessageNotificationStateFailedMessagesTest.kt b/app/src/test/kotlin/com/android/messaging/datamodel/MessageNotificationStateFailedMessagesTest.kt index 4ad655979..98b4aff45 100644 --- a/app/src/test/kotlin/com/android/messaging/datamodel/MessageNotificationStateFailedMessagesTest.kt +++ b/app/src/test/kotlin/com/android/messaging/datamodel/MessageNotificationStateFailedMessagesTest.kt @@ -1,11 +1,15 @@ package com.android.messaging.datamodel +import android.app.Notification import android.app.NotificationManager +import android.content.ContentValues import android.content.Context import androidx.core.content.contentValuesOf import com.android.messaging.FactoryTestAccess +import com.android.messaging.R import com.android.messaging.datamodel.data.MessageData import com.android.messaging.testutil.installTestFactory +import com.android.messaging.util.ContentType import com.android.messaging.util.NotificationChannelUtil import com.android.messaging.util.PendingIntentConstants import io.mockk.every @@ -54,9 +58,9 @@ class MessageNotificationStateFailedMessagesTest { @Test fun checkFailedMessagesPostsNotificationOnExistingChannel() { - insertFailedOutgoingMessage() + insertFailedMessage() - MessageNotificationState.checkFailedMessages() + checkFailedMessages() val shadow = shadowOf(notificationManager) val notification = shadow.getNotification( @@ -81,8 +85,8 @@ class MessageNotificationStateFailedMessagesTest { @Test fun checkFailedMessagesCancelsNotificationOnceMessagesAreSeen() { - insertFailedOutgoingMessage() - MessageNotificationState.checkFailedMessages() + insertFailedMessage() + checkFailedMessages() DataModel.get().database.update( DatabaseHelper.MESSAGES_TABLE, @@ -90,13 +94,145 @@ class MessageNotificationStateFailedMessagesTest { null, null, ) - MessageNotificationState.checkFailedMessages() + checkFailedMessages() val shadow = shadowOf(notificationManager) assertEquals(0, shadow.size()) } - private fun insertFailedOutgoingMessage() { + @Test + fun checkFailedMessagesShowsTheTextOfTheMessageThatFailed() { + insertFailedMessage() + + checkFailedMessages() + + assertEquals( + "the failure notification does not say which message failed", + MESSAGE_TEXT, + postedFailureNotification().extras.getCharSequence(Notification.EXTRA_TEXT).toString(), + ) + } + + @Test + fun checkFailedMessagesNamesTheConversationTheMessageWasFor() { + insertFailedMessage() + + checkFailedMessages() + + assertEquals( + "the failure notification does not say which conversation failed", + CONVERSATION_NAME, + postedFailureNotification().extras + .getCharSequence(Notification.EXTRA_SUB_TEXT).toString(), + ) + } + + @Test + fun checkFailedMessagesStampsTheNotificationWithTheTimeOfTheFailedMessage() { + insertFailedMessage() + + checkFailedMessages() + + assertEquals( + "the failure notification is stamped with the time of the check, not the message", + RECEIVED_TIMESTAMP_MILLIS, + postedFailureNotification().`when`, + ) + } + + @Test + fun checkFailedMessagesOffersToSendTheFailedMessageAgain() { + insertFailedMessage() + + checkFailedMessages() + + val actions = postedFailureNotification().actions + assertNotNull("the failure notification offers no way to retry", actions) + assertEquals("expected exactly one action on the failure notification", 1, actions.size) + assertEquals( + context.getString(R.string.notification_retry_prompt), + actions.single().title.toString(), + ) + } + + @Test + fun checkFailedMessagesDescribesTheAttachmentWhenTheMessageHasNoText() { + insertFailedMessage( + partValues = contentValuesOf( + DatabaseHelper.PartColumns.CONTENT_URI to "content://mms/part/1", + DatabaseHelper.PartColumns.CONTENT_TYPE to ContentType.IMAGE_PNG, + ), + ) + + checkFailedMessages() + + assertEquals( + "an attachment-only message leaves the failure notification blank", + context.getString(R.string.notification_picture), + postedFailureNotification().extras.getCharSequence(Notification.EXTRA_TEXT).toString(), + ) + } + + @Test + fun checkFailedMessagesOffersToDownloadAgainWhenTheDownloadFailed() { + insertFailedMessage(status = MessageData.BUGLE_STATUS_INCOMING_DOWNLOAD_FAILED) + + checkFailedMessages() + + val actions = postedFailureNotification().actions + assertNotNull("the failure notification offers no way to retry the download", actions) + assertEquals( + "a failed download must offer to download again, not to send again", + context.getString(R.string.notification_download_mms), + actions.single().title.toString(), + ) + } + + @Test + fun checkFailedMessagesFallsBackToTheSentTimeWhenNothingWasReceived() { + insertFailedMessage(receivedTimestamp = 0L) + + checkFailedMessages() + + assertEquals( + "a message with no received time leaves the notification stamped with the check", + SENT_TIMESTAMP_MILLIS, + postedFailureNotification().`when`, + ) + } + + private fun checkFailedMessages() { + var failure: Throwable? = null + val thread = Thread( + { MessageNotificationState.checkFailedMessages() }, + "notification-check", + ) + thread.setUncaughtExceptionHandler { _, throwable -> failure = throwable } + thread.start() + thread.join() + failure?.let { throw it } + } + + private fun postedFailureNotification(): Notification { + val notification = shadowOf(notificationManager).getNotification( + BugleNotifications.buildNotificationTag( + PendingIntentConstants.MSG_SEND_ERROR, + null, + ), + PendingIntentConstants.MSG_SEND_ERROR, + ) + assertNotNull("failure notification was not posted", notification) + return notification + } + + private fun insertFailedMessage( + partValues: ContentValues = contentValuesOf( + DatabaseHelper.PartColumns.TEXT to MESSAGE_TEXT, + DatabaseHelper.PartColumns.CONTENT_TYPE to ContentType.TEXT_PLAIN, + ), + status: Int = MessageData.BUGLE_STATUS_OUTGOING_FAILED, + receivedTimestamp: Long = RECEIVED_TIMESTAMP_MILLIS, + ) { val db = DataModel.get().database val participantId = db.insert( @@ -122,19 +258,25 @@ class MessageNotificationStateFailedMessagesTest { DatabaseHelper.MessageColumns.CONVERSATION_ID to conversationId, DatabaseHelper.MessageColumns.SENDER_PARTICIPANT_ID to participantId, DatabaseHelper.MessageColumns.SELF_PARTICIPANT_ID to participantId, - DatabaseHelper.MessageColumns.STATUS to MessageData.BUGLE_STATUS_OUTGOING_FAILED, + DatabaseHelper.MessageColumns.STATUS to status, DatabaseHelper.MessageColumns.SEEN to 0, DatabaseHelper.MessageColumns.READ to 0, - DatabaseHelper.MessageColumns.RECEIVED_TIMESTAMP to RECEIVED_TIMESTAMP_MILLIS, + DatabaseHelper.MessageColumns.RECEIVED_TIMESTAMP to receivedTimestamp, DatabaseHelper.MessageColumns.SENT_TIMESTAMP to SENT_TIMESTAMP_MILLIS, ), ) assertTrue("message insert failed", messageId >= 0) + + partValues.put(DatabaseHelper.PartColumns.MESSAGE_ID, messageId) + partValues.put(DatabaseHelper.PartColumns.CONVERSATION_ID, conversationId) + val partId = db.insert(DatabaseHelper.PARTS_TABLE, null, partValues) + assertTrue("part insert failed", partId >= 0) } private companion object { private const val RECIPIENT = "+15551230000" private const val CONVERSATION_NAME = "Test conversation" + private const val MESSAGE_TEXT = "the message that failed to send" private const val RECEIVED_TIMESTAMP_MILLIS = 1_780_920_000_000L private const val SENT_TIMESTAMP_MILLIS = 1_780_919_999_000L } diff --git a/res/values/strings.xml b/res/values/strings.xml index 23ad1de30..0f402bf9a 100644 --- a/res/values/strings.xml +++ b/res/values/strings.xml @@ -255,6 +255,8 @@ Download Reply + + Try again %d more conversation with new messages diff --git a/src/com/android/messaging/datamodel/MessageNotificationState.java b/src/com/android/messaging/datamodel/MessageNotificationState.java index 9a33f5d6e..99f7f4c67 100644 --- a/src/com/android/messaging/datamodel/MessageNotificationState.java +++ b/src/com/android/messaging/datamodel/MessageNotificationState.java @@ -22,7 +22,6 @@ import android.graphics.Bitmap; import android.graphics.Typeface; import android.net.Uri; -import android.text.Html; import android.text.Spannable; import android.text.SpannableString; import android.text.SpannableStringBuilder; @@ -39,6 +38,8 @@ import com.android.messaging.Factory; import com.android.messaging.R; +import com.android.messaging.datamodel.action.RedownloadMmsAction; +import com.android.messaging.datamodel.action.ResendMessageAction; import com.android.messaging.datamodel.data.ConversationListItemData; import com.android.messaging.datamodel.data.ConversationMessageData; import com.android.messaging.datamodel.data.ConversationParticipantsData; @@ -86,6 +87,8 @@ public class MessageNotificationState { private static final int REPLY_INTENT_REQUEST_CODE_OFFSET = 0; private static final int NUM_EXTRA_REQUEST_CODES_NEEDED = 1; + private static final int REQUEST_CODE_REDOWNLOAD_FAILED_MMS = 103; + private static final int CONTENT_INTENT_REQUEST_CODE_OFFSET = 0; private static final int CLEAR_INTENT_REQUEST_CODE_OFFSET = 1; private static final int NUM_REQUEST_CODES_NEEDED = 2; @@ -696,6 +699,23 @@ static CharSequence applyWarningTextColor(final Context context, return spanBuilder; } + private static CharSequence describeFailedMessage(final MessageData message) { + if (message == null) { + // Deleted between the query above and this read. + return null; + } + final String text = message.getMessageText(); + if (!TextUtils.isEmpty(text)) { + return text; + } + for (final MessagePartData part : message.getParts()) { + if (part.isAttachment()) { + return MessageLineInfo.formatAttachmentTag(part.getContentType()); + } + } + return null; + } + /** * Check for failed messages and post notifications as needed. */ @@ -725,7 +745,7 @@ public static void checkFailedMessages() { final ArrayList failedMessages = new ArrayList(); int cursorPosition = -1; - final long when = 0; + long when = 0; messageDataCursor.moveToPosition(-1); while (messageDataCursor.moveToNext()) { @@ -753,7 +773,6 @@ public static void checkFailedMessages() { CharSequence line1; CharSequence line2; - final boolean isRichContent = false; ConversationIdSet conversationIds = null; PendingIntent destinationIntent; if (failedMessages.size() == 1) { @@ -769,23 +788,44 @@ public static void checkFailedMessages() { conversationIds = ConversationIdSet.createSet(conversationId); - final String failedMessgeSnippet = messageData.getMessageText(); + final String messageId = messageData.getMessageId(); + // Stamp the notification with the message that failed rather than with + // the time of this check, matching the timestamp the thread sorts by. + when = messageData.getReceivedTimeStamp() > 0 + ? messageData.getReceivedTimeStamp() + : messageData.getSentTimeStamp(); + + final ConversationListItemData conversation = + ConversationListItemData.getExistingConversation(db, + conversationId); + if (conversation != null) { + // Says who the message was for without hand-building a bidi-unsafe + // "name - text" line out of two user-supplied strings. + builder.setSubText(conversation.getName()); + } + int failureStringId; if (messageData.getStatus() == MessageData.BUGLE_STATUS_INCOMING_DOWNLOAD_FAILED) { failureStringId = R.string.notification_download_failures_line1_singular; + if (!OsUtil.isSecondaryUser()) { + builder.addAction(R.drawable.ic_file_download_light, + context.getString(R.string.notification_download_mms), + RedownloadMmsAction.getPendingIntentForRedownloadMms( + context, messageId, + REQUEST_CODE_REDOWNLOAD_FAILED_MMS)); + } } else { failureStringId = R.string.notification_send_failures_line1_singular; + builder.addAction(0, + context.getString(R.string.notification_retry_prompt), + ResendMessageAction.getPendingIntentForResendMessage( + context, messageId)); } line1 = resources.getString(failureStringId); - line2 = failedMessgeSnippet; - // Set rich text for non-SMS messages or MMS push notification messages - // which we generate locally with rich text - // TODO- fix this -// if (messageData.isMmsInd()) { -// isRichContent = true; -// } + line2 = describeFailedMessage( + BugleDatabaseOperations.readMessage(db, messageId)); } else { // We have notifications for multiple conversation, go to the conversation // list. @@ -827,17 +867,9 @@ public static void checkFailedMessages() { .setSmallIcon(R.drawable.ic_failed_light) .setDeleteIntent(pendingIntentForDelete) .setContentIntent(destinationIntent) + .setContentText(line2) + .setVisibility(NotificationCompat.VISIBILITY_PRIVATE) .setSound(UriUtil.getUriForResourceId(context, R.raw.message_failure)); - if (isRichContent && !TextUtils.isEmpty(line2)) { - final NotificationCompat.InboxStyle inboxStyle = - new NotificationCompat.InboxStyle(builder); - if (line2 != null) { - inboxStyle.addLine(Html.fromHtml(line2.toString())); - } - builder.setStyle(inboxStyle); - } else { - builder.setContentText(line2); - } if (builder != null) { notificationManager.notify( diff --git a/src/com/android/messaging/datamodel/action/ResendMessageAction.java b/src/com/android/messaging/datamodel/action/ResendMessageAction.java index b442ae620..bc05b80f5 100644 --- a/src/com/android/messaging/datamodel/action/ResendMessageAction.java +++ b/src/com/android/messaging/datamodel/action/ResendMessageAction.java @@ -16,7 +16,9 @@ package com.android.messaging.datamodel.action; +import android.app.PendingIntent; import android.content.ContentValues; +import android.content.Context; import android.os.Parcel; import android.os.Parcelable; @@ -34,6 +36,8 @@ public class ResendMessageAction extends Action implements Parcelable { private static final String TAG = LogUtil.BUGLE_DATAMODEL_TAG; + private static final int REQUEST_CODE_PENDING_INTENT = 102; + private static final String KEY_SUB_ID = "sub_id"; /** @@ -44,6 +48,14 @@ public static void resendMessage(final String messageId) { action.start(); } + public static PendingIntent getPendingIntentForResendMessage( + final Context context, final String messageId) { + final Action action = new ResendMessageAction(messageId); + return ActionService.makeStartActionPendingIntent(context, + action, messageId, REQUEST_CODE_PENDING_INTENT, + false /*launchesAnActivity*/); + } + // Core parameters needed for all types of message private static final String KEY_MESSAGE_ID = "message_id"; From af7c72cd64fd5556176570a72a9e6994921a49c1 Mon Sep 17 00:00:00 2001 From: Artem Smirnov Date: Sat, 29 Aug 2026 14:11:12 +0300 Subject: [PATCH 15/26] Resolve the self phone number from the carrier when the SIM lacks it --- AndroidManifest.xml | 3 + .../SelfPhoneNumberPermissionPromptTest.kt | 54 ++++++++ ...ersationDraftDelegateSendValidationTest.kt | 14 ++ .../util/PhoneUtilsSelfNumberTest.kt | 126 ++++++++++++++++++ res/values/strings.xml | 2 + .../store/SelfPhoneNumberPermissionStore.kt | 35 +++++ .../di/onboarding/OnboardingBindsModule.kt | 16 +++ .../SelfPhoneNumberPermissionPrompt.kt | 37 +++++ src/com/android/messaging/ui/MainActivity.kt | 5 + .../ui/conversation/ConversationActivity.kt | 5 + .../delegate/ConversationDraftDelegate.kt | 7 + .../android/messaging/ui/host/AppNavGraph.kt | 8 ++ .../host/SelfPhoneNumberPermissionEffect.kt | 32 +++++ .../messaging/util/BuglePrefsKeys.java | 7 + .../android/messaging/util/PhoneUtils.java | 47 +++++-- 15 files changed, 390 insertions(+), 8 deletions(-) create mode 100644 app/src/test/kotlin/com/android/messaging/domain/onboarding/usecase/SelfPhoneNumberPermissionPromptTest.kt create mode 100644 app/src/test/kotlin/com/android/messaging/util/PhoneUtilsSelfNumberTest.kt create mode 100644 src/com/android/messaging/data/onboarding/store/SelfPhoneNumberPermissionStore.kt create mode 100644 src/com/android/messaging/domain/onboarding/usecase/SelfPhoneNumberPermissionPrompt.kt create mode 100644 src/com/android/messaging/ui/host/SelfPhoneNumberPermissionEffect.kt diff --git a/AndroidManifest.xml b/AndroidManifest.xml index 7743b6565..6fda90c3d 100644 --- a/AndroidManifest.xml +++ b/AndroidManifest.xml @@ -35,6 +35,9 @@ + + diff --git a/app/src/test/kotlin/com/android/messaging/domain/onboarding/usecase/SelfPhoneNumberPermissionPromptTest.kt b/app/src/test/kotlin/com/android/messaging/domain/onboarding/usecase/SelfPhoneNumberPermissionPromptTest.kt new file mode 100644 index 000000000..574a90bd7 --- /dev/null +++ b/app/src/test/kotlin/com/android/messaging/domain/onboarding/usecase/SelfPhoneNumberPermissionPromptTest.kt @@ -0,0 +1,54 @@ +package com.android.messaging.domain.onboarding.usecase + +import com.android.messaging.data.onboarding.store.SelfPhoneNumberPermissionStore +import io.mockk.every +import io.mockk.mockk +import io.mockk.verify +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +class SelfPhoneNumberPermissionPromptTest { + + @Test + fun consume_whenNeverAsked_promptsAndRecordsTheAsk() { + val store = mockStore(isGranted = false, isRequested = false) + + assertTrue(SelfPhoneNumberPermissionPromptImpl(store).consume()) + + verify(exactly = 1) { + store.markRequested() + } + } + + @Test + fun consume_whenAlreadyAsked_doesNotPromptAgain() { + val store = mockStore(isGranted = false, isRequested = true) + + assertFalse( + "the permission is optional, so a user who declined it once is not asked again", + SelfPhoneNumberPermissionPromptImpl(store).consume(), + ) + } + + @Test + fun consume_whenPermissionAlreadyGranted_doesNotPrompt() { + val store = mockStore(isGranted = true, isRequested = false) + + assertFalse(SelfPhoneNumberPermissionPromptImpl(store).consume()) + + verify(exactly = 0) { + store.markRequested() + } + } + + private fun mockStore( + isGranted: Boolean, + isRequested: Boolean, + ): SelfPhoneNumberPermissionStore { + return mockk(relaxUnitFun = true).also { + every { it.isGranted() } returns isGranted + every { it.isRequested() } returns isRequested + } + } +} diff --git a/app/src/test/kotlin/com/android/messaging/ui/conversation/composer/delegate/draft/ConversationDraftDelegateSendValidationTest.kt b/app/src/test/kotlin/com/android/messaging/ui/conversation/composer/delegate/draft/ConversationDraftDelegateSendValidationTest.kt index 4fcdc46de..898199c17 100644 --- a/app/src/test/kotlin/com/android/messaging/ui/conversation/composer/delegate/draft/ConversationDraftDelegateSendValidationTest.kt +++ b/app/src/test/kotlin/com/android/messaging/ui/conversation/composer/delegate/draft/ConversationDraftDelegateSendValidationTest.kt @@ -7,6 +7,7 @@ import com.android.messaging.domain.conversation.usecase.draft.SendConversationD import com.android.messaging.domain.conversation.usecase.draft.exception.ConversationSimNotReadyException import com.android.messaging.domain.conversation.usecase.draft.exception.DraftDispatchFailedException import com.android.messaging.domain.conversation.usecase.draft.exception.MessageLimitExceededException +import com.android.messaging.domain.conversation.usecase.draft.exception.MissingSelfPhoneNumberForGroupMmsException import com.android.messaging.domain.conversation.usecase.draft.exception.TooManyVideoAttachmentsException import com.android.messaging.domain.conversation.usecase.draft.exception.UnknownConversationRecipientException import com.android.messaging.testutil.TEST_CONVERSATION_ID as CONVERSATION_ID @@ -57,6 +58,19 @@ internal class ConversationDraftDelegateSendValidationTest : BaseConversationDra } } + @Test + fun sendValidationFailure_whenSelfPhoneNumberIsUnknown_emitsAddYourPhoneNumberMessage() { + runTest(context = mainDispatcherRule.testDispatcher) { + assertSendFailureMessage( + exception = MissingSelfPhoneNumberForGroupMmsException( + conversationId = CONVERSATION_ID, + selfSubId = SubId(1), + ), + expectedMessageResId = R.string.cant_send_group_mms_without_self_phone_number, + ) + } + } + @Test fun sendValidationFailure_whenTooManyVideos_setsVideoLimitWarning() { runTest(context = mainDispatcherRule.testDispatcher) { diff --git a/app/src/test/kotlin/com/android/messaging/util/PhoneUtilsSelfNumberTest.kt b/app/src/test/kotlin/com/android/messaging/util/PhoneUtilsSelfNumberTest.kt new file mode 100644 index 000000000..f18d36294 --- /dev/null +++ b/app/src/test/kotlin/com/android/messaging/util/PhoneUtilsSelfNumberTest.kt @@ -0,0 +1,126 @@ +package com.android.messaging.util + +import android.content.Context +import android.telephony.SubscriptionManager +import com.android.messaging.FactoryTestAccess +import com.android.messaging.R +import com.android.messaging.testutil.FakeBuglePrefs +import com.android.messaging.testutil.installTestFactory +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertThrows +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import org.robolectric.Shadows.shadowOf +import org.robolectric.annotation.Config +import org.robolectric.shadows.ShadowSubscriptionManager + +@RunWith(RobolectricTestRunner::class) +@Config(sdk = [36]) +class PhoneUtilsSelfNumberTest { + + private lateinit var context: Context + private lateinit var prefs: FakeBuglePrefs + + @Before + fun setUp() { + ShadowSubscriptionManager.reset() + context = RuntimeEnvironment.getApplication().applicationContext + prefs = FakeBuglePrefs() + installTestFactory(context = context, prefs = prefs) + } + + @After + fun tearDown() { + ShadowSubscriptionManager.reset() + FactoryTestAccess.reset() + } + + @Test + fun getSelfRawNumberResolvesTheNumberTheCarrierKnowsWhenTheSimDoesNotCarryIt() { + givenSubscription(numberOnSim = "") + givenCarrierKnowsNumber(CARRIER_NUMBER) + + assertEquals( + "the self number is read from the SIM alone, so carriers that do not write the" + + " MSISDN to the UICC leave it unresolvable", + CARRIER_NUMBER, + PhoneUtils(SUB_ID).getSelfRawNumber(false), + ) + } + + @Test + fun getSelfRawNumberPrefersTheNumberTheUserTypedIn() { + givenSubscription(numberOnSim = SIM_NUMBER) + givenCarrierKnowsNumber(CARRIER_NUMBER) + prefs.putString(context.getString(R.string.mms_phone_number_pref_key), OVERRIDE_NUMBER) + + assertEquals( + "the number the user entered in settings must win over anything telephony reports", + OVERRIDE_NUMBER, + PhoneUtils(SUB_ID).getSelfRawNumber(true), + ) + } + + @Test + fun getSelfRawNumberFallsBackToTheSimWhenTheCarrierKnowsNoNumber() { + givenSubscription(numberOnSim = SIM_NUMBER) + + assertEquals( + "the SIM's own number must still be used when the carrier reports nothing", + SIM_NUMBER, + PhoneUtils(SUB_ID).getSelfRawNumber(false), + ) + } + + @Test + fun getSelfRawNumberFallsBackToTheSimWhenReadingTheCarrierNumberIsDenied() { + givenSubscription(numberOnSim = SIM_NUMBER) + givenCarrierKnowsNumber(CARRIER_NUMBER) + shadowOf(subscriptionManager()).setReadPhoneNumbersPermission(false) + + assertEquals( + "a denied carrier number read must not lose the number the SIM does carry", + SIM_NUMBER, + PhoneUtils(SUB_ID).getSelfRawNumber(false), + ) + } + + @Test + fun getSelfRawNumberStillReportsThatThereIsNoSubscriptionToReadFrom() { + // Telephony remembers numbers for subscriptions that are no longer active, so asking it + // first would turn "SIM is not ready" into a send that fails further down. + givenCarrierKnowsNumber(CARRIER_NUMBER) + + assertThrows(IllegalStateException::class.java) { + PhoneUtils(SUB_ID).getSelfRawNumber(false) + } + } + + private fun subscriptionManager(): SubscriptionManager { + return context.getSystemService(SubscriptionManager::class.java) + } + + private fun givenSubscription(numberOnSim: String) { + shadowOf(subscriptionManager()).setActiveSubscriptionInfos( + ShadowSubscriptionManager.SubscriptionInfoBuilder.newBuilder() + .setId(SUB_ID) + .setNumber(numberOnSim) + .buildSubscriptionInfo(), + ) + } + + private fun givenCarrierKnowsNumber(number: String) { + shadowOf(subscriptionManager()).setPhoneNumber(SUB_ID, number) + } + + private companion object { + private const val SUB_ID = 2 + private const val SIM_NUMBER = "+15550001111" + private const val CARRIER_NUMBER = "+37253953334" + private const val OVERRIDE_NUMBER = "+37255500002" + } +} diff --git a/res/values/strings.xml b/res/values/strings.xml index 0f402bf9a..11d8bfe81 100644 --- a/res/values/strings.xml +++ b/res/values/strings.xml @@ -451,6 +451,8 @@ Can\'t load attachment. Try again. Network is not ready. Try again. + + Group messages need your phone number. Add it in Settings. Add people New group diff --git a/src/com/android/messaging/data/onboarding/store/SelfPhoneNumberPermissionStore.kt b/src/com/android/messaging/data/onboarding/store/SelfPhoneNumberPermissionStore.kt new file mode 100644 index 000000000..7e23b8e37 --- /dev/null +++ b/src/com/android/messaging/data/onboarding/store/SelfPhoneNumberPermissionStore.kt @@ -0,0 +1,35 @@ +package com.android.messaging.data.onboarding.store + +import android.Manifest +import com.android.messaging.util.BuglePrefs +import com.android.messaging.util.BuglePrefsKeys +import com.android.messaging.util.OsUtil +import javax.inject.Inject + +internal interface SelfPhoneNumberPermissionStore { + fun isGranted(): Boolean + fun isRequested(): Boolean + fun markRequested() +} + +internal class SelfPhoneNumberPermissionStoreImpl @Inject constructor() : + SelfPhoneNumberPermissionStore { + + override fun isGranted(): Boolean { + return OsUtil.hasPermission(Manifest.permission.READ_PHONE_NUMBERS) + } + + override fun isRequested(): Boolean { + return BuglePrefs.getApplicationPrefs().getBoolean( + BuglePrefsKeys.SELF_PHONE_NUMBER_PERMISSION_REQUESTED, + BuglePrefsKeys.SELF_PHONE_NUMBER_PERMISSION_REQUESTED_DEFAULT, + ) + } + + override fun markRequested() { + BuglePrefs.getApplicationPrefs().putBoolean( + BuglePrefsKeys.SELF_PHONE_NUMBER_PERMISSION_REQUESTED, + true, + ) + } +} diff --git a/src/com/android/messaging/di/onboarding/OnboardingBindsModule.kt b/src/com/android/messaging/di/onboarding/OnboardingBindsModule.kt index ca7d4b50e..05770e85b 100644 --- a/src/com/android/messaging/di/onboarding/OnboardingBindsModule.kt +++ b/src/com/android/messaging/di/onboarding/OnboardingBindsModule.kt @@ -4,10 +4,14 @@ import com.android.messaging.data.onboarding.GetMissingPermissionLabels import com.android.messaging.data.onboarding.GetMissingPermissionLabelsImpl import com.android.messaging.data.onboarding.RequiredPermissionsChecker import com.android.messaging.data.onboarding.RequiredPermissionsCheckerImpl +import com.android.messaging.data.onboarding.store.SelfPhoneNumberPermissionStore +import com.android.messaging.data.onboarding.store.SelfPhoneNumberPermissionStoreImpl import com.android.messaging.data.onboarding.store.SmsWarningStore import com.android.messaging.data.onboarding.store.SmsWarningStoreImpl import com.android.messaging.domain.onboarding.usecase.DeterminePermissionRequest import com.android.messaging.domain.onboarding.usecase.DeterminePermissionRequestImpl +import com.android.messaging.domain.onboarding.usecase.SelfPhoneNumberPermissionPrompt +import com.android.messaging.domain.onboarding.usecase.SelfPhoneNumberPermissionPromptImpl import com.android.messaging.domain.onboarding.usecase.ShouldShowOnboarding import com.android.messaging.domain.onboarding.usecase.ShouldShowOnboardingImpl import dagger.Binds @@ -49,4 +53,16 @@ internal abstract class OnboardingBindsModule { abstract fun bindShouldShowOnboarding( impl: ShouldShowOnboardingImpl, ): ShouldShowOnboarding + + @Binds + @Reusable + abstract fun bindSelfPhoneNumberPermissionStore( + impl: SelfPhoneNumberPermissionStoreImpl, + ): SelfPhoneNumberPermissionStore + + @Binds + @Reusable + abstract fun bindSelfPhoneNumberPermissionPrompt( + impl: SelfPhoneNumberPermissionPromptImpl, + ): SelfPhoneNumberPermissionPrompt } diff --git a/src/com/android/messaging/domain/onboarding/usecase/SelfPhoneNumberPermissionPrompt.kt b/src/com/android/messaging/domain/onboarding/usecase/SelfPhoneNumberPermissionPrompt.kt new file mode 100644 index 000000000..112af3860 --- /dev/null +++ b/src/com/android/messaging/domain/onboarding/usecase/SelfPhoneNumberPermissionPrompt.kt @@ -0,0 +1,37 @@ +package com.android.messaging.domain.onboarding.usecase + +import com.android.messaging.data.onboarding.store.SelfPhoneNumberPermissionStore +import com.android.messaging.datamodel.ParticipantRefresh +import javax.inject.Inject + +internal interface SelfPhoneNumberPermissionPrompt { + + /** + * Whether the permission is still worth asking for, spending the single ask the user is owed. + * Consuming before the dialog is shown rather than after it is answered keeps a process death + * mid-prompt from asking again. + */ + fun consume(): Boolean + + fun onGranted() +} + +internal class SelfPhoneNumberPermissionPromptImpl @Inject constructor( + private val store: SelfPhoneNumberPermissionStore, +) : SelfPhoneNumberPermissionPrompt { + + override fun consume(): Boolean { + return when { + store.isGranted() -> false + store.isRequested() -> false + else -> { + store.markRequested() + true + } + } + } + + override fun onGranted() { + ParticipantRefresh.refreshSelfParticipants() + } +} diff --git a/src/com/android/messaging/ui/MainActivity.kt b/src/com/android/messaging/ui/MainActivity.kt index e7f1f5ec4..b237a35ed 100644 --- a/src/com/android/messaging/ui/MainActivity.kt +++ b/src/com/android/messaging/ui/MainActivity.kt @@ -8,6 +8,7 @@ import androidx.activity.enableEdgeToEdge import androidx.compose.foundation.layout.fillMaxSize import androidx.compose.ui.Modifier import androidx.navigation3.runtime.NavKey +import com.android.messaging.domain.onboarding.usecase.SelfPhoneNumberPermissionPrompt import com.android.messaging.domain.onboarding.usecase.ShouldShowOnboarding import com.android.messaging.ui.appsettings.navigation.SettingsNavKey import com.android.messaging.ui.appsettings.navigation.goToSettings @@ -33,6 +34,9 @@ internal class MainActivity : ComponentActivity() { @Inject lateinit var shouldShowOnboarding: ShouldShowOnboarding + @Inject + lateinit var selfPhoneNumberPermissionPrompt: SelfPhoneNumberPermissionPrompt + @Inject lateinit var launchStore: ConversationLaunchStore @@ -61,6 +65,7 @@ internal class MainActivity : ComponentActivity() { showsTwoPanes = listDetailLayout.showsTwoPanes, launchDestinations = launchDestinationFlow, shouldShowOnboarding = shouldShowOnboarding::invoke, + selfPhoneNumberPermissionPrompt = selfPhoneNumberPermissionPrompt, onAppResumed = ::resumeDataModel, onFinish = ::finish, modifier = Modifier.fillMaxSize(), diff --git a/src/com/android/messaging/ui/conversation/ConversationActivity.kt b/src/com/android/messaging/ui/conversation/ConversationActivity.kt index e83f6cba6..46e3b5eb7 100644 --- a/src/com/android/messaging/ui/conversation/ConversationActivity.kt +++ b/src/com/android/messaging/ui/conversation/ConversationActivity.kt @@ -6,6 +6,7 @@ import androidx.activity.ComponentActivity import androidx.activity.compose.setContent import androidx.activity.enableEdgeToEdge import androidx.navigation3.runtime.NavKey +import com.android.messaging.domain.onboarding.usecase.SelfPhoneNumberPermissionPrompt import com.android.messaging.domain.onboarding.usecase.ShouldShowOnboarding import com.android.messaging.ui.MainActivity import com.android.messaging.ui.conversation.entry.ConversationLaunchStore @@ -28,6 +29,9 @@ internal class ConversationActivity : ComponentActivity() { @Inject lateinit var shouldShowOnboarding: ShouldShowOnboarding + @Inject + lateinit var selfPhoneNumberPermissionPrompt: SelfPhoneNumberPermissionPrompt + @Inject lateinit var launchStore: ConversationLaunchStore @@ -59,6 +63,7 @@ internal class ConversationActivity : ComponentActivity() { showsTwoPanes = false, launchDestinations = launchDestinationFlow, shouldShowOnboarding = shouldShowOnboarding::invoke, + selfPhoneNumberPermissionPrompt = selfPhoneNumberPermissionPrompt, onAppResumed = ::resumeDataModel, onFinish = ::finishAfterTransition, ) 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 3a8121d85..d7906be52 100644 --- a/src/com/android/messaging/ui/conversation/composer/delegate/ConversationDraftDelegate.kt +++ b/src/com/android/messaging/ui/conversation/composer/delegate/ConversationDraftDelegate.kt @@ -15,6 +15,7 @@ import com.android.messaging.domain.conversation.usecase.action.ConversationActi import com.android.messaging.domain.conversation.usecase.draft.SendConversationDraft import com.android.messaging.domain.conversation.usecase.draft.exception.ConversationSimNotReadyException import com.android.messaging.domain.conversation.usecase.draft.exception.MessageLimitExceededException +import com.android.messaging.domain.conversation.usecase.draft.exception.MissingSelfPhoneNumberForGroupMmsException import com.android.messaging.domain.conversation.usecase.draft.exception.SendConversationDraftException import com.android.messaging.domain.conversation.usecase.draft.exception.TooManyVideoAttachmentsException import com.android.messaging.domain.conversation.usecase.draft.exception.UnknownConversationRecipientException @@ -560,6 +561,12 @@ internal class ConversationDraftDelegateImpl @Inject constructor( } is UnknownConversationRecipientException -> R.string.unknown_sender + + // Must precede the SendConversationDraftException branch it is a subclass of. + is MissingSelfPhoneNumberForGroupMmsException -> { + R.string.cant_send_group_mms_without_self_phone_number + } + is SendConversationDraftException -> R.string.send_message_failure else -> R.string.send_message_failure } diff --git a/src/com/android/messaging/ui/host/AppNavGraph.kt b/src/com/android/messaging/ui/host/AppNavGraph.kt index a2cdb1bb6..db6e20b3f 100644 --- a/src/com/android/messaging/ui/host/AppNavGraph.kt +++ b/src/com/android/messaging/ui/host/AppNavGraph.kt @@ -7,6 +7,7 @@ import androidx.compose.runtime.remember import androidx.compose.ui.Modifier import androidx.navigation3.runtime.NavKey import androidx.navigation3.scene.SceneStrategy +import com.android.messaging.domain.onboarding.usecase.SelfPhoneNumberPermissionPrompt import com.android.messaging.ui.conversation.navigation.ProvideConversationEntryNavState import com.android.messaging.ui.navigation.AppNavDisplay import com.android.messaging.ui.navigation.LocalNavigator @@ -25,6 +26,7 @@ internal fun AppNavGraph( showsTwoPanes: Boolean, launchDestinations: Flow>, shouldShowOnboarding: () -> Boolean, + selfPhoneNumberPermissionPrompt: SelfPhoneNumberPermissionPrompt, onAppResumed: () -> Unit, onFinish: () -> Unit, modifier: Modifier = Modifier, @@ -45,6 +47,12 @@ internal fun AppNavGraph( onAppResumed = onAppResumed, ) + SelfPhoneNumberPermissionEffect( + backStack = backStack, + shouldShowOnboarding = shouldShowOnboarding, + selfPhoneNumberPermissionPrompt = selfPhoneNumberPermissionPrompt, + ) + AppNavLaunchEffects( launchDestinations = launchDestinations, onResetBackStack = navigator::reset, diff --git a/src/com/android/messaging/ui/host/SelfPhoneNumberPermissionEffect.kt b/src/com/android/messaging/ui/host/SelfPhoneNumberPermissionEffect.kt new file mode 100644 index 000000000..506ddae18 --- /dev/null +++ b/src/com/android/messaging/ui/host/SelfPhoneNumberPermissionEffect.kt @@ -0,0 +1,32 @@ +package com.android.messaging.ui.host + +import android.Manifest +import androidx.activity.compose.rememberLauncherForActivityResult +import androidx.activity.result.contract.ActivityResultContracts +import androidx.compose.runtime.Composable +import androidx.compose.runtime.LaunchedEffect +import androidx.navigation3.runtime.NavKey +import com.android.messaging.domain.onboarding.usecase.SelfPhoneNumberPermissionPrompt + +@Composable +internal fun SelfPhoneNumberPermissionEffect( + backStack: List, + shouldShowOnboarding: () -> Boolean, + selfPhoneNumberPermissionPrompt: SelfPhoneNumberPermissionPrompt, +) { + val launcher = rememberLauncherForActivityResult( + contract = ActivityResultContracts.RequestPermission(), + ) { isGranted -> + if (isGranted) { + selfPhoneNumberPermissionPrompt.onGranted() + } + } + + LaunchedEffect(backStack.lastOrNull()) { + if (shouldShowOnboarding() || !selfPhoneNumberPermissionPrompt.consume()) { + return@LaunchedEffect + } + + launcher.launch(Manifest.permission.READ_PHONE_NUMBERS) + } +} diff --git a/src/com/android/messaging/util/BuglePrefsKeys.java b/src/com/android/messaging/util/BuglePrefsKeys.java index fcbe187c4..e8ae2ca94 100644 --- a/src/com/android/messaging/util/BuglePrefsKeys.java +++ b/src/com/android/messaging/util/BuglePrefsKeys.java @@ -67,6 +67,13 @@ private BuglePrefsKeys() {} // do not instantiate public static final String SMS_WARNING_ACKNOWLEDGED = "sms_warning_acknowledged"; public static final boolean SMS_WARNING_ACKNOWLEDGED_DEFAULT = false; + /** + * Whether the one-off request for READ_PHONE_NUMBERS has already been made. + */ + public static final String SELF_PHONE_NUMBER_PERMISSION_REQUESTED = + "self_phone_number_permission_requested"; + public static final boolean SELF_PHONE_NUMBER_PERMISSION_REQUESTED_DEFAULT = false; + /** * The last selected chooser index in the media picker. */ diff --git a/src/com/android/messaging/util/PhoneUtils.java b/src/com/android/messaging/util/PhoneUtils.java index aa7499bad..b512f4161 100644 --- a/src/com/android/messaging/util/PhoneUtils.java +++ b/src/com/android/messaging/util/PhoneUtils.java @@ -164,15 +164,46 @@ public String getSelfRawNumber(final boolean allowOverride) { } final SubscriptionInfo subInfo = getActiveSubscriptionInfo(); - if (subInfo != null) { - String phoneNumber = subInfo.getNumber(); - if (TextUtils.isEmpty(phoneNumber) && LogUtil.isLoggable(TAG, LogUtil.DEBUG)) { - LogUtil.d(TAG, "SubscriptionInfo phone number for self is empty!"); - } - return phoneNumber; + if (subInfo == null) { + LogUtil.w(TAG, "PhoneUtils.getSelfRawNumber: subInfo is null for " + mSubId); + throw new IllegalStateException("No active subscription"); + } + + final String carrierNumber = getCarrierKnownNumber(subInfo.getSubscriptionId()); + if (!TextUtils.isEmpty(carrierNumber)) { + return carrierNumber; + } + + final String phoneNumber = subInfo.getNumber(); + if (TextUtils.isEmpty(phoneNumber) && LogUtil.isLoggable(TAG, LogUtil.DEBUG)) { + LogUtil.d(TAG, "SubscriptionInfo phone number for self is empty!"); + } + + return phoneNumber; + } + + /** + * Ask telephony for an active subscription's number, from whichever source knows it. + * + *

{@link SubscriptionInfo#getNumber} reads the UICC alone, and plenty of carriers never + * write the MSISDN there -- the number is then only known from the carrier config or from the + * IMS registration. {@link SubscriptionManager#getPhoneNumber} consults all three. + * + * @param subId an active subscription id + * @return the number, or empty if telephony has none or will not tell us + */ + private String getCarrierKnownNumber(final int subId) { + try { + return mSubscriptionManager.getPhoneNumber(subId); + } catch (final SecurityException e) { + // READ_PHONE_NUMBERS is declared but not granted; the UICC is all we get. + LogUtil.w(TAG, "PhoneUtils.getCarrierKnownNumber: not permitted for " + subId); + return ""; + } catch (final IllegalArgumentException e) { + // The subscription went away between the two calls. + LogUtil.w(TAG, "PhoneUtils.getCarrierKnownNumber: unknown subscription " + subId); + return ""; } - LogUtil.w(TAG, "PhoneUtils.getSelfRawNumber: subInfo is null for " + mSubId); - throw new IllegalStateException("No active subscription"); } /** From 93deebff3d7224c24c03752b786cc8cacf46a3bb Mon Sep 17 00:00:00 2001 From: Artem Smirnov Date: Sat, 29 Aug 2026 17:13:30 +0300 Subject: [PATCH 16/26] Only accept a phone number as the self phone number --- .../usecase/IsValidSelfPhoneNumberTest.kt | 80 ++++++++++ .../usecase/SetSubscriptionPhoneNumberTest.kt | 142 ++++++++++++++++++ .../SubscriptionSettingsViewModelTest.kt | 62 +++++++- .../subscription/ui/PhoneNumberDialogTest.kt | 87 +++++++++++ .../util/PhoneUtilsSelfNumberTest.kt | 73 ++++++++- res/values/strings.xml | 2 + .../di/settings/SettingsBindsModule.kt | 8 + .../usecase/IsValidSelfPhoneNumber.kt | 19 +++ .../usecase/SetSubscriptionPhoneNumber.kt | 28 ++-- .../navigation/SettingsNavEntries.kt | 3 + .../SubscriptionSettingsViewModel.kt | 36 ++++- .../model/PhoneNumberDialogUiState.kt | 9 ++ .../model/SubscriptionSettingsAction.kt | 8 +- .../ui/SubscriptionSettingsScreen.kt | 46 ++++-- .../android/messaging/util/PhoneUtils.java | 51 +++++-- 15 files changed, 612 insertions(+), 42 deletions(-) create mode 100644 app/src/test/kotlin/com/android/messaging/domain/subscriptionsettings/usecase/IsValidSelfPhoneNumberTest.kt create mode 100644 app/src/test/kotlin/com/android/messaging/domain/subscriptionsettings/usecase/SetSubscriptionPhoneNumberTest.kt create mode 100644 app/src/test/kotlin/com/android/messaging/ui/appsettings/subscription/ui/PhoneNumberDialogTest.kt create mode 100644 src/com/android/messaging/domain/subscriptionsettings/usecase/IsValidSelfPhoneNumber.kt create mode 100644 src/com/android/messaging/ui/appsettings/subscription/model/PhoneNumberDialogUiState.kt diff --git a/app/src/test/kotlin/com/android/messaging/domain/subscriptionsettings/usecase/IsValidSelfPhoneNumberTest.kt b/app/src/test/kotlin/com/android/messaging/domain/subscriptionsettings/usecase/IsValidSelfPhoneNumberTest.kt new file mode 100644 index 000000000..e72499edc --- /dev/null +++ b/app/src/test/kotlin/com/android/messaging/domain/subscriptionsettings/usecase/IsValidSelfPhoneNumberTest.kt @@ -0,0 +1,80 @@ +package com.android.messaging.domain.subscriptionsettings.usecase + +import android.content.Context +import android.telephony.SubscriptionManager +import com.android.messaging.Factory +import com.android.messaging.FactoryTestAccess +import com.android.messaging.data.subscription.model.SubId +import com.android.messaging.testutil.FakeBuglePrefs +import com.android.messaging.testutil.installTestFactory +import com.android.messaging.util.PhoneUtils +import io.mockk.every +import org.junit.After +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import org.robolectric.Shadows.shadowOf +import org.robolectric.annotation.Config +import org.robolectric.shadows.ShadowSubscriptionManager + +@RunWith(RobolectricTestRunner::class) +@Config(sdk = [36]) +class IsValidSelfPhoneNumberTest { + + private val useCase = IsValidSelfPhoneNumberImpl() + + private lateinit var context: Context + + @Before + fun setUp() { + ShadowSubscriptionManager.reset() + context = RuntimeEnvironment.getApplication().applicationContext + installTestFactory(context = context, prefs = FakeBuglePrefs()) + givenSimOnEstonianCarrier() + every { Factory.get().getPhoneUtils(any()) } returns PhoneUtils(SUB_ID.value) + } + + @After + fun tearDown() { + ShadowSubscriptionManager.reset() + FactoryTestAccess.reset() + } + + @Test + fun invokeAcceptsAnEmptyNumber() { + assertTrue( + "emptying the field is how the number is handed back to the SIM, so it can never" + + " be rejected as invalid", + useCase(SUB_ID, ""), + ) + } + + @Test + fun invokeAcceptsAPhoneNumber() { + assertTrue(useCase(SUB_ID, "5555 0001")) + } + + @Test + fun invokeRejectsTextThatIsNotAPhoneNumber() { + assertFalse(useCase(SUB_ID, "DROP TABLE messages")) + } + + private fun givenSimOnEstonianCarrier() { + val subscriptionManager = context.getSystemService(SubscriptionManager::class.java) + shadowOf(subscriptionManager).setActiveSubscriptionInfos( + ShadowSubscriptionManager.SubscriptionInfoBuilder.newBuilder() + .setId(SUB_ID.value) + .setCountryIso("ee") + .setNumber("+37253953334") + .buildSubscriptionInfo(), + ) + } + + private companion object { + private val SUB_ID = SubId(2) + } +} diff --git a/app/src/test/kotlin/com/android/messaging/domain/subscriptionsettings/usecase/SetSubscriptionPhoneNumberTest.kt b/app/src/test/kotlin/com/android/messaging/domain/subscriptionsettings/usecase/SetSubscriptionPhoneNumberTest.kt new file mode 100644 index 000000000..f446c8374 --- /dev/null +++ b/app/src/test/kotlin/com/android/messaging/domain/subscriptionsettings/usecase/SetSubscriptionPhoneNumberTest.kt @@ -0,0 +1,142 @@ +package com.android.messaging.domain.subscriptionsettings.usecase + +import android.content.Context +import android.telephony.SubscriptionManager +import com.android.messaging.Factory +import com.android.messaging.FactoryTestAccess +import com.android.messaging.R +import com.android.messaging.data.subscription.model.SubId +import com.android.messaging.testutil.FakeBuglePrefs +import com.android.messaging.testutil.installTestFactory +import com.android.messaging.util.PhoneUtils +import io.mockk.every +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.StandardTestDispatcher +import kotlinx.coroutines.test.TestScope +import kotlinx.coroutines.test.runTest +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import org.robolectric.Shadows.shadowOf +import org.robolectric.annotation.Config +import org.robolectric.shadows.ShadowSubscriptionManager + +@OptIn(ExperimentalCoroutinesApi::class) +@RunWith(RobolectricTestRunner::class) +@Config(sdk = [36]) +class SetSubscriptionPhoneNumberTest { + + private lateinit var context: Context + private lateinit var prefs: FakeBuglePrefs + private lateinit var prefKey: String + + @Before + fun setUp() { + ShadowSubscriptionManager.reset() + context = RuntimeEnvironment.getApplication().applicationContext + prefs = FakeBuglePrefs() + prefKey = context.getString(R.string.mms_phone_number_pref_key) + installTestFactory(context = context, prefs = prefs) + givenSimOnEstonianCarrier() + every { Factory.get().getPhoneUtils(any()) } returns PhoneUtils(SUB_ID) + } + + @After + fun tearDown() { + ShadowSubscriptionManager.reset() + FactoryTestAccess.reset() + } + + @Test + fun invokeDoesNotStoreTextThatIsNotAPhoneNumber() = runTest { + setPhoneNumber("DROP TABLE messages") + + assertNull( + "anything stored here becomes the sender identity of every outgoing MMS," + + " so text that is not a phone number must never reach the preference", + prefs.getString(prefKey, null), + ) + } + + @Test + fun invokeStoresAValidNumberInE164() = runTest { + setPhoneNumber("5555 0001") + + assertEquals( + "the stored number is written into the MMS From: header, so it has to be" + + " normalised rather than kept as typed", + "+37255550001", + prefs.getString(prefKey, null), + ) + } + + @Test + fun invokeStoresANumberLibphonenumberOnlyConsidersPossible() { + runTest { + setPhoneNumber(POSSIBLE_BUT_UNASSIGNED_NUMBER) + + assertEquals( + "libphonenumber's metadata trails real numbering plans, and this preference is" + + " the only way to set a number the SIM does not carry, so a number of a" + + " plausible length is stored rather than refused", + "+3721234567", + prefs.getString(prefKey, null), + ) + } + } + + @Test + fun invokeClearsTheOverrideWhenTheFieldIsEmptied() = runTest { + prefs.putString(prefKey, "+3725550001") + + setPhoneNumber("") + + assertNull( + "emptying the field is the documented way back to the number the SIM reports", + prefs.getString(prefKey, null), + ) + } + + @Test + fun invokeClearsTheOverrideWhenTheNumberMatchesTheSim() = runTest { + prefs.putString(prefKey, "+3725550001") + + setPhoneNumber(SIM_NUMBER) + + assertNull( + "an override that repeats the SIM's own number is not an override", + prefs.getString(prefKey, null), + ) + } + + private suspend fun TestScope.setPhoneNumber(phoneNumber: String) { + SetSubscriptionPhoneNumberImpl( + context = context, + ioDispatcher = StandardTestDispatcher(testScheduler), + ).invoke(subId = SubId(SUB_ID), phoneNumber = phoneNumber) + } + + private fun givenSimOnEstonianCarrier() { + val subscriptionManager = context.getSystemService(SubscriptionManager::class.java) + shadowOf(subscriptionManager).setActiveSubscriptionInfos( + ShadowSubscriptionManager.SubscriptionInfoBuilder.newBuilder() + .setId(SUB_ID) + .setCountryIso("ee") + .setNumber(SIM_NUMBER) + .buildSubscriptionInfo(), + ) + } + + private companion object { + private const val SUB_ID = 2 + private const val SIM_NUMBER = "+37253953334" + + /** Estonian landline length, on a prefix the numbering plan does not assign. */ + private const val POSSIBLE_BUT_UNASSIGNED_NUMBER = "1234567" + } +} diff --git a/app/src/test/kotlin/com/android/messaging/ui/appsettings/subscription/SubscriptionSettingsViewModelTest.kt b/app/src/test/kotlin/com/android/messaging/ui/appsettings/subscription/SubscriptionSettingsViewModelTest.kt index a07fb58c6..5e28f4bb6 100644 --- a/app/src/test/kotlin/com/android/messaging/ui/appsettings/subscription/SubscriptionSettingsViewModelTest.kt +++ b/app/src/test/kotlin/com/android/messaging/ui/appsettings/subscription/SubscriptionSettingsViewModelTest.kt @@ -3,8 +3,10 @@ package com.android.messaging.ui.appsettings.subscription import androidx.lifecycle.SavedStateHandle import app.cash.turbine.test import com.android.messaging.data.subscription.model.SubId +import com.android.messaging.domain.subscriptionsettings.usecase.IsValidSelfPhoneNumber import com.android.messaging.testutil.MainDispatcherRule import com.android.messaging.ui.appsettings.subscription.delegate.SubscriptionSettingsDelegate +import com.android.messaging.ui.appsettings.subscription.model.PhoneNumberDialogUiState import com.android.messaging.ui.appsettings.subscription.model.SubscriptionSettingsAction as Action import com.android.messaging.ui.appsettings.subscription.model.SubscriptionSettingsNavEvent import com.android.messaging.ui.appsettings.subscription.model.SubscriptionSettingsScreenEffect @@ -77,16 +79,22 @@ class SubscriptionSettingsViewModelTest { } @Test - fun onPhoneNumberChanged_delegatesWithSeededSubId() { + fun onPhoneNumberConfirmed_delegatesWithSeededSubId() { runTest(context = mainDispatcherRule.testDispatcher) { val delegate = mockDelegate() val viewModel = createViewModel(delegate = delegate, subId = 1) - viewModel.onAction(Action.PhoneNumberChanged(phoneNumber = "+1555000111")) + viewModel.onAction(Action.PhoneNumberClicked) + viewModel.onAction(Action.PhoneNumberConfirmed(phoneNumber = "+1555000111")) verify(exactly = 1) { delegate.onPhoneNumberChanged(subId = SubId(1), phoneNumber = "+1555000111") } + assertEquals( + "a number that was stored leaves nothing to correct, so the dialog closes", + PhoneNumberDialogUiState(), + viewModel.phoneNumberDialogState.value, + ) } } @@ -142,12 +150,62 @@ class SubscriptionSettingsViewModelTest { } } + @Test + fun onPhoneNumberConfirmed_whenNotANumber_marksTheDialogInvalidAndKeepsItOpen() { + runTest(context = mainDispatcherRule.testDispatcher) { + val delegate = mockDelegate() + val viewModel = createViewModel(delegate = delegate, isValidPhoneNumber = false) + viewModel.onAction(Action.PhoneNumberClicked) + + viewModel.onAction(Action.PhoneNumberConfirmed(phoneNumber = "DROP TABLE messages")) + + assertEquals( + PhoneNumberDialogUiState(isVisible = true, isInvalid = true), + viewModel.phoneNumberDialogState.value, + ) + verify(exactly = 0) { + delegate.onPhoneNumberChanged(any(), any()) + } + } + } + + @Test + fun onPhoneNumberErrorDismissed_clearsTheRejection() { + runTest(context = mainDispatcherRule.testDispatcher) { + val viewModel = createViewModel(isValidPhoneNumber = false) + viewModel.onAction(Action.PhoneNumberClicked) + viewModel.onAction(Action.PhoneNumberConfirmed(phoneNumber = "DROP TABLE messages")) + + viewModel.onAction(Action.PhoneNumberErrorDismissed) + + assertEquals( + PhoneNumberDialogUiState(isVisible = true, isInvalid = false), + viewModel.phoneNumberDialogState.value, + ) + } + } + + @Test + fun onPhoneNumberDialogDismissed_closesTheDialogAndForgetsTheRejection() { + runTest(context = mainDispatcherRule.testDispatcher) { + val viewModel = createViewModel(isValidPhoneNumber = false) + viewModel.onAction(Action.PhoneNumberClicked) + viewModel.onAction(Action.PhoneNumberConfirmed(phoneNumber = "DROP TABLE messages")) + + viewModel.onAction(Action.PhoneNumberDialogDismissed) + + assertEquals(PhoneNumberDialogUiState(), viewModel.phoneNumberDialogState.value) + } + } + private fun createViewModel( delegate: SubscriptionSettingsDelegate = mockDelegate(), subId: Int = 1, + isValidPhoneNumber: Boolean = true, ): SubscriptionSettingsViewModel { return SubscriptionSettingsViewModel( subscriptionSettingsDelegate = delegate, + isValidSelfPhoneNumber = { _, _ -> isValidPhoneNumber }, savedStateHandle = SavedStateHandle( mapOf(SUBSCRIPTION_SETTINGS_SUB_ID_ARG to subId), ), diff --git a/app/src/test/kotlin/com/android/messaging/ui/appsettings/subscription/ui/PhoneNumberDialogTest.kt b/app/src/test/kotlin/com/android/messaging/ui/appsettings/subscription/ui/PhoneNumberDialogTest.kt new file mode 100644 index 000000000..c1fda2b6b --- /dev/null +++ b/app/src/test/kotlin/com/android/messaging/ui/appsettings/subscription/ui/PhoneNumberDialogTest.kt @@ -0,0 +1,87 @@ +package com.android.messaging.ui.appsettings.subscription.ui + +import androidx.compose.ui.test.assertIsDisplayed +import androidx.compose.ui.test.assertIsNotDisplayed +import androidx.compose.ui.test.junit4.v2.createComposeRule +import androidx.compose.ui.test.onNodeWithText +import androidx.compose.ui.test.performClick +import androidx.compose.ui.test.performTextInput +import com.android.common.test.helpers.targetContext +import com.android.messaging.R +import com.android.messaging.ui.core.AppTheme +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +@RunWith(RobolectricTestRunner::class) +class PhoneNumberDialogTest { + + @get:Rule + val composeTestRule = createComposeRule() + + private val errorText = targetContext.getString(R.string.invalid_self_phone_number) + private val okText = targetContext.getString(android.R.string.ok) + + private var confirmedNumber: String? = null + private var wasErrorDismissed = false + + @Test + fun dialogShowsTheRejectionItIsGivenWithoutLosingWhatWasTyped() { + setContent(currentNumber = "not a number", isInvalid = true) + + composeTestRule.onNodeWithText(errorText).assertIsDisplayed() + composeTestRule.onNodeWithText("not a number").assertIsDisplayed() + } + + @Test + fun dialogShowsNoErrorUntilItIsGivenOne() { + setContent(currentNumber = "+37255550001", isInvalid = false) + + composeTestRule.onNodeWithText(errorText).assertIsNotDisplayed() + } + + @Test + fun dialogHandsTheTypedNumberUpOnConfirm() { + setContent(currentNumber = "+37255550001", isInvalid = false) + + composeTestRule.onNodeWithText(okText).performClick() + + composeTestRule.runOnIdle { + assertEquals("+37255550001", confirmedNumber) + } + } + + @Test + fun dialogReportsAnEditSoTheRejectionCanBeCleared() { + setContent(currentNumber = "12", isInvalid = true) + + composeTestRule.onNodeWithText("12").performTextInput("3") + + composeTestRule.runOnIdle { + assertTrue( + "the dialog does not decide the error is gone, it reports the edit that clears it", + wasErrorDismissed, + ) + } + } + + private fun setContent( + currentNumber: String, + isInvalid: Boolean, + ) { + composeTestRule.setContent { + AppTheme { + PhoneNumberDialog( + currentNumber = currentNumber, + isInvalid = isInvalid, + onErrorDismissed = { wasErrorDismissed = true }, + onDismiss = {}, + onConfirm = { confirmedNumber = it }, + ) + } + } + } +} diff --git a/app/src/test/kotlin/com/android/messaging/util/PhoneUtilsSelfNumberTest.kt b/app/src/test/kotlin/com/android/messaging/util/PhoneUtilsSelfNumberTest.kt index f18d36294..6d962eeb8 100644 --- a/app/src/test/kotlin/com/android/messaging/util/PhoneUtilsSelfNumberTest.kt +++ b/app/src/test/kotlin/com/android/messaging/util/PhoneUtilsSelfNumberTest.kt @@ -8,6 +8,7 @@ import com.android.messaging.testutil.FakeBuglePrefs import com.android.messaging.testutil.installTestFactory import org.junit.After import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull import org.junit.Assert.assertThrows import org.junit.Before import org.junit.Test @@ -100,15 +101,85 @@ class PhoneUtilsSelfNumberTest { } } + @Test + fun getValidSelfE164NumberRejectsANumberLiftedOutOfText() { + givenSubscription(numberOnSim = SIM_NUMBER, country = "ee") + + assertNull( + "libphonenumber pulls the first number it finds out of surrounding text, and" + + " whatever is stored here becomes the sender identity of every outgoing MMS", + PhoneUtils(SUB_ID).getValidSelfE164Number("Call me at 5551234567"), + ) + } + + @Test + fun getValidSelfE164NumberRejectsAValidNumberLiftedOutOfText() { + givenSubscription(numberOnSim = SIM_NUMBER, country = "ee") + + assertNull( + "text around a number that is valid on its own is lifted just the same, so the" + + " strict branch cannot be trusted to have consumed the whole input either", + PhoneUtils(SUB_ID).getValidSelfE164Number("Call me at +37254810027"), + ) + } + + @Test + fun getValidSelfE164NumberRejectsAVanityNumber() { + givenSubscription(numberOnSim = SIM_NUMBER, country = "us") + + assertNull( + "libphonenumber turns the letters of a vanity number into digits, so accepting" + + " one would store a number nobody typed", + PhoneUtils(SUB_ID).getValidSelfE164Number("1-800-FLOWERS"), + ) + } + + @Test + fun getValidSelfE164NumberAcceptsANumberTheNumberingPlanDoesNotAssign() { + givenSubscription(numberOnSim = SIM_NUMBER, country = "ee") + + assertEquals( + "libphonenumber's metadata trails real numbering plans, and this is the only way" + + " to set a number the SIM does not carry", + "+3721234567", + PhoneUtils(SUB_ID).getValidSelfE164Number("1234567"), + ) + } + + @Test + fun getValidSelfE164NumberAcceptsANumberTypedWithSeparators() { + givenSubscription(numberOnSim = SIM_NUMBER, country = "ee") + + assertEquals( + "a number is normally typed with spaces and punctuation, so rejecting those" + + " would refuse ordinary input", + "+37255550001", + PhoneUtils(SUB_ID).getValidSelfE164Number("+372 (55) 55-0001"), + ) + } + + @Test + fun getValidSelfE164NumberDropsAnExtensionTypedAfterTheNumber() { + givenSubscription(numberOnSim = SIM_NUMBER, country = "ee") + + assertEquals( + "an extension is separated by punctuation rather than letters, so the guard" + + " cannot see it and libphonenumber keeps only the number it hangs off", + "+37255550001", + PhoneUtils(SUB_ID).getValidSelfE164Number("+372 5555 0001#123"), + ) + } + private fun subscriptionManager(): SubscriptionManager { return context.getSystemService(SubscriptionManager::class.java) } - private fun givenSubscription(numberOnSim: String) { + private fun givenSubscription(numberOnSim: String, country: String? = null) { shadowOf(subscriptionManager()).setActiveSubscriptionInfos( ShadowSubscriptionManager.SubscriptionInfoBuilder.newBuilder() .setId(SUB_ID) .setNumber(numberOnSim) + .apply { country?.let(::setCountryIso) } .buildSubscriptionInfo(), ) } diff --git a/res/values/strings.xml b/res/values/strings.xml index 11d8bfe81..9449ed0d2 100644 --- a/res/values/strings.xml +++ b/res/values/strings.xml @@ -385,6 +385,8 @@ Your phone number Unknown + + Not a valid phone number. Try the international format, starting with +. Outgoing message sounds diff --git a/src/com/android/messaging/di/settings/SettingsBindsModule.kt b/src/com/android/messaging/di/settings/SettingsBindsModule.kt index adba795f7..a8927b65c 100644 --- a/src/com/android/messaging/di/settings/SettingsBindsModule.kt +++ b/src/com/android/messaging/di/settings/SettingsBindsModule.kt @@ -4,6 +4,8 @@ import com.android.messaging.data.appsettings.repository.AppSettingsRepository import com.android.messaging.data.appsettings.repository.AppSettingsRepositoryImpl import com.android.messaging.data.subscriptionsettings.repository.SubscriptionSettingsRepository import com.android.messaging.data.subscriptionsettings.repository.SubscriptionSettingsRepositoryImpl +import com.android.messaging.domain.subscriptionsettings.usecase.IsValidSelfPhoneNumber +import com.android.messaging.domain.subscriptionsettings.usecase.IsValidSelfPhoneNumberImpl import com.android.messaging.domain.subscriptionsettings.usecase.SetSubscriptionPhoneNumber import com.android.messaging.domain.subscriptionsettings.usecase.SetSubscriptionPhoneNumberImpl import com.android.messaging.ui.appsettings.general.mapper.AppSettingsUiStateMapper @@ -49,4 +51,10 @@ internal abstract class SettingsBindsModule { abstract fun bindSetSubscriptionPhoneNumber( impl: SetSubscriptionPhoneNumberImpl, ): SetSubscriptionPhoneNumber + + @Binds + @Reusable + abstract fun bindIsValidSelfPhoneNumber( + impl: IsValidSelfPhoneNumberImpl, + ): IsValidSelfPhoneNumber } diff --git a/src/com/android/messaging/domain/subscriptionsettings/usecase/IsValidSelfPhoneNumber.kt b/src/com/android/messaging/domain/subscriptionsettings/usecase/IsValidSelfPhoneNumber.kt new file mode 100644 index 000000000..c5baaa02c --- /dev/null +++ b/src/com/android/messaging/domain/subscriptionsettings/usecase/IsValidSelfPhoneNumber.kt @@ -0,0 +1,19 @@ +package com.android.messaging.domain.subscriptionsettings.usecase + +import com.android.messaging.data.subscription.model.SubId +import com.android.messaging.util.PhoneUtils +import javax.inject.Inject + +internal fun interface IsValidSelfPhoneNumber { + operator fun invoke(subId: SubId, phoneNumber: String): Boolean +} + +internal class IsValidSelfPhoneNumberImpl @Inject constructor() : IsValidSelfPhoneNumber { + + override fun invoke(subId: SubId, phoneNumber: String): Boolean { + return phoneNumber.isEmpty() || + PhoneUtils + .get(subId.value) + .getValidSelfE164Number(phoneNumber) != null + } +} diff --git a/src/com/android/messaging/domain/subscriptionsettings/usecase/SetSubscriptionPhoneNumber.kt b/src/com/android/messaging/domain/subscriptionsettings/usecase/SetSubscriptionPhoneNumber.kt index 7bd317621..8c0f2d275 100644 --- a/src/com/android/messaging/domain/subscriptionsettings/usecase/SetSubscriptionPhoneNumber.kt +++ b/src/com/android/messaging/domain/subscriptionsettings/usecase/SetSubscriptionPhoneNumber.kt @@ -6,6 +6,7 @@ import com.android.messaging.data.subscription.model.SubId import com.android.messaging.datamodel.ParticipantRefresh import com.android.messaging.di.core.IoDispatcher import com.android.messaging.util.BuglePrefs +import com.android.messaging.util.LogUtil import com.android.messaging.util.PhoneUtils import dagger.hilt.android.qualifiers.ApplicationContext import javax.inject.Inject @@ -24,17 +25,26 @@ internal class SetSubscriptionPhoneNumberImpl @Inject constructor( override suspend fun invoke(subId: SubId, phoneNumber: String) { withContext(ioDispatcher) { val phoneUtils = PhoneUtils.get(subId.value) - val canonical = phoneUtils.getCanonicalBySystemLocale(phoneNumber) - val defaultCanonical = phoneUtils.getCanonicalBySystemLocale( - phoneUtils.getCanonicalForSelf(false), - ) - val key = context.getString(R.string.mms_phone_number_pref_key) val subPrefs = BuglePrefs.getSubscriptionPrefs(subId.value) - if (canonical == defaultCanonical || phoneNumber.isEmpty()) { - subPrefs.remove(key) - } else { - subPrefs.putString(key, phoneNumber) + + val e164Number = phoneNumber + .takeIf(String::isNotEmpty) + ?.let(phoneUtils::getValidSelfE164Number) + + when { + phoneNumber.isEmpty() -> subPrefs.remove(key) + + // The dialog rejects these before they get here. Guarded again because whatever + // is stored becomes the sender identity of every outgoing MMS. + e164Number == null -> { + LogUtil.w(LogUtil.BUGLE_TAG, "SetSubscriptionPhoneNumber: not a phone number") + return@withContext + } + + e164Number == phoneUtils.getCanonicalForSelf(false) -> subPrefs.remove(key) + + else -> subPrefs.putString(key, e164Number) } ParticipantRefresh.refreshSelfParticipants() diff --git a/src/com/android/messaging/ui/appsettings/navigation/SettingsNavEntries.kt b/src/com/android/messaging/ui/appsettings/navigation/SettingsNavEntries.kt index 5cc615ff1..71f6dcb89 100644 --- a/src/com/android/messaging/ui/appsettings/navigation/SettingsNavEntries.kt +++ b/src/com/android/messaging/ui/appsettings/navigation/SettingsNavEntries.kt @@ -177,6 +177,8 @@ private fun subscriptionSettingsRouteContent(): @Composable (SubscriptionSetting SeededViewModelStoreOwner(defaultArgs = defaultArgs) { val viewModel = hiltViewModel() val subscription by viewModel.uiState.collectAsStateWithLifecycle() + val phoneNumberDialogState by viewModel.phoneNumberDialogState + .collectAsStateWithLifecycle() val effectHandler = rememberSubscriptionSettingsEffectHandler() LifecycleEventEffect(event = Lifecycle.Event.ON_RESUME) { @@ -202,6 +204,7 @@ private fun subscriptionSettingsRouteContent(): @Composable (SubscriptionSetting title = navKey.title, onAction = viewModel::onAction, onNavigateBack = navigator::back, + phoneNumberDialogState = phoneNumberDialogState, ) } } diff --git a/src/com/android/messaging/ui/appsettings/subscription/SubscriptionSettingsViewModel.kt b/src/com/android/messaging/ui/appsettings/subscription/SubscriptionSettingsViewModel.kt index c541b61bb..1e4b335ab 100644 --- a/src/com/android/messaging/ui/appsettings/subscription/SubscriptionSettingsViewModel.kt +++ b/src/com/android/messaging/ui/appsettings/subscription/SubscriptionSettingsViewModel.kt @@ -4,7 +4,9 @@ import androidx.lifecycle.SavedStateHandle import androidx.lifecycle.ViewModel import androidx.lifecycle.viewModelScope import com.android.messaging.data.subscription.model.SubId +import com.android.messaging.domain.subscriptionsettings.usecase.IsValidSelfPhoneNumber import com.android.messaging.ui.appsettings.subscription.delegate.SubscriptionSettingsDelegate +import com.android.messaging.ui.appsettings.subscription.model.PhoneNumberDialogUiState import com.android.messaging.ui.appsettings.subscription.model.SubscriptionSettingsAction as Action import com.android.messaging.ui.appsettings.subscription.model.SubscriptionSettingsNavEvent as NavEvent import com.android.messaging.ui.appsettings.subscription.model.SubscriptionSettingsScreenEffect as Effect @@ -14,12 +16,15 @@ import dagger.hilt.android.lifecycle.HiltViewModel import javax.inject.Inject import kotlinx.coroutines.channels.Channel import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.SharingStarted import kotlinx.coroutines.flow.StateFlow +import kotlinx.coroutines.flow.asStateFlow import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.receiveAsFlow import kotlinx.coroutines.flow.stateIn +import kotlinx.coroutines.flow.update import kotlinx.coroutines.launch internal const val SUBSCRIPTION_SETTINGS_SUB_ID_ARG = "subId" @@ -27,6 +32,7 @@ internal const val SUBSCRIPTION_SETTINGS_SUB_ID_ARG = "subId" @HiltViewModel internal class SubscriptionSettingsViewModel @Inject constructor( private val subscriptionSettingsDelegate: SubscriptionSettingsDelegate, + private val isValidSelfPhoneNumber: IsValidSelfPhoneNumber, savedStateHandle: SavedStateHandle, ) : ViewModel() { @@ -39,6 +45,10 @@ internal class SubscriptionSettingsViewModel @Inject constructor( private val _effects = Channel(Channel.BUFFERED) val effects: Flow = _effects.receiveAsFlow() + private val _phoneNumberDialogState = MutableStateFlow(PhoneNumberDialogUiState()) + val phoneNumberDialogState: StateFlow = + _phoneNumberDialogState.asStateFlow() + private val _navigationEvents = Channel(Channel.BUFFERED) val navigationEvents: Flow = _navigationEvents.receiveAsFlow() @@ -80,8 +90,20 @@ internal class SubscriptionSettingsViewModel @Inject constructor( subscriptionSettingsDelegate.onGroupMmsChanged(subId, action.enabled) } - is Action.PhoneNumberChanged -> { - subscriptionSettingsDelegate.onPhoneNumberChanged(subId, action.phoneNumber) + Action.PhoneNumberClicked -> { + _phoneNumberDialogState.value = PhoneNumberDialogUiState(isVisible = true) + } + + Action.PhoneNumberDialogDismissed -> { + _phoneNumberDialogState.value = PhoneNumberDialogUiState() + } + + Action.PhoneNumberErrorDismissed -> { + _phoneNumberDialogState.update { it.copy(isInvalid = false) } + } + + is Action.PhoneNumberConfirmed -> { + onPhoneNumberConfirmed(action.phoneNumber) } Action.WirelessAlertsClicked -> { @@ -90,6 +112,16 @@ internal class SubscriptionSettingsViewModel @Inject constructor( } } + private fun onPhoneNumberConfirmed(phoneNumber: String) { + if (!isValidSelfPhoneNumber(subId, phoneNumber)) { + _phoneNumberDialogState.update { it.copy(isInvalid = true) } + return + } + + subscriptionSettingsDelegate.onPhoneNumberChanged(subId, phoneNumber) + _phoneNumberDialogState.value = PhoneNumberDialogUiState() + } + private fun subscriptionOrNull(state: SubscriptionSettingsUiState): SubscriptionUiState? { return state.subscriptions.find { it.subId == subId } } diff --git a/src/com/android/messaging/ui/appsettings/subscription/model/PhoneNumberDialogUiState.kt b/src/com/android/messaging/ui/appsettings/subscription/model/PhoneNumberDialogUiState.kt new file mode 100644 index 000000000..8d97999c3 --- /dev/null +++ b/src/com/android/messaging/ui/appsettings/subscription/model/PhoneNumberDialogUiState.kt @@ -0,0 +1,9 @@ +package com.android.messaging.ui.appsettings.subscription.model + +import androidx.compose.runtime.Immutable + +@Immutable +internal data class PhoneNumberDialogUiState( + val isVisible: Boolean = false, + val isInvalid: Boolean = false, +) diff --git a/src/com/android/messaging/ui/appsettings/subscription/model/SubscriptionSettingsAction.kt b/src/com/android/messaging/ui/appsettings/subscription/model/SubscriptionSettingsAction.kt index f87504612..217bc8449 100644 --- a/src/com/android/messaging/ui/appsettings/subscription/model/SubscriptionSettingsAction.kt +++ b/src/com/android/messaging/ui/appsettings/subscription/model/SubscriptionSettingsAction.kt @@ -4,6 +4,12 @@ internal sealed interface SubscriptionSettingsAction { data object WirelessAlertsClicked : SubscriptionSettingsAction + data object PhoneNumberClicked : SubscriptionSettingsAction + + data object PhoneNumberDialogDismissed : SubscriptionSettingsAction + + data object PhoneNumberErrorDismissed : SubscriptionSettingsAction + data class AutoRetrieveMmsChanged( val enabled: Boolean, ) : SubscriptionSettingsAction @@ -20,7 +26,7 @@ internal sealed interface SubscriptionSettingsAction { val enabled: Boolean, ) : SubscriptionSettingsAction - data class PhoneNumberChanged( + data class PhoneNumberConfirmed( val phoneNumber: String, ) : SubscriptionSettingsAction } diff --git a/src/com/android/messaging/ui/appsettings/subscription/ui/SubscriptionSettingsScreen.kt b/src/com/android/messaging/ui/appsettings/subscription/ui/SubscriptionSettingsScreen.kt index 09c11bc10..5651601e8 100644 --- a/src/com/android/messaging/ui/appsettings/subscription/ui/SubscriptionSettingsScreen.kt +++ b/src/com/android/messaging/ui/appsettings/subscription/ui/SubscriptionSettingsScreen.kt @@ -27,6 +27,7 @@ import androidx.compose.runtime.Composable import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember +import androidx.compose.runtime.saveable.rememberSaveable import androidx.compose.runtime.setValue import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier @@ -42,6 +43,7 @@ import com.android.messaging.ui.appsettings.common.SettingsCategoryHeader import com.android.messaging.ui.appsettings.common.SettingsClickableItem import com.android.messaging.ui.appsettings.common.SettingsSwitchItem import com.android.messaging.ui.appsettings.common.SettingsTopAppBar +import com.android.messaging.ui.appsettings.subscription.model.PhoneNumberDialogUiState import com.android.messaging.ui.appsettings.subscription.model.SubscriptionSettingsAction as Action import com.android.messaging.ui.appsettings.subscription.model.SubscriptionUiState import com.android.messaging.ui.common.text.asLtrText @@ -54,11 +56,11 @@ internal fun SubscriptionSettingsScreen( title: String, onAction: (Action) -> Unit, onNavigateBack: () -> Unit, + phoneNumberDialogState: PhoneNumberDialogUiState, modifier: Modifier = Modifier, ) { val scrollBehavior = TopAppBarDefaults.exitUntilCollapsedScrollBehavior() var showGroupMmsDialog by remember { mutableStateOf(false) } - var showPhoneNumberDialog by remember { mutableStateOf(false) } Scaffold( modifier = modifier.nestedScroll(scrollBehavior.nestedScrollConnection), @@ -78,7 +80,7 @@ internal fun SubscriptionSettingsScreen( subscriptionSettings = subscriptionSettings, onAction = onAction, onGroupMmsClick = { showGroupMmsDialog = true }, - onPhoneNumberClick = { showPhoneNumberDialog = true }, + onPhoneNumberClick = { onAction(Action.PhoneNumberClicked) }, ) advancedSettingsItems( subscriptionSettings = subscriptionSettings, @@ -90,10 +92,9 @@ internal fun SubscriptionSettingsScreen( SubscriptionDialogs( subscriptionSettings = subscriptionSettings, onAction = onAction, + phoneNumberDialogState = phoneNumberDialogState, showGroupMmsDialog = showGroupMmsDialog, onDismissGroupMms = { showGroupMmsDialog = false }, - showPhoneNumberDialog = showPhoneNumberDialog, - onDismissPhoneNumber = { showPhoneNumberDialog = false }, ) } @@ -101,10 +102,9 @@ internal fun SubscriptionSettingsScreen( private fun SubscriptionDialogs( subscriptionSettings: SubscriptionUiState, onAction: (Action) -> Unit, + phoneNumberDialogState: PhoneNumberDialogUiState, showGroupMmsDialog: Boolean, onDismissGroupMms: () -> Unit, - showPhoneNumberDialog: Boolean, - onDismissPhoneNumber: () -> Unit, ) { if (showGroupMmsDialog) { GroupMmsDialog( @@ -119,17 +119,18 @@ private fun SubscriptionDialogs( ) } - if (showPhoneNumberDialog) { + if (phoneNumberDialogState.isVisible) { PhoneNumberDialog( currentNumber = subscriptionSettings.phoneNumber.ifEmpty { subscriptionSettings.defaultPhoneNumber }, - onDismiss = onDismissPhoneNumber, + isInvalid = phoneNumberDialogState.isInvalid, + onErrorDismissed = { onAction(Action.PhoneNumberErrorDismissed) }, + onDismiss = { onAction(Action.PhoneNumberDialogDismissed) }, onConfirm = { phoneNumber -> onAction( - Action.PhoneNumberChanged(phoneNumber), + Action.PhoneNumberConfirmed(phoneNumber), ) - onDismissPhoneNumber() }, ) } @@ -350,12 +351,14 @@ private fun GroupMmsOption( } @Composable -private fun PhoneNumberDialog( +internal fun PhoneNumberDialog( currentNumber: String, + isInvalid: Boolean, + onErrorDismissed: () -> Unit, onDismiss: () -> Unit, onConfirm: (String) -> Unit, ) { - var phoneNumber by remember { mutableStateOf(currentNumber) } + var phoneNumber by rememberSaveable { mutableStateOf(currentNumber) } AlertDialog( onDismissRequest = onDismiss, @@ -365,10 +368,23 @@ private fun PhoneNumberDialog( text = { OutlinedTextField( value = phoneNumber, - onValueChange = { phoneNumber = it }, + onValueChange = { + phoneNumber = it + if (isInvalid) { + onErrorDismissed() + } + }, modifier = Modifier.fillMaxWidth(), keyboardOptions = KeyboardOptions(keyboardType = KeyboardType.Phone), singleLine = true, + isError = isInvalid, + supportingText = when { + isInvalid -> { + { Text(text = stringResource(R.string.invalid_self_phone_number)) } + } + + else -> null + }, ) }, confirmButton = { @@ -393,6 +409,7 @@ private fun SubscriptionSettingsScreenDefaultSmsPreview() { title = "SIM 1", onAction = {}, onNavigateBack = {}, + phoneNumberDialogState = PhoneNumberDialogUiState(), ) } } @@ -406,6 +423,7 @@ private fun SubscriptionSettingsScreenNotDefaultSmsPreview() { title = "SIM 2", onAction = {}, onNavigateBack = {}, + phoneNumberDialogState = PhoneNumberDialogUiState(), ) } } @@ -440,6 +458,8 @@ private fun PhoneNumberDialogPreview() { MessagingPreviewTheme { PhoneNumberDialog( currentNumber = "+31 6 1234 5678", + isInvalid = false, + onErrorDismissed = {}, onDismiss = {}, onConfirm = {}, ) diff --git a/src/com/android/messaging/util/PhoneUtils.java b/src/com/android/messaging/util/PhoneUtils.java index b512f4161..470780abd 100644 --- a/src/com/android/messaging/util/PhoneUtils.java +++ b/src/com/android/messaging/util/PhoneUtils.java @@ -54,10 +54,10 @@ * This class abstracts away platform dependency of calling telephony related * platform APIs, mostly involving TelephonyManager, SubscriptionManager and * a bit of SmsManager. - * + *

* The class instance can only be obtained via the get(int subId) method parameterized * by a SIM subscription ID. - * + *

* A convenient getDefault() method is provided for default subId (-1) on any platform */ public class PhoneUtils { @@ -209,7 +209,7 @@ private String getCarrierKnownNumber(final int subId) { /** * Returns the "effective" subId, or the subId used in the context of actual messages, * conversations and subscription-specific settings, for the given "nominal" sub id. - * + *

* DEFAULT_SELF_SUB_ID will be mapped to the system default subscription id for SMS. * * @param subId The input subId @@ -262,10 +262,10 @@ public boolean getHasPreferredSmsSim() { /** * System may return a negative subId. Convert this into our own subId, so that we consistently * use -1 for invalid or default. - * + *

* see b/18629526 and b/18670346 * - * @param intent The push intent from system + * @param intent The push intent from system * @param extraName The name of the sub id extra * @return the subId that is valid and meaningful for the app */ @@ -277,7 +277,7 @@ public int getEffectiveIncomingSubIdFromSystem(Intent intent, String extraName) /** * Get the subscription_id column value from a telephony provider cursor * - * @param cursor The database query cursor + * @param cursor The database query cursor * @param subIdIndex The index of the subId column in the cursor * @return the subscription_id column value from the cursor */ @@ -497,7 +497,7 @@ private static String getCanonicalFromCache(final String phoneText, String count // Put canonicalized phone number into cache private static void putCanonicalToCache(final String phoneText, String country, - final String canonical) { + final String canonical) { synchronized (sCanonicalPhoneNumberCache) { final ArrayMap countryMap = getOrAddCountryMapInCacheLocked(country); countryMap.put(phoneText, canonical); @@ -508,7 +508,7 @@ private static void putCanonicalToCache(final String phoneText, String country, * Utility method to parse user input number into standard E164 number. * * @param phoneText Phone number text as input by user. - * @param country ISO country code based on which to parse the number. + * @param country ISO country code based on which to parse the number. * @return E164 phone number. Returns null in case parsing failed. */ @Nullable @@ -534,7 +534,32 @@ private static String getValidE164Number( } } catch (final NumberParseException e) { LogUtil.e(TAG, "PhoneUtils.getValidE164Number(): Not able to parse phone number " - + LogUtil.sanitizePII(phoneText) + " for country " + country); + + LogUtil.sanitizePII(phoneText) + " for country " + country); + } + + return null; + } + + @Nullable + public String getValidSelfE164Number(@NonNull final String phoneText) { + if (phoneText.codePoints().anyMatch(Character::isLetter)) { + return null; + } + + final String country = getSimOrDefaultLocaleCountry(); + final String validNumber = getValidE164Number(phoneText, country); + if (validNumber != null) { + return validNumber; + } + + final PhoneNumberUtil phoneNumberUtil = PhoneNumberUtil.getInstance(); + try { + final PhoneNumber phoneNumber = phoneNumberUtil.parse(phoneText, country); + if (phoneNumberUtil.isPossibleNumber(phoneNumber)) { + return phoneNumberUtil.format(phoneNumber, PhoneNumberFormat.E164); + } + } catch (final NumberParseException e) { + // Not a phone number at all; getValidE164Number has already logged it. } return null; @@ -704,7 +729,7 @@ public List getCountryCandidatesForEnteredPhoneNumber() { * This uses an internal cache per country to speed up. * * @param phoneText The phone number to canonicalize - * @param country The ISO country code to use + * @param country The ISO country code to use * @return the canonicalized number, or the original number if can't be parsed */ private String getCanonicalByCountry(final String phoneText, final String country) { @@ -843,10 +868,8 @@ public String formatNormalizedDestinationUsingSimCountry(@Nullable final String @Nullable private static String formatForDisplayInternal( - @Nullable - final String phoneText, - @Nullable - final String country + @Nullable final String phoneText, + @Nullable final String country ) { // Only format a valid number which length >=6 if (TextUtils.isEmpty(phoneText) || From d1666b5ade80b2928114a6ec77d64825b0a6ed86 Mon Sep 17 00:00:00 2001 From: Artem Smirnov Date: Sat, 29 Aug 2026 18:17:43 +0300 Subject: [PATCH 17/26] Announce pinned conversations in the order they are shown --- ...ConversationListItemsTraversalOrderTest.kt | 133 ++++++++++++++++++ .../common/list/ConversationListItems.kt | 11 -- 2 files changed, 133 insertions(+), 11 deletions(-) create mode 100644 app/src/test/kotlin/com/android/messaging/ui/conversationlist/common/list/ConversationListItemsTraversalOrderTest.kt diff --git a/app/src/test/kotlin/com/android/messaging/ui/conversationlist/common/list/ConversationListItemsTraversalOrderTest.kt b/app/src/test/kotlin/com/android/messaging/ui/conversationlist/common/list/ConversationListItemsTraversalOrderTest.kt new file mode 100644 index 000000000..73b6df789 --- /dev/null +++ b/app/src/test/kotlin/com/android/messaging/ui/conversationlist/common/list/ConversationListItemsTraversalOrderTest.kt @@ -0,0 +1,133 @@ +package com.android.messaging.ui.conversationlist.common.list + +import androidx.compose.foundation.layout.PaddingValues +import androidx.compose.foundation.lazy.rememberLazyListState +import androidx.compose.ui.semantics.SemanticsProperties +import androidx.compose.ui.semantics.getOrNull +import androidx.compose.ui.test.SemanticsMatcher +import androidx.compose.ui.test.junit4.v2.createComposeRule +import androidx.compose.ui.unit.dp +import com.android.common.test.helpers.targetContext +import com.android.messaging.FactoryTestAccess +import com.android.messaging.data.conversation.model.ConversationId +import com.android.messaging.testutil.installTestFactory +import com.android.messaging.ui.conversationlist.common.item.ConversationSwipeKind +import com.android.messaging.ui.conversationlist.common.support.conversationListItemTestTag +import com.android.messaging.ui.conversationlist.common.support.previewConversationListItem +import com.android.messaging.ui.conversationlist.model.ConversationListItemUiModel +import com.android.messaging.ui.core.AppTheme +import kotlinx.collections.immutable.persistentSetOf +import kotlinx.collections.immutable.toImmutableList +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Before +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +@RunWith(RobolectricTestRunner::class) +internal class ConversationListItemsTraversalOrderTest { + + @get:Rule + val composeTestRule = createComposeRule() + + @Before + fun setUp() { + installTestFactory(context = targetContext) + } + + @After + fun tearDown() { + FactoryTestAccess.reset() + } + + @Test + fun rowsAreTraversedInListOrderWhenNothingIsPinned() { + val items = listOf( + item(conversationId = "a"), + item(conversationId = "b"), + item(conversationId = "c"), + ) + + setListContent(items) + + assertEquals(tagsOf(items), traversedRowTags(items)) + } + + @Test + fun pinnedRowIsTraversedFirstJustAsItIsRenderedFirst() { + val items = listOf( + item(conversationId = "c", isPinned = true), + item(conversationId = "a"), + item(conversationId = "b"), + ) + + setListContent(items) + + assertEquals(tagsOf(items), traversedRowTags(items)) + } + + @Test + fun pinnedRowsKeepTheirOrderAmongThemselvesAndStayAheadOfTheRest() { + val items = listOf( + item(conversationId = "c", isPinned = true), + item(conversationId = "b", isPinned = true), + item(conversationId = "a"), + ) + + setListContent(items) + + assertEquals(tagsOf(items), traversedRowTags(items)) + } + + private fun setListContent(items: List) { + composeTestRule.setContent { + AppTheme { + ConversationListItems( + items = items.toImmutableList(), + restoredConversationIds = persistentSetOf(), + listState = rememberLazyListState(), + isSelectionMode = false, + scaffoldContentPadding = PaddingValues(), + fabBottomReserve = 0.dp, + pinAnimationController = null, + swipeSpec = ConversationListSwipeSpec( + startToEnd = ConversationSwipeKind.ToggleRead, + endToStart = ConversationSwipeKind.Archive, + ), + onItemEvent = {}, + ) + } + } + } + + /** The row test tags in the order the semantics tree hands them out. */ + private fun traversedRowTags(items: List): List { + val rowTags = tagsOf(items).toSet() + val isConversationRow = SemanticsMatcher("is a conversation row") { node -> + node.config.getOrNull(SemanticsProperties.TestTag) in rowTags + } + + return composeTestRule + .onAllNodes(isConversationRow) + .fetchSemanticsNodes() + .map { node -> node.config[SemanticsProperties.TestTag] } + } + + private fun tagsOf(items: List): List { + return items.map { item -> conversationListItemTestTag(item.conversationId) } + } + + private fun item( + conversationId: String, + isPinned: Boolean = false, + ): ConversationListItemUiModel { + return previewConversationListItem( + conversationId = ConversationId(conversationId), + title = conversationId, + snippetText = conversationId, + isPinned = isPinned, + ) + } +} diff --git a/src/com/android/messaging/ui/conversationlist/common/list/ConversationListItems.kt b/src/com/android/messaging/ui/conversationlist/common/list/ConversationListItems.kt index e5c00b4fe..3e930ed74 100644 --- a/src/com/android/messaging/ui/conversationlist/common/list/ConversationListItems.kt +++ b/src/com/android/messaging/ui/conversationlist/common/list/ConversationListItems.kt @@ -24,7 +24,6 @@ import androidx.compose.ui.platform.testTag import androidx.compose.ui.unit.Dp import androidx.compose.ui.unit.IntOffset import androidx.compose.ui.unit.dp -import androidx.compose.ui.zIndex import com.android.messaging.data.conversation.model.ConversationId import com.android.messaging.ui.common.components.horizontalSafeDrawingInsets import com.android.messaging.ui.common.components.reorder.OverlayReorderAnimationController @@ -41,8 +40,6 @@ import kotlinx.collections.immutable.ImmutableSet private const val CONVERSATION_ROW_CONTENT_TYPE = "conversation_row" -private const val PINNED_ITEM_Z_INDEX = 1f - private val ItemPlacementSpec = spring( dampingRatio = Spring.DampingRatioNoBouncy, stiffness = Spring.StiffnessMediumLow, @@ -168,7 +165,6 @@ private fun LazyItemScope.ConversationListRow( modifier = Modifier .conversationItemAnimation( lazyItemScope = this, - isPinned = item.isPinned, animatePlacement = !isHiddenByPinAnimation, ) .trackPinAnimationBounds( @@ -413,16 +409,9 @@ private fun resolveAnchorScrollRequest( private fun Modifier.conversationItemAnimation( lazyItemScope: LazyItemScope, - isPinned: Boolean, animatePlacement: Boolean, ): Modifier = with(lazyItemScope) { this@conversationItemAnimation - .zIndex( - when { - isPinned -> PINNED_ITEM_Z_INDEX - else -> 0f - }, - ) .animateItem( fadeInSpec = null, fadeOutSpec = null, From 8b2ff750a8573764a18e1da09994dbb8d5d394fa Mon Sep 17 00:00:00 2001 From: Artem Smirnov Date: Sat, 29 Aug 2026 19:43:00 +0300 Subject: [PATCH 18/26] Decide selection toggles from the whole selection, not the first row --- .../ConversationListSelectionTopAppBarTest.kt | 73 +++++++++++++ .../ConversationListUiStateMapperImplTest.kt | 102 +++++++++++++----- .../KotlinCollectionExtensionsTest.kt | 35 ++++++ .../ConversationListSelectionTopAppBar.kt | 24 ++--- .../mapper/ConversationListUiStateMapper.kt | 8 +- .../model/ConversationListSelectionUiState.kt | 6 +- .../extension/KotlinCollectionExtensions.kt | 8 ++ 7 files changed, 209 insertions(+), 47 deletions(-) create mode 100644 app/src/test/kotlin/com/android/messaging/ui/conversationlist/chats/ConversationListSelectionTopAppBarTest.kt create mode 100644 app/src/test/kotlin/com/android/messaging/util/core/extension/KotlinCollectionExtensionsTest.kt create mode 100644 src/com/android/messaging/util/core/extension/KotlinCollectionExtensions.kt diff --git a/app/src/test/kotlin/com/android/messaging/ui/conversationlist/chats/ConversationListSelectionTopAppBarTest.kt b/app/src/test/kotlin/com/android/messaging/ui/conversationlist/chats/ConversationListSelectionTopAppBarTest.kt new file mode 100644 index 000000000..d60b240e5 --- /dev/null +++ b/app/src/test/kotlin/com/android/messaging/ui/conversationlist/chats/ConversationListSelectionTopAppBarTest.kt @@ -0,0 +1,73 @@ +package com.android.messaging.ui.conversationlist.chats + +import androidx.compose.ui.test.junit4.v2.createComposeRule +import androidx.compose.ui.test.onNodeWithContentDescription +import androidx.compose.ui.test.onNodeWithText +import androidx.compose.ui.test.performClick +import com.android.common.test.helpers.targetContext +import com.android.messaging.R +import com.android.messaging.ui.conversationlist.chats.model.ConversationListAction as Action +import com.android.messaging.ui.conversationlist.chats.model.SelectionActionsUiState +import com.android.messaging.ui.core.AppTheme +import org.junit.Assert.assertEquals +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +/** + * The read toggle is the one whose label and action have to move together: a selection that is not + * entirely read must offer *mark as read*, and tapping that must not mark the selection unread. + */ +@RunWith(RobolectricTestRunner::class) +internal class ConversationListSelectionTopAppBarTest { + + @get:Rule + val composeTestRule = createComposeRule() + + private val emittedActions = mutableListOf() + + @Test + fun aSelectionThatIsNotEntirelyReadOffersToMarkItRead() { + setSelectionContent(allSelectedAreRead = false) + + openOverflowMenu() + composeTestRule.onNodeWithText(string(R.string.mark_as_read)).performClick() + + assertEquals(listOf(Action.MarkReadClicked), emittedActions) + } + + @Test + fun anEntirelyReadSelectionOffersToMarkItUnread() { + setSelectionContent(allSelectedAreRead = true) + + openOverflowMenu() + composeTestRule.onNodeWithText(string(R.string.mark_as_unread)).performClick() + + assertEquals(listOf(Action.MarkUnreadClicked), emittedActions) + } + + private fun setSelectionContent(allSelectedAreRead: Boolean) { + composeTestRule.setContent { + AppTheme { + ConversationListSelectionTopAppBar( + selectedCount = 2, + actions = SelectionActionsUiState(allSelectedAreRead = allSelectedAreRead), + onAction = { action -> emittedActions += action }, + onDeleteClick = {}, + onSnoozeClick = {}, + ) + } + } + } + + private fun openOverflowMenu() { + composeTestRule + .onNodeWithContentDescription(string(R.string.more_options)) + .performClick() + } + + private fun string(resId: Int): String { + return targetContext.getString(resId) + } +} diff --git a/app/src/test/kotlin/com/android/messaging/ui/conversationlist/chats/mapper/ConversationListUiStateMapperImplTest.kt b/app/src/test/kotlin/com/android/messaging/ui/conversationlist/chats/mapper/ConversationListUiStateMapperImplTest.kt index 676a5e74f..ba42460b3 100644 --- a/app/src/test/kotlin/com/android/messaging/ui/conversationlist/chats/mapper/ConversationListUiStateMapperImplTest.kt +++ b/app/src/test/kotlin/com/android/messaging/ui/conversationlist/chats/mapper/ConversationListUiStateMapperImplTest.kt @@ -12,6 +12,7 @@ import com.android.messaging.domain.conversation.usecase.participant.CanShowOrAd import com.android.messaging.domain.conversation.usecase.participant.IsContactSaved import com.android.messaging.domain.conversation.usecase.telephony.CanPlacePhoneCall import com.android.messaging.ui.conversationlist.chats.model.ConversationListUiState +import com.android.messaging.ui.conversationlist.chats.model.SelectionActionsUiState import com.android.messaging.ui.conversationlist.conversationItem import com.android.messaging.ui.conversationlist.mapper.ConversationListContentUiStateMapperImpl import com.android.messaging.ui.conversationlist.mapper.ConversationListItemUiMapperImpl @@ -25,6 +26,7 @@ import io.mockk.mockkStatic import io.mockk.unmockkStatic import kotlinx.collections.immutable.persistentListOf import kotlinx.collections.immutable.persistentSetOf +import kotlinx.collections.immutable.toPersistentList import org.junit.After import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse @@ -119,9 +121,9 @@ internal class ConversationListUiStateMapperImplTest { val actions = state.selection.actions assertEquals(0, state.selection.selectedCount) - assertNull(actions.firstSelectedIsPinned) - assertNull(actions.firstSelectedIsSnoozed) - assertNull(actions.firstSelectedIsUnread) + assertNull(actions.allSelectedArePinned) + assertNull(actions.allSelectedAreSnoozed) + assertNull(actions.allSelectedAreRead) } @Test @@ -142,38 +144,52 @@ internal class ConversationListUiStateMapperImplTest { ) val actions = state.selection.actions - assertTrue(requireNotNull(actions.firstSelectedIsPinned)) - assertTrue(requireNotNull(actions.firstSelectedIsSnoozed)) - assertTrue(requireNotNull(actions.firstSelectedIsUnread)) + assertTrue(requireNotNull(actions.allSelectedArePinned)) + assertTrue(requireNotNull(actions.allSelectedAreSnoozed)) + assertFalse(requireNotNull(actions.allSelectedAreRead)) } @Test - fun map_mixedSelection_togglesFollowFirstSelectedConversation() { - val state = mapper.map( - snapshot = snapshotOf( - conversationItem( - conversationId = ConversationId("first"), - isPinned = false, - isSnoozed = false, - ), - conversationItem( - conversationId = ConversationId("second"), - isPinned = true, - isSnoozed = true, - ), + fun map_mixedSelection_offersTheSameTogglesWhicheverRowWasTappedFirst() { + val snapshot = mixedSnapshot() + + val plainTappedFirst = selectionActions(snapshot, "plain", "marked") + val markedTappedFirst = selectionActions(snapshot, "marked", "plain") + + assertEquals(plainTappedFirst, markedTappedFirst) + } + + @Test + fun map_mixedSelection_offersToPinSnoozeAndMarkTheWholeSelectionRead() { + val actions = selectionActions(mixedSnapshot(), "marked", "plain") + + assertFalse(requireNotNull(actions.allSelectedArePinned)) + assertFalse(requireNotNull(actions.allSelectedAreSnoozed)) + assertFalse(requireNotNull(actions.allSelectedAreRead)) + } + + @Test + fun map_selectionThatIsEntirelyPinnedSnoozedAndRead_offersTheOppositeActions() { + val snapshot = snapshotOf( + conversationItem( + conversationId = ConversationId("a"), + isPinned = true, + isSnoozed = true, + isRead = true, ), - selectedConversationIds = persistentListOf( - ConversationId("first"), - ConversationId("second") + conversationItem( + conversationId = ConversationId("b"), + isPinned = true, + isSnoozed = true, + isRead = true, ), - openedConversationId = null, - isScrollToTopVisible = false, - isDebugEnabled = false, ) - val actions = state.selection.actions - assertFalse(requireNotNull(actions.firstSelectedIsPinned)) - assertFalse(requireNotNull(actions.firstSelectedIsSnoozed)) + val actions = selectionActions(snapshot, "a", "b") + + assertTrue(requireNotNull(actions.allSelectedArePinned)) + assertTrue(requireNotNull(actions.allSelectedAreSnoozed)) + assertTrue(requireNotNull(actions.allSelectedAreRead)) } @Test @@ -326,6 +342,36 @@ internal class ConversationListUiStateMapperImplTest { assertFalse(state.selection.actions.canBlock) } + private fun mixedSnapshot(): ConversationListSnapshot { + return snapshotOf( + conversationItem( + conversationId = ConversationId("plain"), + isPinned = false, + isSnoozed = false, + isRead = true, + ), + conversationItem( + conversationId = ConversationId("marked"), + isPinned = true, + isSnoozed = true, + isRead = false, + ), + ) + } + + private fun selectionActions( + snapshot: ConversationListSnapshot, + vararg selectedIds: String, + ): SelectionActionsUiState { + return mapper.map( + snapshot = snapshot, + selectedConversationIds = selectedIds.map(::ConversationId).toPersistentList(), + openedConversationId = null, + isScrollToTopVisible = false, + isDebugEnabled = false, + ).selection.actions + } + private fun singleItem( state: ConversationListUiState, ): ConversationListItemUiModel { diff --git a/app/src/test/kotlin/com/android/messaging/util/core/extension/KotlinCollectionExtensionsTest.kt b/app/src/test/kotlin/com/android/messaging/util/core/extension/KotlinCollectionExtensionsTest.kt new file mode 100644 index 000000000..3c2eaeaee --- /dev/null +++ b/app/src/test/kotlin/com/android/messaging/util/core/extension/KotlinCollectionExtensionsTest.kt @@ -0,0 +1,35 @@ +package com.android.messaging.util.core.extension + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Test + +class KotlinCollectionExtensionsTest { + + @Test + fun allOrNull_emptyCollection_isNullRatherThanVacuouslyTrue() { + assertNull(emptyList().allOrNull { it > 0 }) + } + + @Test + fun allOrNull_everyElementMatches_isTrue() { + assertTrue(requireNotNull(listOf(1, 2, 3).allOrNull { it > 0 })) + } + + @Test + fun allOrNull_oneElementDoesNotMatch_isFalse() { + assertFalse(requireNotNull(listOf(1, -2, 3).allOrNull { it > 0 })) + } + + @Test + fun allOrNull_isIndependentOfElementOrder() { + val elements = listOf(1, -2, 3) + + assertEquals( + elements.allOrNull { it > 0 }, + elements.reversed().allOrNull { it > 0 }, + ) + } +} diff --git a/src/com/android/messaging/ui/conversationlist/chats/ConversationListSelectionTopAppBar.kt b/src/com/android/messaging/ui/conversationlist/chats/ConversationListSelectionTopAppBar.kt index b6c0518db..56e8d2c59 100644 --- a/src/com/android/messaging/ui/conversationlist/chats/ConversationListSelectionTopAppBar.kt +++ b/src/com/android/messaging/ui/conversationlist/chats/ConversationListSelectionTopAppBar.kt @@ -81,9 +81,9 @@ private fun ConversationListSelectionActions( onDeleteClick: () -> Unit, onSnoozeClick: () -> Unit, ) { - actions.firstSelectedIsSnoozed?.let { isSnoozed -> + actions.allSelectedAreSnoozed?.let { areSnoozed -> when { - isSnoozed -> SelectionActionButton( + areSnoozed -> SelectionActionButton( imageVector = Icons.Default.NotificationsActive, labelResId = R.string.unsnooze_chat_setting_title, onClick = { onAction(Action.UnsnoozeClicked) }, @@ -97,9 +97,9 @@ private fun ConversationListSelectionActions( } } - actions.firstSelectedIsPinned?.let { isPinned -> + actions.allSelectedArePinned?.let { arePinned -> when { - isPinned -> SelectionActionButton( + arePinned -> SelectionActionButton( imageVector = Icons.Outlined.PushPin, labelResId = R.string.action_unpin, onClick = { onAction(Action.UnpinClicked) }, @@ -137,16 +137,16 @@ private fun SelectionOverflowMenu( onAction: (Action) -> Unit, ) { OverflowMenu { dismiss -> - actions.firstSelectedIsUnread?.let { isUnread -> + actions.allSelectedAreRead?.let { areRead -> OverflowMenuItem( labelResId = when { - isUnread -> R.string.mark_as_read - else -> R.string.mark_as_unread + areRead -> R.string.mark_as_unread + else -> R.string.mark_as_read }, onClick = { val action = when { - isUnread -> Action.MarkReadClicked - else -> Action.MarkUnreadClicked + areRead -> Action.MarkUnreadClicked + else -> Action.MarkReadClicked } onAction(action) dismiss() @@ -209,9 +209,9 @@ private fun ConversationListSelectionTopAppBarPreview() { actions = SelectionActionsUiState( canAddContact = true, canBlock = true, - firstSelectedIsPinned = false, - firstSelectedIsSnoozed = false, - firstSelectedIsUnread = true, + allSelectedArePinned = false, + allSelectedAreSnoozed = false, + allSelectedAreRead = false, ), onAction = {}, onDeleteClick = {}, diff --git a/src/com/android/messaging/ui/conversationlist/chats/mapper/ConversationListUiStateMapper.kt b/src/com/android/messaging/ui/conversationlist/chats/mapper/ConversationListUiStateMapper.kt index 567d00659..6f6e458d6 100644 --- a/src/com/android/messaging/ui/conversationlist/chats/mapper/ConversationListUiStateMapper.kt +++ b/src/com/android/messaging/ui/conversationlist/chats/mapper/ConversationListUiStateMapper.kt @@ -9,6 +9,7 @@ import com.android.messaging.ui.conversationlist.chats.model.ConversationListUiS import com.android.messaging.ui.conversationlist.chats.model.SelectionActionsUiState import com.android.messaging.ui.conversationlist.mapper.ConversationListContentUiStateMapper import com.android.messaging.ui.conversationlist.model.ConversationListContentUiState +import com.android.messaging.util.core.extension.allOrNull import javax.inject.Inject import kotlinx.collections.immutable.ImmutableList import kotlinx.collections.immutable.ImmutableSet @@ -82,7 +83,6 @@ internal class ConversationListUiStateMapperImpl @Inject constructor( blockedDestinations: ImmutableSet, ): SelectionActionsUiState { val singleSelection = selectedItems.singleOrNull() - val firstSelected = selectedItems.firstOrNull() val canAddSelectedContact = singleSelection?.participant?.let { participant -> canAddContact( isGroup = participant.isGroup, @@ -100,9 +100,9 @@ internal class ConversationListUiStateMapperImpl @Inject constructor( return SelectionActionsUiState( canAddContact = canAddSelectedContact == true, canBlock = canBlockSelected == true, - firstSelectedIsPinned = firstSelected?.isPinned, - firstSelectedIsSnoozed = firstSelected?.notification?.isSnoozed, - firstSelectedIsUnread = firstSelected?.latestMessage?.isRead?.not(), + allSelectedArePinned = selectedItems.allOrNull(ConversationListItem::isPinned), + allSelectedAreSnoozed = selectedItems.allOrNull { it.notification.isSnoozed }, + allSelectedAreRead = selectedItems.allOrNull { it.latestMessage.isRead }, ) } diff --git a/src/com/android/messaging/ui/conversationlist/chats/model/ConversationListSelectionUiState.kt b/src/com/android/messaging/ui/conversationlist/chats/model/ConversationListSelectionUiState.kt index 4d2047f8a..e9c7c3ac7 100644 --- a/src/com/android/messaging/ui/conversationlist/chats/model/ConversationListSelectionUiState.kt +++ b/src/com/android/messaging/ui/conversationlist/chats/model/ConversationListSelectionUiState.kt @@ -12,7 +12,7 @@ internal data class ConversationListSelectionUiState( internal data class SelectionActionsUiState( val canAddContact: Boolean = false, val canBlock: Boolean = false, - val firstSelectedIsPinned: Boolean? = null, - val firstSelectedIsSnoozed: Boolean? = null, - val firstSelectedIsUnread: Boolean? = null, + val allSelectedArePinned: Boolean? = null, + val allSelectedAreSnoozed: Boolean? = null, + val allSelectedAreRead: Boolean? = null, ) diff --git a/src/com/android/messaging/util/core/extension/KotlinCollectionExtensions.kt b/src/com/android/messaging/util/core/extension/KotlinCollectionExtensions.kt new file mode 100644 index 000000000..298a9f784 --- /dev/null +++ b/src/com/android/messaging/util/core/extension/KotlinCollectionExtensions.kt @@ -0,0 +1,8 @@ +package com.android.messaging.util.core.extension + +fun Collection.allOrNull(predicate: (T) -> Boolean): Boolean? { + return when { + isEmpty() -> null + else -> all(predicate) + } +} From c5aa30dd840a1eba4f17c504645243d5e5d1fa7f Mon Sep 17 00:00:00 2001 From: Artem Smirnov Date: Sat, 29 Aug 2026 21:13:57 +0300 Subject: [PATCH 19/26] Let the attachment menu take back without taking the keyboard down --- .../composer/ui/ConversationComposeBarTest.kt | 71 +++++++++++++++++++ .../ui/ConversationComposeMessageField.kt | 11 ++- 2 files changed, 80 insertions(+), 2 deletions(-) diff --git a/app/src/test/kotlin/com/android/messaging/ui/conversation/composer/ui/ConversationComposeBarTest.kt b/app/src/test/kotlin/com/android/messaging/ui/conversation/composer/ui/ConversationComposeBarTest.kt index 8c77386e5..635206d6f 100644 --- a/app/src/test/kotlin/com/android/messaging/ui/conversation/composer/ui/ConversationComposeBarTest.kt +++ b/app/src/test/kotlin/com/android/messaging/ui/conversation/composer/ui/ConversationComposeBarTest.kt @@ -1,5 +1,7 @@ package com.android.messaging.ui.conversation.composer.ui +import android.view.View +import android.view.WindowManager import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.fillMaxSize import androidx.compose.runtime.Composable @@ -55,11 +57,14 @@ import io.mockk.mockk import io.mockk.runs import io.mockk.verify import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotEquals import org.junit.Assert.assertTrue import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith import org.robolectric.RobolectricTestRunner +import org.robolectric.shadow.api.Shadow +import org.robolectric.shadows.ShadowWindowManagerImpl @RunWith(RobolectricTestRunner::class) class ConversationComposeBarTest { @@ -754,6 +759,72 @@ class ConversationComposeBarTest { } } + /** + * Back is delivered to the focused window, so a `FLAG_NOT_FOCUSABLE` menu never sees it and + * the press reaches the navigation stack instead, closing the conversation along with the + * menu. `FLAG_ALT_FOCUSABLE_IM` is what lets the menu be focusable without taking the keyboard + * down with it — the media picker reads the keyboard state to decide whether to restore it. + * Robolectric cannot route key events between windows, so the assertions are on the window + * flags that behaviour depends on. + */ + @Test + fun attachmentMenu_isFocusableForBackButLeavesTheKeyboardUp() { + setContent( + messageText = "", + isSendActionEnabled = false, + isAttachmentActionEnabled = true, + ) + + val menuWindowFlags = openAttachmentMenuAndReadWindowFlags() + + assertEquals( + "the attachment menu window must be focusable to receive back", + 0, + menuWindowFlags and WindowManager.LayoutParams.FLAG_NOT_FOCUSABLE, + ) + assertNotEquals( + "the focused menu must not take the keyboard down with it", + 0, + menuWindowFlags and WindowManager.LayoutParams.FLAG_ALT_FOCUSABLE_IM, + ) + assertNotEquals( + "the bottom anchored menu must stay unclipped by the window it is anchored in", + 0, + menuWindowFlags and WindowManager.LayoutParams.FLAG_LAYOUT_NO_LIMITS, + ) + assertNotEquals( + "the menu must keep watching outside touches to stay tap dismissable", + 0, + menuWindowFlags and WindowManager.LayoutParams.FLAG_WATCH_OUTSIDE_TOUCH, + ) + } + + private fun openAttachmentMenuAndReadWindowFlags(): Int { + val windowsBeforeOpening = addedWindows() + + composeTestRule + .onNodeWithTag( + testTag = CONVERSATION_ATTACHMENT_BUTTON_TEST_TAG, + useUnmergedTree = true, + ) + .performClick() + composeTestRule + .onNodeWithTag(CONVERSATION_ATTACHMENT_MEDIA_MENU_ITEM_TEST_TAG) + .assertExists() + + val menuWindow = addedWindows().single { window -> + windowsBeforeOpening.none { openedEarlier -> openedEarlier === window } + } + + return (menuWindow.layoutParams as WindowManager.LayoutParams).flags + } + + private fun addedWindows(): List { + val windowManager = targetContext.getSystemService(WindowManager::class.java) + + return Shadow.extract(windowManager).views.toList() + } + private fun setContent( audioRecording: ConversationAudioRecordingUiState = ConversationAudioRecordingUiState(), messageText: String, diff --git a/src/com/android/messaging/ui/conversation/composer/ui/ConversationComposeMessageField.kt b/src/com/android/messaging/ui/conversation/composer/ui/ConversationComposeMessageField.kt index 3d2da396c..c34a67ab6 100644 --- a/src/com/android/messaging/ui/conversation/composer/ui/ConversationComposeMessageField.kt +++ b/src/com/android/messaging/ui/conversation/composer/ui/ConversationComposeMessageField.kt @@ -1,5 +1,6 @@ package com.android.messaging.ui.conversation.composer.ui +import android.view.WindowManager import androidx.annotation.StringRes import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.padding @@ -119,9 +120,15 @@ internal fun ConversationComposeAttachmentMenu( x = 0.dp, y = (-8).dp, ), + // Raw flags because no combination of the boolean options expresses this. The menu + // has to be focusable, otherwise back (key or gesture) never reaches it and closes the + // conversation instead; ALT_FOCUSABLE_IM then keeps the keyboard up behind the menu, + // which the media picker relies on to know whether to bring it back. NO_LIMITS keeps + // the bottom anchored menu unclipped, WATCH_OUTSIDE_TOUCH keeps tap-to-dismiss. properties = PopupProperties( - focusable = false, - clippingEnabled = false, + flags = WindowManager.LayoutParams.FLAG_ALT_FOCUSABLE_IM or + WindowManager.LayoutParams.FLAG_LAYOUT_NO_LIMITS or + WindowManager.LayoutParams.FLAG_WATCH_OUTSIDE_TOUCH, ), ) { ConversationComposeAttachmentMenuContent( From a0c2c2367346048d06fd7c6b797fb16122f14369 Mon Sep 17 00:00:00 2001 From: Artem Smirnov Date: Sat, 29 Aug 2026 21:53:08 +0300 Subject: [PATCH 20/26] Fix the subscription settings test for state-driven dialog visibility --- .../ui/SubscriptionSettingsScreenTest.kt | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/app/src/androidTest/kotlin/com/android/messaging/ui/appsettings/subscription/ui/SubscriptionSettingsScreenTest.kt b/app/src/androidTest/kotlin/com/android/messaging/ui/appsettings/subscription/ui/SubscriptionSettingsScreenTest.kt index 633c0241f..66f88efb7 100644 --- a/app/src/androidTest/kotlin/com/android/messaging/ui/appsettings/subscription/ui/SubscriptionSettingsScreenTest.kt +++ b/app/src/androidTest/kotlin/com/android/messaging/ui/appsettings/subscription/ui/SubscriptionSettingsScreenTest.kt @@ -12,6 +12,7 @@ import androidx.compose.ui.test.performClick import androidx.compose.ui.test.performScrollToNode import com.android.messaging.R import com.android.messaging.data.subscription.model.SubId +import com.android.messaging.ui.appsettings.subscription.model.PhoneNumberDialogUiState import com.android.messaging.ui.appsettings.subscription.model.SubscriptionSettingsAction as Action import com.android.messaging.ui.appsettings.subscription.model.SubscriptionUiState import com.android.messaging.ui.core.AppTheme @@ -92,7 +93,7 @@ class SubscriptionSettingsScreenTest { } @Test - fun phoneNumberClick_showsDialog() { + fun phoneNumberClick_reportsTheClick() { val sub = createDefaultSubscription(phoneNumber = "+1234567890") setContent(subscriptionSettings = sub) @@ -100,6 +101,17 @@ class SubscriptionSettingsScreenTest { composeTestRule.onNodeWithText(phoneTitle).performClick() composeTestRule.waitForIdle() + verify { onAction(Action.PhoneNumberClicked) } + } + + @Test + fun visiblePhoneNumberDialogState_showsDialog() { + val sub = createDefaultSubscription(phoneNumber = "+1234567890") + setContent( + subscriptionSettings = sub, + phoneNumberDialogState = PhoneNumberDialogUiState(isVisible = true), + ) + val okText = composeTestRule.activity.getString(android.R.string.ok) composeTestRule.onNodeWithText(okText).assertIsDisplayed() } @@ -273,6 +285,7 @@ class SubscriptionSettingsScreenTest { private fun setContent( subscriptionSettings: SubscriptionUiState = createDefaultSubscription(), + phoneNumberDialogState: PhoneNumberDialogUiState = PhoneNumberDialogUiState(), ) { composeTestRule.setContent { AppTheme { @@ -281,6 +294,7 @@ class SubscriptionSettingsScreenTest { title = "Advanced Settings", onAction = onAction, onNavigateBack = {}, + phoneNumberDialogState = phoneNumberDialogState, ) } } From 0d4885b8b2ac4f5a53c9d37fce21dff7b2d03995 Mon Sep 17 00:00:00 2001 From: Artem Smirnov Date: Sat, 29 Aug 2026 21:53:15 +0300 Subject: [PATCH 21/26] Warn that deleting a conversation cannot be undone --- .../ConversationScreenDeleteDialogsTest.kt | 13 ++++ .../ConversationListDeleteDialogTest.kt | 57 ++++++++++++++++ .../screen/BlockedParticipantsDialogs.kt | 67 +------------------ .../screen/ConversationScreenDialogs.kt | 33 +-------- .../dialog/ConversationListDeleteDialog.kt | 3 + 5 files changed, 78 insertions(+), 95 deletions(-) create mode 100644 app/src/test/kotlin/com/android/messaging/ui/conversationlist/common/dialog/ConversationListDeleteDialogTest.kt diff --git a/app/src/test/kotlin/com/android/messaging/ui/conversation/screen/dialogs/ConversationScreenDeleteDialogsTest.kt b/app/src/test/kotlin/com/android/messaging/ui/conversation/screen/dialogs/ConversationScreenDeleteDialogsTest.kt index 8631e7383..e463b1019 100644 --- a/app/src/test/kotlin/com/android/messaging/ui/conversation/screen/dialogs/ConversationScreenDeleteDialogsTest.kt +++ b/app/src/test/kotlin/com/android/messaging/ui/conversation/screen/dialogs/ConversationScreenDeleteDialogsTest.kt @@ -58,6 +58,19 @@ internal class ConversationScreenDeleteDialogsTest : BaseConversationScreenDialo } } + @Test + fun deleteConversation_warnsThatItCannotBeUndone() { + setDialogsContent( + uiState = createDialogUiState( + isDeleteConversationConfirmationVisible = true, + ), + ) + + composeTestRule + .onNodeWithText(text(R.string.delete_message_confirmation_dialog_text)) + .assertIsDisplayed() + } + @Test fun deleteMessages_multiMessageUsesPluralAndForwardsCallbacks() { val messageIds = persistentSetOf( diff --git a/app/src/test/kotlin/com/android/messaging/ui/conversationlist/common/dialog/ConversationListDeleteDialogTest.kt b/app/src/test/kotlin/com/android/messaging/ui/conversationlist/common/dialog/ConversationListDeleteDialogTest.kt new file mode 100644 index 000000000..cf16fd31e --- /dev/null +++ b/app/src/test/kotlin/com/android/messaging/ui/conversationlist/common/dialog/ConversationListDeleteDialogTest.kt @@ -0,0 +1,57 @@ +package com.android.messaging.ui.conversationlist.common.dialog + +import androidx.compose.ui.test.assertIsDisplayed +import androidx.compose.ui.test.junit4.v2.createComposeRule +import androidx.compose.ui.test.onNodeWithText +import com.android.common.test.helpers.targetContext +import com.android.messaging.R +import com.android.messaging.ui.core.AppTheme +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +/** + * Deleting a conversation is irreversible and is not covered by the archive undo snackbar, so it + * has to carry at least the warning that deleting a single message already carries. + */ +@RunWith(RobolectricTestRunner::class) +internal class ConversationListDeleteDialogTest { + + @get:Rule + val composeTestRule = createComposeRule() + + @Test + fun deletingOneConversationWarnsThatItCannotBeUndone() { + setDeleteDialogContent(selectedCount = 1) + + composeTestRule + .onNodeWithText(string(R.string.delete_message_confirmation_dialog_text)) + .assertIsDisplayed() + } + + @Test + fun deletingSeveralConversationsWarnsThatItCannotBeUndone() { + setDeleteDialogContent(selectedCount = 3) + + composeTestRule + .onNodeWithText(string(R.string.delete_message_confirmation_dialog_text)) + .assertIsDisplayed() + } + + private fun setDeleteDialogContent(selectedCount: Int) { + composeTestRule.setContent { + AppTheme { + ConversationListDeleteDialog( + selectedCount = selectedCount, + onConfirm = {}, + onDismiss = {}, + ) + } + } + } + + private fun string(resId: Int): String { + return targetContext.getString(resId) + } +} diff --git a/src/com/android/messaging/ui/blockedparticipants/screen/BlockedParticipantsDialogs.kt b/src/com/android/messaging/ui/blockedparticipants/screen/BlockedParticipantsDialogs.kt index b43dda9f3..ba7efb81c 100644 --- a/src/com/android/messaging/ui/blockedparticipants/screen/BlockedParticipantsDialogs.kt +++ b/src/com/android/messaging/ui/blockedparticipants/screen/BlockedParticipantsDialogs.kt @@ -1,15 +1,8 @@ package com.android.messaging.ui.blockedparticipants.screen -import androidx.compose.material3.AlertDialog -import androidx.compose.material3.Text -import androidx.compose.material3.TextButton import androidx.compose.runtime.Composable -import androidx.compose.ui.res.pluralStringResource -import androidx.compose.ui.res.stringResource -import androidx.compose.ui.tooling.preview.PreviewLightDark -import com.android.messaging.R import com.android.messaging.ui.blockedparticipants.screen.model.BlockedParticipantsAction as Action -import com.android.messaging.ui.core.MessagingPreviewTheme +import com.android.messaging.ui.conversationlist.common.dialog.ConversationListDeleteDialog @Composable internal fun BlockedParticipantsDialogs( @@ -19,7 +12,7 @@ internal fun BlockedParticipantsDialogs( onDismissDeleteConfirmation: () -> Unit, ) { if (showDeleteConfirmation) { - DeleteSelectedConfirmationDialog( + ConversationListDeleteDialog( selectedCount = selectedCount, onConfirm = { onAction(Action.DeleteSelectedConfirmed) @@ -29,59 +22,3 @@ internal fun BlockedParticipantsDialogs( ) } } - -@Composable -private fun DeleteSelectedConfirmationDialog( - selectedCount: Int, - onConfirm: () -> Unit, - onDismiss: () -> Unit, -) { - AlertDialog( - onDismissRequest = onDismiss, - title = { - Text( - text = pluralStringResource( - R.plurals.delete_conversations_confirmation_dialog_title, - selectedCount, - ), - ) - }, - text = { - Text(text = stringResource(R.string.delete_message_confirmation_dialog_text)) - }, - confirmButton = { - TextButton(onClick = onConfirm) { - Text(text = stringResource(R.string.delete_conversation_confirmation_button)) - } - }, - dismissButton = { - TextButton(onClick = onDismiss) { - Text(text = stringResource(R.string.delete_conversation_decline_button)) - } - }, - ) -} - -@PreviewLightDark -@Composable -private fun DeleteSelectedConfirmationDialogSinglePreview() { - MessagingPreviewTheme { - DeleteSelectedConfirmationDialog( - selectedCount = 1, - onConfirm = {}, - onDismiss = {}, - ) - } -} - -@PreviewLightDark -@Composable -private fun DeleteSelectedConfirmationDialogMultiplePreview() { - MessagingPreviewTheme { - DeleteSelectedConfirmationDialog( - selectedCount = 3, - onConfirm = {}, - onDismiss = {}, - ) - } -} diff --git a/src/com/android/messaging/ui/conversation/screen/ConversationScreenDialogs.kt b/src/com/android/messaging/ui/conversation/screen/ConversationScreenDialogs.kt index 301d63830..33dd29234 100644 --- a/src/com/android/messaging/ui/conversation/screen/ConversationScreenDialogs.kt +++ b/src/com/android/messaging/ui/conversation/screen/ConversationScreenDialogs.kt @@ -34,6 +34,7 @@ import com.android.messaging.ui.conversation.CONVERSATION_SUBJECT_DIALOG_TEXT_FI import com.android.messaging.ui.conversation.screen.model.ConversationAttachmentLimitWarning import com.android.messaging.ui.conversation.screen.model.ConversationMessageDeleteConfirmationUiState import com.android.messaging.ui.conversation.screen.model.ConversationScreenScaffoldUiState +import com.android.messaging.ui.conversationlist.common.dialog.ConversationListDeleteDialog @Composable internal fun ConversationScreenDialogs( @@ -57,7 +58,8 @@ internal fun ConversationScreenDialogs( } if (uiState.isDeleteConversationConfirmationVisible) { - ConversationDeleteConversationDialog( + ConversationListDeleteDialog( + selectedCount = 1, onConfirm = screenModel::confirmDeleteConversation, onDismiss = screenModel::dismissDeleteConversationConfirmation, ) @@ -129,35 +131,6 @@ private fun ConversationAttachmentLimitWarningDialog( ) } -@Composable -private fun ConversationDeleteConversationDialog( - onConfirm: () -> Unit, - onDismiss: () -> Unit, -) { - AlertDialog( - onDismissRequest = onDismiss, - title = { - Text( - text = pluralStringResource( - id = R.plurals.delete_conversations_confirmation_dialog_title, - count = 1, - 1, - ), - ) - }, - confirmButton = { - TextButton(onClick = onConfirm) { - Text(text = stringResource(R.string.delete_conversation_confirmation_button)) - } - }, - dismissButton = { - TextButton(onClick = onDismiss) { - Text(text = stringResource(R.string.delete_conversation_decline_button)) - } - }, - ) -} - @Composable private fun ConversationSubjectFieldDialog( initialSubjectText: String, diff --git a/src/com/android/messaging/ui/conversationlist/common/dialog/ConversationListDeleteDialog.kt b/src/com/android/messaging/ui/conversationlist/common/dialog/ConversationListDeleteDialog.kt index 787baf34a..05975a6cf 100644 --- a/src/com/android/messaging/ui/conversationlist/common/dialog/ConversationListDeleteDialog.kt +++ b/src/com/android/messaging/ui/conversationlist/common/dialog/ConversationListDeleteDialog.kt @@ -26,6 +26,9 @@ internal fun ConversationListDeleteDialog( ), ) }, + text = { + Text(text = stringResource(R.string.delete_message_confirmation_dialog_text)) + }, confirmButton = { TextButton(onClick = onConfirm) { Text(text = stringResource(R.string.delete_conversation_confirmation_button)) From 12800edfb220b1be14d0485607089d76e8c06f58 Mon Sep 17 00:00:00 2001 From: Artem Smirnov Date: Sat, 29 Aug 2026 22:27:59 +0300 Subject: [PATCH 22/26] Store the delivery report sent timestamp in milliseconds --- .../action/ProcessDeliveryReportActionTest.kt | 83 +++++++++++++++++++ .../action/ProcessDeliveryReportAction.java | 7 +- 2 files changed, 85 insertions(+), 5 deletions(-) create mode 100644 app/src/test/kotlin/com/android/messaging/datamodel/action/ProcessDeliveryReportActionTest.kt diff --git a/app/src/test/kotlin/com/android/messaging/datamodel/action/ProcessDeliveryReportActionTest.kt b/app/src/test/kotlin/com/android/messaging/datamodel/action/ProcessDeliveryReportActionTest.kt new file mode 100644 index 000000000..caad2acbc --- /dev/null +++ b/app/src/test/kotlin/com/android/messaging/datamodel/action/ProcessDeliveryReportActionTest.kt @@ -0,0 +1,83 @@ +package com.android.messaging.datamodel.action + +import android.content.ContentValues +import android.net.Uri +import android.provider.Telephony.Sms +import com.android.messaging.FactoryTestAccess +import com.android.messaging.datamodel.BugleDatabaseOperations +import com.android.messaging.datamodel.DataModel +import com.android.messaging.datamodel.DatabaseHelper.MessageColumns +import com.android.messaging.datamodel.MessagingContentProvider +import com.android.messaging.datamodel.data.MessageData +import com.android.messaging.sms.MmsUtils +import com.android.messaging.testutil.installTestFactory +import io.mockk.every +import io.mockk.just +import io.mockk.mockk +import io.mockk.mockkStatic +import io.mockk.runs +import io.mockk.slot +import io.mockk.unmockkAll +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment + +@RunWith(RobolectricTestRunner::class) +class ProcessDeliveryReportActionTest { + + private val dataModel = mockk(relaxed = true) + private val telephonyTimeSent = slot() + private val localValues = slot() + + @Before + fun setUp() { + installTestFactory( + context = RuntimeEnvironment.getApplication().applicationContext, + dataModel = dataModel, + ) + mockkStatic(MmsUtils::class) + every { + MmsUtils.updateSmsStatusAndDateSent(any(), any(), capture(telephonyTimeSent)) + } just runs + mockkStatic(BugleDatabaseOperations::class) + every { BugleDatabaseOperations.readMessageData(any(), any()) } returns + mockk { + every { messageId } returns MESSAGE_ID + every { conversationId } returns CONVERSATION_ID + every { smsMessageUri } returns SMS_MESSAGE_URI + } + every { + BugleDatabaseOperations.updateMessageRow(any(), any(), capture(localValues)) + } just runs + mockkStatic(MessagingContentProvider::class) + every { MessagingContentProvider.notifyMessagesChanged(any()) } just runs + } + + @After + fun tearDown() { + unmockkAll() + FactoryTestAccess.reset() + } + + @Test + fun deliveryReportStoresTheSameSentTimestampLocallyAsInTelephony() { + val before = System.currentTimeMillis() + ProcessDeliveryReportAction(SMS_MESSAGE_URI, Sms.STATUS_COMPLETE).executeAction() + val after = System.currentTimeMillis() + + val sentTimestamp = localValues.captured.getAsLong(MessageColumns.SENT_TIMESTAMP) + assertEquals(telephonyTimeSent.captured, sentTimestamp) + assertTrue(sentTimestamp in before..after) + } + + private companion object { + private val SMS_MESSAGE_URI: Uri = Uri.parse("content://sms/193") + private const val MESSAGE_ID = "193" + private const val CONVERSATION_ID = "17" + } +} diff --git a/src/com/android/messaging/datamodel/action/ProcessDeliveryReportAction.java b/src/com/android/messaging/datamodel/action/ProcessDeliveryReportAction.java index fbd4e82ed..8d5ca3ce3 100644 --- a/src/com/android/messaging/datamodel/action/ProcessDeliveryReportAction.java +++ b/src/com/android/messaging/datamodel/action/ProcessDeliveryReportAction.java @@ -33,15 +33,13 @@ import com.android.messaging.util.Assert; import com.android.messaging.util.LogUtil; -import java.util.concurrent.TimeUnit; - public class ProcessDeliveryReportAction extends Action implements Parcelable { private static final String TAG = LogUtil.BUGLE_DATAMODEL_TAG; private static final String KEY_URI = "uri"; private static final String KEY_STATUS = "status"; - private ProcessDeliveryReportAction(final Uri uri, final int status) { + ProcessDeliveryReportAction(final Uri uri, final int status) { actionParameters.putParcelable(KEY_URI, uri); actionParameters.putInt(KEY_STATUS, status); } @@ -76,8 +74,7 @@ protected Object executeAction() { final int bugleStatus = SyncMessageBatch.bugleStatusForSms(true /*outgoing*/, Telephony.Sms.MESSAGE_TYPE_SENT /* type */, status); values.put(DatabaseHelper.MessageColumns.STATUS, bugleStatus); - values.put(DatabaseHelper.MessageColumns.SENT_TIMESTAMP, - TimeUnit.MILLISECONDS.toMicros(timeSentInMillis)); + values.put(DatabaseHelper.MessageColumns.SENT_TIMESTAMP, timeSentInMillis); final MessageData messageData = BugleDatabaseOperations.readMessageData(db, smsMessageUri); From af471eb5d14ff46269ee93c49c0208b085f2a45b Mon Sep 17 00:00:00 2001 From: Artem Smirnov Date: Sat, 29 Aug 2026 23:47:43 +0300 Subject: [PATCH 23/26] Run instrumented tests from a known device state --- .../test/helpers/FirstRunFlagsHelper.kt | 32 ++++++++ .../common/test/helpers/SmsWarningHelper.kt | 27 ------- .../android/common/test/rules/AppTestRule.kt | 8 +- .../ui/ConversationComposeBarLayoutTest.kt | 79 +++++++++++-------- 4 files changed, 84 insertions(+), 62 deletions(-) create mode 100644 app/src/androidTest/kotlin/com/android/common/test/helpers/FirstRunFlagsHelper.kt delete mode 100644 app/src/androidTest/kotlin/com/android/common/test/helpers/SmsWarningHelper.kt diff --git a/app/src/androidTest/kotlin/com/android/common/test/helpers/FirstRunFlagsHelper.kt b/app/src/androidTest/kotlin/com/android/common/test/helpers/FirstRunFlagsHelper.kt new file mode 100644 index 000000000..496cf9513 --- /dev/null +++ b/app/src/androidTest/kotlin/com/android/common/test/helpers/FirstRunFlagsHelper.kt @@ -0,0 +1,32 @@ +package com.android.common.test.helpers + +import com.android.messaging.util.BuglePrefs +import com.android.messaging.util.BuglePrefsKeys + +/** + * Suppresses the one-off prompts a fresh install shows before the app is usable, so tests land on + * the screen under test instead of the SMS warning or the self phone number permission dialog. + */ +object FirstRunFlagsHelper { + + private val flagDefaults = mapOf( + BuglePrefsKeys.SMS_WARNING_ACKNOWLEDGED to + BuglePrefsKeys.SMS_WARNING_ACKNOWLEDGED_DEFAULT, + BuglePrefsKeys.SELF_PHONE_NUMBER_PERMISSION_REQUESTED to + BuglePrefsKeys.SELF_PHONE_NUMBER_PERMISSION_REQUESTED_DEFAULT, + ) + + fun suppressFirstRunPrompts(): Map { + val prefs = BuglePrefs.getApplicationPrefs() + + return flagDefaults.mapValues { (key, default) -> + prefs.getBoolean(key, default).also { prefs.putBoolean(key, true) } + } + } + + fun restoreFirstRunPrompts(previousFlags: Map) { + val prefs = BuglePrefs.getApplicationPrefs() + + previousFlags.forEach { (key, value) -> prefs.putBoolean(key, value) } + } +} diff --git a/app/src/androidTest/kotlin/com/android/common/test/helpers/SmsWarningHelper.kt b/app/src/androidTest/kotlin/com/android/common/test/helpers/SmsWarningHelper.kt deleted file mode 100644 index 4980b6292..000000000 --- a/app/src/androidTest/kotlin/com/android/common/test/helpers/SmsWarningHelper.kt +++ /dev/null @@ -1,27 +0,0 @@ -package com.android.common.test.helpers - -import com.android.messaging.util.BuglePrefs -import com.android.messaging.util.BuglePrefsKeys - -object SmsWarningHelper { - - fun acknowledgeSmsWarning(): Boolean { - val wasAcknowledged = BuglePrefs.getApplicationPrefs().getBoolean( - BuglePrefsKeys.SMS_WARNING_ACKNOWLEDGED, - BuglePrefsKeys.SMS_WARNING_ACKNOWLEDGED_DEFAULT, - ) - BuglePrefs.getApplicationPrefs().putBoolean( - BuglePrefsKeys.SMS_WARNING_ACKNOWLEDGED, - true, - ) - - return wasAcknowledged - } - - fun restoreSmsWarning(wasAcknowledged: Boolean) { - BuglePrefs.getApplicationPrefs().putBoolean( - BuglePrefsKeys.SMS_WARNING_ACKNOWLEDGED, - wasAcknowledged, - ) - } -} diff --git a/app/src/androidTest/kotlin/com/android/common/test/rules/AppTestRule.kt b/app/src/androidTest/kotlin/com/android/common/test/rules/AppTestRule.kt index de86fb699..5c7593506 100644 --- a/app/src/androidTest/kotlin/com/android/common/test/rules/AppTestRule.kt +++ b/app/src/androidTest/kotlin/com/android/common/test/rules/AppTestRule.kt @@ -1,7 +1,7 @@ package com.android.common.test.rules +import com.android.common.test.helpers.FirstRunFlagsHelper import com.android.common.test.helpers.ShellCommandHelper -import com.android.common.test.helpers.SmsWarningHelper import org.junit.rules.TestRule import org.junit.runner.Description import org.junit.runners.model.Statement @@ -15,7 +15,7 @@ class AppTestRule : TestRule { return object : Statement() { override fun evaluate() { val previousSmsRoleHolders = ShellCommandHelper.setupSmsDefaultRole() - val wasSmsWarningAcknowledged = SmsWarningHelper.acknowledgeSmsWarning() + val previousFirstRunFlags = FirstRunFlagsHelper.suppressFirstRunPrompts() var baseFailure: Throwable? = null try { base.evaluate() @@ -23,8 +23,8 @@ class AppTestRule : TestRule { baseFailure = throwable } finally { try { - SmsWarningHelper.restoreSmsWarning( - wasAcknowledged = wasSmsWarningAcknowledged, + FirstRunFlagsHelper.restoreFirstRunPrompts( + previousFlags = previousFirstRunFlags, ) ShellCommandHelper.restoreSmsDefaultRole( previousRoleHolders = previousSmsRoleHolders, diff --git a/app/src/androidTest/kotlin/com/android/messaging/ui/conversation/composer/ui/ConversationComposeBarLayoutTest.kt b/app/src/androidTest/kotlin/com/android/messaging/ui/conversation/composer/ui/ConversationComposeBarLayoutTest.kt index 85e25158b..cb8355922 100644 --- a/app/src/androidTest/kotlin/com/android/messaging/ui/conversation/composer/ui/ConversationComposeBarLayoutTest.kt +++ b/app/src/androidTest/kotlin/com/android/messaging/ui/conversation/composer/ui/ConversationComposeBarLayoutTest.kt @@ -2,11 +2,14 @@ package com.android.messaging.ui.conversation.composer.ui import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.fillMaxSize +import androidx.compose.runtime.CompositionLocalProvider import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier +import androidx.compose.ui.platform.LocalDensity import androidx.compose.ui.test.getUnclippedBoundsInRoot import androidx.compose.ui.test.junit4.v2.createComposeRule import androidx.compose.ui.test.onNodeWithTag +import androidx.compose.ui.unit.Density import androidx.test.ext.junit.runners.AndroidJUnit4 import com.android.messaging.domain.conversation.usecase.draft.model.ConversationDraftSendProtocol import com.android.messaging.ui.conversation.CONVERSATION_SEND_BUTTON_TEST_TAG @@ -25,37 +28,46 @@ class ConversationComposeBarLayoutTest { val composeTestRule = createComposeRule() @Test - fun singleLineInput_keepsTextFieldAndSendButtonHeightsEqual() { + fun singleLineInputAtDefaultFontScale_keepsTextFieldAndSendButtonHeightsEqual() { composeTestRule.setContent { - AppTheme { - Box( - modifier = Modifier.fillMaxSize(), - contentAlignment = Alignment.BottomCenter, - ) { - ConversationComposeBar( - audioRecording = ConversationAudioRecordingUiState(), - messageText = "Hello", - subjectText = "", - sendProtocol = ConversationDraftSendProtocol.SMS, - segmentCounter = null, - isMessageFieldEnabled = true, - isAttachmentActionEnabled = false, - isRecordActionEnabled = true, - isSendActionEnabled = true, - shouldShowRecordAction = false, - onContactAttachClick = {}, - onMediaPickerClick = {}, - onLockedAudioRecordingStartRequest = {}, - onMessageTextChange = {}, - onAudioRecordingStartRequest = {}, - onAudioRecordingFinish = {}, - onAudioRecordingLock = { false }, - onAudioRecordingCancel = {}, - onSendClick = {}, - onSendActionLongClick = {}, - onSubjectChipClick = {}, - onSubjectChipClear = {}, - ) + val density = LocalDensity.current + + CompositionLocalProvider( + LocalDensity provides Density( + density = density.density, + fontScale = DEFAULT_FONT_SCALE, + ), + ) { + AppTheme { + Box( + modifier = Modifier.fillMaxSize(), + contentAlignment = Alignment.BottomCenter, + ) { + ConversationComposeBar( + audioRecording = ConversationAudioRecordingUiState(), + messageText = "Hello", + subjectText = "", + sendProtocol = ConversationDraftSendProtocol.SMS, + segmentCounter = null, + isMessageFieldEnabled = true, + isAttachmentActionEnabled = false, + isRecordActionEnabled = true, + isSendActionEnabled = true, + shouldShowRecordAction = false, + onContactAttachClick = {}, + onMediaPickerClick = {}, + onLockedAudioRecordingStartRequest = {}, + onMessageTextChange = {}, + onAudioRecordingStartRequest = {}, + onAudioRecordingFinish = {}, + onAudioRecordingLock = { false }, + onAudioRecordingCancel = {}, + onSendClick = {}, + onSendActionLongClick = {}, + onSubjectChipClick = {}, + onSubjectChipClear = {}, + ) + } } } } @@ -74,7 +86,12 @@ class ConversationComposeBarLayoutTest { assertEquals( textFieldHeight.value, sendButtonHeight.value, - 0.5f, + HEIGHT_ASSERTION_DELTA_DP, ) } + + private companion object { + private const val DEFAULT_FONT_SCALE = 1f + private const val HEIGHT_ASSERTION_DELTA_DP = 0.5f + } } From c05d7f53872fdc6517db8d606b588c3637f7e74b Mon Sep 17 00:00:00 2001 From: Artem Smirnov Date: Sun, 30 Aug 2026 00:07:26 +0300 Subject: [PATCH 24/26] Run instrumented tests on an Android 17 emulator image --- .github/workflows/build.yml | 50 ++++++++++++++++++++++++++++--------- 1 file changed, 38 insertions(+), 12 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 090c1d900..5a85e4d3b 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -135,7 +135,7 @@ jobs: if-no-files-found: ignore instrumented-tests: - name: Instrumented tests (Android 16 x86_64) + name: Instrumented tests (Android 17 x86_64) runs-on: ubuntu-latest timeout-minutes: 45 @@ -152,12 +152,29 @@ jobs: java-version: 17 cache: gradle + - name: Build app and instrumented test APKs + run: > + ./gradlew :app:assembleDebug :app:assembleDebugAndroidTest + --no-daemon --stacktrace --console=plain + - name: Enable KVM run: | echo 'KERNEL=="kvm", GROUP="kvm", MODE="0666", OPTIONS+="static_node=kvm"' | sudo tee /etc/udev/rules.d/99-kvm4all.rules sudo udevadm control --reload-rules sudo udevadm trigger --name-match=kvm + - name: Update Android command line tools + id: cmdline-tools + run: | + SDK="${ANDROID_HOME:-/usr/local/lib/android/sdk}" + yes | "$SDK/cmdline-tools/latest/bin/sdkmanager" --install 'cmdline-tools;latest' > /dev/null + if [ -d "$SDK/cmdline-tools/latest-2" ]; then + rm -rf "$SDK/cmdline-tools/latest" + mv "$SDK/cmdline-tools/latest-2" "$SDK/cmdline-tools/latest" + fi + version=$("$SDK/cmdline-tools/latest/bin/sdkmanager" --version 2>/dev/null | grep -Eo '^[0-9][0-9.]*' | head -1) + echo "version=$version" >> "$GITHUB_OUTPUT" + - name: Restore AVD cache id: avd-cache uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 @@ -165,15 +182,19 @@ jobs: path: | ~/.android/avd/* ~/.android/adb* - key: avd-36-x86_64-default + key: avd-37.0-x86_64-google_apis-${{ steps.cmdline-tools.outputs.version }} - name: Create AVD and generate snapshot for caching if: steps.avd-cache.outputs.cache-hit != 'true' uses: reactivecircus/android-emulator-runner@a421e43855164a8197daf9d8d40fe71c6996bb0d # v2.38.0 with: - api-level: 36 + api-level: "37.0" arch: x86_64 - target: default + target: google_apis + cores: 4 + ram-size: 4096M + heap-size: 576M + disk-size: 8G force-avd-creation: false emulator-options: -no-window -gpu swiftshader_indirect -noaudio -no-boot-anim -camera-back none disable-animations: false @@ -182,18 +203,22 @@ jobs: - name: Run instrumented tests and coverage gate uses: reactivecircus/android-emulator-runner@a421e43855164a8197daf9d8d40fe71c6996bb0d # v2.38.0 with: - api-level: 36 + api-level: "37.0" arch: x86_64 - target: default + target: google_apis + cores: 4 + ram-size: 4096M + heap-size: 576M + disk-size: 8G force-avd-creation: false emulator-options: -no-snapshot-save -no-window -gpu swiftshader_indirect -noaudio -no-boot-anim -camera-back none disable-animations: true - script: > - ./gradlew :app:connectedDebugAndroidTest :app:jacocoAndroidTestVerification - -PandroidTestCoverage=true - -PandroidTestMinCoverage=80 - -PandroidTestMinBranchCoverage=50 - --no-daemon --stacktrace --console=plain + script: | + adb logcat -G 16M + ./gradlew :app:connectedDebugAndroidTest :app:jacocoAndroidTestVerification -PandroidTestCoverage=true -PandroidTestMinCoverage=80 -PandroidTestMinBranchCoverage=50 --no-daemon --stacktrace --console=plain + rc=$? + adb logcat -d > logcat.txt + exit $rc - name: Upload instrumented test reports if: failure() @@ -205,4 +230,5 @@ jobs: app/build/outputs/androidTest-results/connected/ app/build/reports/jacoco/jacocoAndroidTestReport/ app/build/reports/androidTests/connected/ + logcat.txt if-no-files-found: ignore From 939a2c45238f02d1a345b23a03221f6321440505 Mon Sep 17 00:00:00 2001 From: Artem Smirnov Date: Mon, 31 Aug 2026 13:40:54 +0300 Subject: [PATCH 25/26] Drop the dead null check in the message failure notification --- .../datamodel/MessageNotificationState.java | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/com/android/messaging/datamodel/MessageNotificationState.java b/src/com/android/messaging/datamodel/MessageNotificationState.java index 99f7f4c67..7407ed09d 100644 --- a/src/com/android/messaging/datamodel/MessageNotificationState.java +++ b/src/com/android/messaging/datamodel/MessageNotificationState.java @@ -871,13 +871,11 @@ public static void checkFailedMessages() { .setVisibility(NotificationCompat.VISIBILITY_PRIVATE) .setSound(UriUtil.getUriForResourceId(context, R.raw.message_failure)); - if (builder != null) { - notificationManager.notify( - BugleNotifications.buildNotificationTag( - PendingIntentConstants.MSG_SEND_ERROR, null), - PendingIntentConstants.MSG_SEND_ERROR, - builder.build()); - } + notificationManager.notify( + BugleNotifications.buildNotificationTag( + PendingIntentConstants.MSG_SEND_ERROR, null), + PendingIntentConstants.MSG_SEND_ERROR, + builder.build()); } else { notificationManager.cancel( BugleNotifications.buildNotificationTag( From f2e5935b1c0859b125e6b5bde3747e4ac72cb0f8 Mon Sep 17 00:00:00 2001 From: Artem Smirnov Date: Mon, 31 Aug 2026 13:40:54 +0300 Subject: [PATCH 26/26] Remember the group MMS dialog across configuration changes --- .../subscription/ui/SubscriptionSettingsScreen.kt | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/com/android/messaging/ui/appsettings/subscription/ui/SubscriptionSettingsScreen.kt b/src/com/android/messaging/ui/appsettings/subscription/ui/SubscriptionSettingsScreen.kt index 5651601e8..393e4995d 100644 --- a/src/com/android/messaging/ui/appsettings/subscription/ui/SubscriptionSettingsScreen.kt +++ b/src/com/android/messaging/ui/appsettings/subscription/ui/SubscriptionSettingsScreen.kt @@ -26,7 +26,6 @@ import androidx.compose.material3.TopAppBarDefaults import androidx.compose.runtime.Composable import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf -import androidx.compose.runtime.remember import androidx.compose.runtime.saveable.rememberSaveable import androidx.compose.runtime.setValue import androidx.compose.ui.Alignment @@ -60,7 +59,7 @@ internal fun SubscriptionSettingsScreen( modifier: Modifier = Modifier, ) { val scrollBehavior = TopAppBarDefaults.exitUntilCollapsedScrollBehavior() - var showGroupMmsDialog by remember { mutableStateOf(false) } + var showGroupMmsDialog by rememberSaveable { mutableStateOf(false) } Scaffold( modifier = modifier.nestedScroll(scrollBehavior.nestedScrollConnection), @@ -287,7 +286,7 @@ private fun GroupMmsDialog( onDismiss: () -> Unit, onConfirm: (Boolean) -> Unit, ) { - var selectedEnabled by remember { mutableStateOf(isEnabled) } + var selectedEnabled by rememberSaveable { mutableStateOf(isEnabled) } AlertDialog( onDismissRequest = onDismiss,