Closed Bug 568459 Opened 10 years ago Closed 10 years ago

read system locale from gconf

Categories

(Core :: Internationalization, defect)

All
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 2.0a1+ ---

People

(Reporter: wolfiR, Assigned: wolfiR)

References

Details

Attachments

(2 files, 4 obsolete files)

Extend the mechanism of reading the locale (for matchOS pref) to be able to read from GConf.
Assignee: smontagu → mozilla
Status: NEW → ASSIGNED
Attached patch patch (obsolete) — Splinter Review
Comment on attachment 451639 [details] [diff] [review]
patch

This feature is MAEMO6/MeeGo specific. The application framework there defines that gconf setting as authoritative.
It's not on other Linux systems.
Attachment #451639 - Flags: review?(smontagu)
Comment on attachment 451639 [details] [diff] [review]
patch

>+                if ( rv && !gconfLocaleString.IsEmpty() ) {

This should be |if ((NS_SUCCEEDED(rv)....|, no?
Comment on attachment 451639 [details] [diff] [review]
patch

oops, this was an older version. Will attach the correct one asap.
Attachment #451639 - Flags: review?(smontagu)
Attached patch correct patch (obsolete) — Splinter Review
This is the current patch.
It's changing the order a bit compared to the previous (and already has the right if statement :-() The gconf setting is meant to override any other LC_ environment variables and is finally authoritative for the UI locale
Attachment #451639 - Attachment is obsolete: true
Attachment #451878 - Flags: review?(smontagu)
Comment on attachment 451878 [details] [diff] [review]
correct patch

>+            if (lang == nsnull) {
>+                nsresult rv;
>+                nsCOMPtr<nsIGConfService> gconf =
>+                    do_GetService(NS_GCONFSERVICE_CONTRACTID, &rv);
>+                if (NS_SUCCEEDED(rv)) {
>+                    nsCAutoString gconfLocaleString;
>+                    rv = gconf->GetString(NS_LITERAL_CSTRING("/meegotouch/i18n/language"),
>+                                          gconfLocaleString);
>+                    if (NS_SUCCEEDED(rv) && !gconfLocaleString.IsEmpty()) {
>+                        lang = static_cast<const char*>PromiseFlatCString(gconfLocaleString).get();
>+                    }
>+                }
>+            }

...

>+                CopyASCIItoUTF16(lang, platformLocale);
>+                result = posixConverter->GetXPLocale(lang, xpLocale); 


I think it would be safer to use ToNewCString rather than get here, but I'd like a second opinion
Attachment #451878 - Flags: review?(smontagu)
Attachment #451878 - Flags: review?(benjamin)
Attachment #451878 - Flags: review+
Comment on attachment 451878 [details] [diff] [review]
correct patch

>diff --git a/intl/locale/src/nsLocaleService.cpp b/intl/locale/src/nsLocaleService.cpp

>+#if (MOZ_PLATFORM_MAEMO == 6)
>+#  include "nsIGConfService.h"
>+#endif

==6, not >= 6?


>+            const char* lang = getenv("LANG");
>+#if (MOZ_PLATFORM_MAEMO == 6)
>+            if (lang == nsnull) {
>+                nsresult rv;
>+                nsCOMPtr<nsIGConfService> gconf =
>+                    do_GetService(NS_GCONFSERVICE_CONTRACTID, &rv);
>+                if (NS_SUCCEEDED(rv)) {
>+                    nsCAutoString gconfLocaleString;
>+                    rv = gconf->GetString(NS_LITERAL_CSTRING("/meegotouch/i18n/language"),
>+                                          gconfLocaleString);
>+                    if (NS_SUCCEEDED(rv) && !gconfLocaleString.IsEmpty()) {
>+                        lang = static_cast<const char*>PromiseFlatCString(gconfLocaleString).get();
>+                    }
>+                }
>+            }

This is definitely not safe the PromiseFlatCString goes out of scope immediately, so you can't use the char* beyond its lifetime. Besides which, you don't actually need PromiseFlatCString because nsCAutoString is always flat to begin with. I suggest you hoist gconfLocaleString up to the same block as "lang", and then you can just set lang = gconfLocaleString.get().
Attachment #451878 - Flags: review?(benjamin) → review-
Attached patch patch #2 (obsolete) — Splinter Review
> ==6, not >= 6?

Who knows but for now I'm fine (and even in favour) of using >=6.


From reading I see it going out of scope but then I'm wondering why it succeeded in every test we've made.
Did what you suggested in this one.
Attachment #451878 - Attachment is obsolete: true
Attachment #454442 - Flags: review?
Attachment #454442 - Flags: review? → review?(benjamin)
Attachment #454442 - Flags: review?(benjamin) → review+
Bad timing. I need to make a tiny change which changes the priorities of settings. The platform also defines LANG now to a default value for other reasons. The GConf key needs to be used as primary source now and LANG as fallback only. Slightly changed patch coming up soon.
Attached patch patch #3 (obsolete) — Splinter Review
Slightly changed patch to use the gconf setting if available instead of preferring LANG.
That's needed as the platform in question changed a bit and LANG is now "misused" a bit.
Attachment #454442 - Attachment is obsolete: true
Attachment #458574 - Flags: review?(benjamin)
Attachment #458574 - Flags: review?(benjamin) → review+
Need that for Fennec2 -> asking blocking
tracking-fennec: --- → ?
Blocks: 583135
Request push. Its +.
During another review I found that the patch does the right thing on the platform but doesn't do it correctly or "standard conform" enough (and not efficient asking gconf too many times).
I'm currently rewriting it a bit.
Attached patch patch #4Splinter Review
This moves the GConf access out of the for loop (also the getenv LANG which was there originally also).
For standard compliance I don't override LC_* settings anymore.

(Two not related whitespace fixes)
Attachment #458574 - Attachment is obsolete: true
Attachment #462734 - Flags: review?(benjamin)
Attachment #462734 - Flags: review?(benjamin) → review+
+                lang = gconfLocaleString.get();
this line cause 
intl/locale/src/nsLocaleService.cpp: In constructor 'nsLocaleService::nsLocalService()':
intl/locale/src/nsLocaleService.cpp:192: error: invalid conversion from 'const char*' to 'char*'

g++  4.3.3
Yes, I removed the const by accident before submission while I tested with it :-(
Need to be considered when pushed.
-+                lang = gconfLocaleString.get();
++                lang = static_cast<const char*>(gconfLocaleString.get());
approval? autoapproval?
tracking-fennec: ? → 2.0a1+
http://hg.mozilla.org/projects/electrolysis/rev/4084caac398f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.