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.

8 files changed, 113 insertions, 15 deletions

diff --git a/src/dbus/server/pim/folks.cpp b/src/dbus/server/pim/folks.cpp
index a28f29ea..9af83e7c 100644
--- a/src/dbus/server/pim/folks.cpp
+++ b/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
diff --git a/src/dbus/server/pim/folks.h b/src/dbus/server/pim/folks.h
index 4b2bc5ef..0f6d86fe 100644
--- a/src/dbus/server/pim/folks.h
+++ b/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);
 
diff --git a/src/dbus/server/pim/full-view.cpp b/src/dbus/server/pim/full-view.cpp
index fdc7fa79..bc9fbe11 100644
--- a/src/dbus/server/pim/full-view.cpp
+++ b/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
diff --git a/src/dbus/server/pim/full-view.h b/src/dbus/server/pim/full-view.h
index 9c17fd89..40531fb9 100644
--- a/src/dbus/server/pim/full-view.h
+++ b/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;
diff --git a/src/dbus/server/pim/locale-factory-boost.cpp b/src/dbus/server/pim/locale-factory-boost.cpp
index e68cd50c..583602b7 100644
--- a/src/dbus/server/pim/locale-factory-boost.cpp
+++ b/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;
     }
 };
 
diff --git a/src/dbus/server/pim/locale-factory.cpp b/src/dbus/server/pim/locale-factory.cpp
index 869f57c1..3b5b9601 100644
--- a/src/dbus/server/pim/locale-factory.cpp
+++ b/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;
diff --git a/src/dbus/server/pim/locale-factory.h b/src/dbus/server/pim/locale-factory.h
index 2ca6968a..9a043acc 100644
--- a/src/dbus/server/pim/locale-factory.h
+++ b/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
diff --git a/src/dbus/server/pim/testpim.py b/src/dbus/server/pim/testpim.py
index 57958b48..2bcf88a2 100755
--- a/src/dbus/server/pim/testpim.py
+++ b/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)