Closed
Bug 568459
Opened 14 years ago
Closed 14 years ago
read system locale from gconf
Categories
(Core :: Internationalization, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
fennec | 2.0a1+ | --- |
People
(Reporter: wolfiR, Assigned: wolfiR)
References
Details
Attachments
(2 files, 4 obsolete files)
3.12 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
2.99 KB,
patch
|
Details | Diff | Splinter Review |
Extend the mechanism of reading the locale (for matchOS pref) to be able to read from GConf.
Assignee | ||
Updated•14 years ago
|
Assignee: smontagu → mozilla
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•14 years ago
|
||
Assignee | ||
Comment 2•14 years ago
|
||
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 3•14 years ago
|
||
Comment on attachment 451639 [details] [diff] [review] patch >+ if ( rv && !gconfLocaleString.IsEmpty() ) { This should be |if ((NS_SUCCEEDED(rv)....|, no?
Assignee | ||
Comment 4•14 years ago
|
||
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)
Assignee | ||
Comment 5•14 years ago
|
||
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 6•14 years ago
|
||
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 7•14 years ago
|
||
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-
Assignee | ||
Comment 8•14 years ago
|
||
> ==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?
Assignee | ||
Updated•14 years ago
|
Attachment #454442 -
Flags: review? → review?(benjamin)
Updated•14 years ago
|
Attachment #454442 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 9•14 years ago
|
||
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.
Assignee | ||
Comment 10•14 years ago
|
||
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)
Updated•14 years ago
|
Attachment #458574 -
Flags: review?(benjamin) → review+
Comment 12•14 years ago
|
||
Request push. Its +.
Assignee | ||
Comment 13•14 years ago
|
||
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.
Assignee | ||
Comment 14•14 years ago
|
||
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)
Updated•14 years ago
|
Attachment #462734 -
Flags: review?(benjamin) → review+
Comment 15•14 years ago
|
||
+ 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
Assignee | ||
Comment 16•14 years ago
|
||
Yes, I removed the const by accident before submission while I tested with it :-( Need to be considered when pushed.
Comment 17•14 years ago
|
||
-+ lang = gconfLocaleString.get(); ++ lang = static_cast<const char*>(gconfLocaleString.get()); approval? autoapproval?
Updated•14 years ago
|
tracking-fennec: ? → 2.0a1+
Comment 18•14 years ago
|
||
http://hg.mozilla.org/projects/electrolysis/rev/4084caac398f
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•