GNOME Bugzilla – Bug 689622
Build summary with normalized phone numbers
Last modified: 2013-09-14 16:56:09 UTC
Users enjoy to freely format their phone numbers. The annotate them with spaces, dashes, dots, parenthesis and countless other crazy characters. This makes it hard for software to compare a given phone number with another. To help with the situation libraries like libphonenumber[1] were created, that can parse phone numbers and rewrite them into standard formats like E.164 or RFC 3966. Still for matching normalized phone numbers one has to read each single contact, parse them apply normalization to each phone number found. This is slow. To overcome this situation we would like to store normalized phone numbers in the addressbook summary. To enable phonenumber matching a new EBookQueryTest would be used. [1] http://code.google.com/p/libphonenumber/
Current work is in http://git.gnome.org/browse/evolution-data-server/log/?h=openismus-phonenumber-work (on top of the openismus-work) branch. Can you please check this API additions for sanity? http://git.gnome.org/browse/evolution-data-server/tree/libedataserver/e-phone-utils.h?h=openismus-phonenumber-work http://git.gnome.org/browse/evolution-data-server/tree/addressbook/libebook-contacts/e-book-query.h?h=openismus-phonenumber-work
Created attachment 230933 [details] [review] full diff between openismus-work and openismus-phonenumber-work branch
Created attachment 230934 [details] [review] full diff between openismus-work and openismus-phonenumber-work branch now really (not just the stat)
EPhoneNumberMatch Can you give examples of partial matches? I assume it is possible to get E_PHONE_NUMBER_MATCH_NATIONAL in combination with E_PHONE_NUMBER_MATCH_SHORT, but not with E_PHONE_NUMBER_MATCH_EXACT? E_PHONE_NUMBER_ERROR_TOO_LONG: FYI, some of the phone numbers produced by your vcard generator trigger that error. That's actually a good thing, because not all user's vcards will be valid either. /** * e_phone_number_from_string: * @phone_number: the phone number to parse * @country_code: (allow-none): a 2-letter country code, or %NULL What happens if country_code is NULL? Other than that the util API looks good to me. I've not seen documentation for the new queries as introduced in the patch.
Updated the docs in the branch.
Pushed my changes to the openismus-work branch: (37b7c16 Prototype a D-Bus unit test harness.) (5e0461a tests: Add a few missing license headers) (09c10aa tests: Add configured datadir to the D-Bus dirs) 925dad4 tests: Use private D-Bus session in EBookClient test (03b4e98 tests: Add shared new_custom_temp_client()) 8895e2f tests: Port libebook/test-query.c to GTest 9f3f0cb tests: Verify more EBook queries 9239e34 build: Move langinfo checks into autoconf macro 6f37438 libedata-book: Use slice allocator for EbSdbSearchData bc45131 libedata-book: Simplify signature of sexp helpers f417382 libebook-contacts: Reduce code-duplication in e_book_query_from_string() (2b88a5b libebook-contacts: Document EBookQueryTest) a61bb1e libedataserver: Don't print warning if e_sexp_parse() fails e941ebf libedataserver: Add phone number utilities 9d25a21 libebook-contacts: Add phone number specific queries a10f31e sqlitedb: Permit indexing of phone numbers 1527c35 sqlitedb: Store E.164 param in vcards Commits in parenthesis are applied to master already. 03b4e98 should get reviewed by Tristan. So we have those cleanups that need review: http://git.gnome.org/browse/evolution-data-server/commit/?id=925dad4 http://git.gnome.org/browse/evolution-data-server/commit/?id=8895e2f http://git.gnome.org/browse/evolution-data-server/commit/?id=9f3f0cb http://git.gnome.org/browse/evolution-data-server/commit/?id=9239e34 http://git.gnome.org/browse/evolution-data-server/commit/?id=6f37438 http://git.gnome.org/browse/evolution-data-server/commit/?id=bc45131 http://git.gnome.org/browse/evolution-data-server/commit/?id=f417382 http://git.gnome.org/browse/evolution-data-server/commit/?id=a61bb1e And those feature patches that need review: http://git.gnome.org/browse/evolution-data-server/commit/?id=e941ebf http://git.gnome.org/browse/evolution-data-server/commit/?id=9d25a21 http://git.gnome.org/browse/evolution-data-server/commit/?id=a10f31e http://git.gnome.org/browse/evolution-data-server/commit/?id=1527c35
(In reply to comment #6) > http://git.gnome.org/browse/evolution-data-server/commit/?id=9239e34 Uh, this (trivial) patch also should be considered a feature patch as it adds the check for _NL_ADDRESS_COUNTRY_AB2.
Pushed those patches after checking with Milan: http://git.gnome.org/browse/evolution-data-server/commit/?id=8895e2f http://git.gnome.org/browse/evolution-data-server/commit/?id=9f3f0cb http://git.gnome.org/browse/evolution-data-server/commit/?id=9239e34 http://git.gnome.org/browse/evolution-data-server/commit/?id=6f37438 http://git.gnome.org/browse/evolution-data-server/commit/?id=bc45131 http://git.gnome.org/browse/evolution-data-server/commit/?id=a61bb1e
Updated the remaining patches for git master: http://git.gnome.org/browse/evolution-data-server/log/?h=openismus-phonenumber-work-master So I still need approval for the following patches: http://git.gnome.org/browse/evolution-data-server/commit/d97db8eb libedata-book: Add phone number utilities This functionality covers parsing, formatting and comparing phone numbers. Note that this commit introduces a depedency on C++. Also note that the API documentation is kept in header files because gtkdoc-mkdb explicitly only scans .h and .c files, but ignores .cpp files. http://git.gnome.org/browse/evolution-data-server/commit/2e31227e libebook: Reduce code-duplication in e_book_query_from_string() Merge almost identical sexp handlers used by that functions. Also merge two de-facto identical error branches. http://git.gnome.org/browse/evolution-data-server/commit/a557fa01 libebook: Support phone number specific queries This adds new values to the EBookQueryTest enumeration: * E_BOOK_QUERY_EQUALS_PHONE_NUMBER * E_BOOK_QUERY_EQUALS_NATIONAL_PHONE_NUMBER * E_BOOK_QUERY_EQUALS_SHORT_PHONE_NUMBER This values get translated to the binary sexp functions "eqphone", "eqphone_national" and "eqphone_short". http://git.gnome.org/browse/evolution-data-server/commit/0076599d sqlitedb: Permit indexes with normalized phone numbers Creating indexes with normalized phone numbers permits fast phone number lookup even if the stored vCards or the queried phone numbers are freely formatted or incomplete. The numbers are formatted according to E.164 when building the index. Incomplete numbers are prefixed with a country-code matching the current locale settings. The index is rebuilt if the addressbook is re-opened after a locale change. http://git.gnome.org/browse/evolution-data-server/commit/5b94eece sqlitedb: Store E.164 param in vcards The E.164 normalized phone number is of interest for handset related applications. With this change a X-EVOLUTION-E164 attribute is added to each TEL attribute if the contact summary contains a E.164 formatted variant of the phone number. This shall avoid overhead and inconsistency that would occur if clients would use their own mechanism to compute that already stored information.
Proper links to the commits: http://git.gnome.org/browse/evolution-data-server/commit/?id=d97db8eb http://git.gnome.org/browse/evolution-data-server/commit/?id=2e31227e http://git.gnome.org/browse/evolution-data-server/commit/?id=a557fa01 http://git.gnome.org/browse/evolution-data-server/commit/?id=0076599d http://git.gnome.org/browse/evolution-data-server/commit/?id=5b94eece
This change is not correct, you add .cpp into HEADERS. libedata_bookinclude_HEADERS = \ - libedata-book.h \ + e-book-backend-cache.h \ + e-book-backend-db-cache.h \ e-book-backend-factory.h \ + e-book-backend-phone-number.cpp \ I do not know autotools much, but is this construction legal? +endif ENABLE_PHONENUMBER I know it's not in C/C++, thus it surprised me with autotools. Can we have the C++ dependency used only if the phone number is enabled? I'd prefer that, rather than hard-depend on C++. I think it was mentioned somewhere, probably on IRC, the best if the library itself provides C interface, but that's probably uninfluenceable by you. What I would like to see is to have an e-book-backend-phone-number.c/.h and then dedicated and private e-book-backend-phone-number-lib.cpp/.h. On the other hand, placing it into e-book-backend prefix is not optimal, you may want to use it from other than backend part too, one day, thus I suggest to move this into addressbook/libebook. You can see similar, but significantly simpler, functions in e-contact.c/.h, thus let's introduce e-phone-number.c/.h (see how the file name matches the actual function names namespace now) and then a private e-phone-number-lib.cpp/.h. I hope it makes sense. I didn't move further with review, because this feels like a significant change.
What would be the purpose of splitting between e-book-backend-phone-number.c/.h and e-book-backend-phone-number-lib.cpp/.h?
That the .cpp and g++ will be required and used only if allowed during ./configure, not always. Which will make the C++ dependency optional/soft.
Updated as requested: http://git.gnome.org/browse/evolution-data-server/commit/?id=63ba71303f5c62dcd2093a6c855bf400a6769000 http://git.gnome.org/browse/evolution-data-server/commit/?id=3ed3734269f2770cf08f289bc2f1441433b53ba1 http://git.gnome.org/browse/evolution-data-server/commit/?id=7ee7772022739bdceb6ed65754804b91ef68031e http://git.gnome.org/browse/evolution-data-server/commit/?id=98ff84e9a2a7430dcc394741126a914e0a41f27e http://git.gnome.org/browse/evolution-data-server/commit/?id=1a5b62da698525a5fb185849b8217fca4f47d2a7
Re: http://git.gnome.org/browse/evolution-data-server/commit/?id=63ba71303f5c62dcd2093a6c855bf400a6769000 I'd include the private header only if the phone library is enabled, same as it's done in the Makefile.am. I see you have in your newly create files "Copyright Copyright (C)". Is it anywhere done already, where you copied this from? I saw the same in Tristan's patches, thus I'm wondering whether it's just wrong in all the files, or I misunderstood this. + /* Without phonenumber support there shouldn't be any instances. */ + g_warn_if_fail (phone_number != NULL); The above means that it'll warn if the phone_number will be NULL. Did you intend to do that this way? It's usually better to accept NULL in e_phone_number_free(), it's easier when passing this to some g_slist_free_full() or such functions. +/* NOTE: Keeping API documentation in this header file because gtkdoc-mkdb + * explicitly only scans .h and .c files, but ignores .cpp files. */ The above seems to be obsolete now. I was told, some months ago, that G_GNUC_CONST is potentially dangerous, thus it's better to avoid it. If I recall correctly, it was about code optimisation, and the fact that the _get_type() aren't const, because they change global variable. It made sense to me, thus I'd avoid using that macro. +# Extra options to supply to gtkdoc-mkdb +MKDB_OPTIONS = --sgml-mode --output-format=xml --name-space=e + Is this still needed? In the test, I believe the #include <libedata-book/libedata-book.h> should be #include <libebook/libebook.h>
Re: http://git.gnome.org/browse/evolution-data-server/commit/?id=3ed3734269f2770cf08f289bc2f1441433b53ba1 Untested, but looks good. Please commit.
Re: http://git.gnome.org/browse/evolution-data-server/commit/?id=7ee7772022739bdceb6ed65754804b91ef68031e + g_assert_not_reached (); Please do not g_assert* () in library code. + E_BOOK_QUERY_EQUALS_SHORT_PHONE_NUMBER, no comma at the end. Apart of the two above looks good, feel free to commit.
Re: http://git.gnome.org/browse/evolution-data-server/commit/?id=98ff84e9a2a7430dcc394741126a914e0a41f27e + E_BOOK_INDEX_PHONE, no comma at the end. E_BOOK_INDEX_E164 is used only in the doc for the function. + INDEX_PHONE = (1 << 2), no comma at the end. In +eqphone_func initialize the GError to NULL. + g_error ("%s:%d (%s): %s", __FILE__, __LINE__, __func__, error->message); two things about the above: a) why would it crash the application? b) either use G_STRLOC or G_STRFUNC, up to you which one you'll choose. Hmm, should there be any setting for current country code for the summary? Guessing it from current locale is most likely wrong, as for example I run my system in English locale, but my phone numbers are in different country. What about storing phone numbers in summary without country code, and let the final filtering being done in the caller, which is done anyway? At least for views it is. By the way, what will happen if the caller tries to search the book without phonenumber enabled? I currently get: > ERROR:test-client-custom-summary.c:124:search_test: assertion failed > (g_slist_length (results) == data->num_contacts): (0 == 1) but I'm not sure whether it's related or not. I do not have eds compiled with phonenumber. Just in case, is the caller supposed to check if phone numbers are supported first, and only then try the query on it? This should be written in the doc as a notice for sure, if so.
Re: http://git.gnome.org/browse/evolution-data-server/commit/?id=1a5b62da698525a5fb185849b8217fca4f47d2a7 There is that "Copyright Copyright (C)" again. The test uses int and char, instead of gint, gchar. Otherwise I'm wondering whether the setlocale (LC_ALL, "en_US.UTF-8"); call shouldn't be in the test itself, as it seems to be the only one which depends on it, but I do not have much preference on it too, thus if you'd like to keep it there, then I'm fine.
(In reply to comment #15) > Re: > http://git.gnome.org/browse/evolution-data-server/commit/?id=63ba71303f5c62dcd2093a6c855bf400a6769000 > > I'd include the private header only if the phone library is enabled, same as > it's done in the Makefile.am. This is because _e_phone_number_set_error() is used in both the C and the C++ code. I still should hide the C++ wrappers in the private header when phone numbers are disabled, and I must fix Makefile.am. > I see you have in your newly create files "Copyright Copyright (C)". Is it > anywhere done already, where you copied this from? Yuck. Fixed. > + /* Without phonenumber support there shouldn't be any instances. */ > + g_warn_if_fail (phone_number != NULL); > > The above means that it'll warn if the phone_number will be NULL. Did you > intend to do that this way? It's usually better to accept NULL in > e_phone_number_free(), it's easier when passing this to some > g_slist_free_full() or such functions. Good catch. The check should be the otherway around. With phonenumbers disabled we always must get NULL here. Fixed. > > +/* NOTE: Keeping API documentation in this header file because gtkdoc-mkdb > + * explicitly only scans .h and .c files, but ignores .cpp files. */ > > The above seems to be obsolete now. Need more Coffee. Fixed. > I was told, some months ago, that G_GNUC_CONST is potentially dangerous, thus > it's better to avoid it. If I recall correctly, it was about code optimisation, > and the fact that the _get_type() aren't const, because they change global > variable. It made sense to me, thus I'd avoid using that macro. True, good point. So leaving the G_GNUC_CONST at e_phone_number_is_supported() only. > +# Extra options to supply to gtkdoc-mkdb > +MKDB_OPTIONS = --sgml-mode --output-format=xml --name-space=e > + > Is this still needed? Yes, it's needed for the HTML table about phone number matching. Will improve the comment. > > In the test, I believe the > #include <libedata-book/libedata-book.h> > should be > #include <libebook/libebook.h> Ok, not just Coffee. Redbull is needed. Fixed.
(In reply to comment #16) > Re: > http://git.gnome.org/browse/evolution-data-server/commit/?id=3ed3734269f2770cf08f289bc2f1441433b53ba1 > > Untested, but looks good. Please commit. Thank you. Pushed to master. (In reply to comment #17) > Re: > http://git.gnome.org/browse/evolution-data-server/commit/?id=7ee7772022739bdceb6ed65754804b91ef68031e > > + g_assert_not_reached (); > Please do not g_assert* () in library code. > > + E_BOOK_QUERY_EQUALS_SHORT_PHONE_NUMBER, > no comma at the end. > > Apart of the two above looks good, feel free to commit. Fixed, but must hold back for the EPhoneNumber patch.
(In reply to comment #19) > Re: > http://git.gnome.org/browse/evolution-data-server/commit/?id=1a5b62da698525a5fb185849b8217fca4f47d2a7 > > There is that "Copyright Copyright (C)" again. Fixed. > The test uses int and char, instead of gint, gchar. ... [censored]. Fixed. > Otherwise I'm wondering whether the > setlocale (LC_ALL, "en_US.UTF-8"); > call shouldn't be in the test itself, as it seems to be the only one which > depends on it, but I do not have much preference on it too, thus if you'd like > to keep it there, then I'm fine. Besides obvious things like date formatting, the locale also affects sort order. So in my experience it's a good idea to fixate the locale for tests. This should be done by g_test_init() actually. Coincidentally Murray, Tristan and me just spent a few hours last year on debugging some EDS related issue. In the end it turned out that the CI server run our tests with the "C" locale. I'll add a comment.
Updates pushed: http://git.gnome.org/browse/evolution-data-server/commit/?id=10dc9503 http://git.gnome.org/browse/evolution-data-server/commit/?id=233407d5 http://git.gnome.org/browse/evolution-data-server/commit/?id=4335edbd http://git.gnome.org/browse/evolution-data-server/commit/?id=8d01414f 10dc9503, 233407d5 and 8d01414f should be ok now. looking at 4335edbd now to deal with your comments.
(In reply to comment #18) > + E_BOOK_INDEX_PHONE, > no comma at the end. Fixed. > E_BOOK_INDEX_E164 is used only in the doc for the function. Fixed. > + INDEX_PHONE = (1 << 2), > no comma at the end. Fixed. > In +eqphone_func initialize the GError to NULL. Oops! > + g_error ("%s:%d (%s): %s", __FILE__, __LINE__, __func__, > error->message); > two things about the above: > a) why would it crash the application? The code invoking this function is pretty nested already. So when implementing this I didn't feel like adding more sophisticated tests. Well, and next day I forgot about the g_error(). ;-) Turning into a g_warning() now and looking how to catch those errors. > b) either use G_STRLOC or G_STRFUNC, up to you which one you'll choose. Fixed. > Hmm, should there be any setting for current country code for the summary? > Guessing it from current locale is most likely wrong, as for example I run my > system in English locale, but my phone numbers are in different country. POSIX locales have quite some options to configure the message language separately from all other locale settings, e.g: LANG=en_US.UTF-8 LC_ADDRESS=de_DE.UTF-8 locale LC_MESSAGES=en_US.UTF-8 LANG=de_DE.UTF-8 locale LANG=en_US.UTF-8 LC_ADDRESS=de_DE.UTF-8 locale LC_ADDRESS LC_MESSAGES=en_US.UTF-8 LANG=de_DE.UTF-8 locale LC_ADDRESS LANG=en_US.UTF-8 LC_ADDRESS=de_DE.UTF-8 gettext gtk30 unknown; echo LC_MESSAGES=en_US.UTF-8 LANG=de_DE.UTF-8 gettext gtk30 unknown; echo LANG=de_DE.UTF-8 gettext gtk30 unknown; echo Guess we rather should fix the control center if this doesn't work yet rather than hacking around such issues in EDS, shouldn't we? > What about storing phone numbers in summary without country code, and let the > final filtering being done in the caller, which is done anyway? At least for > views it is. Yes, I am also not too happy about storing fragile data like a guessed country code in the database. The need for rebuilding the database when the regional settings change is pure nasty - luckily this evil shouldn't happen too often for regular users. Still I think this is the most reliable solution: To utilize the indexes we must store some kind of normalized phone number. At the same time we must make sure that the stored numbers still match with numbers normalized via libphonenumber. Also Patrick is not happy with the idea of computing the full E.164 number (see next patch) over and over again. So IMHO this really seems to be the best solution, that doesn't involve crazy hacks like custom sqlite functions and postprocessing the results we get from sqlite. > By the way, what will happen if the caller tries to search the book without > phonenumber enabled? I currently get: > > > ERROR:test-client-custom-summary.c:124:search_test: assertion failed > > (g_slist_length (results) == data->num_contacts): (0 == 1) > > but I'm not sure whether it's related or not. I do not have eds compiled with > phonenumber. Just in case, is the caller supposed to check if phone numbers are > supported first, and only then try the query on it? This should be written in > the doc as a notice for sure, if so. Looks like we need better error handling here.
I think the g_warning() in e_phone_number_...() functions when the libphonenumber is not enabled are not needed too. If the caller application doesn't check the supportability properly, then it's no need to warn also a console/user about it. What to store in summary: Why not to store and search phone numbers without country code? I do not think you'll get that many hits anyway, and post-processing the result (filtering out false-positives) is not that bad like redoing whole index for this. There is a post-processing being done already anyway, because the contacts the summary returns are passed through EBookBackendSexp machinery, at least those in views. Another option would be to add two columns for phones, one with a phone number, the other with a country code and if the country code + phone number will return 0 matches, then check with phone number only. Quite simple, isn't it? An unrelated note, could you attach the changes as patches here, please? It's easier for review, at least for me.
hmm, storing country code and national number in two separate columns: create table p(cc, nn); insert into p values('+49', '3099887766'); insert into p values(null, '4055443322'); and extracting like this: select coalesce(cc, '+49') || nn from p; hmm... could work. indeed. must try tomorrow morning.
(In reply to comment #25) > I think the g_warning() in e_phone_number_...() functions when the > libphonenumber is not enabled are not needed too. If the caller application > doesn't check the supportability properly, then it's no need to warn also a > console/user about it. The warnings only are shown for broken programs and they help fixing the issue quickly. We should keep them.
Created attachment 234398 [details] [review] [PATCH 1/6] build: Use dedicated compiler warning for C and C++ code
Created attachment 234399 [details] [review] [PATCH 1/6] libebook: Define boxed EPhoneNumber type at single place
Created attachment 234400 [details] [review] [PATCH 2/6] libebook: Better region guessing for phone numbers
Created attachment 234401 [details] [review] [PATCH 4/6] libebook: Give access to country code and national phone
(In reply to comment #31) > Created an attachment (id=234401) [details] [review] > [PATCH 4 [details]/6] libebook: Give access to country code and national phone Forgot API docs on this one...
Review of attachment 234398 [details] [review]: Looks fine, please commit.
Review of attachment 234399 [details] [review]: Looks fine, please commit.
Review of attachment 234400 [details] [review]: Otherwise looks fine, just fix it and commit. ::: addressbook/libebook/e-phone-number-private.h @@ +60,2 @@ E_PHONE_NUMBER_LOCAL EPhoneNumber * _e_phone_number_cxx_from_string (const gchar *phone_number, + const gchar *region, here should be 'region_code', to be consistent with implementation of the function.
Review of attachment 234401 [details] [review]: Looks fine, the below nitpick is no issue. Please commit. ::: addressbook/libebook/e-phone-number-private.cpp @@ +176,3 @@ + * PhoneNumberUtil::Parse() produces FROM_DEFAULT_COUNTRY, + * PhoneNumberUtil::ParseAndKeepRawInput() produces the other. + * As we only use PhoneNumberUtil::Parse() right now, let's This doesn't seem to be true anymore, you've above a conditional compile with both Parse variants, though only one based on the library version/functionality, if I read it correctly.
Are there missing [5/6], [6/6] patches?
Created attachment 234981 [details] [review] [PATCH 3/6] build: Use dedicated compiler warning for C and C++ code Misunderstood what AC_PROVIDE_IFELSE does. Therefore had to extract EVO_PHONENUMBER_ARGS, setting evo_with_cxx: diff -u b/configure.ac b/configure.ac --- b/configure.ac +++ b/configure.ac @@ -164,6 +164,13 @@ dnl ************************************** ACLOCAL="$ACLOCAL $ACLOCAL_FLAGS" +dnl ********************************************* +dnl Figure out early if we'll need a C++ compiler +dnl ********************************************* + +evo_with_cxx=no +EVO_PHONENUMBER_ARGS + dnl ****************************** dnl Compiler Warning Flags dnl ****************************** @@ -200,13 +207,16 @@ AM_CFLAGS="$WARNING_FLAGS -fno-strict-aliasing" AC_SUBST(AM_CFLAGS) -AC_PROVIDE_IFELSE([AC_PROG_CXX],, - [AC_LANG_PUSH([C++]) - AS_COMPILER_FLAGS(CXX_WARNING_FLAGS, [$proposed_cxx_warning_flags]) - AC_SUBST(CXX_WARNING_FLAGS) - AM_CXXFLAGS="$CXX_WARNING_FLAGS" - AC_SUBST(AM_CXXFLAGS) - AC_LANG_POP([C++])]) +if test "x$evo_with_cxx" = xyes; then + AC_PROG_CXX + + AC_LANG_PUSH([C++]) + AS_COMPILER_FLAGS(CXX_WARNING_FLAGS, [$proposed_cxx_warning_flags]) + AC_SUBST(CXX_WARNING_FLAGS) + AM_CXXFLAGS="$CXX_WARNING_FLAGS" + AC_SUBST(AM_CXXFLAGS) + AC_LANG_POP([C++]) +fi dnl ******************************* dnl Check for --enable-strict @@ -292,18 +302,6 @@ AC_SUBST(localedir) dnl ****************************** -dnl libphonenumber support -dnl ****************************** - -dnl The EVO_PHONENUMBER_SUPPORT macro calls AC_PROG_CXX if libphonenumber -dnl support got requested. Therefore this macro must be expanded before -dnl the libtool macros. Feel free to move back to the other optional -dnl dependencies if you know how to fix the autoconf issue, or if you -dnl concluded that C++ actually is pretty awesome and should be a hard -dnl dependency. -EVO_PHONENUMBER_SUPPORT - -dnl ****************************** dnl Initialize libtool dnl ****************************** LT_PREREQ(2.2) @@ -401,6 +399,12 @@ PKG_CHECK_MODULES(GIO_UNIX, [gio-unix-2.0]) fi +dnl ****************************** +dnl Check for libphonenumber +dnl ****************************** + +EVO_PHONENUMBER_SUPPORT + dnl ************************* dnl Check for GTK+ dnl ************************* only in patch2: unchanged: --- a/m4/evo_phonenumber.m4 +++ b/m4/evo_phonenumber.m4 @@ -1,15 +1,21 @@ -dnl EVO_PHONENUMBER_SUPPORT([default]) -dnl Check for Google's libphonenumber. Adds a --with-phonenumber option -dnl to explicitly enable and disable phonenumber support, but also for -dnl pointing to libphonenumber's install prefix. -AC_DEFUN([EVO_PHONENUMBER_SUPPORT],[ - AC_MSG_CHECKING([whether to enable phonenumber support]) +dnl EVO_PHONENUMBER_ARGS([default]) +dnl +dnl Checks configure script options for requesting libphonenumber support. +dnl Adds a --with-phonenumber option to explicitly enable and disable +dnl phonenumber support, but also for pointing to libphonenumber's install +dnl prefix. +dnl +dnl Must be called before any other macro that might use the C++ compiler. +AC_DEFUN([EVO_PHONENUMBER_ARGS],[ + AC_BEFORE([$0], [AC_COMPILE_IFELSE]) + AC_BEFORE([$0], [AC_LINK_IFELSE]) + AC_BEFORE([$0], [AC_PROG_CXX]) + AC_BEFORE([$0], [AC_RUN_IFELSE]) + AC_BEFORE([$0], [LT_INIT]) evo_phonenumber_prefix= - msg_phonenumber=no - PHONENUMBER_INCLUDES= - PHONENUMBER_LIBS= + AC_MSG_CHECKING([whether to enable phonenumber support]) AC_ARG_WITH([phonenumber], [AS_HELP_STRING([--with-phonenumber@<:@=PREFIX@:>@], @@ -19,8 +25,26 @@ AC_DEFUN([EVO_PHONENUMBER_SUPPORT],[ AC_MSG_RESULT([$with_phonenumber]) + AS_VAR_IF([with_phonenumber],[no],,[evo_with_cxx=yes]) +]) + +dnl EVO_PHONENUMBER_SUPPORT +dnl +dnl Check for Google's libphonenumber. Adds a --with-phonenumber option +dnl to explicitly enable and disable phonenumber support, but also for +dnl pointing to libphonenumber's install prefix. +dnl +dnl You most probably want to place a call to EVO_PHONENUMBER_ARGS near +dnl to the top of your configure script. +AC_DEFUN([EVO_PHONENUMBER_SUPPORT],[ + AC_REQUIRE([EVO_PHONENUMBER_ARGS]) + + msg_phonenumber=no + + PHONENUMBER_INCLUDES= + PHONENUMBER_LIBS= + AS_VAR_IF([with_phonenumber], [no],, [ - AC_REQUIRE([AC_PROG_CXX]) AC_LANG_PUSH(C++) PHONENUMBER_LIBS="-lphonenumber -lboost_thread"
Created attachment 234984 [details] [review] [PATCH 4/6] libebook: Give access to country code and national phone This adds API documentation and adjusts the M4 macros for the updated compiler warnings patch.
(In reply to comment #39) > Created an attachment (id=234984) [details] [review] > [PATCH 4 [details]/6] libebook: Give access to country code and national phone > > This adds API documentation and adjusts the M4 macros for the updated compiler > warnings patch. diff -u b/addressbook/libebook/e-phone-number-private.cpp b/addressbook/libebook/e-phone-number-private.cpp --- b/addressbook/libebook/e-phone-number-private.cpp +++ b/addressbook/libebook/e-phone-number-private.cpp @@ -170,14 +170,11 @@ case PhoneNumber::FROM_NUMBER_WITH_IDD: return E_PHONE_NUMBER_COUNTRY_FROM_IDD; - /* Country codes with a source of FROM_NUMBER_WITHOUT_PLUS_SIGN - * also are computed from the default country. Only difference - * to FROM_DEFAULT_COUNTRY is the function that parsed the number: - * PhoneNumberUtil::Parse() produces FROM_DEFAULT_COUNTRY, - * PhoneNumberUtil::ParseAndKeepRawInput() produces the other. - * As we only use PhoneNumberUtil::Parse() right now, let's - * not bother our users with that difference, and a barely - * understandable enumeration value they'll never encounter. + /* FROM_NUMBER_WITHOUT_PLUS_SIGN only is used internally + * by libphonenumber to properly(???) reconstruct raw input + * from PhoneNumberUtil::ParseAndKeepRawInput(). Let's not + * bother our users with that barely understandable and + * almost undocumented implementation detail. */ case PhoneNumber::FROM_NUMBER_WITHOUT_PLUS_SIGN: case PhoneNumber::FROM_DEFAULT_COUNTRY: diff -u b/addressbook/libebook/e-phone-number.c b/addressbook/libebook/e-phone-number.c --- b/addressbook/libebook/e-phone-number.c +++ b/addressbook/libebook/e-phone-number.c @@ -150,6 +150,24 @@ #endif /* ENABLE_PHONENUMBER */ } +/** + * e_phone_number_compare: + * @first_number: the first EPhoneNumber to compare +} + +/** + * e_phone_number_get_country_code: + * @phone_number: the phone number to query + * @source: an optional location for storing the phone number's origin, or %NULL + * + * Queries the @phone_number's country code and optionally stores the country code's + * origin in @source. For instance when parsing "+1-617-5423789" this function + * would return one and assing E_PHONE_NUMBER_COUNTRY_FROM_FQTN to @source. + * + * Returns: A valid country code, or zero if no country code is known. + * + * Since: 3.8 + **/ gint e_phone_number_get_country_code (const EPhoneNumber *phone_number, EPhoneNumberCountrySource *source) @@ -166,6 +184,18 @@ #endif /* ENABLE_PHONENUMBER */ } +/** + * e_phone_number_get_national_number: + * @phone_number: the phone number to query + * + * Queries the national portion of @phone_number without any call-out + * prefixes. For instance when parsing "+1-617-5423789" this function would + * return the string "6175423789". + * + * Returns: (transfer full): The national portion of @phone_number. + * + * Since: 3.8 + **/ gchar * e_phone_number_get_national_number (const EPhoneNumber *phone_number) { diff -u b/addressbook/libebook/e-phone-number.h b/addressbook/libebook/e-phone-number.h --- b/addressbook/libebook/e-phone-number.h +++ b/addressbook/libebook/e-phone-number.h @@ -156,6 +156,19 @@ E_PHONE_NUMBER_ERROR_TOO_LONG } EPhoneNumberError; +/** + * EPhoneNumberCountrySource: + * @E_PHONE_NUMBER_COUNTRY_FROM_FQTN: the EPhoneNumber was build from a + * fully qualified telephone number that contained a valid country code + * @E_PHONE_NUMBER_COUNTRY_FROM_IDD: the parsed phone number started + * with the current locale's international call prefix, followed by a + * valid country code + * @E_PHONE_NUMBER_COUNTRY_FROM_DEFAULT: the parsed phone didn't start + * with a (recognizable) country code, the country code was chosen by + * checking the current locale settings + * + * The origin of a parsed EPhoneNumber's country code. + */ typedef enum { E_PHONE_NUMBER_COUNTRY_FROM_FQTN = 1, E_PHONE_NUMBER_COUNTRY_FROM_IDD = 5, interdiff impossible; taking evasive action reverted: --- b/m4/evo_phonenumber.m4 +++ a/m4/evo_phonenumber.m4 @@ -46,46 +46,15 @@ AC_MSG_ERROR([libphonenumber cannot be used. Use --with-phonenumber to specify the library prefix.])]) ]) + CXXFLAGS="$evo_cxxflags_saved" + LDFLAGS="$evo_ldflags_saved" + LIBS="$evo_libs_saved" AS_VAR_IF([evo_phonenumber_prefix],, [msg_phonenumber=$with_phonenumber], [msg_phonenumber=$evo_phonenumber_prefix]) AC_MSG_RESULT([$with_phonenumber]) - - AS_VAR_IF( - [with_phonenumber],[yes], - [AC_MSG_CHECKING([whether ParseAndKeepRawInput() is needed]) - AC_RUN_IFELSE( - [AC_LANG_PROGRAM( - [[#include <phonenumbers/phonenumberutil.h>]], - [[namespace pn = i18n::phonenumbers;i18n::phonenumbers; - - pn::PhoneNumber n; - - if (pn::PhoneNumberUtil::GetInstance ()-> - Parse("049(800)46663", "DE", &) == pn::PhoneNumberUtil::NO_PARSING_ERROR - && n.has_country_code_source () - && n.country_code_source () == 49) - return EXIT_SUCCESS; - - return EXIT_FAILURE;]] - )], - - [AC_MSG_RESULT([no])], - [AC_MSG_RESULT([yes]) - - AC_DEFINE_UNQUOTED( - [PHONENUMBER_RAW_INPUT_NEEDED], 1, - [Whether Parse() or ParseAndKeepRawInput() must be used to get the country-code source]) - ]) - ]) - - - CXXFLAGS="$evo_cxxflags_saved" - LDFLAGS="$evo_ldflags_saved" - LIBS="$evo_libs_saved" - AC_LANG_POP(C++) ]) unchanged: --- a/m4/evo_phonenumber.m4 +++ b/m4/evo_phonenumber.m4 @@ -70,15 +70,46 @@ AC_DEFUN([EVO_PHONENUMBER_SUPPORT],[ AC_MSG_ERROR([libphonenumber cannot be used. Use --with-phonenumber to specify the library prefix.])]) ]) - CXXFLAGS="$evo_cxxflags_saved" - LDFLAGS="$evo_ldflags_saved" - LIBS="$evo_libs_saved" AS_VAR_IF([evo_phonenumber_prefix],, [msg_phonenumber=$with_phonenumber], [msg_phonenumber=$evo_phonenumber_prefix]) AC_MSG_RESULT([$with_phonenumber]) + + AS_VAR_IF( + [with_phonenumber],[yes], + [AC_MSG_CHECKING([whether ParseAndKeepRawInput() is needed]) + AC_RUN_IFELSE( + [AC_LANG_PROGRAM( + [[#include <phonenumbers/phonenumberutil.h>]], + [[namespace pn = i18n::phonenumbers;i18n::phonenumbers; + + pn::PhoneNumber n; + + if (pn::PhoneNumberUtil::GetInstance ()-> + Parse("049(800)46663", "DE", &) == pn::PhoneNumberUtil::NO_PARSING_ERROR + && n.has_country_code_source () + && n.country_code_source () == 49) + return EXIT_SUCCESS; + + return EXIT_FAILURE;]] + )], + + [AC_MSG_RESULT([no])], + [AC_MSG_RESULT([yes]) + + AC_DEFINE_UNQUOTED( + [PHONENUMBER_RAW_INPUT_NEEDED], 1, + [Whether Parse() or ParseAndKeepRawInput() must be used to get the country-code source]) + ]) + ]) + + + CXXFLAGS="$evo_cxxflags_saved" + LDFLAGS="$evo_ldflags_saved" + LIBS="$evo_libs_saved" + AC_LANG_POP(C++) ]) only in patch2: unchanged: --- a/docs/reference/addressbook/libebook/libebook-sections.txt +++ b/docs/reference/addressbook/libebook/libebook-sections.txt @@ -560,12 +560,15 @@ ESourceBackendSummarySetupPrivate <SECTION> <FILE>e-phone-number</FILE> EPhoneNumber +EPhoneNumberCountrySource EPhoneNumberError EPhoneNumberFormat EPhoneNumberMatch e_phone_number_is_supported e_phone_number_from_string e_phone_number_to_string +e_phone_number_get_country_code +e_phone_number_get_national_number e_phone_number_compare e_phone_number_compare_strings e_phone_number_copy
Created attachment 234990 [details] [review] [PATCH 5/6] libebook: Export the API for phone region guessing
Review of attachment 234981 [details] [review]: Looks fine, please commit.
Review of attachment 234984 [details] [review]: Looks fine, please commit (I thought this is already addressed, but you see the way of review on my side)
Review of attachment 234990 [details] [review]: Looks fine, just write "Since: 3.8" in the doc comment for newly added public functions.
I hope these are basically the last changes in this bug report, as this should be finished before 3.7.90, which is to be done in two weeks, when API/ABI freeze starts.
(In reply to comment #45) > I hope these are basically the last changes in this bug report, as this should > be finished before 3.7.90, which is to be done in two weeks, when API/ABI > freeze starts. The updated "sqlitedb: Permit indexes with normalized phone numbers" patch is rotting on my disk still. Let my check why I didn't upload yet. I think the unit test regarding locale changes was missing still. Well, and I don't know yet how to get initially proposed X-EVOLUTION-E164 parameter done, without rewriting the database on locale changes.
(In reply to comment #46) > Well, and I don't know yet how to get initially proposed X-EVOLUTION-E164 > parameter done, without rewriting the database on locale changes. I thought that breaking into two columns, one for country code (can be empty) and one for phone number itself, fixed the issue about the locale, in more generic way than just guessing the country code from currently used locale. Did you actually commit the locale-being-used patch in master? I do not follow closely all the changes here, it's just too many of them for me.
(In reply to comment #47) > (In reply to comment #46) > > Well, and I don't know yet how to get initially proposed X-EVOLUTION-E164 > > parameter done, without rewriting the database on locale changes. > > I thought that breaking into two columns, one for country code (can be empty) > and one for phone number itself, fixed the issue about the locale, in more > generic way than just guessing the country code from currently used locale. Did > you actually commit the locale-being-used patch in master? I do not follow > closely all the changes here, it's just too many of them for me. Nah, that's not the issue. Even getting along with just one single column. Just doing the locale change test and then uploading that patch. Problem is how to do those E.164 attributes: TEL;X-EVOLUTION-E164=+492285423789:+49(228)5423-789 With the original patch I just hacked the E.164 number into the vCard before saving. This doesn't work anymore - of course.
Created attachment 235374 [details] [review] sqlitedb: Permit indexes with normalized phone numbers Let me know if I shall cut into smaller pieces for easier review.
Created attachment 235382 [details] [review] sqlitedb: Permit indexes with normalized phone numbers ...spotted a typo
Created attachment 235390 [details] [review] sqlitedb: Permit indexes with normalized phone numbers all good things are three...
Created attachment 235433 [details] [review] sqlitedb: Permit indexes with normalized phone numbers Found some more small cleanups.
Created attachment 235434 [details] [review] sqlitedb: Store E.164 parameter in vcards Finally also this one.
I cannot build the first patch, it claims: > e-book-backend-sqlitedb.c: In function 'convert_match_exp': > e-book-backend-sqlitedb.c:3203:6: error: implicit declaration of function > 'setlocale' [-Werror=implicit-function-declaration] > e-book-backend-sqlitedb.c:3203:6: warning: nested extern declaration of > 'setlocale' [-Wnested-externs] > e-book-backend-sqlitedb.c:3203:17: error: 'LC_ADDRESS' undeclared (first use > in this function) > e-book-backend-sqlitedb.c:3203:17: note: each undeclared identifier is > reported only once for each function it appears in I thought the final solution will be completely without these calls, except of the tests, maybe. I also noticed few minor coding style "issues" spaces before function arguments, but that doesn't matter much. With respect of the second patch, feel free to use either g_strcmp0() or g_str_equal(), when you strcmp() == 0 (both functions have its advantages). Both looks otherwise fine, except of the buildability :)
By the way, does the note about indexing correct E164 phone numbers mean that if my book will use the phone index and the phone number will not be properly formatted, then I'll not find it with a search?
(In reply to comment #54) > I cannot build the first patch, it claims: > > > e-book-backend-sqlitedb.c: In function 'convert_match_exp': > > e-book-backend-sqlitedb.c:3203:6: error: implicit declaration of function > > 'setlocale' [-Werror=implicit-function-declaration] > > e-book-backend-sqlitedb.c:3203:6: warning: nested extern declaration of > > 'setlocale' [-Wnested-externs] > > e-book-backend-sqlitedb.c:3203:17: error: 'LC_ADDRESS' undeclared (first use > > in this function) > > e-book-backend-sqlitedb.c:3203:17: note: each undeclared identifier is > > reported only once for each function it appears in Seems I've got an implicit <locale.h> include. Fixing. > I thought the final solution will be completely without these calls, except of > the tests, maybe. I also noticed few minor coding style "issues" spaces before > function arguments, but that doesn't matter much. libphonenumber needs a country code to handle phone numbers not conforming to E.164 (starting with a '+'): http://en.wikipedia.org/wiki/List_of_international_call_prefixes#Variations Difference now is, that we don't do the mistake of storing the 2-letter country code, or the cuntry calling code in the database. > With respect of the second patch, feel free to use either g_strcmp0() or > g_str_equal(), when you strcmp() == 0 (both functions have its advantages). g_str_equal() is strictly for GHashTable, got reminded to that way too many times on #gtk+. Will double check if some uses of strcmp() risk to face NULL pointers. Will use g_strcmp0() then. (In reply to comment #55) > By the way, does the note about indexing correct E164 phone numbers mean that > if my book will use the phone index and the phone number will not be properly > formatted, then I'll not find it with a search? Good point, should be added to the docs.
Created attachment 235956 [details] [review] sqlitedb: Permit indexes with normalized phone numbers Added the missing include and fixed a few code-style issues.
Created attachment 235957 [details] [review] sqlitedb: Permit indexes with normalized phone numbers Guard against possible NULL pointers from sexpr parser.
Except for the sexpr parsers I've kept the strcmp() calls as they are: - using g_str_equal() without hash tables doesn't match the intended purpose of that API, the other EDS code is giving bad example here. see http://developer.gnome.org/glib/stable/glib-Hash-Tables.html#g-str-equal - the remaining uses for strcmp() __are__ NULL safe.
(In reply to comment #59) > - the remaining uses for strcmp() __are__ NULL safe. ...so let's benefit from strcmp() being a builtin functions with gcc.
You use setlocale() also in e-book-backend-sqlitedb.c, but there is no #include for the locale.h. The error messages I pasted above came from this file. I think you'll update the patch for recent tristan changes, the libebook file movements, but it might not be any issue. Please commit before the 3.7.90 release, with the above changed.
Created attachment 236245 [details] [review] sqlitedb: Store E.164 parameter in vcards (updated for master) New patch applies to master
Created attachment 236246 [details] [review] sqlitedb: Permit indexes with normalized phone numbers (updated for master) This patch applies to current master, and added #include <locale.h> to address possible warnings. I had to patch these by hand, so the committer changed (added notes that I was updating Mathias' patches)
Comment on attachment 236245 [details] [review] sqlitedb: Store E.164 parameter in vcards (updated for master) Committing after changing the test case to be disabled if EDS is compiled without phone number support.
Comment on attachment 236246 [details] [review] sqlitedb: Permit indexes with normalized phone numbers (updated for master) Committed after changing the SqliteDB code to conditionally try PhoneNumber support only if e_phone_number_is_supported()
Mathias has been under the weather this week, so since this is just before API freeze I stepped in to commit these. As I was unable to easily compile libphonenumber on my computer, I just ensured that this code be non-intrusive for builds where E.164 phone numbers are not supported. This involved a little bit of special casing added to e-book-backend-sqlitedb.c, without which g_warnings() are spewed to the console for many tests. In any case, after applying this, all EBookClient tests pass as expected. I suspect Mathias will come back and double-check this this coming week, as the patches were already approved and he's spent significant effort on them already I'm pretty sure they are all OK (I did do a quick review of the SQLite code involved too), just needed to get this final API in before freeze time.