Closed Bug 536739 Opened 10 years ago Closed 10 years ago

nsMsgPrompt should use FormatString instead of hand-rolling string insertion

Categories

(MailNews Core :: Composition, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: neil, Assigned: neil)

Details

(Keywords: intl)

Attachments

(1 file, 1 obsolete file)

nsMsgPrompt uses %P0% (and potentially %P1%) string insertions instead of the standard %S or %1$S and %2$S string insertions provided by string bundles.

Note: it is unfortunately not obviously possible to rename the strings.
Attached patch Proposed patch (obsolete) — Splinter Review
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #419161 - Flags: superreview?(bienvenu)
Attachment #419161 - Flags: review?(bugzilla)
Comment on attachment 419161 [details] [diff] [review]
Proposed patch

couldn't you rename the strings by changing the error code generated in nsMsgComposeStrings.h, and then use the new error codes in composeMsg.properties?
(In reply to comment #2)
> (From update of attachment 419161 [details] [diff] [review])
> couldn't you rename the strings by changing the error code generated in
> nsMsgComposeStrings.h, and then use the new error codes in
> composeMsg.properties?
So I could change them to 12594/5 for instance?
iirc it has been said before that l10n don't like just numbers. How much would a lookup table cost us so that we could use proper strings?
Comment on attachment 419161 [details] [diff] [review]
Proposed patch

Ok, based on the conversation on mozilla.dev.l10n and the fact no-one objected to Simon's suggestion. I'm happy to give r+ to this on the basis that you co-ordinate with him for updating the strings in all the locales repositories.

>+12500=Unable to open the file %S.
>+12501=Unable to open the temporary file %S. Check your 'Temporary Directory' setting.

For each of these we should have a localisation note clarifying what %S will get replaced with.

I've thought about a test for this, but I don't see there's anything sensible as these seem to be error cases so I think its not worth trying to write the a test in this case. Although I'll let David override me if he thinks otherwise.
Attachment #419161 - Flags: review?(bugzilla) → review+
AFAICT, the code is using symbolic names for the lookup in the code anyway. The numbers should really die.

Note that not all localizations out there are in our repos, at least primarily. narro-hosted localizations for example won't import the change, and break. As will upcoming localizations that are not yet in our eco-system at all.

I strongly suggest to go with real names, and to turn the defines into literals.
(In reply to comment #6)
> AFAICT, the code is using symbolic names for the lookup in the code anyway. 
> The numbers should really die.
> 
> Note that not all localizations out there are in our repos, at least 
> primarily. narro-hosted localizations for example won't import the change, 
> and break. As will upcoming localizations that are not yet in our 
> eco-system at all.
> 
> I strongly suggest to go with real names, and to turn the defines into
> literals.

I'd strongly support this route.
Attached patch Revised patchSplinter Review
OK, so in this version I split up nsMsgBuildErrorMessageByID;
nsMsgGetMessageByID returns an unformatted message using the existing codes,
nsMsgBuildMessageWithFile returns a "Cannot open file %S" message, while
nsMsgBuildMessageWithTmpFile returns a "Cannot open temporary file %S" message. The latter two functions are just stubs that call an internal nsMsgBuildMessageByName function passing in an appropriate unichar string.
(I wanted to avoid flag-passing or duplicating the literal string.)
Attachment #419161 - Attachment is obsolete: true
Attachment #426980 - Flags: superreview?(bienvenu)
Attachment #426980 - Flags: review?(bugzilla)
Attachment #419161 - Flags: superreview?(bienvenu)
Comment on attachment 426980 [details] [diff] [review]
Revised patch

>diff -r 36643bb71021 mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
>--- a/mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties	Sun Feb 14 19:27:16 2010 +0100
>+++ b/mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties	Mon Feb 15 13:46:45 2010 +0000
>@@ -38,10 +38,10 @@
> # The following are used by the compose back end
> #
> ## @name NS_MSG_UNABLE_TO_OPEN_FILE 
>-12500=Unable to open the file %P0%.
>+unableToOpenFile=Unable to open the file %S.
> 
> ## @name NS_MSG_UNABLE_TO_OPEN_TMP_FILE
>-12501=Unable to open the temporary file %P0%. Check your 'Temporary Directory' setting.
>+unableToOpenTmpFile=Unable to open the temporary file %S. Check your 'Temporary Directory' setting.

These should have proper localisation notes with them. e.g.

## LOCALIZATION NOTE (unableToOpenFile): %S is the name of the file that is being attempted to be opened.

r=Standard8 with that fixed (in 4 places).
Attachment #426980 - Flags: review?(bugzilla) → review+
Attachment #426980 - Flags: superreview?(bienvenu) → superreview+
Pushed changeset 3a182a52feb5 to comm-central.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.