137324 - should have dialog reminding users to restart when switching locale

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect)

Description

[Robert Kaiser]

I took the dialog, which was in place for content packs but never worked, out of
the code to fix bug 133764.

This bug is to bring the dialog back, working and working right (also warning
users when switching language).

This is post-1.0 work - and I'm willing to fix it, so I'm taking this one...

Comment 1

[Robert Kaiser]

Targetting to 1.2alpha for now, we'll see if I manage to do it before that (I
hope so)...

Comment 2

[Robert Kaiser]

Attachment: patch: add a dialog into language/content switching

OK, I've got a patch now - earlier than I originally thought ;-)

After doing the actual switching work, this checks again if we had to switch
content or language pack, and if we did anything, we display the dialog. I
copied the dialog code itself from themes panel and modified it to fit our
needs.

The patch also includes contentpacks.properties in the jar.mn file (I had to
insert one space in all other lines in ther to not disturb the ascii layout, so
don't wonder there, only one line is inserted), and changes to strings in
contentpacks.properties to reflect that it's for language and content packs
now.

Comment 3

[Robert Kaiser]

Adding cbiesinger and dbragg - could one of you review this one?

I removed the dialog in bug 133764 because it was broken and it broke major
switching functionality. Now the dialog is back and working with my patch,
similar to the dialog you get when switching themes.

It would be nice to get this at least in the trunk.

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

+        var strBundleService = 
Components.classes["@mozilla.org/intl/stringbundle;1"].getService();
+        strBundleService = 
strBundleService.QueryInterface(Components.interfaces.nsIStringBundleService);

you can use:
var strBundleService = 
Components.classes["@mozilla.org/intl/stringbundle;1"].getService(Components.int
erfaces.nsIStringBundleService); if you want

nominating for 1.0 since users will be confused if switching the language shows 
no effect.

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

really nominating

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 81232 [details] [diff] [review]
patch: add a dialog into language/content switching

r=biesi either way

you might want to ask mpt if the wording is OK

Comment 7

[Robert Kaiser]

Attachment: patch addressing biesi's comment (getting strBundleService in one line)

OK, this new patch is addressing your comment about getting strBundleService in
one line (with passing an argument to getService).

I tested this several times in my build, it's working also this way ;-)

Comment 8

[Robert Kaiser]

Comment on attachment 81310 [details] [diff] [review]
patch addressing biesi's comment (getting strBundleService in one line)

transferring r=biesi to new patch

Comment 9

[Robert Kaiser]

Hmm, I talked with mpt on IRC #ui channel, and he said we should display another
dialog, we just should make the existing text in the pref panel more visible:

<mpt> There should just be a single sentence, underneath the menu in the prefs
dialog, saying `Changes take effect on restart.'
<mpt> Currently there's about three sentences above the menu, which is just
waaaaaaay too long.
<mpt> The length of the description is directly attributable to the fact that
the feature should not exist.
<KaiRo> mpt: true. it's much too long. noone will read it. same with themes, btw...
<mpt> And at that point, I threw up my hands in despair, because I suck at
designing UI for stuff which shouldn't exist.

I'll probably do a patch for doing that, when I got time again.

Comment 10

[dbragg]

*** Bug 141282 has been marked as a duplicate of this bug. ***

Comment 11

[Robert Kaiser]

Attachment: Patch doing it the way mpt suggested

This new patch is moving the comment requesting for a restart to the bottom of
the pref panel, just above the OK button - like mpt suggested.

With this patch, everyone clicking 'OK' should see the comment, and does not
have to click another dialog.

Comment 12

[Robert Kaiser]

CCing mpt to get a UI review here. Should I attach a screen shot?

please r/sr, would be good to get this in finally....

Comment 13

[Matthew T (active 1999-2002)]

Comment on attachment 90390 [details] [diff] [review]
Patch doing it the way mpt suggested

> Index: mozilla/extensions/content-packs/resources/content/pref-contentpacks.xul
>...
> +
> +  <separator class="thin"/>
> +   <description>&restartOnLangChange.label;</description>

You're adding this, but I don't see you removing anything. Won't this cause the
label to be present twice?

>...
> Index: mozilla/extensions/content-packs/resources/locale/en-US/pref-contentpacks.dtd
>...
> -<!ENTITY languageIntro.label            "Selecting a new language pack changes the language for text that appears in dialog boxes, menus, toolbars and button labels. You must restart &brandShortName; for a new language pack to take effect.">
> +<!ENTITY contentIntro.label             "Selecting a new content pack changes items in the Sidebar and the Search menu, and changes the home page, certain bookmarks, and other items. You will not lose bookmarks and other items that you have customized when you switch content packs.">

There is no `Search' menu! (As I predicted when content packs were introduced:
their existence is a bug, because all the things they customize are bugs.)

