From 4cdd1f70acaf6241f965bc585b0928b262e10e92 Mon Sep 17 00:00:00 2001 From: Alex Hart Date: Wed, 10 Jun 2026 11:57:23 -0300 Subject: [PATCH] Fix contact search list flickering on query change. --- .../securesms/contacts/ContactRepository.java | 6 + .../paged/ContactSearchPagedDataSource.kt | 44 +++++--- .../ContactSearchPagedDataSourceRepository.kt | 4 - .../contacts/paged/ContactSearchViewModel.kt | 21 ++-- ...SearchPagedDataSourceTest_letterHeaders.kt | 104 ++++++++++++++++++ .../java/org/signal/paging/PagedData.java | 13 ++- 6 files changed, 162 insertions(+), 30 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/contacts/ContactRepository.java b/app/src/main/java/org/thoughtcrime/securesms/contacts/ContactRepository.java index 2b680b6974..21fcf4dc4b 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/contacts/ContactRepository.java +++ b/app/src/main/java/org/thoughtcrime/securesms/contacts/ContactRepository.java @@ -35,6 +35,7 @@ public class ContactRepository { public static final String ID_COLUMN = "id"; public static final String NAME_COLUMN = "name"; + public static final String SORT_NAME_COLUMN = "sort_name"; static final String NUMBER_COLUMN = "number"; static final String NUMBER_TYPE_COLUMN = "number_type"; static final String LABEL_COLUMN = "label"; @@ -55,6 +56,11 @@ public class ContactRepository { return Util.getFirstNonEmpty(system, profile); })); + // The key the results are actually ordered by (nickname/system/profile/username, lowercased). Letter + // headers must derive from this rather than NAME_COLUMN, which omits nickname/username and can begin + // with a different letter than the row's sort position. + add(new Pair<>(SORT_NAME_COLUMN, cursor -> CursorUtil.requireString(cursor, RecipientTable.SORT_NAME))); + add(new Pair<>(NUMBER_COLUMN, cursor -> { String phone = CursorUtil.requireString(cursor, RecipientTable.E164); String email = CursorUtil.requireString(cursor, RecipientTable.EMAIL); diff --git a/app/src/main/java/org/thoughtcrime/securesms/contacts/paged/ContactSearchPagedDataSource.kt b/app/src/main/java/org/thoughtcrime/securesms/contacts/paged/ContactSearchPagedDataSource.kt index 848cf4ef6f..a4726264d0 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/contacts/paged/ContactSearchPagedDataSource.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/contacts/paged/ContactSearchPagedDataSource.kt @@ -18,7 +18,6 @@ import org.thoughtcrime.securesms.database.model.ThreadWithRecipient import org.thoughtcrime.securesms.keyvalue.StorySend import org.thoughtcrime.securesms.phonenumbers.NumberUtil import org.thoughtcrime.securesms.recipients.Recipient -import org.thoughtcrime.securesms.recipients.RecipientId import org.thoughtcrime.securesms.search.MessageResult import org.thoughtcrime.securesms.search.MessageSearchResult import org.thoughtcrime.securesms.search.SearchRepository @@ -240,13 +239,33 @@ class ContactSearchPagedDataSource( return 0 } - private fun getNonGroupHeaderLetterMap(section: ContactSearchConfiguration.Section.Individuals, query: String?): Map { - return contactSearchPagedDataSourceRepository.querySignalContactLetterHeaders( - query = query, - includeSelfMode = section.includeSelfMode, - includePush = true, - includeSms = false - ) + /** + * Returns the letter header to display above the recipient at the cursor's current row, or null if + * none should be shown. A header is shown only when this row begins a new letter group, determined by + * comparing its letter to the immediately preceding row in display order. Peeking that single adjacent + * row means a letter group split across pages still yields exactly one header, anchored to the first + * row of the group, without re-scanning the whole contact set. + * + * The cursor is restored to its original position before returning so iteration is unaffected. + */ + private fun getHeaderLetterForCurrentRow(cursor: Cursor): String? { + val position = cursor.position + val currentLetter = letterForCurrentRow(cursor) ?: return null + + if (position <= 0) { + return currentLetter + } + + cursor.moveToPosition(position - 1) + val previousLetter = letterForCurrentRow(cursor) + cursor.moveToPosition(position) + + return if (previousLetter != currentLetter) currentLetter else null + } + + private fun letterForCurrentRow(cursor: Cursor): String? { + val sortName = cursor.getString(cursor.getColumnIndexOrThrow(ContactRepository.SORT_NAME_COLUMN)) + return sortName?.takeIf { it.isNotEmpty() }?.first()?.uppercaseChar()?.toString() } private fun getStoriesSearchIterator(query: String?): ContactSearchIterator { @@ -379,12 +398,6 @@ class ContactSearchPagedDataSource( } private fun getNonGroupContactsData(section: ContactSearchConfiguration.Section.Individuals, query: String?, startIndex: Int, endIndex: Int): List { - val headerMap: Map = if (section.includeLetterHeaders) { - getNonGroupHeaderLetterMap(section, query) - } else { - emptyMap() - } - return getNonGroupSearchIterator(section, query).use { records -> readContactData( records = records, @@ -394,7 +407,8 @@ class ContactSearchPagedDataSource( endIndex = endIndex, recordMapper = { val recipient = contactSearchPagedDataSourceRepository.getRecipientFromSearchCursor(it) - ContactSearchData.KnownRecipient(section.sectionKey, recipient, headerLetter = headerMap[recipient.id]) + val headerLetter = if (section.includeLetterHeaders) getHeaderLetterForCurrentRow(it) else null + ContactSearchData.KnownRecipient(section.sectionKey, recipient, headerLetter = headerLetter) } ) } diff --git a/app/src/main/java/org/thoughtcrime/securesms/contacts/paged/ContactSearchPagedDataSourceRepository.kt b/app/src/main/java/org/thoughtcrime/securesms/contacts/paged/ContactSearchPagedDataSourceRepository.kt index bf64ec4ba4..e47a190872 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/contacts/paged/ContactSearchPagedDataSourceRepository.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/contacts/paged/ContactSearchPagedDataSourceRepository.kt @@ -43,10 +43,6 @@ open class ContactSearchPagedDataSourceRepository( return contactRepository.querySignalContacts(contactsSearchQuery) } - open fun querySignalContactLetterHeaders(query: String?, includeSelfMode: RecipientTable.IncludeSelfMode, includePush: Boolean, includeSms: Boolean): Map { - return SignalDatabase.recipients.querySignalContactLetterHeaders(query ?: "", includeSelfMode, includePush, includeSms) - } - open fun queryGroupMemberContacts(query: String?): Cursor? { return contactRepository.queryGroupMemberContacts(query ?: "") } diff --git a/app/src/main/java/org/thoughtcrime/securesms/contacts/paged/ContactSearchViewModel.kt b/app/src/main/java/org/thoughtcrime/securesms/contacts/paged/ContactSearchViewModel.kt index 1abb9a4e2a..49c9ad8658 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/contacts/paged/ContactSearchViewModel.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/contacts/paged/ContactSearchViewModel.kt @@ -20,7 +20,6 @@ import kotlinx.coroutines.flow.SharedFlow import kotlinx.coroutines.flow.SharingStarted import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.combine -import kotlinx.coroutines.flow.debounce import kotlinx.coroutines.flow.drop import kotlinx.coroutines.flow.flatMapLatest import kotlinx.coroutines.flow.flowOf @@ -98,7 +97,7 @@ class ContactSearchViewModel( init { viewModelScope.launch { - rawQuery.drop(1).debounce(300).collect { query -> + rawQuery.drop(1).collect { query -> savedStateHandle[QUERY] = query internalConfigurationState.update { it.copy(query = query) } } @@ -148,15 +147,17 @@ class ContactSearchViewModel( } suspend fun setConfiguration(contactSearchConfiguration: ContactSearchConfiguration) { - val pagedDataSource = ContactSearchPagedDataSource( - contactSearchConfiguration, - arbitraryRepository = arbitraryRepository, - searchRepository = searchRepository, - contactSearchPagedDataSourceRepository = contactSearchPagedDataSourceRepository - ) - val size = withContext(Dispatchers.IO) { pagedDataSource.size() } + val (pagedDataSource, size) = withContext(Dispatchers.IO) { + val source = ContactSearchPagedDataSource( + contactSearchConfiguration, + arbitraryRepository = arbitraryRepository, + searchRepository = searchRepository, + contactSearchPagedDataSourceRepository = contactSearchPagedDataSourceRepository + ) + source to source.size() + } internalTotalCount.value = size - pagedData.value = PagedData.createForStateFlow(pagedDataSource, pagingConfig) + pagedData.value = PagedData.createForStateFlow(pagedDataSource, pagingConfig, data.value) } fun setQuery(query: String?) { diff --git a/app/src/test/java/org/thoughtcrime/securesms/contacts/paged/ContactSearchPagedDataSourceTest_letterHeaders.kt b/app/src/test/java/org/thoughtcrime/securesms/contacts/paged/ContactSearchPagedDataSourceTest_letterHeaders.kt index dfdef9f659..c476d3b33c 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/contacts/paged/ContactSearchPagedDataSourceTest_letterHeaders.kt +++ b/app/src/test/java/org/thoughtcrime/securesms/contacts/paged/ContactSearchPagedDataSourceTest_letterHeaders.kt @@ -20,6 +20,7 @@ import org.signal.paging.PagedDataSource import org.thoughtcrime.securesms.database.RecipientTable import org.thoughtcrime.securesms.database.SignalDatabase import org.thoughtcrime.securesms.keyvalue.StorySend +import org.thoughtcrime.securesms.profiles.ProfileName import org.thoughtcrime.securesms.recipients.RecipientId import org.thoughtcrime.securesms.testutil.RecipientTestRule import java.util.UUID @@ -67,6 +68,109 @@ class ContactSearchPagedDataSourceTest_letterHeaders { ) } + @Test + fun `each letter group yields exactly one header anchored to its first contact`() { + val aaron = recipients.createRecipient("Aaron") + val bella = recipients.createRecipient("Bella") + val carol = recipients.createRecipient("Carol") + val carrie = recipients.createRecipient("Carrie") + val casey = recipients.createRecipient("Casey") + val dana = recipients.createRecipient("Dana") + + val dataSource = buildIndividualsDataSource() + val size = dataSource.size() + val rows = dataSource.load(0, size, size, PagedDataSource.CancellationSignal { false }) + + // Single-contact groups always carry their own header. + assertEquals("A", rows.headerFor(aaron)) + assertEquals("B", rows.headerFor(bella)) + assertEquals("D", rows.headerFor(dana)) + + // The three-contact C group must contribute exactly one "C" header (on whichever sorts first), + // not one per contact. + val cHeaders = listOf(carol, carrie, casey).mapNotNull { rows.headerFor(it) } + assertEquals("exactly one C header across the C group", listOf("C"), cHeaders) + } + + @Test + fun `a letter group split across two page loads still yields a single header on the first page`() { + val aaron = recipients.createRecipient("Aaron") + val bella = recipients.createRecipient("Bella") + val carol = recipients.createRecipient("Carol") + val carrie = recipients.createRecipient("Carrie") + val casey = recipients.createRecipient("Casey") + val dana = recipients.createRecipient("Dana") + + val dataSource = buildIndividualsDataSource() + val size = dataSource.size() + + // Sorted order is [Aaron(A), Bella(B), C, C, C, Dana(D)], so loading in two pages of three splits + // the C group: one C ends the first page, two C's begin the second. + val firstPage = dataSource.load(0, 3, size, PagedDataSource.CancellationSignal { false }) + val secondPage = dataSource.load(3, 3, size, PagedDataSource.CancellationSignal { false }) + + val cContacts = listOf(carol, carrie, casey) + val firstPageCHeaders = cContacts.mapNotNull { firstPage.knownRecipient(it)?.headerLetter } + val secondPageCHeaders = cContacts.mapNotNull { secondPage.knownRecipient(it)?.headerLetter } + + // The single C header lands on the first contact of the run (first page); the C's that begin the + // second page must NOT repeat it. + assertEquals("C header belongs to the first contact of the run, on the first page", listOf("C"), firstPageCHeaders) + assertEquals("the second page continues the C group and must not repeat the header", emptyList(), secondPageCHeaders) + assertEquals("D", secondPage.headerFor(dana)) + + // Sanity: the C group genuinely straddled the page boundary. + assertEquals("one C contact on the first page", 1, cContacts.count { firstPage.knownRecipient(it) != null }) + assertEquals("two C contacts on the second page", 2, cContacts.count { secondPage.knownRecipient(it) != null }) + } + + @Test + fun `letter header reflects the sort name, not the display name, when a nickname diverges from the profile name`() { + recipients.createRecipient("Bella") + val mikeSortsAsCharlie = recipients.createRecipient("Mike") + SignalDatabase.recipients.setNicknameAndNote(mikeSortsAsCharlie, ProfileName.fromParts("Charlie", null), "") + recipients.createRecipient("Dan") + + val dataSource = buildIndividualsDataSource() + val size = dataSource.size() + val rows = dataSource.load(0, size, size, PagedDataSource.CancellationSignal { false }) + + // This contact's nickname ("Charlie") sorts it among the C's even though its profile name is "Mike". + // The header must follow the sort key (C), never the display name (M). + assertEquals("header follows the sort key, not the display name", "C", rows.headerFor(mikeSortsAsCharlie)) + + // End to end: headers march B, C, D in order, with no stray "M" leaking out of the display name. + val headers = rows.filterIsInstance().mapNotNull { it.headerLetter } + assertEquals(listOf("B", "C", "D"), headers) + } + + private fun buildIndividualsDataSource(): ContactSearchPagedDataSource { + return ContactSearchPagedDataSource( + contactConfiguration = ContactSearchConfiguration.build { + addSection( + ContactSearchConfiguration.Section.Individuals( + includeHeader = false, + includeSelfMode = RecipientTable.IncludeSelfMode.Exclude, + includeLetterHeaders = true, + transportType = ContactSearchConfiguration.TransportType.ALL + ) + ) + }, + contactSearchPagedDataSourceRepository = object : ContactSearchPagedDataSourceRepository(ApplicationProvider.getApplicationContext()) { + override fun getLatestStorySends(activeStoryCutoffDuration: Long): List = emptyList() + } + ) + } + + private fun List.knownRecipient(id: RecipientId): ContactSearchData.KnownRecipient? { + return filterIsInstance().firstOrNull { it.recipient.id == id } + } + + private fun List.headerFor(id: RecipientId): String? { + val match = knownRecipient(id) ?: error("Recipient $id not present in $this") + return match.headerLetter + } + private fun insertUnregisteredSystemContact(name: String): RecipientId { val rowId = SignalDatabase.recipients.writableDatabase.insertOrThrow( RecipientTable.TABLE_NAME, diff --git a/lib/paging/src/main/java/org/signal/paging/PagedData.java b/lib/paging/src/main/java/org/signal/paging/PagedData.java index 268cb5f616..6cf3131294 100644 --- a/lib/paging/src/main/java/org/signal/paging/PagedData.java +++ b/lib/paging/src/main/java/org/signal/paging/PagedData.java @@ -40,7 +40,18 @@ public class PagedData { @AnyThread public static StateFlowPagedData createForStateFlow(@NonNull PagedDataSource dataSource, @NonNull PagingConfig config) { - MutableStateFlow> stateFlow = kotlinx.coroutines.flow.StateFlowKt.MutableStateFlow(java.util.Collections.emptyList()); + return createForStateFlow(dataSource, config, java.util.Collections.emptyList()); + } + + /** + * Same as {@link #createForStateFlow(PagedDataSource, PagingConfig)}, but seeds the backing + * {@link StateFlow} with {@code initialValue} instead of an empty list. Useful when rebuilding a + * source (e.g. on a query change) and you want the previously-loaded items to remain visible until + * the new source's first page arrives, rather than briefly collapsing the list to empty. + */ + @AnyThread + public static StateFlowPagedData createForStateFlow(@NonNull PagedDataSource dataSource, @NonNull PagingConfig config, @NonNull List initialValue) { + MutableStateFlow> stateFlow = kotlinx.coroutines.flow.StateFlowKt.MutableStateFlow(initialValue); PagingController controller = new BufferedPagingController<>(dataSource, config, stateFlow::setValue); return new StateFlowPagedData<>(stateFlow, controller);