69433 - %foo% begone, summon %s

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[timeless]

Mail needs to be converted to use correct property foo.

Comment 1

[Peter "jag" Annema]

Before this gets closed as "timeless is high", let me explain on his behalf what
he probably means...

In some of the .properties files strings can be found like:

fooLabel=Foo: %bar% s

Then in JS code someone does

var myString = gFooBUndle.getString("fooLabel").replace(/%bar%/, someBar);

This should in most cases be changed to:

fooLabel=Foo: %S s

and:

var myString = gFooBUndle.getFormattedString("fooLabel", [someBar]);

This because usually %bar% is some English word which confuses L10N, who seem to
be used to %S. Another reason is that the %S version can easily be used from C++
(would the need ever arise) or any other language which can use nsIStringBundle,
where the %bar% version would require the user of the string to write the
language's equivalent of JS' |.replace()|, which for C++ for example is rather
involved.

Comment 2

[scottputterman]

marking nsbeta1- and moving to future milestone.  However, if owners of this
code don't mind have their placeholder names changes, someone else should feel
free to do this.

Comment 3

[Mark Olson]

Seth, I was working on a similar bug outside of mailnews.  If you want to give
this bug to me, I can take care of this.

Comment 4

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

thanks for the help, mark.

Comment 5

[Mark Olson]

Attachment: patch - replace .replace() with getFormattedString where appropriate

Comment 6

[Mark Olson]

So, there are still a few cases where .replace() is used incorrectly for what
looks like l12y, but they're against .dtd entities, which is odd.  My feeling is
that's another bug.

Comment 7

[timeless]

- .replace(/%emailProvider%/g, emailProvider)
- .replace(/%mailIDDesc%/g, emailIDDesc)
ack. sigh.

+accountName=%1$S - %2$S
I think %s - %s should work here
(it of course wouldn't work for the /g entries)

why change this:
-verboseFolderFormat=%S on %S

Comment 8

[Mark Olson]

> - .replace(/%emailProvider%/g, emailProvider)
> - .replace(/%mailIDDesc%/g, emailIDDesc)
> ack. sigh.

Please at least look at the stringbundle entry.  Those should be fine.

> +accountName=%1$S - %2$S
> I think %s - %s should work here
> (it of course wouldn't work for the /g entries)

Yes, %S would work, but with more than one replacement, might as well be
explicit, especially since l10n might rearrange things.

> why change this:
> -verboseFolderFormat=%S on %S

See above.

Comment 9

[timeless]

re /g Yeah i did, i know what they were doing, i just didn't like it [what you 
did was fine]

making the %s stuff explicit is probably not required, the translators know 
how to handle this stuff, right mkaply?

...
people are complaining about us wasting bytes in js and xul, so we probably 
shouldn't be wasteful in cases where we aren't even adding clarification....

Comment 10

[Mike Kaply [:mkaply]]

As long as the translation note above the item in the properties file indicated 
what the subsitution is, there is really no reason to actually spell out the 
name.

Just be clear that "%S will be replaced with servername" or "%1 will be replaced 
with email address" in the translation notes.

Comment 11

[Mark Olson]

Attachment: better localization notes, same functionality

Comment 12

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

there's more of the bad "%foo%" / replace stuff in mozilla/profile.

mark, can you wack that stuff as well?  I'll log a new bug on you for that.

Comment 13

[Mark Olson]

Seth, I already have another one of these, this was just for mailnews stuff
(although that doesn't seem obvious now)

Comment 14

[Peter "jag" Annema]

msgFolderPickerOverlay.js

you seem to be using 2 space indent in tab country...

r=jag

Seth, can you sr=?

Comment 15

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

looks good to me, but I'd like racham to review / test it since that's code he 
owns.

once racham gives it the thumbs up, sr=sspitzer

please wait until he gets a chance to look at it.

Comment 16

[racham]

These changes are good to go (reviewed & tested).

I am sure you will notice as you update that commondialog is deprecated and
promptService is in.

You have a change related to that in MsgComposeCommands.js.

thanks for helping on this one.

r=racham

Comment 17

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

sr=sspitzer

Comment 18

[Mark Olson]

Attachment: patch that got checked in - for historical purposes

Comment 19

[Mark Olson]

fix checked in

Comment 20

[[e-mail] (gone - use [e-mail] instead)]

Verified via bonsai/tinderbox.