Hi, I want do land the attached patch. It adds GError arguments to the functions e_phone_number_get_country_code_for_region() and e_phone_number_get_default_region(). This is to permit better error handling and to avoid unit tests to break because of warnings, if phone number support is missing.
The functions in question were newly introduced within the 3.8 cycle and to far they only are used within EDS itself and outside of GNOME. Ciao, Mathias
>From e7e2bd8b43d6110723fb36d53c6523fbff38bd3e Mon Sep 17 00:00:00 2001 From: Mathias Hasselmann <[email protected]> Date: Thu, 7 Mar 2013 13:25:01 +0100 Subject: [PATCH] libebook: Report missing phone number support by GError Add GError arguments to e_phone_number_get_default_region() and e_phone_number_get_country_code_for_region() to properly report missing phone number: Warnings cannot be handled programatically and make GTest based unit tests failed. --- addressbook/libebook-contacts/e-phone-number.c | 26 +++++++--- addressbook/libebook-contacts/e-phone-number.h | 5 +- .../libedata-book/e-book-backend-sqlitedb.c | 17 ++++-- tests/libebook/test-ebook-phone-number.c | 54 ++++++++++++++++++-- 4 files changed, 83 insertions(+), 19 deletions(-) diff --git a/addressbook/libebook-contacts/e-phone-number.c b/addressbook/libebook-contacts/e-phone-number.c index b8f7c2b..55ef37c 100644 --- a/addressbook/libebook-contacts/e-phone-number.c +++ b/addressbook/libebook-contacts/e-phone-number.c @@ -96,6 +96,7 @@ e_phone_number_is_supported (void) * e_phone_number_get_country_code_for_region: * @region_code: (allow-none): a two-letter country code, a locale name, or * %NULL + * @error: (out): a #GError to set an error, if any * * Retrieves the preferred country calling code for @region_code, * e.g. 358 for "fi" or 1 for "en_US@UTF-8". @@ -109,7 +110,8 @@ e_phone_number_is_supported (void) * Since: 3.8 */ gint -e_phone_number_get_country_code_for_region (const gchar *region_code) +e_phone_number_get_country_code_for_region (const gchar *region_code, + GError **error) { #ifdef ENABLE_PHONENUMBER @@ -117,7 +119,7 @@ e_phone_number_get_country_code_for_region (const gchar *region_code) #else /* ENABLE_PHONENUMBER */ - g_warning ("%s: The library was built without phone number support.", G_STRFUNC); + _e_phone_number_set_error (error, E_PHONE_NUMBER_ERROR_NOT_IMPLEMENTED); return 0; #endif /* ENABLE_PHONENUMBER */ @@ -125,6 +127,7 @@ e_phone_number_get_country_code_for_region (const gchar *region_code) /** * e_phone_number_get_default_region: + * @error: (out): a #GError to set an error, if any * * Retrieves the current two-letter country code that's used by default for * parsing phone numbers in e_phone_number_from_string(). It can be useful @@ -141,7 +144,7 @@ e_phone_number_get_country_code_for_region (const gchar *region_code) * Since: 3.8 */ gchar * -e_phone_number_get_default_region (void) +e_phone_number_get_default_region (GError **error) { #ifdef ENABLE_PHONENUMBER @@ -149,7 +152,7 @@ e_phone_number_get_default_region (void) #else /* ENABLE_PHONENUMBER */ - g_warning ("%s: The library was built without phone number support.", G_STRFUNC); + _e_phone_number_set_error (error, E_PHONE_NUMBER_ERROR_NOT_IMPLEMENTED); return NULL; #endif /* ENABLE_PHONENUMBER */ @@ -216,6 +219,9 @@ e_phone_number_to_string (const EPhoneNumber *phone_number, #else /* ENABLE_PHONENUMBER */ + /* The EPhoneNumber instance must be invalid. We'd also bail out with + * a warning if phone numbers are supported. Any code triggering this + * is broken and should be fixed. */ g_warning ("%s: The library was built without phone number support.", G_STRFUNC); return NULL; @@ -245,6 +251,9 @@ e_phone_number_get_country_code (const EPhoneNumber *phone_number, #else /* ENABLE_PHONENUMBER */ + /* The EPhoneNumber instance must be invalid. We'd also bail out with + * a warning if phone numbers are supported. Any code triggering this + * is broken and should be fixed. */ g_warning ("%s: The library was built without phone number support.", G_STRFUNC); return 0; @@ -272,6 +281,9 @@ e_phone_number_get_national_number (const EPhoneNumber *phone_number) #else /* ENABLE_PHONENUMBER */ + /* The EPhoneNumber instance must be invalid. We'd also bail out with + * a warning if phone numbers are supported. Any code triggering this + * is broken and should be fixed. */ g_warning ("%s: The library was built without phone number support.", G_STRFUNC); return NULL; @@ -299,9 +311,9 @@ e_phone_number_compare (const EPhoneNumber *first_number, #else /* ENABLE_PHONENUMBER */ - /* NOTE: This calls for a dedicated return value, but I sense broken - * client code that only checks for E_PHONE_NUMBER_MATCH_NONE and then - * treats the "not-implemented" return value as a match */ + /* The EPhoneNumber instance must be invalid. We'd also bail out with + * a warning if phone numbers are supported. Any code triggering this + * is broken and should be fixed. */ g_warning ("%s: The library was built without phone number support.", G_STRFUNC); return E_PHONE_NUMBER_MATCH_NONE; diff --git a/addressbook/libebook-contacts/e-phone-number.h b/addressbook/libebook-contacts/e-phone-number.h index 17d2d1b..1870f66 100644 --- a/addressbook/libebook-contacts/e-phone-number.h +++ b/addressbook/libebook-contacts/e-phone-number.h @@ -209,9 +209,10 @@ GQuark e_phone_number_error_quark (void); gboolean e_phone_number_is_supported (void) G_GNUC_CONST; gint e_phone_number_get_country_code_for_region - (const gchar *region_code); + (const gchar *region_code, + GError **error); gchar * e_phone_number_get_default_region - (void); + (GError **error); EPhoneNumber * e_phone_number_from_string (const gchar *phone_number, const gchar *region_code, diff --git a/addressbook/libedata-book/e-book-backend-sqlitedb.c b/addressbook/libedata-book/e-book-backend-sqlitedb.c index 33f4a46..486c131 100644 --- a/addressbook/libedata-book/e-book-backend-sqlitedb.c +++ b/addressbook/libedata-book/e-book-backend-sqlitedb.c @@ -1194,7 +1194,7 @@ create_collation (gpointer data, db, name, SQLITE_UTF8, GINT_TO_POINTER (country_code), ixphone_compare_for_country); } else if (strcmp (name, "ixphone_national") == 0) { - country_code = e_phone_number_get_country_code_for_region (NULL); + country_code = e_phone_number_get_country_code_for_region (NULL, NULL); ret = sqlite3_create_collation_v2 ( db, name, SQLITE_UTF8, @@ -2042,8 +2042,12 @@ e_book_backend_sqlitedb_new_contacts (EBookBackendSqliteDB *ebsdb, return FALSE; } - if (e_phone_number_is_supported ()) - default_region = e_phone_number_get_default_region (); + if (e_phone_number_is_supported ()) { + default_region = e_phone_number_get_default_region (error); + + if (default_region == NULL) + success = FALSE; + } for (l = contacts; success && l != NULL; l = g_slist_next (l)) { EContact *contact = (EContact *) l->data; @@ -3170,7 +3174,7 @@ field_name_and_query_term (EBookBackendSqliteDB *ebsdb, * o Normalize the string * o Check the E.164 column instead */ - const gint country_code = e_phone_number_get_country_code_for_region (region); + const gint country_code = e_phone_number_get_country_code_for_region (region, NULL); if (ebsdb->priv->summary_fields[summary_index].type == E_TYPE_CONTACT_ATTR_LIST) { field_name = g_strdup ("multi.value_phone"); @@ -4624,7 +4628,10 @@ upgrade_contacts_table (EBookBackendSqliteDB *ebsdb, if (e_phone_number_is_supported ()) { g_message ("The phone number indexes' format has changed. Rebuilding them."); - default_region = e_phone_number_get_default_region (); + default_region = e_phone_number_get_default_region (error); + + if (default_region == NULL) + success = FALSE; } for (l = vcard_data; success && l; l = l->next) { diff --git a/tests/libebook/test-ebook-phone-number.c b/tests/libebook/test-ebook-phone-number.c index 63b8c39..5939c7d 100644 --- a/tests/libebook/test-ebook-phone-number.c +++ b/tests/libebook/test-ebook-phone-number.c @@ -339,22 +339,66 @@ test_supported (void) static void test_country_code_for_region (void) { + GError *error = NULL; + gint code; + g_assert_cmpstr (setlocale (LC_ADDRESS, NULL), ==, "en_US.UTF-8"); - g_assert_cmpint (e_phone_number_get_country_code_for_region ("CH"), ==, 41); - g_assert_cmpint (e_phone_number_get_country_code_for_region (NULL), ==, 1); - g_assert_cmpint (e_phone_number_get_country_code_for_region ("C"), ==, 0); - g_assert_cmpint (e_phone_number_get_country_code_for_region (""), ==, 1); + +#ifdef ENABLE_PHONENUMBER + + code = e_phone_number_get_country_code_for_region ("CH", &error); + g_assert_cmpstr (error ? error->message : NULL, ==, NULL); + g_assert_cmpint (code, ==, 41); + + code = e_phone_number_get_country_code_for_region (NULL, &error); + g_assert_cmpstr (error ? error->message : NULL, ==, NULL); + g_assert_cmpint (code, ==, 1); + + code = e_phone_number_get_country_code_for_region ("C", &error); + g_assert_cmpstr (error ? error->message : NULL, ==, NULL); + g_assert_cmpint (code, ==, 0); + + code = e_phone_number_get_country_code_for_region ("", &error); + g_assert_cmpstr (error ? error->message : NULL, ==, NULL); + g_assert_cmpint (code, ==, 1); + +#else /* ENABLE_PHONENUMBER */ + + code = e_phone_number_get_country_code_for_region ("CH", &error); + + g_assert (error != NULL); + g_assert (error->domain == E_PHONE_NUMBER_ERROR); + g_assert (error->code == E_PHONE_NUMBER_ERROR_NOT_IMPLEMENTED); + g_assert (error->message != NULL); + g_assert_cmpint (code, ==, 0); + +#endif /* ENABLE_PHONENUMBER */ } static void test_default_region (void) { + GError *error = NULL; gchar *country; g_assert_cmpstr (setlocale (LC_ADDRESS, NULL), ==, "en_US.UTF-8"); + country = e_phone_number_get_default_region (&error); + +#ifdef ENABLE_PHONENUMBER - country = e_phone_number_get_default_region (); + g_assert_cmpstr (error ? error->message : NULL, ==, NULL); g_assert_cmpstr (country, ==, "US"); + +#else /* ENABLE_PHONENUMBER */ + + g_assert (error != NULL); + g_assert (error->domain == E_PHONE_NUMBER_ERROR); + g_assert (error->code == E_PHONE_NUMBER_ERROR_NOT_IMPLEMENTED); + g_assert (error->message != NULL); + g_assert_cmpstr (country, ==, NULL); + +#endif /* ENABLE_PHONENUMBER */ + g_free (country); } -- 1.7.10.4
_______________________________________________ [email protected] https://mail.gnome.org/mailman/listinfo/release-team Release-team lurker? Do NOT participate in discussions.
