PIM: explicitly re-calculate pre-computed data on locale change

The normalization of phone numbers depends on the locale. This commit
adds a test for that and code which works without relying on EDS to
re-calculate those normalized numbers.

This is not the normal mode of operation and thus this can't be the
final solution. The right solution would be for EDS to notice the
locale change, re-check all precomputed phone numbers
(X-EVOLUTION-E164 parameter) and emit change notifications. Those then
would get picked up by folks and from there, update the PIM views.

The tests rely on running dbus-session.sh with
DBUS_SESSION_SH_SYSTEM_BUS set.

src/dbus/server/pim/folks.cpp

@@ -90,18 +90,20 @@ boost::shared_ptr<IndividualCompare> IndividualCompare::defaultCompare()
     return compare;
 }
 
-void IndividualData::init(const IndividualCompare *compare,
+bool IndividualData::init(const IndividualCompare *compare,
                           const LocaleFactory *locale,
                           FolksIndividual *individual)
 {
+    bool precomputedModified = false;
     m_individual = FolksIndividualCXX(individual, ADD_REF);
     if (compare) {
         m_criteria.clear();
         compare->createCriteria(individual, m_criteria);
     }
     if (locale) {
-        locale->precompute(individual, m_precomputed);
+        precomputedModified = locale->precompute(individual, m_precomputed);
     }
+    return precomputedModified;
 }
 
 bool IndividualCompare::compare(const Criteria_t &a, const Criteria_t &b) const

src/dbus/server/pim/folks.h

@@ -116,8 +116,10 @@ struct IndividualData
      * Sets all members to match the given individual, using the
      * compare instance to compute values. Both compare and locale may
      * be NULL.
+     *
+     * Returns true if the precomputed values changed.
      */
-    void init(const IndividualCompare *compare,
+    bool init(const IndividualCompare *compare,
               const LocaleFactory *locale,
               FolksIndividual *individual);
 

src/dbus/server/pim/full-view.cpp

@@ -21,6 +21,8 @@
 
 #include <syncevo/BoostHelper.h>
 
+#include <boost/dynamic_bitset.hpp>
+
 #include <syncevo/declarations.h>
 SE_BEGIN_CXX
 
@@ -28,6 +30,7 @@ FullView::FullView(const FolksIndividualAggregatorCXX &folks,
                    const boost::shared_ptr<LocaleFactory> &locale) :
     m_folks(folks),
     m_locale(locale),
+    m_localeChanged(false), // set only after explicit setLocale()
     m_isQuiescent(false),
     // Ensure that there is a sort criteria.
     m_compare(IndividualCompare::defaultCompare())
@@ -108,6 +111,7 @@ boost::shared_ptr<FullView> FullView::create(const FolksIndividualAggregatorCXX
 void FullView::setLocale(const boost::shared_ptr<LocaleFactory> &locale)
 {
     m_locale = locale;
+    m_localeChanged = true;
 
     // Don't recompute all IndividualData content. That will be done
     // as part of setCompare(), which must be called later.
@@ -306,8 +310,17 @@ void FullView::setCompare(const boost::shared_ptr<IndividualCompare> &compare)
     memcpy(old.get(), m_entries.c_array(), sizeof(IndividualData *) * m_entries.size());
 
     // Change sort criteria and sort.
-    BOOST_FOREACH (IndividualData &data, m_entries) {
-        data.init(m_compare.get(), NULL, data.m_individual);
+    // Optionally also re-compute locale-dependent values, if
+    // the locale changed (see setLocale()).
+    LocaleFactory *locale = m_localeChanged ? m_locale.get() : NULL;
+    m_localeChanged = false;
+    boost::dynamic_bitset<size_t> modified(locale ? m_entries.size() : 0);
+    for (size_t i = 0; i < m_entries.size(); i++ ) {
+        IndividualData &data = m_entries[i];
+        bool preComputedModified = data.init(m_compare.get(), locale, data.m_individual);
+        if (locale && preComputedModified) {
+            modified.set(i);
+        }
     }
     m_entries.sort(IndividualDataCompare(m_compare));
 
@@ -315,7 +328,8 @@ void FullView::setCompare(const boost::shared_ptr<IndividualCompare> &compare)
     for (size_t index = 0; index < m_entries.size(); index++) {
         IndividualData &previous = *old[index],
             &current = m_entries[index];
-        if (previous.m_individual != current.m_individual) {
+        if (previous.m_individual != current.m_individual ||
+            (locale && modified[index])) {
             // Contact at the index changed. Don't try to find out
             // where it came from now. The effect is that temporarily
             // the same contact might be shown at two different

src/dbus/server/pim/full-view.h

@@ -33,6 +33,7 @@ class FullView : public IndividualView
 {
     FolksIndividualAggregatorCXX m_folks;
     boost::shared_ptr<LocaleFactory> m_locale;
+    bool m_localeChanged;
     bool m_isQuiescent;
     boost::weak_ptr<FullView> m_self;
     Timeout m_waitForIdle;

src/dbus/server/pim/locale-factory-boost.cpp

@@ -1014,7 +1014,7 @@ class LocaleFactoryBoost : public LocaleFactory
 public:
     LocaleFactoryBoost() :
         m_phoneNumberUtil(*i18n::phonenumbers::PhoneNumberUtil::GetInstance()),
-        m_edsSupportsPhoneNumbers(e_phone_number_is_supported()),
+        m_edsSupportsPhoneNumbers(e_phone_number_is_supported() && !getenv("SYNCEVOLUTION_PIM_EDS_NO_E164")),
         m_locale(genLocale()),
         m_country(std::use_facet<boost::locale::info>(m_locale).country()),
         m_defaultCountryCode(StringPrintf("+%d", m_phoneNumberUtil.GetCountryCodeForRegion(m_country)))
@@ -1178,9 +1178,10 @@ public:
         return res ? res : LocaleFactory::createFilter(filter, level);
     }
 
-    virtual void precompute(FolksIndividual *individual, Precomputed &precomputed) const
+    virtual bool precompute(FolksIndividual *individual, Precomputed &precomputed) const
     {
-        precomputed.m_phoneNumbers.clear();
+        LocaleFactory::Precomputed old;
+        std::swap(old, precomputed);
 
         FolksPhoneDetails *phoneDetails = FOLKS_PHONE_DETAILS(individual);
         GeeSet *phones = folks_phone_details_get_phone_numbers(phoneDetails);
@@ -1251,6 +1252,9 @@ public:
                 }
             }
         }
+
+        // Now check if any phone number changed.
+        return old != precomputed;
     }
 };
 

src/dbus/server/pim/locale-factory.cpp

@@ -41,6 +41,23 @@ std::string SimpleE164::toString() const
     return out.str();
 }
 
+bool LocaleFactory::Precomputed::operator == (const LocaleFactory::Precomputed &other) const
+{
+    if (other.m_phoneNumbers.size() != m_phoneNumbers.size()) {
+        return false;
+    }
+    PhoneNumbers::const_iterator ita = other.m_phoneNumbers.begin();
+    PhoneNumbers::const_iterator itb = m_phoneNumbers.begin();
+    while (ita != other.m_phoneNumbers.end()) {
+        if (*ita != *itb) {
+            return false;
+        }
+        ++ita;
+        ++itb;
+    }
+    return true;
+}
+
 class Filter2StringVisitor : public boost::static_visitor<void>
 {
     std::ostringstream m_out;

src/dbus/server/pim/locale-factory.h

@@ -71,6 +71,13 @@ class SimpleE164
      * empty string if both are unset.
      */
     std::string toString() const;
+
+    bool operator == (const SimpleE164 &other) const 
+    {
+        return m_countryCode == other.m_countryCode &&
+        m_nationalNumber == other.m_nationalNumber;
+    }
+    bool operator != (const SimpleE164 &other) const { return !(*this == other); }
 };
 
 /**
@@ -147,13 +154,16 @@ class LocaleFactory
      */
     struct Precomputed
     {
-        std::vector<SimpleE164> m_phoneNumbers;
+        typedef std::vector<SimpleE164> PhoneNumbers;
+        PhoneNumbers m_phoneNumbers;
+        bool operator == (const Precomputed &other) const;
+        bool operator != (const Precomputed &other) const { return !(*this == other); }
     };
 
     /**
      * (Re)set pre-computed data for an individual.
      */
-    virtual void precompute(FolksIndividual *individual, Precomputed &precomputed) const = 0;
+    virtual bool precompute(FolksIndividual *individual, Precomputed &precomputed) const = 0;
 };
 
 SE_END_CXX

src/dbus/server/pim/testpim.py

@@ -2449,6 +2449,7 @@ END:VCARD''']):
         self.setUpView()
 
         msg = None
+        view = None
         try:
              # Insert new contacts and calculate their family names.
              names = []
@@ -2519,6 +2520,7 @@ END:VCARD
                   raise Exception('%s:\n%s' % (msg, repr(ex))), None, info[2]
              else:
                   raise
+        return view
 
     @timeout(60)
     @property("ENV", "LC_TYPE=ja_JP.UTF-8 LC_ALL=ja_JP.UTF-8 LANG=ja_JP.UTF-8")
@@ -2565,7 +2567,7 @@ END:VCARD
                        )
 
     @timeout(60)
-    @property("ENV", "LC_TYPE=zh_CN.UTF-8 LANG=zh_CN.UTF-8 DBUS_TEST_LOCALED=session")
+    @property("ENV", "LC_TYPE=zh_CN.UTF-8 LANG=zh_CN.UTF-8")
     def testLocaled(self):
          # Use mixed Chinese/Western names, because then the locale really matters.
          namespinyin = ('Adams', 'Jeffries', u'江', 'Meadows', u'鳥', u'女性' )
@@ -2573,7 +2575,7 @@ END:VCARD
          numtestcases = len(namespinyin)
          self.doFilter(namespinyin, ())
 
-         daemon = localed.Localed(bus)
+         daemon = localed.Localed()
          msg = None
          try:
               # Broadcast Locale value together with PropertiesChanged signal.
@@ -2582,7 +2584,7 @@ END:VCARD
               logging.log('reading contacts, German')
               self.runUntil('German sorting',
                             check=lambda: self.assertEqual([], self.view.errors),
-                            until=lambda: self.view.quiescentCount > 0)
+                            until=lambda: self.view.quiescentCount > 1)
               self.view.read(0, numtestcases)
               self.runUntil('German contacts',
                             check=lambda: self.assertEqual([], self.view.errors),
@@ -2597,7 +2599,7 @@ END:VCARD
               logging.log('reading contacts, Pinyin')
               self.runUntil('Pinyin sorting',
                             check=lambda: self.assertEqual([], self.view.errors),
-                            until=lambda: self.view.quiescentCount > 0)
+                            until=lambda: self.view.quiescentCount > 1)
               self.view.read(0, numtestcases)
               self.runUntil('Pinyin contacts',
                             check=lambda: self.assertEqual([], self.view.errors),
@@ -2612,6 +2614,52 @@ END:VCARD
              else:
                   raise
 
+    @timeout(60)
+    # Must disable usage of pre-computed phone numbers from EDS, because we can't tell EDS
+    # when locale is meant to change.
+    @property("ENV", "LANG=en_US.UTF-8 SYNCEVOLUTION_PIM_EDS_NO_E164=1")
+    def testLocaledPhone(self):
+         # Parsing of 1234-5 depends on locale: US drops the 1 from 1234
+         # Germany (and other countries) don't. Use that to match (or not match)
+         # a contact.
+         testcases = (('Doe', '''BEGIN:VCARD
+VERSION:3.0
+FN:Doe
+N:Doe;;;;
+TEL:12 34-5
+END:VCARD
+'''),)
+         names = ('Doe')
+         numtestcases = len(testcases)
+         view = self.doFilter(testcases,
+                              (([['phone', '+12345']], ('Doe',)),))
+
+         daemon = localed.Localed()
+         msg = None
+         try:
+              # Contact no longer matched because it's phone number normalization
+              # becomes different.
+              view.quiescentCount = 0
+              daemon.SetLocale(['LANG=de_DE.UTF-8'], True)
+              self.runUntil('German locale',
+                            check=lambda: self.assertEqual([], view.errors),
+                            until=lambda: view.quiescentCount > 1)
+              self.assertEqual(len(view.contacts), 0)
+
+              # Switch back to US.
+              view.quiescentCount = 0
+              daemon.SetLocale(['LANG=en_US.UTF-8'], True)
+              self.runUntil('US locale',
+                            check=lambda: self.assertEqual([], view.errors),
+                            until=lambda: view.quiescentCount > 1)
+              self.assertEqual(len(view.contacts), 1)
+         except Exception, ex:
+             if msg:
+                  info = sys.exc_info()
+                  raise Exception('%s:\n%s' % (msg, repr(ex))), None, info[2]
+             else:
+                  raise
+
     # Not supported correctly by ICU?
     # See icu-support "Subject: Austrian phone book sorting"
     # @timeout(60)