344148 - Leading/trailing whitespace in override.properties should be removed according to the spec of the properties files.

Status: RESOLVED FIXED

(Firefox :: Installer, defect)

Description

[dynamis (Tomoya ASAI)]

In the override.properties file, we use double quote to define strings that require a space at the end:
http://lxr.mozilla.org/mozilla1.8/source/browser/locales/en-US/installer/override.properties
 53 # Strings that require a space at the end should be enclosed with double
 54 # quotes and the double quotes will be removed. To add quotes to the beginning
 55 # and end of a strong enclose the add and additional double quote to the
 56 # beginning and end of the string (e.g. ""This will include quotes"").

This works correctly for en-US and other builds but concerning ja locale, double quote is not removed and appear in the installer UI.

As far as I checked, my japanese override.properties is correctly written:
http://lxr.mozilla.org/l10n-mozilla1.8/source/ja/browser/installer/override.properties
but in the installer, double quotes are not removed.

Comment 1

[dynamis (Tomoya ASAI)]

Attachment: Installer screenshot which includes double quote.

Screenshot of Japanese Firefox Installer (mozilla 1.8 branch).
At the bottom of this page, in en-US you can see like:
  Space required: 17.2MB
  Space available: 9.4GB
without any quotes but in the screenshot, double quotes are remain.

Comment 2

[dynamis (Tomoya ASAI)]

A little late but I found one more thins relating this now.
In the screenshot (attachment 228710 [details]), units of file size (MB/GB) are also incorrect. They should be "MB" or "GB" but actually "M B" or "G B".

I think that this is from the difference of the format.
ja locale files are containing space after "=" to make things clear:
http://lxr.mozilla.org/l10n-mozilla1.8/source/ja/browser/installer/override.properties#118
  118 Byte                            = B
  119 Kilo                            = K
  120 Mega                            = M
  121 Giga                            = G
On the otehr hand en-US don't contain any whitespace around "="
http://lxr.mozilla.org/mozilla1.8/source/browser/locales/en-US/installer/override.properties#115
  115 Byte=B
  116 Kilo=K
  117 Mega=M
  118 Giga=G

So, I think override.properties file is not "properties file", in which all the whitespace around "=" will be ignored.

Then, maybe if I remove all space around "=", japanese installer will be fine.
# I'm not sure yet but proberbably.

But if override.properties file is not "properties file" and it will treat all the string including space after "=" as a part of the entity, why you remove double quote (as you write comment in the file)?
That is, I think defining with double quote and removing it before using is because properties file will remove heading/tailing spaces. If heading space will be in part of the entity, there is no reason to do like that.

Now, our options are:
  A. Remove whitespace from ja override.properties file. I'm not sure this will work correctlly but maybe.
OR
  B. Fix the treatment of override.properties file (same as other properties file).

IMHO ideally, to avoid bohering localizer like me, we should select option B, but if you don't have enough time to fix this, I will just try option A.

Should I wait fixing or try option A right now?
# in case no answer will come, I'll try option A.

Comment 3

[dynamis (Tomoya ASAI)]

