GNOME Bugzilla – Bug 536387
gdm does not set locale names correctly
Last modified: 2010-12-15 11:29:55 UTC
Currently gdm does not set locale names correctly because gdm_parse_language_name() changes the codeset names to the normalized one. It's a problem because the normalized locale names does not exist on Solaris and add_locale() calls language_name_is_valid() before gdm_parse_language_name() is called. I'm attaching the patch.
Created attachment 112031 [details] [review] Patch for data/Makefile.am, data/locale.alias, gui/simple-greeter/gdm-language-chooser-widget.c, gui/simple-greeter/gdm-languages.c I'd suggestion the following points with the patch: 1. Remove normalize_codeset() in gdm_parse_language_name() so that the locale->name has the correct locale names. 2. Back compatibility of locale.alias file. I think the UTF-8 only is better by default however I'd like to think to add the locales instead of the user's gconf value "recent-languages". I added collect_locales_from_locale_file() and separated add_utf8_locale() from add_locale(). 3. Remove the short locale names with #ifdef __sun. e.g. fr.UTF-8 is not needed and fr_FR.UTF-8 is needed by default. I added the code in collect_locales_from_directory(). BTW, I don't understand the problem of gdm-03-fixcrash.diff Thanks.
Takao. An earlier version of GDM crashed in the language code on Solaris, and the gdm-03-fixcrash.diff was a patch to work around the crash. If you didn't encounter any crashing problems when you disabled this patch, then I am guessing that the latest GDM 2.22.0 code was reworked so the crash did not happen anymore. In other words, this patch might not have been needed anymore. It was a temporary workaround until we were able to provide something more sensible. I have updated SFE so that we now build with your patch and removed the gdm-03-fixcrash.diff patch. When building this way, I notice that no languages show up in the languages list. When I select "Other" in the language list, I get a dialog that shows no language choices. This is an improvement over the previous behavior of GDM just crashing. However, is the empty list expected? I notice that the locale.alias file provided in your patch has nothing but comments in it. Is this the reason why no languages show up? Should this be fixed?
Created attachment 112100 [details] screenshot of the language list > When I select "Other" in the language list, I get a dialog that shows no language choices. I can show the languages. I attached the my result. Currently collect_locales_from_directory() checks /usr/share/locale/$LOCALE so I guess you don't install SUNWgnome-l10nmessages*. If it's your problem, probably it's better to change /usr/share/locale to /usr/lib/locale. Also language_name_is_valid() checks /usr/lib/locale/$LOCALE and language_has_font() checks if the fonts are installed. Please check your /usr/lib/locale, /usr/share/locale and fonts.
The picture looks good. You are right I do not have any SUNWgnome-l10nmessages packages on my system. In my /usr/share/locale directory, I do see a bunch of subdirectories, but none of them contain any .po files. The subdirectories only contain LC_MESSAGES files. In /usr/lib/locale, I only have C, POSIX, and iso_8859_1 directories (and the geo and lcttab files). So, based on this, I think you are right that I do not see any languages because I do not have any languages installed. However, wouldn't GDM be smarter if it did not bother showing the language selection choice or dialog if there are not any languages to choose?
OK, probably I think launching a warning dialog is useful for users to know the null langauge list. I think it's good not to show locales which don't exist in /usr/lib/locale because setlocale() fails. I guess some distributers like to check /usr/share/locale instead of /usr/lib/locale because the directory names might be wrong but generally I think we should check /usr/lib/locale. If directories exist in /usr/lib/locale but not in /usr/share/locale, the locales work in LC_TIME, LC_COLLATE, LC_NUMERIC but not in LC_MESSAGES. It means GUI messages are shown in English but the locale is available. It's also needed for us because we support such a partial locale. So I try to change /usr/share/locale to /usr/lib/locale in collect_locales_from_directory() and launch the warning dialog in the next patch with your concern. Does it make sense? BTW, localeadm command is available in case you want to install locales/fonts in Solaris.
To be honest, I'm not familiar enough with the /usr/lib/locale versus /usr/share/locale issue. I'm sure that you can figure out the best approach here. I think it would be better to simply not show the Language selection dialog in the panel at all if there are no choices. Or to disable the "Other" choice in the selection list so users cannot pop-up the dialog if the dialog will be empty.
Created attachment 112475 [details] [review] Update of id=112031 I updated the patch. I think if we use /usr/lib/locale, the language dialog does not have the null list. So don't we need to implement the additional codes, do we?
How is the status?
This patch is still not upstream. Jon, Ray, any opinions?
I don't think the patch is right. en_US.utf8, en_US.UTF-8 and en_US.UTF8 are all valid and point to the same locale so you have to condense all 3 into one canonical form. locale.alias doesn't seem to be the right place to get the list of locales. it's only a backward compatible mapping. Does solaris have locale -a? what format does it return? Honestly, I think we should just g_spawn locale -a and parse the output (filtering out locales that don't have po files in /usr/share/locale/*/LC_MESSAGES or don't have font coverage). parsing locale -a seems to be the most portable answer. I wonder if there would bbe performance hit.
> en_US.utf8, en_US.UTF-8 and en_US.UTF8 are all valid and point to the same locale so you have to condense all 3 into one canonical form. en_US.utf8 and en_US.UTF8 are invalid on Solaris. We don't support such a name schema at present and fall back to C locale so I think my patch is right on Solaris. > locale.alias doesn't seem to be the right place to get the list of locales. it's only a backward compatible mapping. Are you talking about Fedora? > Does solaris have locale -a? what format does it return? % locale -a C POSIX ar ar_EG.UTF-8 ar_SA.UTF-8 bg_BG bg_BG.ISO8859-5 bg_BG.UTF-8 ca ca_ES ca_ES.ISO8859-1 ca_ES.ISO8859-15 ca_ES.ISO8859-15@euro ca_ES.UTF-8 ... > Honestly, I think we should just g_spawn locale -a and parse the output Do you prefer 'locale -a' to parsing '/usr/lib/locale/*'? Either option is ok with me. > (filtering out locales that don't have po files in /usr/share/locale/*/LC_MESSAGES or don't have font coverage). We want to show locales even though the locales doesn't have .mo files. setlocale() depends on /usr/lib/locale instead of /usr/share/locale. Thanks.
Why would you want to show locales that don't have .mo files? That's just another way of saying "we want to show locales that don't have translations"
> Why would you want to show locales that don't have .mo files? Because we support the locales. Supported locales doesn't mean to have LC_MESSAGES. The partial locales have LC_CTYPE and LC_TIME. >That's just another way of saying "we want to show locales that don't have translations" Not sure what you mean. Available locales mean we should check /usr/lib/locale so that setlocale() works.
Created attachment 118884 [details] [review] get the codeset from nl_langinfo this patch should at least help the ".utf8 is invalid but .UTF-8 is valid on solaris" problem. I'll commit it after the freeze unless there is objections to it.
Created attachment 118885 [details] [review] filter dupes from language list The above patch is going to cause duplicate entries to show up in gconf, we'll need to filter them out with this patch.
I think /usr/lib/locale needs to be checked instead of /usr/share/locale and language_name_has_translations() needs to be removed. Otherwise your patch doesn't work on Solaris at least. My patch also adds C locale in add_available_languages() and loads locale.alias file for the back compatibility. I'll check the patch with details next week.
(In reply to comment #16) > I think /usr/lib/locale needs to be checked instead of /usr/share/locale and > language_name_has_translations() needs to be removed. Otherwise your patch > doesn't work on Solaris at least. > > My patch also adds C locale in add_available_languages() and loads locale.alias > file for the back compatibility. > > I'll check the patch with details next week. Takao, any update?
I saw Ray's two patches are committed into trunk and gnome-2-24 branch.
Any news here? Ray, are your two patches already in and can this be closed?
The patches are in, but they only address part of the problem in this report.
hey Ray, If the two patches were commited please mark them as commited then, it's easier to triage :)
I think this should be set now. We're checking /usr/lib/locale instead of /usr/share/locale by default now. If there's still a problem, please reopen.
Very sorry for the delayed response. patch id=118884 doesn't resolve this problem.
Created attachment 141542 [details] [review] Patch for configure.ac, data/Makefile.am, data/locale.alias, gui/simple-greeter/gdm-languages.c Updated of id=112475. Patch 118884 uses make_codeset_canonical_for_locale() in gdm_normalize_language_name() only. But gdm_parse_language_name() is used in the several functions. I set language_name_is_valid() in gdm_parse_language_name() to check the normalized codeset.
Created attachment 141629 [details] [review] Patch for configure.ac, data/Makefile.am, data/locale.alias, gui/simple-greeter/gdm-languages.c Revised the patch 141542 again. It seems when none UTF-8 is configured, it's shown as duplicated name. I changed if the codeset is none UTF-8, the label is "language (territory) (codeset)". If the codeset is UTF-8, the label is not changed.
Created attachment 141730 [details] [review] Patch for configure.ac, data/Makefile.am, data/locale.alias, gui/simple-greeter/gdm-languages.c I forgot to update scandir (LIBLOCALEDIR) instead of scandir (GNOMELOCALEDIR). If localedef is not used, the directory needs to exist.
Created attachment 141831 [details] [review] Patch for configure.ac, data/Makefile.am, data/locale.alias, gui/simple-greeter/gdm-language-chooser-widget.c, gui/simple-greeter/gdm-languages.c OK, I checked Fedora 11 too. The patch works fine on Fedora and Solaris. I added g_warning() when setlocale is failed and fonts are not found. I completed the patch. We don't have GUI configuration tool likes previous gdmsetup. After the GUI is available, I may try to look into it again.
Created attachment 142376 [details] [review] Patch for configure.ac, data/Makefile.am, data/locale.alias, gui/simple-greeter/gdm-language-chooser-widget.c, gui/simple-greeter/gdm-languages.c Revised the patch with the maintainer's comment. - s/--enable-mo-check/--with-incomplete-locales/ - s/ENABLE_MO_CHECK/WITH_INCOMPLETE_TRANSLATIONS/ I'll try GDM GUI tool with another bug later.
Created attachment 142747 [details] [review] updated patch That latest patch no longer seems to apply to GDM 2.27.90. Here is an updated patch which applies.
Takao, I notice that this patch adds the --with-incomplete-locales configuration option. Should we be using this option or not when building for Solaris? Also, I notice that this patch adds a locale.alias file. On Solaris, could you explain whether we need to modify this file. If so, if you could give an example of how this file should be modified so things work properly, that would be helpful. The file currently has the following description: +# You could insert none UTF-8 locales likes C, ja_JP.eucJP +# The format is language label, space and locale name but +# the language label is no longer used. To be honest, those instructions don't seem very clear to me.
We talked off line. Just record. > Should we be using this option or not when building for Solaris? Yes, I think it's better to use the option for Solaris since Solaris has many modifier locales and no mo files. > a locale.alias file. > To be honest, those instructions don't seem very clear to me. Sorry for inconvenient explanation. Probably I think it's better to enable C locale for Solaris since the installer supports C locale.
Comment on attachment 142747 [details] [review] updated patch why is there an if defined(linux) ??
Created attachment 142954 [details] [review] Patch for configure.ac, data/Makefile.am, data/locale.alias, gui/simple-greeter/gdm-chooser-widget.c, gui/simple-greeter/gdm-language-chooser-widget.c, gui/simple-greeter/gdm-languages.c OK, I removed if linux. Also added the following fixes. - Show modifier on language dialog if exist - Stop greeter SEGV after click "OK" without selections on language dialog. - Rename "Japanese (Japan) (eucJP)" to "Japanese (Japan) [eucJP]" - Rename "Unspecified" to "English [C]".
I noticed localedir part was fixed by bug 570003 and I removed it in my latest patch attachment 142954 [details] [review] .
Comment on attachment 142954 [details] [review] Patch for configure.ac, data/Makefile.am, data/locale.alias, gui/simple-greeter/gdm-chooser-widget.c, gui/simple-greeter/gdm-language-chooser-widget.c, gui/simple-greeter/gdm-languages.c Please commit the segfault fix separately. It's good to go in now. I don't think it makes sense to show the modifier ever. modifiers are for things like "Show this in cyrillic instead of latin characters". We already solve that problem by showing both items in the specified script. I don't think it makes sense to call the C locale English, since it's not necessarily English. Having said that, as long as distributions that don't normally show C, don't get it in the list, I don't really care. doing g_strcasecmp (codeset_code, "utf8") is not sufficient for determining if a locale is utf8. That's why we have language_name_is_utf8 (). you have: + if (is_fallback_language (language_code) && codeset_code == NULL) + codeset_code = g_strdup (language_code); that says basically "if locale is 'C' then set codeset to 'C'" which makes no sense to me. I think what you're doing is abusing the variables to hold values that don't make sense for the their names, so the output string looks like you want. I'd much rather you either renamed the variables or used an if statement to isolate your special case and do a different g_strdup_printf call in that case.
Created attachment 143043 [details] [review] Patch for configure.ac, data/Makefile.am, data/locale.alias, gui/simple-greeter/gdm-language-chooser-widget.c, gui/simple-greeter/gdm-languages.c Revised the patch. Committed the SEGV patch separately. http://git.gnome.org/cgit/gdm/commit/?id=aeea38dd59c49272db1a7475994b8e2c11bb574f > modifiers are for things like "Show this in cyrillic instead of latin characters". It's one of the usage. modifier is used for several usage. E.g. for different LC_TIME, input method. The following example is dict. http://www.opengroup.org/onlinepubs/007908799/xbd/envvar.html The problem is, when locale xx_XX.UTF-8 is loaded by default and a user add xx_XX.UTF-8@foo, the listed labels are same between the two locales on GUI so users cannot recognize the locales. I think the two labels need to be different. > I don't think it makes sense to call the C locale English, since it's not necessarily English. You're right. How about "Latin alphabet"? http://en.wikipedia.org/wiki/ISO/IEC_646 The "Unspecified" might be a too short word for GUI users? > doing g_strcasecmp (codeset_code, "utf8") is not sufficient for determining if a locale is utf8. That's why we have language_name_is_utf8 (). OK, I replaced it with language_name_is_utf8(). To get the codeset, I added one argument in the function. > that says basically "if locale is 'C' then set codeset to 'C'" which makes no sense to me. It was my failure. I removed it. > I'd much rather you either renamed the variables or used an if statement to isolate your special case and do a different g_strdup_printf call in that case. Do you mean you prefer the "if" condition to the '!is_utf8 ? codeset_code : ""' statement? I could modify it.
Created attachment 143044 [details] [review] Patch for configure.ac, data/Makefile.am, data/locale.alias, gui/simple-greeter/gdm-language-chooser-widget.c, gui/simple-greeter/gdm-languages.c > I'd much rather you either renamed the variables or used an if statement to isolate your special case and do a different g_strdup_printf call in that case. I put the different g_strdup_printf().
Comment on attachment 143044 [details] [review] Patch for configure.ac, data/Makefile.am, data/locale.alias, gui/simple-greeter/gdm-language-chooser-widget.c, gui/simple-greeter/gdm-languages.c I don't think we should show modifier in most (all?) cases. We're showing the languages in native script, so for example sr_RS and sr_RS@latin will not be show a duplicate. We don't want to show an @ sign in the UI in any case.
Also, I don't think calling the language "Latin Alphabet" makes sense. The list is a list of languages, not alphabets. (C isn't a language either, which is why we don't normally show it in the list)
Created attachment 146017 [details] [review] Patch for configure.ac, data/Makefile.am, data/locale.alias, gui/simple-greeter/gdm-language-chooser-widget.c, gui/simple-greeter/gdm-languages.c I revised the patch with your comments. - Removed modifiers in the language list. - Reverted "Latin Alphabet" to "Unspecified". - Replaced WITH_INCOMPLETE_TRANSLATIONS with WITH_INCOMPLETE_LOCALES . - Added utf8_only flag together with language_name_has_translations() check. OK, probably showing the modifier is not a good appearance and I removed it. Probably I'd like to think a configuration to show the locale names besides the language names with another bug. I reverted "Latin Alphabet" to "Unspecified" at the moment. However "Unspecified" may be too short for me. "Unspecified language" might be an idea. Probably it's good to put "Unspecified" at last in the language name sort. I added 'utf8_only && !language_name_has_translations()' so that C locale is added by manual.
Comment on attachment 146017 [details] [review] Patch for configure.ac, data/Makefile.am, data/locale.alias, gui/simple-greeter/gdm-language-chooser-widget.c, gui/simple-greeter/gdm-languages.c okay, i commited this with a few minor changes.
Thx for your integration. - data/locale.alias is missed and it causes a build error - font warning is missed in gui/simple-greeter/gdm-language-chooser-widget.c --- gdm-2.27.4/gui/simple-greeter/gdm-language-chooser-widget.c.orig +++ gdm-2.27.4/gui/simple-greeter/gdm-language-chooser-widget.c @@ -196,6 +196,8 @@ add_available_languages (GdmLanguageChoo for (i = 0; language_names[i] != NULL; i++) { if (!language_has_font (language_names[i])) { + g_warning ("Your locale '%s' was not configured fonts", + language_names[i]); continue; } gdm_language_chooser_widget_add_language (widget, I think the warning is useful when users configure a new fontconfig.
I met same build error what Takao said on comment #42 with either gdm-2.29.5 tarball or git-master code base, so I'd like to reopen this bug.
Now locale.alias is installed in HEAD. I moved the font warning issue to bug 607727.
> We're showing the languages in native script, so for example sr_RS and > sr_RS@latin will not be show a duplicate. Bollocks. https://bugzilla.gnome.org/attachment.cgi?id=176440 In particular, be and be@latin > I don't think it makes sense to show the modifier ever. modifiers are for > things like "Show this in cyrillic instead of latin characters". We already > solve that problem by showing both items in the specified script. No, they are not for this. They are for *any* locale variants, at user's discretion. If a locale xx uses Roman numerals for months in dates, and user change them to 3-letter contractions and names new locale xx@foo, or any such not script-related change, he should *see* the difference when selecting locale.