GNOME Bugzilla – Bug 751826
Use g_get_language_names() for other locale categories
Last modified: 2018-03-13 12:19:59 UTC
Users sometimes wish to use LC_MESSAGES=C and LC_CTYPE=foo for several fonts and input methods. And I'd use g_get_language_names() for other locale categories besides LC_MESSAGES.
Created attachment 306591 [details] [review] Patch for glib/gcharset.c Attached the proposed patch.
Created attachment 307222 [details] [review] Patch for glib/gcharset.c Revised the patch for multiple language category names.
Review of attachment 307222 [details] [review]: This seems reasonable. Sorry for the delay in reviewing the patch. Would you be able to add some unit tests to the patch, before it can be accepted? ::: glib/gcharset.c @@ +558,3 @@ +/** + * g_get_language_names_with_category: + * @locale: a locale category name s/locale/category_name/ @@ +562,3 @@ + * Computes a list of applicable locale names with a locale category name. + * g_get_language_names() returns + * g_get_language_names_with_category("LC_MESSAGES"). This description should mention what order g_get_language_names_with_category() consults the environment variables, how `category_name` fits in to that order, and give an example of when g_get_language_names_with_category() should be used. @@ +567,3 @@ + * that must not be modified or freed. + * + * Since: 2.46 This will now need to be 2.56. @@ +568,3 @@ + * + * Since: 2.46 + **/ Nitpick: Ending a gtk-doc comment with `**/` is now deprecated; use `*/` instead. ::: glib/gcharset.h @@ +36,3 @@ GLIB_AVAILABLE_IN_ALL const gchar * const * g_get_language_names (void); +GLIB_AVAILABLE_IN_2_46 This should now be GLIB_AVAILABLE_IN_2_56.
Created attachment 367813 [details] [review] Patch for glib/gcharset.c I revised the patch.
Review of attachment 367813 [details] [review]: ::: glib/gcharset.c @@ +580,3 @@ + * that must not be modified or freed. + * + * Since: 2.56 Sorry, this missed the 2.56 API freeze (https://wiki.gnome.org/Schedule#Release_Schedule), so it will have to wait until we branch 2.56 and start developing 2.58 on master. Please change this to `Since: 2.58` pre-emptively. ::: glib/gcharset.h @@ +35,3 @@ GLIB_AVAILABLE_IN_ALL const gchar * const * g_get_language_names (void); +GLIB_AVAILABLE_IN_2_56 GLIB_AVAILABLE_IN_2_58 ::: glib/tests/charset.c @@ +1,1 @@ +#include "config.h" This test file needs a license and copyright header. It also needs to be plumbed into the autotools and Meson build systems. List it in $(test_programs) in Makefile.am, and in `glib_tests` in meson.build. @@ +31,3 @@ + +int +main (int argc, char *argv[]) Please use g_test_init() and g_test_add_func() to move the test code into a unit test path. That means you get automatic TAP support, and the test will hook into the rest of GLib’s test framework better. See glib/tests/bytes.c for an example. @@ +34,3 @@ +{ + const gchar * const *language_names = NULL; + int i, j; gsize, as you’re indexing arrays. @@ +37,3 @@ + + for (i = 0; TEST_TABLE[i]; ++i) + { At the start of each iteration, I suggest printing the iteration number to make debugging easier: g_test_message ("Test %" G_GSIZE_FORMAT, i); @@ +41,3 @@ + { + g_warning ("g_setenv(\"%s\", \"%s\", TRUE) is failed.", + TEST_TABLE[i], TEST_LOCALE); In a unit test, you should use assertions. So you can reformulate this if-block as: g_assert_true (g_setenv (TEST_TABLE[i], TEST_LOCALE, TRUE)); @@ +45,3 @@ + } + language_names = g_get_language_names_with_category ("LC_CTYPE"); + if (g_strv_length ((gchar **)language_names) != g_strv_length ((gchar **)TEST_RESULT)) g_assert_cmpuint (g_strv_length ((gchar **) language_names), ==, g_strv_length ((gchar **) TEST_RESULT)); @@ +54,3 @@ + for (j = 0; language_names[j]; ++j) + { + if (g_strcmp0 (language_names[j], TEST_RESULT[j])) g_assert_cmpstr (languages_names[j], ==, TEST_RESULT[j]);
Created attachment 369322 [details] [review] Patch for glib/gcharset.c I revised the patch.
Review of attachment 369322 [details] [review]: Thanks, this will get merged after we branch 2.56 and start development on 2.58. There are a couple of small remaining issues which I’ll fix when merging. ::: glib/tests/charset.c @@ +11,3 @@ + +#undef G_DISABLE_ASSERT +#undef G_LOG_DOMAIN These two lines are unnecessary and can be dropped. I’ll do that before merging. ::: glib/tests/meson.build @@ +7,3 @@ 'bytes', 'cache', + 'charset', A similar addition needs to be made to glib/tests/Makefile.am. I’ll do that before merging.
Oops, I forgot to add Makefile.am. Thanks.