The "func" variable was correctly initialized to NULL if no comparsion
matches, but cppcheck 1.65 warns anyway. Use the more readable
intialization to NULL in the final else clause.
The parsed number always has a country code, whereas SyncEvolution expected it
to be zero for strings without an explicit country code. This caused a caller
ID lookup of numbers like "089788899" in DE to find only telephone numbers in
the current default country, instead of being more permissive and also finding
"+189788899". The corresponding unit test was broken and checked for the wrong
result. Found while investigating an unrelated test failure when updating
libphonenumber.
EDS handles this differently, by calling ParseAndKeepRawInput() if necessary
(checked by configure) and looking at the source of the country code. Instead
of replicating that logic, let's use EPhoneNumber. This means that EDS has to
be compiled with libphonenumber support, because SyncEvolution can no longer
fall back to using libphonenumber directly.
For testing purposes it is still useful to not depend on X-EVOLUTION-E164.
testLocaledPhone uses this at the moment, because re-generating
X-EVOLUTION-E164 during a locale change seems to be broken at the moment
in the intel-work-3-12 branch.
The test itself has to be updated for the newer libphonenumber (6.1.1 instead
of 5.3.2). The "12345" string it relied upon now gets parsed consistently in
US and DE. Instead we use the "01164 3 331 6005" string (as in libphonenumber
tests) which is treated differently.
The "Fixed compilation error when using libphonenumber from revision
>= 568" patch caused a double free error because SetLogger() owns the
logger instance and, with libphonenumber >= r571 actually frees the
instance.
Old libphonenumber release are compatible with the revised call,
however, they never free the instance.
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.
Accent-insensitive search ignores accents, using the same code as in
EDS. Transliterated search ignores foreign scripts by transliterating
search term and contact properties to Latin first. That one is using
ICU directly in the same way as EDS, but doesn't use the EDS
ETransliterator class to avoid extra string copying.
This commit changes the default behavior such that searching is by
default most permissive (case- and accent-insensitive, does
transliteration). Flags exist to restore more restrictive matching.
Previously, the current default country was used to turn phone numbers
without an explicit country code into full E164 numbers, which then
had to match the search term when doing a caller ID lookup.
This was inconsistent with EDS, where a weaker
EQUALS_NATIONAL_PHONE_NUMBER was done. The difference is that a
comparison between a number with country code matches one without if
the national number of the same, regardless of the current default
country. This is better because it reduces the influence of the hard
to guess default country on matching.
SyncEvolution also differed from EDS by doing a prefix comparison,
which in theory might have also ignored differences caused by
extensions. It is uncertain whether that was useful, so for the sake
of consistency, a full number comparison of the national number is now
done.
Another advantage of this change is the lower memory consumption and
faster comparison, because strings are now stored in 4 + 8 byte
numbers instead of strings of varying length.
The operation is a runtime parameter of different classes, whereas
extracting the right values to compare via the operation is hard-coded
at compile time. This is a rather arbitrary compromise between code
duplication, simplicity and performance (which, in fact, was not
measured at all).
The code for selecting case-sensitivity and the normalization before
the string operations is shared with the older 'any-contains'
operation.
This changes the signature of the filter parameter in Search(),
RefineSearch() and ReplaceSearch() from 'as' (array of strings) to
'av' (array of variants). Allowed entries in the variant are arrays
containing strings and/or other such arrays (recursive!).
A single string as value is not supported, which is the reason why
'av' instead of just plain 'v' makes sense (mismatches can already be
found in the sender, potentially at compile time). It also helps
Python choose the right type when asked to send an empty list. When
just using 'v', Python cannot decide automatically.
Error messages include a backtrace of all terms that the current,
faulty term was included in. That helps to locate the error in a
potentially larger filter.
The scope of supported searches has not changed (yet).
As with glib before, now GeeCollCXX also makes the parameter for declaring the
pointer ownership mandatory. The ensuing code analysis showed that in two
cases involving gee_multi_map_get() the transfer of ownership was not handled
correctly, leading to memory leaks.
We have to hard code this instead of always using it because ICU does
not properly fall back to non-phonebook defaults. Without phonebook as
collation, sorting was not done correctly in Germany.
Now also pay attention to case and punctuation. Previously these were
ignored for performance reasons. The reasoning that case doesn't
matter was wrong, it is useful as a tie breaker (which is what the
higher level does).
Full interleaving of Pinyin transliterations of Chinese names with
Western names can be done by doing an explicit Pinyin transliteration
as part of computing the sort keys.
This is done using ICU's Transliteration("Han-Latin"), which we have
to call directly because boost::locale does not expose that API.
We hard-code this behavior for all "zh" languages (as identified by
boost::locale), because by default, ICU would sort Pinyin separately
from Western names when using the "pinyin" collation.
Following the boost::instrusive_ptr example and making "add_ref =
true" the default in our CXX GLib and GObject wrappers led to some
memory leaks because it didn't enforce thinking about whether the
plain pointer is already owned by us or not.
It is better to use a mandatory enum value, ADD_REF and TRANSFER_REF,
and force explicit construction. Doing that revealed that the
assignment operator was implemented as constructing a CXX instance
with increased ref count and/or that in some places, a real leak was
caused by increasing the ref count unnecessarily.
Running under valgrind gave a false sense of security. Some of the
real leaks only showed up randomly in tests.
The new ReplaceSearch is more flexible than RefineSearch. It can
handle both tightening the search and relaxing it. The downside of it
is the more expensive implementation (must check all contacts again,
then find minimal set of change signals to update view).
Previously, a search which had no filter set at all at the begining
could not be refined. This limitation of the implementation gets
removed by always using a FilteredView, even if the initial filter is
empty.
When the parent of a FilteredView emits signals, it is not necessarily
always in a consistent state and the FilteredView must not invoke
methods in the parent. Stressing the FilteredView by using it in tests
originally written for the FullView showed that the filling up a view
violated that rule and led to contacts being present multiple
times. Fixed by delaying the reading from the parent into either an
idle callback or the parent's quiescence signal processing, whichever
comes first.
If phone number search is possible, then the direct search in EDS now
uses the more accurate E_BOOK_QUERY_EQUALS_NATIONAL_PHONE_NUMBER
comparison, with the E164 formatted caller ID as value to compare
against. That value includes the current country code.
For testing purposes, setting SYNCEVOLUTION_PIM_EDS_SUBSTRING forces
the usage of the traditional suffix match. This is used to test both
the old and new flavor of testFilterStartupRefine.
FilterStartupRefineSmart is the one which uses phone number matching.
When available, the pre-computed E164 number from EDS will be used
instead of doing one libphonebook parser run for each telephone number
while reading. Benchmarking showed that this parsing was the number
one hotspot, so this is a considerable improvement.
Don't let libphonenumber write to stdout. Instead redirect into
SyncEvolution logging and manage severity level. For example,
previously parsing errors of phone numbers were logged as [ERROR] by
libphonenumber. Now that appears as "phonenumber error: ...".
Bypass folks while it still loads contacts and search for a phone
number directly in EDS. This is necessary to ensure prompt responses
for the caller ID lookup.
Done with a StreamingView which translates EContacts into
FolksIndividuals with the help of folks-eds = edsf.
Combining these intermediate results and switching to the final
results is done by a new MergeView class.
A quiescence signal is emitted after receiving the EDS results and
after folks is done. The first signal will be skipped when folks
finishes first. The second signal will always be send, even if
switching to folks did not change anything.
Together with an artificial delay before folks is considered done,
that approach make testing more reliable.
A 'limit' search term with a number as parameter (formatted as string)
can be added to a 'phone' or 'any-contains' search term to truncate the
search results after a certain number of contacts. Example:
Search([['any-contains', 'Joe'], ['limit', '10']])
=> return the first 10 Joes.
As with any other search, the resulting view will be updated if
contact data changes.
The limit must not be changed in a RefineSearch(). A 'limit' term may
(but doesn't have to) be given. If it is given, its value must match
the value set when creating the search. This limitation simplifies the
implementation and its testing. The limitation could be removed if
there is sufficient demand.
Looking up by phone number spends most of its cycles in normalizing of
the phone numbers in the unified address book. Instead of doing that
work over and over again during the search, do it once while loading.
This has implications for several classes:
- IndividualData must be extended to hold the pre-computed values;
what kind of data can be stored there is currently hard-coded
in the LocaleFactory base class, to keep it simple and minimize
memory fragmentation.
- Internal contact changes and access must be based on IndividualData,
not FolksIndividual.
- FullView must know which LocaleFactory will pre-compute the data.
- Changing contacts may have no effect on sorting, but may lead
to different pre-computed data.
Looking up a phone number only once does not gain from this change, it
even gets slower (more memory intensive, less cache locality). Only
searching multiple times becomes faster.
Ultimately it would be best to store the normalized strings together
with the telephone number inside EDS when the contact gets
created.
Lookup and search are different: the former is based on a valid
number, the later on user input.
A lookup can compare normalized numbers including the country code, to
ensure that the lookup is exact and does not mismatch numbers from
different countries. Heuristics like suffix matching do not do this
correctly in all cases.
A search is based on user input, which might contain just some digits
in the middle of the phone number. The search ignores formatting in
both input and address book.
In both cases, alpha characters are treated as aliases for their
corresponding digit on a keypad.
The implementation uses Google's libphonebook to implement phone
number normalization and boost::locale to get the ISO-3199 country
code of the current locale.
Here some examples how the implementation works (from the tests),
based on de_DE.UTF-8 locale = DE = +49:
'any-contains 1234', 'any-contains 23', 'any-contains 12/34' find 1234.
'any-contains 5678' finds 56/78.
'any-contains 366227', 'any-contains +1-800-foobar' find +1-800-FOOBAR.
'phone +1800366227' matches +1-800-FOOBAR.
'phone +49897888', 'phone 0897888' all match 089/7888-99 and not
+189788899.
'phone +49897888000' does not match 089/7888-99.
Covers the "any-contains" search, using boost::locale to get a
case-independent representation when asked for case-insensitive
searching. boost::icontains doesn't work as well because it can only
ignore the case of ASCII characters when dealing with UTF-8.
Telephone search still needs to be implemented.
Change the --enable-dbus-service-pim parameter into one which takes a
parameter that specifies how locale-aware sorting and searching is to
be implemented. The default implementation uses boost::locale. It is
expected to get replaced or augemented by OEMs which want to implement
more complex sorting or searching (like ignoring Tussenvoegsel in the
Netherlands).
The LocaleFactory instance takes the current locale from the
environment. Making it and its users aware of locale changes at
runtime might be needed at some point but is not part of the API at
the moment.
The Manager class uses the factory to handle sorting and searching
requests coming in via D-Bus. Right now, that is not functional yet
because the boost::locale implementation is just a stub. It only
compiles and links.
FullView::setSortOrder is now functional.
Clean up view code a bit:
- All views delay populating their content until the caller asks for
it. For the FullView this will only happen once, so the caller must be
able to handle an already populated view, which was missing
in ViewResource. Still need a test for this.
- Use init(<smart pointer) consistently.
Jonas Smedegaard
Patrick Ohly
Mateusz Polrola