From 4e815f9cb92f868e173017dcc9a1783324d27886 Mon Sep 17 00:00:00 2001 From: zachh Date: Mon, 4 Dec 2017 17:08:04 -0800 Subject: Removed timestamps from PhoneLookup APIs. Also added onSuccessfulBulkUpdate method. It is safer for each PhoneLookup to keep track of its own last processed time via shared prefs where the value saved is the actual last processed timestamp from the underlying data. This is because it is difficult or impossible to select a single time that spans lookups due to queries being run and processed at different times. The onSuccessfulBulkUpdate method is provided as a hook for PhoneLookups to persist their shared pref once they know the result of bulkUpdate have been successfully saved. Finally, removed usage of the lastModified timestamp from PhoneLookupHistory in PhoneLookupDataSource since I believe the cases it was originally intended to cover are now handled by populateInserts(). Bug: 34672501 Test: unit PiperOrigin-RevId: 177891586 Change-Id: I072409fc217e4d7e36816548862e8b358aebf165 --- .../android/dialer/phonelookup/PhoneLookup.java | 26 ++++++--- .../composite/CompositePhoneLookup.java | 19 +++++-- .../dialer/phonelookup/cp2/Cp2PhoneLookup.java | 66 ++++++++++++++++++---- 3 files changed, 87 insertions(+), 24 deletions(-) (limited to 'java/com/android/dialer/phonelookup') diff --git a/java/com/android/dialer/phonelookup/PhoneLookup.java b/java/com/android/dialer/phonelookup/PhoneLookup.java index 66f166d6d..183277569 100644 --- a/java/com/android/dialer/phonelookup/PhoneLookup.java +++ b/java/com/android/dialer/phonelookup/PhoneLookup.java @@ -27,8 +27,8 @@ import com.google.common.util.concurrent.ListenableFuture; * Provides operations related to retrieving information about phone numbers. * *

