Closed Bug 516492 Opened 10 years ago Closed 10 years ago

Support switching locales in Fennec

Categories

(Firefox for Android Graveyard :: General, defect)

x86
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
fennec1.0b4

People

(Reporter: mfinkle, Assigned: mfinkle)

References

Details

(Whiteboard: [fennec l10n])

Attachments

(1 file, 4 obsolete files)

Attached patch patch (obsolete) — Splinter Review
If we decide to ship Fennec with multiple locales in the same package we need a UI and simple mechanism to support switching locales.

This is a basic patch that builds on bug 516122
Attachment #400568 - Flags: ui-review?(madhava)
Attachment #400568 - Flags: review?(gavin.sharp)
Whiteboard: [fennec l10n]
Attachment #400568 - Flags: review-
Comment on attachment 400568 [details] [diff] [review]
patch

r- from me, selecting your language by locale code isn't user-friendly.

Note that we're trying to get our locale codes closer to match http://tools.ietf.org/html/bcp47, see bug 356038 for partial work there.

To make my argument more consistent, on all.html, we download Firefox builds by locale code, too, but they're not what the user chooses.
(In reply to comment #1)
> (From update of attachment 400568 [details] [diff] [review])
> r- from me, selecting your language by locale code isn't user-friendly.

Completely agree, but that might be a followup bug
Comment on attachment 400568 [details] [diff] [review]
patch

I'm not a friend of follow up bugs if one is already late.

This code shouldn't be in unconditionally, too, for single-locale builds, or maybe more general for builds that only offer one locale.

As mentioned in mail, this code does not fix any of the other problems we're having with multi-locale builds like bookmarks and search at least.
Just to get the points from Axel's mail into this bug thread:

- we have no infrastructure to coordinate the shipping of multi-locale
stuff, from build, to build reporting, to release infra, whichever
- there's no UI for locale selection in any of our products
- matchOS isn't code that is maintained by us, and the locale choice
heuristics are unknown. And that algorithm doesn't impact locale code
choice on either glibc nor in mozilla.
- search?
- bookmarks?
- dictionaries? 
- firefox-l10n.js pref file per build: in firefox, some locales choose a custom suffix for domain completion, also Japanese redirecting more prefs to region.properties for their massive count of search engines
Attached patch patch 2 (obsolete) — Splinter Review
This patch adds a user-friendly string for the locale name
Assignee: nobody → mark.finkle
Attachment #400568 - Attachment is obsolete: true
Attachment #400688 - Flags: review?(gavin.sharp)
Attachment #400568 - Flags: ui-review?(madhava)
Attachment #400568 - Flags: review?(gavin.sharp)
I'll look into moving the bookmarks into the locale JAR. I think it's a simple JSON file that we could load via chrome://browser/locale/bookmarks.json

I'll talk to the startup people about doing the same to searchplugins. Moving those into a locale JAR might even be faster for startup.
Comment on attachment 400688 [details] [diff] [review]
patch 2

BTW, the friendly names I chose were mainly from the all.html page and I tried to cover the languages shipped on my n900.
Comment on attachment 400688 [details] [diff] [review]
patch 2

r-, files that shouldn't be localized shouldn't be in l10n, but in content.

Not sure if restricting to the locales on one of our devices is the right thing to do for a device-independent UI.

For locales for which we don't have a good name in all.html, it should probably fall back to use languageNames.properties and regionNames.properties, and whatever bug 356038 comes up with for other tags.
Attachment #400688 - Flags: review-
Attached patch patch 3 (obsolete) — Splinter Review
This patch moves the list of language names to content. The patch also hides the language setting if only one locale is present.
Attachment #400688 - Attachment is obsolete: true
Attachment #400794 - Flags: review?(gavin.sharp)
Attachment #400688 - Flags: review?(gavin.sharp)
I've asked Pascal to come up with some php script that exports the locale names into .properties format, and he's made some good progress. Not sure if the url is OK to put in the bug, Pascal?
>Not sure if the url is OK to put in the bug, Pascal?

yes, no problem with that:

http://granary.stage.mozilla.com/localeslist/index-properties.php
Attached patch patch v4 (obsolete) — Splinter Review
Updated patch to use pascal's properties file data

some screenshots:
http://people.mozilla.com/~mfinkle/fennec/screenshots/fennec-language-selector.png
http://people.mozilla.com/~mfinkle/fennec/screenshots/fennec-language-list.png
Attachment #400794 - Attachment is obsolete: true
Attachment #401162 - Flags: review?(gavin.sharp)
Attachment #400794 - Flags: review?(gavin.sharp)
Nice.

Two thoughts:  it probably shouldn't be in the Privacy and Security Section.  Are we going to need a "General Section"?  Or maybe we can just stick it up at the top after the "About Firefox" section.

Suggested revision to the descriptive line -- actually, I've tried coming up with something that doesn't use any jargon, and nothing I can think of is actually more useful than just the title line "Language"... what about just cutting the description?
(In reply to comment #13)
> Nice.
> 
> Two thoughts:  it probably shouldn't be in the Privacy and Security Section. 
> Are we going to need a "General Section"?  Or maybe we can just stick it up at
> the top after the "About Firefox" section.

Moving under "About Firefox" is a good idea. Then we get a "General" section without needing to add a "General" section title
 
> Suggested revision to the descriptive line -- actually, I've tried coming up
> with something that doesn't use any jargon, and nothing I can think of is
> actually more useful than just the title line "Language"... what about just
> cutting the description?

I liked telling the user the language was for the app, not webpages
Attached patch patch v5Splinter Review
Attachment #401233 - Flags: review?
Comment on attachment 401233 [details] [diff] [review]
patch v5

This patch:
* Moves the setting to the top section of the prefs
* Changes the description to "Choose your preferred language for Fennec"
* Adds a restart notification box like we use in add-ons manager, so the user knows a restart is needed
* Adds a preferences.js file since we have enough code to support it now
Attachment #401233 - Flags: review? → review?(gavin.sharp)
Attachment #401162 - Attachment is obsolete: true
Attachment #401162 - Flags: review?(gavin.sharp)
tracking-fennec: --- → ?
Attachment #401233 - Flags: review?(gavin.sharp) → review+
Comment on attachment 401233 [details] [diff] [review]
patch v5

>diff --git a/chrome/content/browser.xul b/chrome/content/browser.xul

>   <stringbundleset id="stringbundleset">
>     <stringbundle id="bundle_browser" src="chrome://browser/locale/browser.properties"/>
>     <stringbundle id="bundle_brand" src="chrome://branding/locale/brand.properties"/>
>+    <stringbundle id="bundle_languages" src="chrome://browser/content/languages.properties"/>
>   </stringbundleset>

I think we should probably avoid using <stringbundle> elements where possible (since they introduce mostly unnecessary XBL overhead). For this one we can just use nsIStringBundleService directly inside loadLocales, right?

(Might even want to have a smart getter for the bundle service in the global scope and cache bundles there to get rid of all of these in a followup?)

>diff --git a/chrome/content/preferences.js b/chrome/content/preferences.js

It would be nice to avoid duplicating much of the restart-notification/panel-startup logic between this file and extensions.js. Another followup?

>diff --git a/locales/en-US/chrome/browser.properties b/locales/en-US/chrome/browser.properties

>+# Preferences
>+prefsRestart=Restart to complete changes
>+prefsRestartButton.label=Restart

Could just rename addonsRestart and use that instead, no? Seems silly to have separate strings for these.

r=me with those addressed.
Comment on attachment 401233 [details] [diff] [review]
patch v5

We should get a follow up bug on displaying something good for locales not in languages.properties, too. That'd be for users installing a language pack from AMO for a language we don't have on our website.
Duplicate of this bug: 503611
(In reply to comment #17)

> I think we should probably avoid using <stringbundle> elements where possible
> (since they introduce mostly unnecessary XBL overhead). For this one we can
> just use nsIStringBundleService directly inside loadLocales, right?

Done

> (Might even want to have a smart getter for the bundle service in the global
> scope and cache bundles there to get rid of all of these in a followup?)

Bug 518111

> It would be nice to avoid duplicating much of the
> restart-notification/panel-startup logic between this file and extensions.js.
> Another followup?

Bug 518109

> >+# Preferences
> >+prefsRestart=Restart to complete changes
> >+prefsRestartButton.label=Restart
> 
> Could just rename addonsRestart and use that instead, no? Seems silly to have
> separate strings for these.

Done
pushed (without a checkin comment):
https://hg.mozilla.org/mobile-browser/rev/8112ded3b2f9
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → B4
how can I verify this change?
(In reply to comment #22)
> how can I verify this change?

The way I tested it was:
* Get a localized archive of Fennec from the l10n site
* Extract the language .jar and .manifest
* Copy the files to the Fennec chrome/ folder (where the en-US files are
* Start Fennec
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.