658278 - revise language pref UI to match new system

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Madhava Enros [:madhava]]

The new first-run language selection UI revises the list that users see on first-run -- more detail in bug 653141 (allow language choice on first-run).

To match (and to make the overall flow consistent), we have to revise what gets shown in the Prefs panel as well.  Design is outlined here:

http://farm3.static.flickr.com/2520/5735218463_396c6c8112_o.jpg

Comment 1

[Aaron Train [:aaronmt]]

How/where are restarts after installation handled in the flow?

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I assume a restart notifcation should be shown at the top of the preferences panel?

Comment 3

[Wesley Johnston (:wesj)]

Attachment: WIP

Pretty quick patch to open the new locale selector (in a new window) when the locale button is pressed. Work on device for me though. Just needs testing and a few edge cases dealt with.

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch v1

First version patch for review. Applies on top of the patch in Bg 653141.

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Updated to match some changes I made in Bug 653141. This applies on top of the "no reloads required" patch.

Comment 6

[Wesley Johnston (:wesj)]

Attachment: Patch v3

Updated. This shows the same locale picker ui, but replaces "Continue" with "Restart" (to switch we have to close/open the window which can result in data loss and should be treated as a restart for the users. I'm doing a real restart here. I guess we could attempt to close/open the window instead).

There's also a link to my dropbox and some language packs. I should probably remove that, but it makes testing much much easier.

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Updated patch

Updated. I removed the "Restart" button I was using because it was just making the code more confusing.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 552530 [details] [diff] [review]
Updated patch

>diff --git a/mobile/chrome/content/localePicker.js b/mobile/chrome/content/localePicker.js

>   installAddon: function() {

>     if (locale.install) {
>+      LocaleUI._currentInstall = locale;
>       LocaleUI.selectedPanel = LocaleUI._installerPage;
>       locale.install.addListener(installListener);
>       locale.install.install();
>     } else {
>-      this.closePicker();
>+      this.closeWindow(window.opener ? true : false);
>     }

>     // restore the last known "good" locale
>     this.language = this.defaultLanguage;
>     this.updateStrings();
>-    this.closePicker();
>+    if (window.opener)
>+      this.closeWindow(false);
>+    else
>+      this.closePicker();

Why use closePicker() here but remove it above?

>+      if (!window.opener) {
>+        let win = Services.ww.openWindow(window, "chrome://browser/content/browser.xul", "_blank", "chrome,dialog=no,all", argString);
>+        win.addEventListener("UIReady", function(aEvent) {
>+          win.removeEventListener("UIReady", arguments.callee, false);
>+          ss.restoreLastSession(true);
>+        }, false);
>+      }

I'm a little worried about the ss.restoreLastSession() call. We do some juggling of tabs in browser.js when we need to call this. We don't do that here. Should we?

waiting for answers before r+

Comment 9

[Wesley Johnston (:wesj)]

I think that is probably cruft left over from me trying to fix up the broken session restore behavior. I don't think we need this at all.

Still looking at the other guy. I think it should probably be in bug 668838.

Comment 10

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Addressed comments and refiddled this a bit. This should now just return you to the main ui with the "restart" notification bar shown.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 557857 [details] [diff] [review]
Patch v2


>diff --git a/mobile/chrome/content/localePicker.js b/mobile/chrome/content/localePicker.js

>   closePicker: function() {
>-    if (this._currentInstall) {
>+    if (LocaleUI._currentInstall) {

If "_currentInstall" should be used publicly, rename it "currentInstall"
Technically, "currentInstall" is a bit vague. Is it the currently installed locale?

Same goes for any other LocaleUI data members

>       Services.prefs.setBoolPref("intl.locale.matchOS", false);
>       Services.prefs.setCharPref("general.useragent.locale", getTargetLocale(this._currentInstall));

You still use this._currentInstall" here. Is that right?

>   installAddon: function() {

>     } else {
>-      this.closePicker();
>+      this.closeWindow(window.opener ? true : false);

closeWindow doesn't seem to take any arguements

>   closeWindow : function() {

See ^

>+    if (window.opener) {
>+      if (LocaleUI._currentInstall || this.locale != this.defaultLocale) {
>+        window.opener.PreferencesView.showRestart();
>+        window.opener.PreferencesView._loadLocales();

I don't like accessing the PreferencesView here. It couples things together too tightly. Can't we fire an observer notification or listen for preference changes (using an observer) in PreferencesView?

Sorry for keeping this going...

Comment 12

[Wesley Johnston (:wesj)]

Attachment: Patch v3

> If "_currentInstall" should be used publicly, rename it "currentInstall"
> Technically, "currentInstall" is a bit vague. Is it the currently installed locale?
I renamed this to pendingInstall. I renamed a whole bunch of "private" members to public ones.

> You still use this._currentInstall" here. Is that right?
Yep. Updated. Thanks.

> closeWindow doesn't seem to take any arguements
Cruft. Yanked now

> I don't like accessing the PreferencesView here. It couples things together too
> tightly. Can't we fire an observer notification or listen for preference changes 
> (using an observer) in PreferencesView?

Don't know why I didn't think of that. Updated now. We have to be a bit tricky. nsIToolkitChromeRegistry,getSelectedLocale("browser") also uses an observer to update the selected locale, so I added a setTimeout(0) to ensure we fire after it. However, it will still return the wrong locale if the locale is newly installed. So in order to make the language button have a semi-correct label, I'm using the pref instead. For newly installed languages we'll (currently) show something like "es-AR" on the button (which will change to "Espanol" on restart).

Its also kinda tricky to know if the selected Locale actually changed or not (i.e. changed to spanish, then back to english). We shouldn't show a restart prompt there, but... like I said, its tough to know.

Also fixed a little bug where I was using the appBuildId in one place and platformBuildID in another. Not sure which is better, but they both use platformBuildID now.

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 560972 [details] [diff] [review]
Patch v3

We could probably cache the currentLocale in preferences.js and use that to turn off a "restart" if the user switches back to the current locale.

Followup bug?

Comment 14

[Wesley Johnston (:wesj)]

hg.mozilla.org/integration/mozilla-inbound/rev/611545ce4357

filed follow up bug 688824

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/611545ce4357

Comment 16

[Aaron Train [:aaronmt]]

Mozilla/5.0 (Android; Linux armv7l; rv:10.0a1) Gecko/20111003 Firefox/10.0a1 Fennec/10.0a1
Mozilla/5.0 (Android; Linux armv7l; rv:9.0a2) Gecko/2011103 Firefox/9.0a2 Fennec/9.0a2