Fix bug 3121292: Contact photo not shown correctly for SIP calls

The problem was that when we did a contact lookup based on a SIP address,
the resulting CallerInfo object did not have the person_id field set
correctly.  That meant we had no way to look up the photo for that person.

This was because of a missing case in the logic to determine which column
(in the resulting cursor) to use for the person_id lookup.  We were
handling lookups fine in the PhoneLookup and Phone tables, but were
missing a case for direct lookups in the Data table (which is how we look
up SIP addresses.)

The fix is to add a case for URIs like
"content://com.android.contacts/data" when looking up the person_id.

Also, since the person_id lookup is pretty hairy (and includes ~20 lines
of comments to explain what it's doing!) refactor it out into a helper
method.

TESTED: Both SIP and PSTN calls; verified that contact name *and* photo
are displayed correctly in all cases.

Bug: 3121292
Change-Id: I2b0083cc5394c1a49bbdc9a4e5651854aedb82f7

1 files changed, 90 insertions, 28 deletions

diff --git a/telephony/java/com/android/internal/telephony/CallerInfo.java b/telephony/java/com/android/internal/telephony/CallerInfo.java
index f7506c6..e4d7943 100644
--- a/telephony/java/com/android/internal/telephony/CallerInfo.java
+++ b/telephony/java/com/android/internal/telephony/CallerInfo.java
@@ -20,12 +20,13 @@ import android.content.Context;
 import android.database.Cursor;
 import android.graphics.drawable.Drawable;
 import android.net.Uri;
-import android.provider.ContactsContract.PhoneLookup;
 import android.provider.ContactsContract.CommonDataKinds.Phone;
-import static android.provider.ContactsContract.RawContacts;
-import android.text.TextUtils;
-import android.telephony.TelephonyManager;
+import android.provider.ContactsContract.Data;
+import android.provider.ContactsContract.PhoneLookup;
+import android.provider.ContactsContract.RawContacts;
 import android.telephony.PhoneNumberUtils;
+import android.telephony.TelephonyManager;
+import android.text.TextUtils;
 import android.util.Log;
 
 
@@ -164,33 +165,17 @@ public class CallerInfo {
                     }
                 }
 
-                // Look for the person ID.
-
-                // TODO: This is pretty ugly now, see bug 2269240 for
-                // more details. The column to use depends upon the type of URL,
-                // for content://com.android.contacts/data/phones the "contact_id"
-                // column is used. For content/com.andriod.contacts/phone_lookup"
-                // the "_ID" column is used. If it is neither we leave columnIndex
-                // at -1 and no person ID will be available.
-
-                columnIndex = -1;
-                String url = contactRef.toString();
-                if (url.startsWith("content://com.android.contacts/data/phones")) {
-                    if (VDBG) Log.v(TAG,
-                        "URL path starts with 'data/phones' using RawContacts.CONTACT_ID");
-                    columnIndex = cursor.getColumnIndex(RawContacts.CONTACT_ID);
-                } else if (url.startsWith("content://com.android.contacts/phone_lookup")) {
-                    if (VDBG) Log.v(TAG,
-                        "URL path starts with 'phone_lookup' using PhoneLookup._ID");
-                    columnIndex = cursor.getColumnIndex(PhoneLookup._ID);
-                } else {
-                    Log.e(TAG, "Bad contact URL '" + url + "'");
-                }
-
+                // Look for the person_id.
+                columnIndex = getColumnIndexForPersonId(contactRef, cursor);
                 if (columnIndex != -1) {
                     info.person_id = cursor.getLong(columnIndex);
+                    if (VDBG) Log.v(TAG, "==> got info.person_id: " + info.person_id);
                 } else {
-                    Log.e(TAG, "person_id column missing for " + contactRef);
+                    // No valid columnIndex, so we can't look up person_id.
+                    Log.w(TAG, "Couldn't find person_id column for " + contactRef);
+                    // Watch out: this means that anything that depends on
+                    // person_id will be broken (like contact photo lookups in
+                    // the in-call UI, for example.)
                 }
 
                 // look for the custom ringtone, create from the string stored
