Rewrite SimImport to Kotlin + Compose by sdsantos · Pull Request #33 · GrapheneOS/platform_packages_apps_Contacts

sdsantos · 2026-06-26T12:03:56Z

Closes #34

Feature Changes:

When there are no selected contacts to import, the import button is disabled instead of hidden.
Added Select and Deselect All icon buttons to the topbar.
Split contacts in two lists: to import and already imported.

Video of the final result:

sim_import.mp4

Implementation Notes:

Not sure if the verification-metadata.xml my changes should be committed.
OptIn were kept at the class and statement level, and not applied globally.
Accessibility scanner is passing without suggestions.

RankoR

Please also check for unused resources and imports.

RankoR · 2026-07-20T19:32:04Z

+            loadSimContacts = { flowOf(SimContactsResult(contacts = persistentListOf(contact))) },
+            startSimImport = { a, b, c -> startSimImportCall = Triple(a, b, c) },
+        )
+


advanceUntilIdle seems to be required here, otherwise init collectors will not run, currentAccount will be null and startImport will early-return

RankoR · 2026-07-20T19:34:15Z

+                    state.copy(
+                        isLoading = false,
+                        accounts = accounts,
+                        currentAccount = if (accounts.contains(state.currentAccount)) {


AccountInfo doesn't have equals and hashCode, so selected account silently resets to default on every accounts reload.

Also, not strictly required, but when would look better here (and in many similar cases)

RankoR · 2026-07-20T19:35:44Z

+            selectedContacts.update { oldSelectedContactsMap ->
+                accounts.associate { account ->
+                    val oldSelectedContacts = oldSelectedContactsMap[account.account]
+                    val selectedContacts = if (oldSelectedContacts.isNullOrEmpty()) {


isNullOrEmpty() conflates "account never initialized" (null) with "user deselected everything" (empty set). After Deselect All, the next re-emission - guaranteed by broken distinctUntilChanged on any accounts broadcast - re-selects every contact. A partial selection survives correctly; only the user's most explicit "import nothing" intent is destroyed. The old Java distinguished ids == null from an empty long[].

Well caught. I introduced nullable states in SimImportUiState and simContacts to ensure we can distinguish between the not having received any value yet, and empty lists. The code get's slightly more complex with nullability checks, but it should help avoiding making this issues.

RankoR · 2026-07-20T19:37:10Z

+
+    override operator fun invoke(subscriptionId: Int): Flow<SimContactsResult> =
+        buildBroadcastReceiverFlow(IntentFilter(AccountTypeManager.BROADCAST_ACCOUNTS_CHANGED))
+            .onStart { emit(Unit) }


Make sure to have catch downstream, otherwise any exception will crash the app.

RankoR · 2026-07-20T19:39:03Z

 /**
 * Holds an {@link AccountWithDataSet} and the corresponding {@link AccountType} for an account.
 */
+@Immutable


It's not immutable. We should probably implement a separate UI-layer model (and a mapper, see examples in Messaging), that will be truly immutable.

Went with 2 domain models (AndroidModel and AccountDisplayModel) and a AndroidUIModel.

RankoR · 2026-07-20T19:53:55Z

+}
+
+@VisibleForTesting
+const val TEST_TAG_SIM_IMPORT_CONTACTS_TO_IMPORT_TITLE = "sim_import_contacts_to_import_title"


Test tags should be in a separate file and in most cases with internal visibility

RankoR · 2026-07-20T19:54:54Z

+import com.android.contacts.ui.core.ContactsPreviewColumn
+
+@Composable
+internal fun SimContactCell(


modifier: Modifier = Modifier is missing. Check other Composables too.

RankoR · 2026-07-20T19:56:49Z

+        title = {
+            Text(
+                text = if (uiState.selectedContactsCount > 0) {
+                    uiState.selectedContactsCount.toString()


While it mirrors old behavior, it's not a good UX to display just a number. Should be a plural string res (English-only is enough for now)

RankoR · 2026-07-20T19:57:56Z

+
+    override fun handle(effect: Effect) {
+        when (effect) {
+            Effect.Close -> activity.finish()


setResult(RESULT_OK) / setResult(RESULT_CANCELED) are lost here

RankoR · 2026-07-20T19:58:56Z

 androidx-compose-ui-test-manifest = { module = "androidx.compose.ui:ui-test-manifest" }
 androidx-compose-ui-tooling = { module = "androidx.compose.ui:ui-tooling" }
 androidx-compose-ui-tooling-preview = { module = "androidx.compose.ui:ui-tooling-preview" }
+androidx-lifecycle-viewmodel-compose = { module = "androidx.lifecycle:lifecycle-viewmodel-compose" }


Version isn't covered by the Compose BOM, so version must be set

m4pl

Only had time to test this on a device so far, the comments above are what I ran into. Will go through the code properly in the next pass.

m4pl · 2026-08-04T14:18:56Z

+package com.android.contacts.domain.accounts.mapper
+
+import com.android.contacts.domain.accounts.model.AccountDisplayModel
+import com.android.contacts.domain.accounts.model.AccountModel


Unused import

m4pl · 2026-08-04T14:19:23Z

 import com.android.contacts.model.account.AccountInfo;
 import com.android.contacts.model.account.AccountWithDataSet;
+import com.android.contacts.ui.UIIntents;
+import com.android.contacts.ui.simimport.SimImportActivity;


Unused import

m4pl · 2026-08-04T14:22:12Z

+}
+
+@Composable
+private fun itemClipShape(


Corner radius changes with selection, so a checked row in the middle of the list becomes fully rounded while the rows above and below stay square, leaving odd notches (screenshot). The shape of a row shouldn't depend on whether it is checked.

I followed the Material spec and the latest Google Contacts app: https://m3.material.io/components/lists/specs
But I also feel that the notches were a bit too sharp, so in the next PR, I made all corners have at least 2.dp of corner size. I'll update this PR with the same.

m4pl · 2026-08-04T14:24:13Z

+    isExpanded: Boolean,
+) {
+    val currentAccountLabel = current.name.orEmpty()
+    OutlinedTextField(


The account name behaves like editable text: long pressing it selects the text and opens the copy/share toolbar. I think it shouldn't be selectable.

That's the default behavior of readOnly TextFields. Adding a DisableSelection around it doesn't fix it.

If we really don't want text selection there, we need to design the custom Button ourselves. Possible, just a bit more work. Is it preferable?

Makes sense, not worth a custom component. Let's leave it.

m4pl · 2026-08-04T14:25:23Z

+            }
+        },
+        title = {
+            Text(


Title wraps to two lines on small screens.

m4pl · 2026-08-04T14:28:05Z

+            )
+        },
+        actions = {
+            IconButton(


Two separate buttons where only one is ever enabled. Could this be a single control?

Both can be enabled, if you just selected some but not all contacts.

Got it. Two icons plus the Import button still feels like a lot for the app bar, but it's not a blocker. Fine as is.

m4pl · 2026-08-04T14:33:56Z

-            android:layout_width="match_parent"
-            android:layout_height="?attr/actionBarSize"
-            android:elevation="3dp"
-            android:theme="@style/LightToolbarThemeOverlay"


These are no longer referenced anywhere after the old SIM import layouts were deleted: LightToolbarNavigationButtonStyle, LightToolbarThemeOverlay, LightToolbarStyle, FullScreenDialogAnimationStyle, PeopleThemeAppCompat.FullScreenDialog, PeopleThemeAppCompat.FullScreenDialog.SimImportActivity, and res/anim/slide_and_fade_out.xml.

m4pl · 2026-08-04T14:45:55Z

Once contacts are imported there's no way to undo it from this screen, you have to delete them one by one in the contacts list. I'd like them to be undoable, but maybe that's not this screen's job.

@RankoR what do you think, is undo in scope here?

I think it's out of scope for this PR

m4pl · 2026-08-04T14:49:12Z

+
+internal class AccountDisplayModelMapperImpl @Inject constructor() : AccountDisplayModelMapper {
+    override fun map(accountInfo: AccountInfo): AccountDisplayModel {
+        return AccountDisplayModel(


The account picker shows an empty name on a phone with no Google account, so it's not clear where contacts will be saved (screenshot). account.name is null for device and SIM accounts, AccountInfo.nameLabel has the "Device" / "SIM" label for that case.

m4pl · 2026-08-04T15:11:59Z

+    private fun load(subscriptionId: Int): SimContactsResult {
+        val sim = simContactDao.getSimBySubscriptionId(subscriptionId) ?: return SimContactsResult()
+        val contacts = simContactDao.loadContactsForSim(sim).orEmpty()
+        val accountsMap = simContactDao.findAccountsOfExistingSimContacts(contacts).orEmpty()


Importing the same SIM contact twice creates a duplicate. Import succeeds, but reopening the screen lists the contact under "Contacts to import" again.

The check matches raw contacts by phone and name. We insert only StructuredName.DISPLAY_NAME and the provider re-derives it from the parsed name parts, so 01.Balance is stored as 01 Balance and the name comparison in SimContact.findByPhoneAndName fails. Verified on device, the query returns the raw contacts and every one comes back as no match. Names without punctuation are fine.

Not caused by this PR, the matching code is untouched. Up to you whether to fix it here or create a separate issue.

Got it. I can reproduce the issue as well.

I feel it should be a separate ticket. For example, importing from a VCard also allows duplicates, and I'm not sure we want that as well there.

sdsantos · 2026-08-06T10:02:01Z

@RankoR rebased to the latest main

RankoR · 2026-08-11T13:30:05Z

+
+    init {
+        if (subscriptionId != SimCard.NO_SUBSCRIPTION_ID) {
+            loadSimCards()


There's no catch, so it can crash the app

Done. But the previous and existing code did not catch any exception from the Dao method, and the only mention I could find of a throw was for a UnsupportedOperationException, so I added a catch for that one.

sdsantos · 2026-08-13T08:40:04Z

Rebased to the latest main

m4pl · 2026-08-13T10:23:04Z

@@ -1,5 +1,4 @@
-<?xml version="1.0" encoding="utf-8"?>
-<!-- Copyright (C) 2006 The Android Open Source Project
+<?xml version="1.0" encoding="utf-8"?><!-- Copyright (C) 2006 The Android Open Source Project


Looks like the line break should be restored.

m4pl · 2026-08-13T10:26:41Z

+    val account: AccountModel,
+    val name: String?,
+    val type: String? = null,
+    val icon: Drawable? = null,


Drawable can change and is tied to Resources, so this model isn't really immutable. It also ends up in AccountUiModel, which is marked @Immutable, and that's no longer true. Could we keep an id here and build the icon in the UI?

It comes as a Drawable from inside the AccounType. Usually it starts from an icon resource, but sometimes it gets mutated before reaching here.

Maybe we can agree to mark the AccountUiModel as @Stable, knowning it's unlikely it's actually going to change, without the type changing as well?

The alternative is ignoring the icon we get from the AccountType, and attributing our own icon resources for every known AccountType (external accounts are the most complex scenario).

getDisplayIcon() returns a new Drawable on every call, so accounts.contains(it) in findCurrentAccount never matches. @Stable wouldn't change that.

Own icon set isn't needed though. AccountInfo.getType() gives you the AccountType, where syncAdapterPackageName and iconRes are public. Those plus the grey tint from FallbackAccountType / SimAccountType are enough for the UI to build it.

m4pl · 2026-08-13T10:27:30Z

+ktlint_class_signature_rule_force_multiline_when_parameter_count_greater_or_equal_than = 1
 ij_kotlin_line_break_after_multiline_when_entry = false
 ktlint_code_style = android_studio
+ktlint_class_signature_rule_force_multiline_when_parameter_count_greater_or_equal_than = 1


Duplicate of line 17.

Copied from GrapheneOS/app-docs#2. Already fixed here and left a comment there.

m4pl · 2026-08-13T10:27:38Z

 ktlint_standard_trailing-comma-on-call-site = disabled
 ktlint_standard_blank-line-between-when-conditions = disabled
-max_line_length = 100
+max_line_length = 100


Missing final newline.

Copied from GrapheneOS/app-docs#2. Already fixed here and left a comment there.

m4pl · 2026-08-13T10:29:02Z

+        val contact: SimContactUiModel,
+        val isSelected: Boolean,
+    ) : SimImportAction
+


Blank lines inconsistent.

m4pl · 2026-08-13T11:42:13Z

+
+internal object AccountModelFactory {
+    fun build(
+        name: String = "Account ${Random.nextInt().toString().take(4)}",


take(4) on a signed int leaves only ~9900 names, so two accounts can come out identical and some tests need them to differ. Flaky. A counter instead? Same at AccountDisplayModelFactory and AccountUiModelFactory.

You're right. And I feel it's a bad idea to rely on the Factory for uniqueness. Better leave that responsibility to the test, if it really needs multiple unique objects.

SimContactFactory.kt:8 and SimContactUiModelFactory.kt:10 still default recordNumber to Random.nextInt().

m4pl · 2026-08-13T11:46:50Z

+    val effectHandler by rememberUpdatedState(effectHandler)
+
+    LaunchedEffect(screenModel) {
+        screenModel.effects.collect(effectHandler::handle)


effectHandler::handle is a bound reference, so rememberUpdatedState has no effect.

m4pl · 2026-08-13T11:49:33Z

+            .onStart { emit(Unit) }
+            .map { load(subscriptionId) }
+            .catch {
+                if (it is CancellationException) {


This swallows the exception instead of rethrowing it. Throw it, or drop the branch?

m4pl · 2026-08-13T11:53:38Z

+import kotlinx.collections.immutable.ImmutableList
+
+internal fun interface StartSimImport {
+    operator fun invoke(


operator is here but not in LoadSimContactsImpl / BuildBroadcastReceiverFlowImpl.

But operator is present in LoadSimContactsImpl and BuildBroadcastReceiverFlowImpl 🤔
Or am I missing something?

My mistake. Ignore this one.

m4pl · 2026-08-13T11:54:55Z

+) {
+    ContactsPreviewTheme(modifier = modifier) {
+        Column(
+            verticalArrangement = Arrangement.spacedBy(16.dp),


Real list uses spacedBy(1.dp), so cell previews don't match.

m4pl

Added a few comments in the existing threads. Also worth another pass on unused imports: kotlin.random.Random in AccountModelFactory.kt:4, AccountDisplayModelFactory.kt:6, AccountUiModelFactory.kt:6, AccountModel in AccountDisplayModelFactory.kt:5, ImmutableList in StartSimImport.kt:10. ktlint doesn't flag them here.

m4pl · 2026-08-18T21:21:05Z

+                end = contentPadding.calculateEndPadding(layoutDirection),
+            ),
+        ) {
+            if (uiState is State.Ready) {


Should the picker still show on the "no contacts" screen? It's only rendered for State.Ready now.

We could show it yeah, we have the info.

m4pl · 2026-08-18T21:22:22Z

+
+    private fun selectAll() {
+        val account = currentAccount.value ?: return
+        val state = (uiState.value as? State.Ready) ?: return


selectAll() and getSelectedContacts() read uiState.value, so the handlers depend on the state they produce. Could they use simContacts + existingContacts + currentAccount directly?

m4pl · 2026-08-18T21:23:14Z

+
+    private fun restoreSelectedContacts(): Map<AccountModel, Set<Int>> {
+        val entries = savedStateHandle.get<List<AccountContactsEntry>>(KEY_SELECTED_CONTACTS)
+        return entries


Returns Map<AccountModel, Set<Int>> but still builds it with toImmutableSet() / toImmutableMap() / persistentMapOf().

m4pl · 2026-08-18T21:23:41Z

+        }
+    }
+
+    @Suppress("detekt:ReturnCount")


Would a when avoid the suppression here?

I've split the method in two steps.

RankoR suggested changes Jul 20, 2026

View reviewed changes

sdsantos force-pushed the sim-import-refactor branch 2 times, most recently from 5f67695 to 733eda4 Compare July 22, 2026 10:01

sdsantos requested a review from RankoR July 22, 2026 12:14

RankoR requested a review from m4pl August 3, 2026 16:19

m4pl reviewed Aug 4, 2026

View reviewed changes

sdsantos force-pushed the sim-import-refactor branch from 7092d5e to 0ef831a Compare August 5, 2026 10:10

sdsantos requested a review from m4pl August 5, 2026 10:17

thestinger force-pushed the main branch 3 times, most recently from cbaca32 to c9ac9d9 Compare August 5, 2026 18:47

sdsantos force-pushed the sim-import-refactor branch from 0ef831a to 8eb4e48 Compare August 6, 2026 10:01

RankoR reviewed Aug 11, 2026

View reviewed changes

sdsantos mentioned this pull request Aug 11, 2026

Settings Screen: Java/XML to Kotlin/Compose migration #56

Open

RankoR approved these changes Aug 12, 2026

View reviewed changes

sdsantos added 7 commits August 13, 2026 09:09

Migrate SimImport to Kotlin + Compose

125b5b9

Refactor SimImport to use Hilt and data/domain/ui package structure

cb8e82f

Ensure compose stability in SimImport

1c9b124

Address code review feedback

0ea4959

Introduce SimContactUiModel. Move androidTests to unitTests.

d65678c

Address code review comments

f4b7447

Catch UnsupportedOperationException when loading SIM cards

fedb8b7

sdsantos force-pushed the sim-import-refactor branch from ad16541 to fedb8b7 Compare August 13, 2026 08:39

m4pl reviewed Aug 13, 2026

View reviewed changes

sdsantos force-pushed the sim-import-refactor branch 2 times, most recently from 5753783 to 1846520 Compare August 13, 2026 16:48

Address new code review comments

f193e07

sdsantos force-pushed the sim-import-refactor branch from 1846520 to f193e07 Compare August 14, 2026 10:21

sdsantos requested a review from m4pl August 14, 2026 10:21

m4pl reviewed Aug 18, 2026

View reviewed changes

Address new code review comments (second batch)

232a5fb

sdsantos requested a review from m4pl August 19, 2026 10:24

Update verification metadata

9731bc6

Uh oh!

Uh oh!

Conversation

sdsantos commented Jun 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RankoR left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

m4pl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdsantos commented Aug 6, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdsantos commented Aug 13, 2026

Uh oh!

sdsantos commented Jun 26, 2026 •

edited

Loading

sdsantos Aug 13, 2026 •

edited

Loading