Some operations defined by this interface are generally targeted towards specific use cases; - * for example {@link #isDirty(ImmutableSet, long)} and {@link #bulkUpdate(ImmutableMap, long)} are - * generally intended to be used by the call log. + * for example {@link #isDirty(ImmutableSet)}, {@link #bulkUpdate(ImmutableMap)}, and {@link + * #onSuccessfulBulkUpdate()} are generally intended to be used by the call log. */ public interface PhoneLookup { @@ -43,14 +43,14 @@ public interface PhoneLookup { /** * Returns a future which returns true if the information for any of the provided phone numbers - * has changed since {@code lastModified} according to this {@link PhoneLookup}. + * has changed, usually since {@link #onSuccessfulBulkUpdate()} was last invoked. */ - ListenableFuture isDirty( - ImmutableSet phoneNumbers, long lastModified); + ListenableFuture isDirty(ImmutableSet phoneNumbers); /** - * Given a set of existing information and a timestamp, returns a set of information with any - * changes made since the timestamp according to this {@link PhoneLookup}. + * Performs a bulk update of this {@link PhoneLookup}. The returned map must contain the exact + * same keys as the provided map. Most implementations will rely on last modified timestamps to + * efficiently only update the data which needs to be updated. * *

If there are no changes required, it is valid for this method to simply return the provided * {@code existingInfoMap}. @@ -59,5 +59,15 @@ public interface PhoneLookup { * deleted) the returned map should contain an empty {@link PhoneLookupInfo} for that number. */ ListenableFuture> bulkUpdate( - ImmutableMap existingInfoMap, long lastModified); + ImmutableMap existingInfoMap); + + /** + * Called when the results of the {@link #bulkUpdate(ImmutableMap)} have been applied by the + * caller. + * + *

Typically implementations will use this to store a "last processed" timestamp so that future + * invocations of {@link #isDirty(ImmutableSet)} and {@link #bulkUpdate(ImmutableMap)} can be + * efficiently implemented. + */ + ListenableFuture onSuccessfulBulkUpdate(); } diff --git a/java/com/android/dialer/phonelookup/composite/CompositePhoneLookup.java b/java/com/android/dialer/phonelookup/composite/CompositePhoneLookup.java index 59a845774..520c46f9e 100644 --- a/java/com/android/dialer/phonelookup/composite/CompositePhoneLookup.java +++ b/java/com/android/dialer/phonelookup/composite/CompositePhoneLookup.java @@ -70,11 +70,10 @@ public final class CompositePhoneLookup implements PhoneLookup { } @Override - public ListenableFuture isDirty( - ImmutableSet phoneNumbers, long lastModified) { + public ListenableFuture isDirty(ImmutableSet phoneNumbers) { List> futures = new ArrayList<>(); for (PhoneLookup phoneLookup : phoneLookups) { - futures.add(phoneLookup.isDirty(phoneNumbers, lastModified)); + futures.add(phoneLookup.isDirty(phoneNumbers)); } // Executes all child lookups (possibly in parallel), completing when the first composite lookup // which returns "true" completes, and cancels the others. @@ -90,11 +89,11 @@ public final class CompositePhoneLookup implements PhoneLookup { */ @Override public ListenableFuture> bulkUpdate( - ImmutableMap existingInfoMap, long lastModified) { + ImmutableMap existingInfoMap) { List>> futures = new ArrayList<>(); for (PhoneLookup phoneLookup : phoneLookups) { - futures.add(phoneLookup.bulkUpdate(existingInfoMap, lastModified)); + futures.add(phoneLookup.bulkUpdate(existingInfoMap)); } return Futures.transform( Futures.allAsList(futures), @@ -118,4 +117,14 @@ public final class CompositePhoneLookup implements PhoneLookup { }, MoreExecutors.directExecutor()); } + + @Override + public ListenableFuture onSuccessfulBulkUpdate() { + List> futures = new ArrayList<>(); + for (PhoneLookup phoneLookup : phoneLookups) { + futures.add(phoneLookup.onSuccessfulBulkUpdate()); + } + return Futures.transform( + Futures.allAsList(futures), unused -> null, MoreExecutors.directExecutor()); + } } diff --git a/java/com/android/dialer/phonelookup/cp2/Cp2PhoneLookup.java b/java/com/android/dialer/phonelookup/cp2/Cp2PhoneLookup.java index 4ebd401c3..307f998ea 100644 --- a/java/com/android/dialer/phonelookup/cp2/Cp2PhoneLookup.java +++ b/java/com/android/dialer/phonelookup/cp2/Cp2PhoneLookup.java @@ -17,11 +17,13 @@ package com.android.dialer.phonelookup.cp2; import android.content.Context; +import android.content.SharedPreferences; import android.database.Cursor; import android.provider.ContactsContract.CommonDataKinds.Phone; import android.provider.ContactsContract.Contacts; import android.provider.ContactsContract.DeletedContacts; import android.support.annotation.NonNull; +import android.support.annotation.Nullable; import android.support.v4.util.ArrayMap; import android.support.v4.util.ArraySet; import android.telecom.Call; @@ -33,6 +35,7 @@ import com.android.dialer.phonelookup.PhoneLookup; import com.android.dialer.phonelookup.PhoneLookupInfo; import com.android.dialer.phonelookup.PhoneLookupInfo.Cp2Info; import com.android.dialer.phonelookup.PhoneLookupInfo.Cp2Info.Cp2ContactInfo; +import com.android.dialer.storage.Unencrypted; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.util.concurrent.ListenableFuture; @@ -46,6 +49,9 @@ import javax.inject.Inject; /** PhoneLookup implementation for local contacts. */ public final class Cp2PhoneLookup implements PhoneLookup { + private static final String PREF_LAST_TIMESTAMP_PROCESSED = + "cp2PhoneLookupLastTimestampProcessed"; + private static final String[] CP2_INFO_PROJECTION = new String[] { Phone.DISPLAY_NAME_PRIMARY, // 0 @@ -64,10 +70,14 @@ public final class Cp2PhoneLookup implements PhoneLookup { private static final int CP2_INFO_CONTACT_ID_INDEX = 5; private final Context appContext; + private final SharedPreferences sharedPreferences; + @Nullable private Long currentLastTimestampProcessed; @Inject - Cp2PhoneLookup(@ApplicationContext Context appContext) { + Cp2PhoneLookup( + @ApplicationContext Context appContext, @Unencrypted SharedPreferences sharedPreferences) { this.appContext = appContext; + this.sharedPreferences = sharedPreferences; } @Override @@ -76,14 +86,13 @@ public final class Cp2PhoneLookup implements PhoneLookup { } @Override - public ListenableFuture isDirty( - ImmutableSet phoneNumbers, long lastModified) { + public ListenableFuture isDirty(ImmutableSet phoneNumbers) { // TODO(calderwoodra): consider a different thread pool - return MoreExecutors.newDirectExecutorService() - .submit(() -> isDirtyInternal(phoneNumbers, lastModified)); + return MoreExecutors.newDirectExecutorService().submit(() -> isDirtyInternal(phoneNumbers)); } - private boolean isDirtyInternal(ImmutableSet phoneNumbers, long lastModified) { + private boolean isDirtyInternal(ImmutableSet phoneNumbers) { + long lastModified = sharedPreferences.getLong(PREF_LAST_TIMESTAMP_PROCESSED, 0L); return contactsUpdated(queryPhoneTableForContactIds(phoneNumbers), lastModified) || contactsDeleted(lastModified); } @@ -149,7 +158,12 @@ public final class Cp2PhoneLookup implements PhoneLookup { return appContext .getContentResolver() - .query(Contacts.CONTENT_URI, new String[] {Contacts._ID}, where, args, null); + .query( + Contacts.CONTENT_URI, + new String[] {Contacts._ID, Contacts.CONTACT_LAST_UPDATED_TIMESTAMP}, + where, + args, + null); } /** Returns true if any contacts were deleted after {@code lastModified}. */ @@ -169,13 +183,16 @@ public final class Cp2PhoneLookup implements PhoneLookup { @Override public ListenableFuture> bulkUpdate( - ImmutableMap existingInfoMap, long lastModified) { + ImmutableMap existingInfoMap) { return MoreExecutors.newDirectExecutorService() - .submit(() -> bulkUpdateInternal(existingInfoMap, lastModified)); + .submit(() -> bulkUpdateInternal(existingInfoMap)); } private ImmutableMap bulkUpdateInternal( - ImmutableMap existingInfoMap, long lastModified) { + ImmutableMap existingInfoMap) { + currentLastTimestampProcessed = null; + long lastModified = sharedPreferences.getLong(PREF_LAST_TIMESTAMP_PROCESSED, 0L); + // Build a set of each DialerPhoneNumber that was associated with a contact, and is no longer // associated with that same contact. Set deletedPhoneNumbers = @@ -214,6 +231,21 @@ public final class Cp2PhoneLookup implements PhoneLookup { return newInfoMapBuilder.build(); } + @Override + public ListenableFuture onSuccessfulBulkUpdate() { + return MoreExecutors.newDirectExecutorService() + .submit( + () -> { + if (currentLastTimestampProcessed != null) { + sharedPreferences + .edit() + .putLong(PREF_LAST_TIMESTAMP_PROCESSED, currentLastTimestampProcessed) + .apply(); + } + return null; + }); + } + /** * 1. get all contact ids. if the id is unset, add the number to the list of contacts to look up. * 2. reduce our list of contact ids to those that were updated after lastModified. 3. Now we have @@ -261,12 +293,18 @@ public final class Cp2PhoneLookup implements PhoneLookup { // after lastModified, such that Contacts._ID is in our set of contact IDs we build above. try (Cursor cursor = queryContactsTableForContacts(contactIds, lastModified)) { int contactIdIndex = cursor.getColumnIndex(Contacts._ID); + int lastUpdatedIndex = cursor.getColumnIndex(Contacts.CONTACT_LAST_UPDATED_TIMESTAMP); cursor.moveToPosition(-1); while (cursor.moveToNext()) { // Find the DialerPhoneNumber for each contact id and add it to our updated numbers set. // These, along with our number not associated with any Cp2ContactInfo need to be updated. long contactId = cursor.getLong(contactIdIndex); updatedNumbers.addAll(getDialerPhoneNumber(existingInfoMap, contactId)); + long lastUpdatedTimestamp = cursor.getLong(lastUpdatedIndex); + if (currentLastTimestampProcessed == null + || currentLastTimestampProcessed < lastUpdatedTimestamp) { + currentLastTimestampProcessed = lastUpdatedTimestamp; + } } } @@ -379,7 +417,7 @@ public final class Cp2PhoneLookup implements PhoneLookup { .getContentResolver() .query( DeletedContacts.CONTENT_URI, - new String[] {DeletedContacts.CONTACT_ID}, + new String[] {DeletedContacts.CONTACT_ID, DeletedContacts.CONTACT_DELETED_TIMESTAMP}, where, args, null); @@ -389,11 +427,17 @@ public final class Cp2PhoneLookup implements PhoneLookup { private Set findDeletedPhoneNumbersIn( ImmutableMap existingInfoMap, Cursor cursor) { int contactIdIndex = cursor.getColumnIndexOrThrow(DeletedContacts.CONTACT_ID); + int deletedTimeIndex = cursor.getColumnIndexOrThrow(DeletedContacts.CONTACT_DELETED_TIMESTAMP); Set deletedPhoneNumbers = new ArraySet<>(); cursor.moveToPosition(-1); while (cursor.moveToNext()) { long contactId = cursor.getLong(contactIdIndex); deletedPhoneNumbers.addAll(getDialerPhoneNumber(existingInfoMap, contactId)); + long deletedTime = cursor.getLong(deletedTimeIndex); + if (currentLastTimestampProcessed == null || currentLastTimestampProcessed < deletedTime) { + // TODO(zachh): There's a problem here if a contact for a new row is deleted? + currentLastTimestampProcessed = deletedTime; + } } return deletedPhoneNumbers; } -- cgit v1.2.3