>...
> +<!ENTITY restartOnLangChange.label      "You must restart &brandShortName; for a new content or language pack to take effect.">
>...

"Changes to content or language packs take effect when you restart
&brandShortName;."

Yes, please attach a screenshot when done.

Comment 14

[Matthew T (active 1999-2002)]

> You're adding this, but I don't see you removing anything. Won't this cause the
> label to be present twice?

Ignore that bit, I was asleep. The rest of the review stands, though.

Comment 15

[Robert Kaiser]

Attachment: patch v4: addressing mpt's review comments

OK, killing the part mentioning "Search Menu" (per mpt's suggestin on IRC), and
changing the restart label per mpt's suggestion.

screen shot follows...

Comment 16

[Robert Kaiser]

Attachment: screen shot of dialog with patch v4 applied

Comment 17

[Matthew T (active 1999-2002)]

Comment on attachment 90498 [details]
screen shot of dialog with patch v4 applied

That's still abominable, but it's much better than what we have now. Thankyou.

Comment 18

[Robert Kaiser]

to clear up mpt's last comment (citing IRC #ui channel):

<KaiRo> mpt9728: does your last comment in 137324 count as a review for the
recent patch? if yes, could you mark that in the patch?
<mpt9728> KaiRo: No, sorry, I'm not a code reviewer
<KaiRo> mpt9728: so this means, design is OKed by you, and I need additional
code review from a coder?
<mpt9728> yes.

can anyone review patch v4? biesi?

Comment 19

[Robert Kaiser]

Attachment: patch v5: also changing "home page" to "default home page"

per Pike's comment in IRC:
<Pike> KaiRo: I'd prefer to have "default homepage" to "homepage", as
"homepage" isn't listed as one of the things that don't change once customized

I changed this to "default home page" so that this gets reflected.
we should also cvs remove
mozilla/extensions/content-packs/resources/locale/en-US/contentpacks.properties

as this is not needed any more (I can't do this in my tree without write access
though).

Comment 20

[Axel Hecht]

Comment on attachment 90762 [details] [diff] [review]
patch v5: also changing "home page" to "default home page"

r=[e-mail]

Comment 21

[Alec Flett]

Comment on attachment 90762 [details] [diff] [review]
patch v5: also changing "home page" to "default home page"

sr=alecf

Comment 22

[Asa Dotzler [:asa]]

Comment on attachment 90762 [details] [diff] [review]
patch v5: also changing "home page" to "default home page"

a=asa (on behalf of drivers) for checkin to the 1.1 trunk.

Comment 23

[Robert Kaiser]

patch v5 checked into the trunk:

Checking in content/pref-contentpacks.xul;
/cvsroot/mozilla/extensions/content-packs/resources/content/pref-contentpacks.xul,v
 <--  pref-contentpacks.xul
new revision: 1.19; previous revision: 1.18
done
Checking in locale/en-US/pref-contentpacks.dtd;
/cvsroot/mozilla/extensions/content-packs/resources/locale/en-US/pref-contentpacks.dtd,v
 <--  pref-contentpacks.dtd
new revision: 1.9; previous revision: 1.8
done