After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 709272 - language: Remove .utf8 extension
language: Remove .utf8 extension
Status: RESOLVED FIXED
Product: gnome-initial-setup
Classification: Applications
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: GNOME Initial Setup maintainer(s)
GNOME Initial Setup maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2013-10-02 14:38 UTC by Colin Walters
Modified: 2013-10-26 15:40 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
language: Remove .utf8 extension (2.64 KB, patch)
2013-10-02 14:39 UTC, Colin Walters
needs-work Details | Review
language: Don't use .utf8 extension (3.12 KB, patch)
2013-10-02 16:47 UTC, Colin Walters
reviewed Details | Review
language: Don't use .utf8 extension (3.23 KB, patch)
2013-10-02 17:41 UTC, Colin Walters
rejected Details | Review
language: Also show UTF-8 locales which are not suffixed with .utf8 (4.03 KB, patch)
2013-10-08 17:43 UTC, Colin Walters
committed Details | Review

Description Colin Walters 2013-10-02 14:38:59 UTC
On at least Fedora, "locale -a" outputs each locale twice; once as
e.g. "en_US", and once as "en_US.utf8".

This code (which apparently long ago derives from gdm-language.c), is
hardcoded to suffix .utf8.  If those variants *aren't* available
(because the system just defaults to utf8, as is the case with
OpenEmbedded's eglibc), then we end up hiding the languages.

I think this patch won't break modern Fedora/glibc, and should
potentially help other systems.  If it breaks, we can revisit this and
just add both variants to the hash table.
Comment 1 Colin Walters 2013-10-02 14:39:10 UTC
Created attachment 256283 [details] [review]
language: Remove .utf8 extension
Comment 2 Jasper St. Pierre (not reading bugmail) 2013-10-02 14:40:38 UTC
This code also appears in gnome-control-center. Do you have a patch for those as well?
Comment 3 Colin Walters 2013-10-02 14:50:05 UTC
(In reply to comment #2)
> This code also appears in gnome-control-center. Do you have a patch for those
> as well?

Not yet, but I will if this patch is accepted.
Comment 4 Colin Walters 2013-10-02 15:28:18 UTC
Possibly related:

https://bugzilla.gnome.org/show_bug.cgi?id=702344

Ok, hmm.  I think we should probably just import this code into gnome-desktop and deduplicate it there.
Comment 5 Rui Matos 2013-10-02 16:23:19 UTC
Review of attachment 256283 [details] [review]:

We just need to change the is_current_locale() check too. Something like:

return g_str_has_prefix (setlocale (LC_CTYPE, NULL), locale);

should do.
Comment 6 Jasper St. Pierre (not reading bugmail) 2013-10-02 16:31:17 UTC
Review of attachment 256283 [details] [review]:

::: gnome-initial-setup/pages/language/cc-common-language.c
@@ +253,3 @@
         g_debug ("We have translations for %s", lang);
 
+        key = g_strdup_printf ("%s", lang);

uh, why is the strdup needed?
Comment 7 Colin Walters 2013-10-02 16:34:50 UTC
Anyone object to moving the cc-common-language.h APIs into libgnome-desktop?
Comment 8 Colin Walters 2013-10-02 16:38:05 UTC
(In reply to comment #6)
> Review of attachment 256283 [details] [review]:
> 
> ::: gnome-initial-setup/pages/language/cc-common-language.c
> @@ +253,3 @@
>          g_debug ("We have translations for %s", lang);
> 
> +        key = g_strdup_printf ("%s", lang);
> 
> uh, why is the strdup needed?

The key is owned by the hash table.
Comment 9 Jasper St. Pierre (not reading bugmail) 2013-10-02 16:41:01 UTC
(In reply to comment #7)
> Anyone object to moving the cc-common-language.h APIs into libgnome-desktop?

Not at all, just copy over the used methods to gnome-language.c and then add these as followup commits.
Comment 10 Colin Walters 2013-10-02 16:47:40 UTC
Created attachment 256295 [details] [review]
language: Don't use .utf8 extension

Now with g_str_has_prefix and an improved commit message
Comment 11 Jasper St. Pierre (not reading bugmail) 2013-10-02 16:52:24 UTC
Review of attachment 256295 [details] [review]:

::: gnome-initial-setup/pages/language/cc-common-language.c
@@ +253,3 @@
         g_debug ("We have translations for %s", lang);
 
+        key = g_strdup_printf ("%s", lang);

If you can't eliminate this copy somehow, please use g_strdup instead of g_strdup_printf and see if you can make it local to the g_hash_table_insert.
Comment 12 Colin Walters 2013-10-02 17:41:41 UTC
Created attachment 256305 [details] [review]
language: Don't use .utf8 extension

Now with plain g_strdup() and a further improved commit message.
Comment 13 Ray Strode [halfline] 2013-10-08 16:19:26 UTC
UTF8 isn't necessarily implied if no codeset is explicitly specified..  run locale -a -v on a fedora system to see what i mean.

For instance, to use your example of en_US:


locale: en_US           archive: /usr/lib/locale/locale-archive
-------------------------------------------------------------------------------
    title | English locale for the USA
   source | Free Software Foundation, Inc.
  address | http://www.gnu.org/software/libc/
    email | bug-glibc-locales@gnu.org
 language | English
territory | USA
 revision | 1.0
     date | 2000-06-24
  codeset | ISO-8859-1
Comment 14 Colin Walters 2013-10-08 17:43:22 UTC
Created attachment 256747 [details] [review]
language: Also show UTF-8 locales which are not suffixed with .utf8

Right...but per my commit message, gnome-languages.c is only returning
UTF-8 locales; we don't need to second-guess it by hardcoding a .utf8
suffix.

Oh...except, the hardcoded list of default languages will need the
.utf8 suffix *only* on systems which have it.  Ug.  I guess there are
a few options here:

1) Add a compile time option --enable-system-needs-utf8-suffix
2) Check for both variants, but only add ones which are actually in 
   gnome_get_all_locales().

Hm, that happens anyways implicitly because of the way
cc-language-chooser.c iterates over the result of
gnome_get_all_locales().

Ok, let's go with 2.
Comment 15 Rui Matos 2013-10-08 17:45:27 UTC
Review of attachment 256305 [details] [review]:

Unfortunately this doesn't work in Fedora. You basically get the reverse of what happens on gnome-continuous, i.e. gnome_get_all_locales() returns strings with the .utf8 suffix and we then can't find them in the hashtable.

I can see a few ways to fix this properly but it would require a big refactoring of these dialogs which will happen at some point since Allan has updated designs. It won't be today though.

The ugly workaround that will work on both camps for now is:

diff --git a/gnome-initial-setup/pages/language/cc-language-chooser.c b/gnome-initial-setup/pages/language/cc-language-chooser.c
index 2dea0dc..216fe0d 100644
--- a/gnome-initial-setup/pages/language/cc-language-chooser.c
+++ b/gnome-initial-setup/pages/language/cc-language-chooser.c
@@ -218,7 +218,13 @@ add_languages (CcLanguageChooser  *chooser,
                 if (!cc_common_language_has_font (locale_id))
                         continue;
 
-                is_initial = (g_hash_table_lookup (initial, locale_id) != NULL);
+                if (g_str_has_suffix (locale_id, ".utf8")) {
+                        is_initial = (g_hash_table_lookup (initial, locale_id) != NULL);
+                } else {
+                        gchar *tmp = g_strdup_printf ("%s.utf8", locale_id);
+                        is_initial = (g_hash_table_lookup (initial, tmp) != NULL);
+                        g_free (tmp);
+                }
                 widget = language_widget_new (locale_id, !is_initial);
 
                 gtk_container_add (GTK_CONTAINER (priv->language_list), widget);
Comment 16 Rui Matos 2013-10-08 17:55:41 UTC
Review of attachment 256747 [details] [review]:

Ok, this also works for now.

But drop the changes in cc-input-chooser.c since they aren't needed in this case and they're actively harmful in Fedora due to the way gnome_get_language_from_locale() works, e.g. it adds the codeset to the returned string if it's not UTF-8 so you end up with strings like "Italian (Italy) [ISO-8859-1]" in the UI.
Comment 17 Colin Walters 2013-10-08 18:16:13 UTC
(In reply to comment #16)
> Review of attachment 256747 [details] [review]:
> 
> Ok, this also works for now.
> 
> But drop the changes in cc-input-chooser.c since they aren't needed in this
> case and they're actively harmful in Fedora due to the way
> gnome_get_language_from_locale() 

But we're using gnome_get_all_locales() which will only give us UTF-8 locales, correct?
Comment 18 Rui Matos 2013-10-08 19:03:51 UTC
(In reply to comment #17)
> (In reply to comment #16)
> > But drop the changes in cc-input-chooser.c since they aren't needed in this
> > case and they're actively harmful in Fedora due to the way
> > gnome_get_language_from_locale() 
> 
> But we're using gnome_get_all_locales() which will only give us UTF-8 locales,
> correct?

Yes, but it's not the return from gnome_get_all_locales() that we feed into gnome_get_language_from_locale(), it's the synthesized %s_%s.utf8 strings.

I did it like that because the IBus API that returns the locale a given engine applies to, never returns the codeset suffix so I basically decided to "normalize" it to always be in the same form to lookup in the hash table.
Comment 19 Colin Walters 2013-10-08 19:30:13 UTC
(In reply to comment #18)

> I did it like that because the IBus API that returns the locale a given engine
> applies to, never returns the codeset suffix so I basically decided to
> "normalize" it to always be in the same form to lookup in the hash table.

I don't understand this...do you mean the IBus API *does* include the codeset suffix?  I apparently only have xkb engines currently in Continuous so I'm not getting anything in the ibus_engines hash table.

Alternatively can you make a patch on top of mine?
Comment 20 Rui Matos 2013-10-08 19:42:01 UTC
(In reply to comment #19)
> (In reply to comment #18)
> 
> > I did it like that because the IBus API that returns the locale a given engine
> > applies to, never returns the codeset suffix so I basically decided to
> > "normalize" it to always be in the same form to lookup in the hash table.
> 
> I don't understand this...do you mean the IBus API *does* include the codeset
> suffix?

I mean it does *not* include the codeset. And since I have to lookup what it returns in the main locales table, it needs to be normalized somehow and when I did this I chose to always add the utf8 suffix.

> Alternatively can you make a patch on top of mine?

No need. Just drop the changes to cc-input-chooser.c on yours and it's fine to land.
Comment 21 Colin Walters 2013-10-08 21:20:02 UTC
(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #18)
> > 
> > > I did it like that because the IBus API that returns the locale a given engine
> > > applies to, never returns the codeset suffix so I basically decided to
> > > "normalize" it to always be in the same form to lookup in the hash table.
> > 
> > I don't understand this...do you mean the IBus API *does* include the codeset
> > suffix?
> 
> I mean it does *not* include the codeset. And since I have to lookup what it
> returns in the main locales table, it needs to be normalized somehow and when I
> did this I chose to always add the utf8 suffix.

Ok, I see.  This is definitely ugly.  It's kind of tempting to change gnome-desktop to always add a codeset suffix.  Or alternatively, since the heuristic bit here is matching up what we get from IBus, change that lookup code to try both the .utf8 variant and one without.
Comment 22 Antoine Jacoutot 2013-10-26 13:15:25 UTC
*** Bug 710412 has been marked as a duplicate of this bug. ***
Comment 23 Colin Walters 2013-10-26 15:39:52 UTC
Attachment 256747 [details] pushed as c3d842f - language: Also show UTF-8 locales which are not suffixed with .utf8
Comment 24 Colin Walters 2013-10-26 15:40:22 UTC
Dropped the changes to cc-input-chooser and pushed, let me know if there are any issues!