FYI:
(In reply to comment #1)
> Screenshot of Japanese Firefox Installer (mozilla 1.8 branch).
> At the bottom of this page, in en-US you can see like:
>   Space required: 17.2MB
>   Space available: 9.4GB
> without any quotes but in the screenshot, double quotes are remain.

Following entities are used here:
http://lxr.mozilla.org/mozilla1.8/source/browser/locales/en-US/installer/override.properties#80
  80 SpaceAvailable="Space available: "
  81 SpaceRequired="Space required: "
http://lxr.mozilla.org/l10n-mozilla1.8/source/ja/browser/installer/override.properties#83
  83 SpaceAvailable                  = "空き容量: "
  84 SpaceRequired                   = "必要な容量: "

And in the screenshot, quotes of SpaceAvailable and SpaceRequired are remained.


And, override.properties file is read here:
http://lxr.mozilla.org/mozilla1.8/source/toolkit/mozapps/installer/windows/nsis/preprocess-locale.pl#115
And file format will be changed here:
http://lxr.mozilla.org/mozilla1.8/source/toolkit/mozapps/installer/windows/nsis/preprocess-locale.pl#102
102 while( $line = <infile> ) {
103   $line =~ s/[\r\n]*//g;    # remove \r and \n
104   next if ($line =~ m|^#|);
105   my @values = split('=', $line, 2);
106   next if (@values[0] eq undef) || (@values[1] eq undef);
107   my $value = @values[1];
108   $value =~ s/^"(.*)"$/$1/g; # remove " at the beginning and end of the value
109   $value =~ s/(")/\$\\$1/g;  # prefix " with $\
110   print outfile "LangString  ^@values[0] $langID \"$value\"\r\n";
111   $lnum++;
112 }

So, if we change line 108 like the following, override.properties will be treated same as other properties files.
-$value =~ s/^"(.*)"$/$1/g; # remove " at the beginning and end of the value
+$value =~ s/^\s*"(.*)"\s*$/$1/g; # remove whitespace and " at the beginning and end of the value

Comment 4

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

Does it work if you don't add spaces before and after the equal sign?

Comment 5

[dynamis (Tomoya ASAI)]

Attachment: patch to remove heading/tailing whitespaces

This is the patch to fix preprocess-locale.pl as I wrote previous comment.
Sorry but I don't have build environment and I cannot confirm this by myself.

Comment 6

[dynamis (Tomoya ASAI)]

(In reply to comment #4)
> Does it work if you don't add spaces before and after the equal sign?

As far as I read the code, YES.
# Sorry but I don't have build environment and I cannot check by myself... :(

Comment 7

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

.properties files typically aren't formatted as name    = value. The space after the equal sign would break the regexp to remove the double quote and I highly suspect that just formatting them as name=value as is done in the other locales properties files would fix this for you.

Comment 8

[dynamis (Tomoya ASAI)]

(In reply to comment #7)
> .properties files typically aren't formatted as name    = value. The space
> after the equal sign would break the regexp to remove the double quote and I
> highly suspect that just formatting them as name=value as is done in the other
> locales properties files would fix this for you.

I know typically aren't formatted as name = value but as a specification of the properties file, it's allowed and easy to read. After I read the code, I also hightly suspect that formatting as name=value will fix for me. But this behavior is not standard properties file and a little confusing (with comment about removing quote and without comment about no whitespaces allowed).

The thing I care about now is: it's just confusing as long as the file name is *.properties. My quite simple patch will fix the regexp to remove the double quote and then, things will be more clear and no more localizer will be confused. I think it's better to fix regexp for the future.
# I Changed the summary.

If you or the owner of preprocess-locale.pl don't agree, I'll just change the format of override.properties as name=value.

Comment 9

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

I'll take a look after beta 1 is finished. Though it is most likely safe there isn't time currently to verify this change wouldn't cause other problems.

Comment 10

[dynamis (Tomoya ASAI)]

Attachment: patch to remove heading/tailing whitespaces

Previous patch only trim whitespaces when the value is double-quoted.
This patch always trim heading/ending whitespaces, which is the behavior of the standard *.properties files.
This patch allow not only the format like name = "value ", but also allow format like:
foo=bar  
    ~~~~~
This is the case that localizer left whitespaces by some sccident. 
Localizers expect the value of foo will be "bar" but without this patch, it will be "bar  ".

Comment 11

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

Asai Tomoya, does the last patch cover all the conditions you want?

Comment 12

[dynamis (Tomoya ASAI)]

(In reply to comment #11)
> Asai Tomoya, does the last patch cover all the conditions you want?

Yes.
Last patch make override.properties meet specification of *.properties (any heading/tailing spaces of value will be ignored) and cover all the conditions I want.

Comment 13

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

Attachment: Patch from Asai Tomoya w/ updated comments

Asai Tomoya, thanks for the patch. I'll request 1.8.1 on this after the Tinderbox cycles a couple of times.

Comment 14

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

Checked in on trunk

Comment 15

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

Comment on attachment 230670 [details] [diff] [review]
Patch from Asai Tomoya w/ updated comments

Requesting 1.8.1 - better support for properties file formats when preprocessing the installer locale files

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 230670 [details] [diff] [review]
Patch from Asai Tomoya w/ updated comments

a=drivers. Please land this on the MOZILLA_1_8_BRANCH.

Comment 17

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

Checked in to MOZILLA_1_8_BRANCH