399665 - Support replacing BrandShortName and BrandFullName in the installer

Status: RESOLVED FIXED

(Firefox :: Installer, defect, P2)

Description

[Robert Strong (they/them - no direct email)]

PRD Item INST-005a - Support reading in a localizable string with a partner name

We can also support replacing all of the text on the Welcome and Finish pages in the wizard.

Comment 1

[Robert Strong (they/them - no direct email)]

To do this we will need to introduce at minimum three new vars. BrandShortName, BrandFullName, and BrandFullNameDA (DA stands for double ampersand which prevents &'s from being used as the key to activate a control).

If BrandShortName or BrandFullName is provided via an ini file this will be used to set these vars. BrandFullNameDA will just be a sanitized version of BrandFullName.

So we don't have to modify all locales I am planning on fixing up the existing defines and lang strings to use the new vars in preprocess-locale.pl

Since we have to use vars there will be a slight increase in the size of the installer.

Comment 2

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

Comment 3

[Robert Strong (they/them - no direct email)]

Comment on attachment 284799 [details] [diff] [review]
patch rev1

Benjamin, I used compare-locales.pl to create check-locales.pl which can be used to verify miscellaneous things within the locale files for the installer. I also added compare-locales.pl to repackage-win32-installer since the tboxen don't appear to be checking these.

Besides that this patch makes it so vars are used for BrandShortName, BrandFullName, and BrandFullNameDA so distributions can customize these strings. The ini file must be named setup.ini and it must live in the distributions directory. Also see bug 399381 which has a patch for customizing the installer images.

Comment 4

[Robert Strong (they/them - no direct email)]

Axel, heads up about some changes to the installer locale files. This adds compare-locales checks and an additional check for deprecated string usage in the installer locale files that will cause the tinderboxen to go red for l10n when they are not correct.

Comment 5

[Axel Hecht [:Pike]]

Hrm. compare-locales.pl is unmaintained, I'm not too fond of the idea to fork that code. The fact that tinderboxens still call it is a different bug, IMHO.

Given that we already have filter.py, http://lxr.mozilla.org/mozilla/source/browser/locales/filter.py, which is supported by the (maintained) python version of compare-locales, I'd rather see us using something like that. We could even check that the output encoding works :-).

Forking the variable format between branch and trunk isn't too beautiful, do we really need that?

Comment 6

[Robert Strong (they/them - no direct email)]

Axel, I didn't fork it. I used it as a template for the additional checks I need. I'll convert it to py. Changing these strings in the locale files from a NSIS define to a NSIS variable really shouldn't be any worse than changing a string from what I can see especially since there are no changes to the branch and it really is needed.

Comment 7

[Robert Strong (they/them - no direct email)]

Axel, is compare-locales.pl still being used? It seems to be called quite a lot in the l10n trunk build logs.

Comment 8

[Basil Hashem [:baz]]

Sorry for the delay. Yes, indeed, it would be great to have the main text on the Welcome and Finish pages in the installer and uninstaller be customizable.

Comment 9

[(not reading, please use [e-mail] instead)]

from a sit down review with robert:

1)

+SURVEY_TEXT=&Tell us what you thought of $BrandShortName

This should be $BrandFullNameDA

2)

This is unused, and can be removed:

+APP_DESC=Required files for the $BrandShortName application

/browser/locales/en-US/installer/custom.properties, line 53 -- APP_DESC=Required files for the ${BrandShortName} application
/mail/locales/en-US/installer/custom.properties, line 53 -- APP_DESC=Required files for the ${BrandShortName} application
/calendar/locales/en-US/installer/custom.properties, line 52 -- APP_DESC=Required files for the ${BrandShortName} application

3)

+    foreach my $entity (@entities) {
+        if ($entity =~ m|.*(\$\(\^Name\)).*| ||
+            $entity =~ m|.*(\$\{BrandFullName\}).*|) {
+            push @invalid, "$entity\n    *** use \$BrandFullName in place of $1 and verify against en-US";
+        } elsif ($entity =~ m|.*(\$\(\^NameDA\)).*|) {
+            push @invalid, "$entity\n    *** use \$BrandFullNameDA in place of $1 and verify against en-US";
+        } elsif ($entity =~ m|.*(\$\{BrandShortName\}).*|) {
+            push @invalid, "$entity\n    *** use \$BrandShortName in place of $1 and verify against en-US";
+        }
+    }

To avoid potentially mislead localizers, Robert wants to fix this to be:

+    foreach my $entity (@entities) {
+        if ($entity =~ m|.*(\$\(\^Name\)).*| ||
+            $entity =~ m|.*(\$\{BrandFullName\}).*|
+            $entity =~ m|.*(\$\(\^NameDA\)).*|) {
+            $entity =~ m|.*(\$\{BrandShortName\}).*|) {
+            push @invalid, "$entity\n    *** verify against en-US";
+        }
+    }