@@ -404,6 +389,83 @@ public class CallerInfo {
     }
 
     /**
+     * Returns the column index to use to find the "person_id" field in
+     * the specified cursor, based on the contact URI that was originally
+     * queried.
+     *
+     * This is a helper function for the getCallerInfo() method that takes
+     * a Cursor.  Looking up the person_id is nontrivial (compared to all
+     * the other CallerInfo fields) since the column we need to use
+     * depends on what query we originally ran.
+     *
+     * Watch out: be sure to not do any database access in this method, since
+     * it's run from the UI thread (see comments below for more info.)
+     *
+     * @return the columnIndex to use (with cursor.getLong()) to get the
+     * person_id, or -1 if we couldn't figure out what colum to use.
+     *
+     * TODO: Add a unittest for this method.  (This is a little tricky to
+     * test, since we'll need a live contacts database to test against,
+     * preloaded with at least some phone numbers and SIP addresses.  And
+     * we'll probably have to hardcode the column indexes we expect, so
+     * the test might break whenever the contacts schema changes.  But we
+     * can at least make sure we handle all the URI patterns we claim to,
+     * and that the mime types match what we expect...)
+     */
+    private static int getColumnIndexForPersonId(Uri contactRef, Cursor cursor) {
+        // TODO: This is pretty ugly now, see bug 2269240 for
+        // more details. The column to use depends upon the type of URL:
+        // - content://com.android.contacts/data/phones ==> use the "contact_id" column
+        // - content://com.android.contacts/phone_lookup ==> use the "_ID" column
+        // - content://com.android.contacts/data ==> use the "contact_id" column
+        // If it's none of the above, we leave columnIndex=-1 which means
+        // that the person_id field will be left unset.
+        //
+        // The logic here *used* to be based on the mime type of contactRef
+        // (for example Phone.CONTENT_ITEM_TYPE would tell us to use the
+        // RawContacts.CONTACT_ID column).  But looking up the mime type requires
+        // a call to context.getContentResolver().getType(contactRef), which
+        // isn't safe to do from the UI thread since it can cause an ANR if
+        // the contacts provider is slow or blocked (like during a sync.)
+        //
+        // So instead, figure out the column to use for person_id by just
+        // looking at the URI itself.
+
+        if (VDBG) Log.v(TAG, "- getColumnIndexForPersonId: contactRef URI = '"
+                        + contactRef + "'...");
+        // Warning: Do not enable the following logging (due to ANR risk.)
+        // if (VDBG) Log.v(TAG, "- MIME type: "
+        //                 + context.getContentResolver().getType(contactRef));
+
+        String url = contactRef.toString();
+        String columnName = null;
+        if (url.startsWith("content://com.android.contacts/data/phones")) {
+            // Direct lookup in the Phone table.
+            // MIME type: Phone.CONTENT_ITEM_TYPE (= "vnd.android.cursor.item/phone_v2")
+            if (VDBG) Log.v(TAG, "'data/phones' URI; using RawContacts.CONTACT_ID");
+            columnName = RawContacts.CONTACT_ID;
+        } else if (url.startsWith("content://com.android.contacts/data")) {
+            // Direct lookup in the Data table.
+            // MIME type: Data.CONTENT_TYPE (= "vnd.android.cursor.dir/data")
+            if (VDBG) Log.v(TAG, "'data' URI; using Data.CONTACT_ID");
+            // (Note Data.CONTACT_ID and RawContacts.CONTACT_ID are equivalent.)
+            columnName = Data.CONTACT_ID;
+        } else if (url.startsWith("content://com.android.contacts/phone_lookup")) {
+            // Lookup in the PhoneLookup table, which provides "fuzzy matching"
+            // for phone numbers.
+            // MIME type: PhoneLookup.CONTENT_TYPE (= "vnd.android.cursor.dir/phone_lookup")
+            if (VDBG) Log.v(TAG, "'phone_lookup' URI; using PhoneLookup._ID");
+            columnName = PhoneLookup._ID;
+        } else {
+            Log.w(TAG, "Unexpected prefix for contactRef '" + url + "'");
+        }
+        int columnIndex = (columnName != null) ? cursor.getColumnIndex(columnName) : -1;
+        if (VDBG) Log.v(TAG, "==> Using column '" + columnName
+                        + "' (columnIndex = " + columnIndex + ") for person_id lookup...");
+        return columnIndex;
+    }
+
+    /**
      * @return a string debug representation of this instance.
      */
     public String toString() {