Re: gnome_i18n_get_language_list -> g_i18n_get_language_list

[Owen Taylor <otaylor redhat com>]

Martin Baulig <martin@home-of-linux.org> writes:

> Hi guys,
> 
> any chance of getting this into glib ?
> 
> This will allow us to drop the last GNOME dependency from gnome-vfs.

Well, probably it can go in, but not exactly verbatim.

First, I have an I'm a bit hesitant about the fact that it 
uses the environment rather than setlocale() to determine the
language list. 

(Admittedly, gtk_setlocale() automatically calls )
 
Perhaps a compromise position would be to have:

 g_i18n_get_language_list (const gchar *locale,
                           const gchar *category_name);

Where, if locale is non-NULL, it is used instead of 
searching the environment. (Though perhaps since
we either have to have lang == NULL, or category_name == NULL,
it should be a separate subfunction

 g_i18n_get_language_list_for_locale (const gchar *locale);


Second, the are some formatting problems with the patch,
mostly from missing spaces.

Examples:

> static GHashTable *category_table= NULL;
                                   ^
>       g_strstrip(buf);
                  ^

>       if (buf[0]=='#' || buf[0]=='\0')
                  ^ ^            ^ ^ 

>       p = strtok (buf,"\t ");
                        ^
>       if (! category_value)

  (extra one)

There are quite a few more of these, and they all need to 
be fixed.

Finally, somd specific issues:

>   while (fgets (buf,256,fp))
>     {
>       char *p;
>       g_strstrip(buf);
>       if (buf[0]=='#' || buf[0]=='\0')
>         continue;
>       p = strtok (buf,"\t ");

glib is reentrant, so you need to avoid strtok. Since strtok_r
isn't sufficiently portable, you'll probably want to recode
in some other fashion.
 
> /*return the un-aliased language as a newly allocated string*/
> static char *
> unalias_lang (char *lang)

Comment about 'newly allocated' has no relation to reality.

> {
>   char *p;
>   int i;
>   if (!alias_table)
>     {
>       read_aliases ("/usr/share/locale/locale.alias");
>       read_aliases ("/usr/local/share/locale/locale.alias");
>       read_aliases ("/usr/lib/X11/locale/locale.alias");
>       read_aliases ("/usr/openwin/lib/locale/locale.alias");

I'm not so crazy about this indiscriminant reading of locale
alias files. If the alias isn't in the system locale alias
database, then it really doens't make much sense to honor
it.

>   while ((p=g_hash_table_lookup(alias_table,lang)) && strcmp(p, lang))

[ I'd encourage strcmp (p, lang) != 0 in glib/gtk+ code; not really
  mandatory ]

[...]

>       *codeset = g_new (gchar, 1 + at_pos - dot_pos);
>       strncpy (*codeset, dot_pos, at_pos - dot_pos);
>       (*codeset)[at_pos - dot_pos] = '\0';

This can be written more compactly as:

 *codeset = g_strndup (*dot_pos, at_pos - dot_pos);

>   /* Iterate through all possible combinations, from least attractive
>    * to most attractive.
>    */
>   for (i=0; i<=mask; i++)
>     if ((i & ~mask) == 0)
>       {
> 	gchar *val = g_strconcat(language,
> 				 (i & COMPONENT_TERRITORY) ? territory : "",
> 				 (i & COMPONENT_CODESET) ? codeset : "",
> 				 (i & COMPONENT_MODIFIER) ? modifier : "",
> 				 NULL);
> 	retval = g_list_prepend (retval, val);
>       }

I'd like to see this handle the normalized codeset. See the libc info
page for information about how this works. The relevant function has
already been cut-and-pasted into gtk/gtkrc.c, so you can get it from
there.

> /* The following is (partly) taken from the gettext package.
>    Copyright (C) 1995, 1996, 1997, 1998 Free Software Foundation, Inc.  */
> 
> static const gchar *
> guess_category_value (const gchar *categoryname)
> {
>   const gchar *retval;
> 
>   /* The highest priority value is the `LANGUAGE' environment
>      variable.  This is a GNU extension.  */
>   retval = g_getenv ("LANGUAGE");
>   if (retval != NULL && retval[0] != '\0')
>     return retval;

LANGUAGE only applies to LC_MESSAGES, not to all categories.
 
> const GList *
> g_i18n_get_language_list (const gchar *category_name)

'const GList *' just doesn't make sense. ->next would
be non-constant. 

Actually, I'm not so sure that returning a GList is a good
idea in general. 

GLists are nice dynamic data structures, but if the library function
can do the work once to convert it into a typesafe array, then I think
it should do so.

A NULL-terminated array like g_strsplit() I think would be the
best choice. (Alternatively, we could do array plus count;
we aren't terribly consistent in this matter in GLib. Array
+ count is probably a bit more common even.)

> {
>   GList *list;
> 
>   if (!category_name)
>     category_name= "LC_ALL";

LC_ALL is not a category, so this is a silly default. For
backwards compatibility, LC_MESSAGES might be a good choice.
 
> 	  while (category_value[0] != '\0' && category_value[0] == ':')

How about just:

	  while (category_value[0] == ':')

Regards,
                                        Owen