Closed Bug 658278 Opened 8 years ago Closed 8 years ago

revise language pref UI to match new system

Categories

(Firefox for Android Graveyard :: General, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 9

People

(Reporter: madhava, Assigned: wesj)

References

Details

(Keywords: verified-aurora)

Attachments

(1 file, 6 obsolete files)

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
How/where are restarts after installation handled in the flow?
I assume a restart notifcation should be shown at the top of the preferences panel?
Attached patch WIP (obsolete) — Splinter Review
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.
Assignee: nobody → wjohnston
Attached patch Patch v1 (obsolete) — Splinter Review
First version patch for review. Applies on top of the patch in Bg 653141.
Attachment #533708 - Attachment is obsolete: true
Attachment #533867 - Flags: review?(mark.finkle)
Attached patch Patch v2 (obsolete) — Splinter Review
Updated to match some changes I made in Bug 653141. This applies on top of the "no reloads required" patch.
Attachment #534612 - Flags: review?(mark.finkle)
Attached patch Patch v3 (obsolete) — Splinter Review
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.
Attachment #533867 - Attachment is obsolete: true
Attachment #534612 - Attachment is obsolete: true
Attachment #533867 - Flags: review?(mark.finkle)
Attachment #534612 - Flags: review?(mark.finkle)
Attachment #541808 - Flags: review?(mark.finkle)
Attached patch Updated patch (obsolete) — Splinter Review
Updated. I removed the "Restart" button I was using because it was just making the code more confusing.
Attachment #541808 - Attachment is obsolete: true
Attachment #541808 - Flags: review?(mark.finkle)
Attachment #552530 - Flags: review?(mark.finkle)
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+
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.
Attached patch Patch v2 (obsolete) — Splinter Review
Addressed comments and refiddled this a bit. This should now just return you to the main ui with the "restart" notification bar shown.
Attachment #552530 - Attachment is obsolete: true
Attachment #552530 - Flags: review?(mark.finkle)
Attachment #557857 - Flags: review?(mark.finkle)
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...
Attachment #557857 - Flags: review?(mark.finkle) → review-
Attached patch Patch v3Splinter Review
> 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.
Attachment #557857 - Attachment is obsolete: true
Attachment #560972 - Flags: review?(mark.finkle)
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?
Attachment #560972 - Flags: review?(mark.finkle) → review+
hg.mozilla.org/integration/mozilla-inbound/rev/611545ce4357

filed follow up bug 688824
Blocks: 688824
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/611545ce4357
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → Firefox 9
Depends on: 689267
Depends on: 690022
Depends on: 689966
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
Status: RESOLVED → VERIFIED
Keywords: verified-aurora
You need to log in before you can comment on or make changes to this bug.