Closed Bug 399665 Opened 12 years ago Closed 12 years ago

Support replacing BrandShortName and BrandFullName in the installer

Categories

(Firefox :: Installer, defect, P2)

x86
Windows Vista
defect

Tracking

()

RESOLVED FIXED
Firefox 3 beta1

People

(Reporter: rstrong, Assigned: rstrong)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
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.
Attached patch patch rev1 (obsolete) — Splinter Review
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.
Attachment #284799 - Flags: review?(benjamin)
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.
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?
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.
Axel, is compare-locales.pl still being used? It seems to be called quite a lot in the l10n trunk build logs.
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.
Attachment #284799 - Flags: review?(benjamin) → review?(sspitzer)
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 on attachment 284799 [details] [diff] [review]
patch rev1

a v2 is coming from robert, so waiting for that to give the r=+
Attachment #284799 - Flags: review?(sspitzer)
(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
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.
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.
Attachment #285253 - Attachment description: patch rev2 → patch rev2 (requires patch from bug 399381 to apply cleanly)
Comment on attachment 285253 [details] [diff] [review]
patch rev2 (requires patch from bug 399381 to apply cleanly)

r=sspitzer
Attachment #285253 - Flags: review?(sspitzer) → review+
Comment on attachment 285253 [details] [diff] [review]
patch rev2 (requires patch from bug 399381 to apply cleanly)

Requesting a1.9 for this PRD item
Attachment #285253 - Flags: approval1.9?
Priority: -- → P2
Target Milestone: --- → Firefox 3 M9
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.
Attachment #285253 - Flags: approval1.9? → approvalM9?
Attachment #285253 - Flags: approvalM9?
Attachment #285253 - Flags: approvalM9+
Attachment #285253 - Flags: approval1.9+
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
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 432644
No longer depends on: 432644
You need to log in before you can comment on or make changes to this bug.