4)

I'm not sure I understand Axel's comments about compare-locales vs. compare-locales.pl

I see that the python version (note, named compare-locales, not compare-locals.py) is used from setup.py,
but the perl version is used in all other places (http://lxr.mozilla.org/mozilla/search?string=compare-locales.pl)

http://mxr.mozilla.org/mozilla/source/testing/tests/l10n/scripts/compare-locales
http://mxr.mozilla.org/mozilla/source/testing/tests/l10n/setup.py#26

If the plan is to remove compare-locales.pl and switch to compare-locales (python), I think we should take that to a spin off bug.

Comment 10

[(not reading, please use [e-mail] instead)]

Comment on attachment 284799 [details] [diff] [review]
patch rev1

a v2 is coming from robert, so waiting for that to give the r=+

Comment 11

[Robert Strong (they/them - no direct email)]

(In reply to comment #8)
> Sorry for the delay. Yes, indeed, it would be great to have the main text on
> the Welcome and Finish pages in the installer and uninstaller be customizable.
Filed Bug 399921 for this

Comment 12

[Axel Hecht [:Pike]]

I filed bug 400010 to get tinderboxens moved over, too.

About compare-locales.pl, it has known bugs, and it's a nightmare to maintain. Copying code out there over to other places just makes life worse, in particular when it comes down to the parser code.

Comment 13

[Robert Strong (they/them - no direct email)]

Sounds good and it should be simple enough to get the python version of compare-locales to do the installer properties files in bug 4000010. As for the checking of entries in the 3 installer files I'll maintain that the same as I do the other perl script so it should not be an issue.

Comment 14

[Robert Strong (they/them - no direct email)]

Attachment: patch rev2 (requires patch from bug 399381 to apply cleanly)

Comment 15

[(not reading, please use [e-mail] instead)]

Comment on attachment 285253 [details] [diff] [review]
patch rev2 (requires patch from bug 399381 to apply cleanly)

r=sspitzer

Comment 16

[Robert Strong (they/them - no direct email)]

Comment on attachment 285253 [details] [diff] [review]
patch rev2 (requires patch from bug 399381 to apply cleanly)

Requesting a1.9 for this PRD item

Comment 17

[Robert Strong (they/them - no direct email)]

Attachment: unbitrotted patch

Comment 18

[Robert Strong (they/them - no direct email)]

Comment on attachment 285253 [details] [diff] [review]
patch rev2 (requires patch from bug 399381 to apply cleanly)

requesting approvalM9 for this P2 PRD Item.

btw: I looked at this earlier after the announcement of the new approvalM9 flag and the previous request had it... since then it went pack to approval1.9.

Comment 19

[Robert Strong (they/them - no direct email)]

Checked in to trunk

Checking in mozilla/browser/installer/windows/nsis/installer.nsi;
/cvsroot/mozilla/browser/installer/windows/nsis/installer.nsi,v  <--  installe                                                                                                                                 r.nsi
new revision: 1.39; previous revision: 1.38
done
Checking in mozilla/browser/installer/windows/nsis/uninstaller.nsi;
/cvsroot/mozilla/browser/installer/windows/nsis/uninstaller.nsi,v  <--  uninst                                                                                                                                 aller.nsi
new revision: 1.16; previous revision: 1.15
done
Checking in mozilla/browser/locales/Makefile.in;
/cvsroot/mozilla/browser/locales/Makefile.in,v  <--  Makefile.in
new revision: 1.56; previous revision: 1.55
done
Checking in mozilla/browser/locales/en-US/installer/custom.properties;
/cvsroot/mozilla/browser/locales/en-US/installer/custom.properties,v  <--  cus                                                                                                                                 tom.properties
new revision: 1.12; previous revision: 1.11
done
Checking in mozilla/browser/locales/en-US/installer/mui.properties;
/cvsroot/mozilla/browser/locales/en-US/installer/mui.properties,v  <--  mui.pr                                                                                                                                 operties
new revision: 1.3; previous revision: 1.2
done
Checking in mozilla/browser/locales/en-US/installer/override.properties;
/cvsroot/mozilla/browser/locales/en-US/installer/override.properties,v  <--  o                                                                                                                                 verride.properties
new revision: 1.2; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/mozapps/installer/windows/nsis/check-locale                                                                                                                                 s.pl,v
done
Checking in mozilla/toolkit/mozapps/installer/windows/nsis/check-locales.pl;
/cvsroot/mozilla/toolkit/mozapps/installer/windows/nsis/check-locales.pl,v  <-                                                                                                                                 -  check-locales.pl
initial revision: 1.1
done
Checking in mozilla/toolkit/mozapps/installer/windows/nsis/common.nsh;
/cvsroot/mozilla/toolkit/mozapps/installer/windows/nsis/common.nsh,v  <--  com                                                                                                                                 mon.nsh
new revision: 1.32; previous revision: 1.31
done