Repository navigation
Conversation
The About sheet keeps its recipient in a StateFlow, which drops any new value equal to the current one, and Recipient.equals compares only ids. So a change to the recipient while the sheet is open, such as a new profile name, never reaches it. Nothing on the sheet changes the recipient itself yet, which may be why this has gone unnoticed, but linking and unlinking a phone contact from the sheet will. Count recipient content changes in the state, as the conversation settings screens already do, so each one makes a new state.
RecipientTable.merge() copies the linked record's system contact fields onto the merged recipient, but it left out system_phone_type. The merged recipient kept the other record's type, usually -1, alongside the copied contact URI and label, so its phone label type was lost. Read system_phone_type into RecipientRecord and carry it through the merge with the rest of the system contact fields.
A recipient's link to a system contact records the contact, and the type and label of the phone number it matched on, such as "Mobile," but not the number itself. So whether the contact holds the recipient's current number cannot be told apart from whether it held a number they have since changed. Add system_phone_e164, the recipient's number as it was matched on the linked contact. The contact sync writes it with the rest of the link, merges carry it, and clearing a link clears it. Existing links were all made by matching the recipient's current number, so the migration fills the column from it. Nothing reads it yet: the next change uses it to decide when the address book holds a recipient's number.
Recipient.shouldShowE164 treats any system contact link as proof that the user's address book holds the recipient's phone number. That is true today only because every link is made by matching a number, and a full sync drops links that stop matching. Allowing links that are not tied to a number would break that assumption and could reveal a number its owner has hidden. Add Recipient.isSystemContactByPhoneNumber, true only when the linked contact holds the recipient's current number (system_phone_e164 equals their E164) and the recipient has not turned off phone number discovery, unless they share their number. Use it wherever a decision depends on the address book holding the number: shouldShowE164, digit search, the non-Signal contact list, and the shared contact card's Invite button. The "Update contact" button after a number change needs both: the user knows the person, and the new number may be shown. Callers that mean "the user knows this recipient" keep isSystemContact. Today every link's contact holds the number it matched on, so the changes in behavior are narrow. A partial contact sync never removes links, so a recipient who turns off discovery could keep a link until the next full sync, and their number is now hidden during that window. Likewise, a recipient's new number no longer counts as being in the address book while the link still holds their old one.
A recipient is linked to a system contact exactly when it has a system contact URI. That cannot tell a recipient who was never linked from one whose contact went away, which the coming lookup-key based sync needs. Add system_contact_link_state: NONE, LINKED, or NEEDED (was linked and lost the contact; nothing sets it yet). The migration marks every recipient with a URI as LINKED, and linking by number sets it. The system contact fields describe a single link, so a recipient merge now takes all of them, with the state, from whichever record has the stronger link, preferring the E164 record as before when they are equal. Until now the merge always took the E164 record's fields, which would drop a link held only by the ACI record.
The contact sync's reader, CursorContactIterator, has no tests. It decides which name a contact gets (the provider's name source, never Signal's own raw contact), which number type wins when a contact lists a number twice, and what happens when a provider rejects the columns those choices depend on. It carries each number's label and photo, which the sync stores, and it relies on the query's sort order, latest row first, wherever a choice is otherwise tied. Add a fake contacts provider that serves fixed rows, sorted by the query's sort order as the real provider sorts them, and tests that pin those choices down, ahead of sharing the reader with another caller. The fixture rows are deliberately listed out of order, so a change to the sort order shows up as a different name or number order.
CursorContactIterator both finds each contact, by walking a cursor sorted by lookup key, and loads it: formatting its numbers, keeping the lowest type of a repeated number, and choosing its name. Keeping a link current needs that same loading for a contact found another way. Move the loading into ContactDataReader, which takes one contact's data rows and produces its numbers and name, and share the query's columns and sort order. The iterator keeps the walking. No change in behavior; the tests added in the previous commit pass unchanged.
Signal links a recipient to a system contact by the contact's lookup key, but reads contacts only by walking the whole address book. Keeping a link current needs the other direction: given a stored key, what does the contact look like now, if it still exists? Add SystemContactsRepository.getLinkedContact. It resolves the key, which the provider does even after the user joins contacts, and loads the contact's rows with the same ContactDataReader the full read uses, so a linked contact gets the same numbers and name either way. It adds what the full read cannot supply for a contact with no phone number: the display name and photo from the contact itself, and the contact's current lookup URI. Rows from Signal's own account are skipped, so a contact kept alive only by Signal's "Message via Signal" raw contact reads as missing. A provider that cannot be queried reads as unavailable rather than missing, so a caller can tell "gone" from "unknown." Nothing calls it yet.
A system contact link is stored as a lookup URI, content://com.android.contacts/contacts/lookup/<lookup key>/<id>, whose last segment Android defines as the contact id: Contacts.getLookupUri() takes a contact id. The contact reader passes the matching phone row's Data row id instead, so the URI names the right contact by its key, but a row of a different table by its id. Nothing visibly breaks today. When the contacts provider resolves a lookup URI, it first looks for a contact with both that id and that key, and falls back to the key alone when none matches. A mismatched id therefore only costs that first attempt; it cannot select a different person, since the key must match too. Signal itself only ever reads the key back out (lookupKeyFromLookupUri), and the contacts app, given the URI to open, goes through the same provider resolution. The coming changes keep links current by lookup key: each refreshed link's URI is rewritten as Contacts.getLookupUri(contactId, currentKey), and a link the user makes by hand is stored the same way. Without this change, the same column would hold contact ids for those links, and Data row ids for links newly found by number until their first refresh. Build new lookup URIs from the row's contact id, so every link written from here on has the documented form. Links stored earlier keep their Data row id until a refresh rewrites them. That URI was the only reader of the Data row id, so it leaves the query's columns. It remains the sort key, so a contact's latest row still wins ties, and sorting needs no projected column. The reader test recorded the old form: it expected the id of Alice's winning phone row (11). It now expects her contact id (1), and also checks that both of her numbers carry the same URI, which under row ids they did not (11 and 10).
The contact sync turns a matched contact into recipient columns in two places: discovery derives the system name from a contact's structured or display name, and the bulk update maps the contact's details onto the system contact columns. Keeping links current from the contact itself, coming next, needs both again. Move each into a helper: ContactDiscovery.systemProfileName and RecipientTable.systemContactValues. No change in behavior.
A link says a recipient and an address book entry are the same person. The stored link has always been the entry's lookup key, but every full contact sync re-matches links by phone number and removes any it cannot match. So a link is lost whenever the recipient changes numbers, the user edits the number on the entry, or the recipient turns off phone number discovery. The recipient then loses the entry's name and photo, even though the user still has the entry and still knows who the recipient is. Phone numbers now only discover links: matching by number fills only recipients that are not already linked, and keeps refusing to link an undiscoverable recipient. The number-based prune is gone. Instead, both full-sync paths refresh every link from its entry by lookup key, with SystemContactsRepository.getLinkedContact: the name, the photo, the entry's current lookup key, and whether the entry still holds the recipient's number. A provider that cannot be queried leaves links alone. When the entry itself is gone, the recipient becomes NEEDED rather than losing everything the entry gave it. It keeps the name, label, and the rest, and drops only the contact and photo URIs, which pointed into the deleted entry. Matching by number can link a NEEDED recipient again, and dismissSystemContactLinkNeeded returns one to NONE, keeping the name, for when the user does not want to link it. Phone number privacy is unchanged where it matters. Discovery still never links an undiscoverable recipient, and a link whose entry lacks the recipient's number, or whose recipient is undiscoverable without sharing their number, still does not show that number (see isSystemContactByPhoneNumber). Nothing about a link is written to the address book, and Signal's own address book rows keep their existing rule. The system_info_pending column is no longer used.
A system contact's lookup key is not portable between phones: for a contact with no sync account, it holds a device-local row id, which the new phone's contacts provider can resolve to a different person. The restore paths treat links inconsistently today. A legacy backup or device transfer carries the old phone's URIs verbatim until a full sync re-matches or sweeps them, while backup v2 and the storage service carry only the contact's name, leaving a name with no link. Make every restore end the same way: each recipient that was linked becomes NEEDED, keeping what the old contact gave it, such as its name, and dropping the contact and photo URIs. - Legacy backups and device transfers: every recipient with a URI, in runPostBackupRestoreTasks, after the imported database is migrated. - Backup v2 and storage service inserts on a primary device: every recipient with a system name, since only a link sets those names. The post-registration contact sync then links again whatever it can match by number. The rest stay NEEDED until the user links them by hand, which the following changes make possible.
Links have only ever been made by matching a phone number, so a recipient with no number, a hidden number, or a number that differs from the one in the address book can never get the system contact's name and photo. Add the database and sync side of linking by hand: ContactDiscovery.linkSystemContact reads a system contact the user picked and links the recipient to it, whatever the recipient's number or discoverability, replacing any earlier link. As with any link, the system contact counts as holding the recipient's number only if it does, and only if the recipient lets the number be known. unlinkSystemContact removes a link the user no longer wants, and getSystemContactLinksNeeded lists the recipients waiting for a new link. Nothing calls them yet.
The About sheet says when a recipient is in the user's system contacts, but offers no way to open that system contact, and no way to link one when matching by phone number cannot. On the About sheet, the "is in your phone contacts" row now opens the system contact. For a recipient who is not linked, a "Link to phone contact" row opens a new screen, LinkPhoneContactActivity, which links the recipient either to a system contact the user picks or to a new one prefilled with the name, photo, and, when the recipient lets it be seen, number that Signal shows for them. Linking asks for contacts permission first, since the contact sync keeps the link current. An "Unlink phone contact" row removes a link, unless the next contact sync would just link the recipient again by number, which happens only when the system contact holds the recipient's number and the recipient is discoverable. So it is offered for a recipient with no number, one whose system contact's number was changed to another, and one linked by hand while undiscoverable. Linking is offered on primary devices only, which are the ones that keep links.
A recipient becomes NEEDED when its system contact is deleted, or when the user restores onto a new phone and the contact sync cannot link it again by number. Nothing shows the user which recipients those are. Add an "Unlinked Signal contacts" screen, UnlinkedSignalContactsActivity, reached from the new-chat menu while any recipient needs a link. It lists those recipients, links one when tapped, and dismisses one or all, keeping their names. Without contacts permission it offers to grant it first, after which a contact sync links again every recipient it can match by number.
Contributor
|
Thanks for the effort, we'll consider this. Just fyi, given the size and impact we aren't likely to move forward with it. I would also suggest pursing the community forum feature request flow to drive support for this feature change. |
Author
Okay, I will see what I can do. I was hoping it would be relatively straight-forward to review. The "big" changes are mainly tests and UI layout. The actual logic is small and pretty self-contained, and re-uses the existing Contact reading/syncing code. |
|
Cody, I’m wondering if you could suggest what a smaller change would need to look like in order for a fix on contract photos to be accepted? @kees If you set up a community feature forum post, drop a link in 13827 so we can express our support. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First time contributor checklist
Contributor checklist
Fixes #1234syntaxtl;dr: Link Signal contacts to phone contacts so I can actually have (my) photos for my friends
But this is a big change, so here's a long description:
Description
A recipient's link to an address book entry is what gives them the entry's name and photo in Signal. Today a link is created and kept alive only by phone number: every full contact sync re-matches links by number and removes any it cannot confirm. So the name and photo vanish when:
ae884d79a1(January 2024) removes links to undiscoverable recipients;and a recipient with no number in Signal, or registered under a different number than the one in the address book, can never be linked at all.
This is one of the causes collected in #13827 ("my friend hid their number and their photo vanished"). Forum threads asking to manually associate a contact get "not possible without phone number discovery." This PR addresses that cause; it does not close #13827, which has others.
Phone number privacy is fundamentally unchanged
Since links that used to disappear will now persist, this walks through why there isn't a change to privacy from this PR.
Removing the link when a recipient hides their number protected nothing: anyone with a link already had that number in their own address book. Dropping the link took away the name and photo, not the knowledge of the association (and Signal nicknames persist). The protections that matter are all kept:
setSystemContactInfois unchanged.shouldShowE164showed it. A link no longer implies that. The newRecipient.isSystemContactByPhoneNumberrequires the linked entry to hold the recipient's current number, and the recipient to be discoverable or to share their number. It replacesisSystemContactwherever a decision depends on the address book holding the number: showing the number, digit search, the SMS invite list, and the shared contact card. Everything that means "the user knows this recipient" keepsisSystemContact.A link carries the same effects it always has (the profile key is shared, the storage service marks the contact as whitelisted, and linked devices get the contact), however it was made.
What changes
A link means "same person," and is kept by lookup key. The stored link was always the entry's lookup key, never a number. Phone numbers now only discover links; once made, a link survives the recipient changing or hiding their number. Each link has a state in a new column:
NONELINKEDNEEDEDThe sync refreshes links instead of pruning them. Matching by number fills only
NONEandNEEDEDrecipients. Each full sync then reads every linked entry by its lookup key (SystemContactsRepository.getLinkedContact) and refreshes the name, the photo, the entry's current key, and whether the entry still holds the recipient's number. A deleted entry makes its recipientNEEDEDrather than wiping what the entry gave it; a provider that cannot be queried leaves links alone.Restores relink instead of trusting old keys. A lookup key is not portable between phones: for an entry with no sync account, it holds a device-local row id, which can resolve to a different person on the new phone. So after a legacy backup, a device transfer, a backup v2 restore, or a storage service restore, every previously linked recipient becomes
NEEDED, and the post-registration sync relinks whatever it can match by number.UI.
NEEDED, lists them for relinking or dismissal.Schema and formats
system_phone_e164(the number a link matched on), and V330 addssystem_contact_link_state.system_info_pendingis no longer used; I left it in place.Backup.proto, the storage service protos, or anything else shared with iOS or Desktop. Restores recognize previously linked recipients by the system name fields they already carry.Commits
Each commit builds and passes its own tests, and the behavior change is isolated in one commit for review.
StateFlowdropped recipient updates becauseRecipient.equalscompares only ids.)Testing
lib/contacts, which had no tests. The tests for each change to existing behavior fail without that change. The full unit suite, ktlint, and fast-lint pass.NEEDED; and the Unlinked screen. The new screens look right in light and dark themes, and very long names truncate.ACTION_INSERT. Google Contacts does; I have not tried other vendors' contacts apps. If one does not, nothing is linked (the screen says so when the app reports success without a URI), and picking the newly created contact afterward still works.No in-app help needed changes. Signal's support articles about contact names and photos may want updating once this ships, since a hidden number no longer removes them.