alexrothenberg / motion-addressbook

MIT License
89 stars 30 forks source link

Duplicate Contacts With Linked Cards #4

Closed jamonholmgren closed 11 years ago

jamonholmgren commented 12 years ago

Have you dealt with the duplicate contacts issue described here?

http://stackoverflow.com/questions/11351454/dealing-with-duplicate-contacts-due-to-linked-cards-in-ios-address-book-api

Looks doable to implement in motion-addressbook.

alexrothenberg commented 12 years ago

This is new to me and not something I've thought about.

Let's think about the API to expose this. I'm brainstorming a couple of different options and am not sure what feels best.

Option 1: AddressBook::Person can return either unified (no duplicates) or list with duplicates to all finder methods. Downside is we would need to duplicate all the methods or be fancy with the method_missing

AddressBook::Person.all
# returns a list of all people (including duplicates for linked people)
AddressBook::Person.all_unified
# returns a list of all people (with linked people combined)

Option 2: Add an optional "unified" flag to all finder methods. Each method could return unified or with-duplicates people. Downside its not clear what the "true" means when reading the client code

AddressBook::Person.all
# returns a list of all people (including duplicates for linked people)
AddressBook::Person.all(true)
# returns a list of all people (with linked people combined)

Option 3: Add a subclass of Person called UnifiedPerson Downside would UnifiedPerson just be used for the finder methods? Would it still return an array of Person objects?

AddressBook::Person.all
# returns a list of all people (including duplicates for linked people)
AddressBook::UnifiedPerson.all
# returns a list of all people (with linked people combined)

WDYT?

jamonholmgren commented 12 years ago

I like option 2, but possibly with a named parameter to make it more clear:

AddressBook::Person.all
# returns a list of all people (including duplicates for linked people)

AddressBook::Person.all(unique:true)
# returns a list of all people (with linked people combined)

unique/unified/combined/deduplicated ... whatever you think.

We would just have to change the all method to have parameters and then check that:

def self.all(options={})
  people = ABAddressBookCopyArrayOfAllPeople(AddressBook.address_book).map do |ab_person|
    new({}, ab_person)
  end
  people = deduplicate(people) if options[:unique]
  people.sort! { |a,b| "#{a.first_name} #{a.last_name}" <=> "#{b.first_name} #{b.last_name}" }
  people
end

(Probably not what it will look like, but just to get an example)

jamonholmgren commented 12 years ago

Actually, the example code that I linked to would probably be used within your map iterator to deduplicate.

jmay commented 11 years ago

Dealing with multiple-cards-for-the-same-person is more complicated than this.

Example: If the user has enabled Facebook integration on their device, then Facebook contacts will appear as ABPerson records. But these records will be read-only - I'm not sure what kind of error will be thrown if you try to update any record properties.

OSX does the same thing in Contacts.app, guessing which records from different sources are for the same person and then combining properties from linked records into a single view in the UI.

I don't think that motion-addressbook should attempt to duplicate this record-unification logic. But it would make sense to expose the linked records in the API. I suggest that Person#all should continue to work as currently (just passing through the results of ABAddressBookCopyArrayOfAllPeople). But perhaps we should add an allInSource methods that exposes the results of ABAddressBookCopyArrayOfAllPeopleInSource.

alexrothenberg commented 11 years ago

I just came across an objective-c cocoapod that wraps addressbook similar to this gem - https://github.com/heardrwt/RHAddressBook

It looks like they are exposing an array of linkedPeople for each person using the same ABPersonCopyArrayOfAllLinkedPeople api we have been talking about.

If I understand what they did there may still be multiple records for the same person but they would all be linked together. That still feels a little awkward to use but might be a simple first pass that we could refine over time.

jmay commented 11 years ago

The iOS ABPersonCopyArrayOfAllLinkedPeople function returns a list that includes the record being queried, so you always get back at least one entry.

Does it make sense to preserve this behavior? If we filter out self from #linkedPeople then we could use #any? to find those records that have partners.

It would also make sense to expose the ABSource information. It looks to me that LinkedPeople is only for matching across sources, there's no facility for linking records within the same source.

jmay commented 11 years ago

Access to the source & linkedPeople information is now available through the gem. Perhaps this is a good solution or at least a first step for dealing with duplicates.