Review NS_* definitions in nsComposeStrings.h

RESOLVED FIXED in Thunderbird 31.0

Status

MailNews Core
Composition
--
minor
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: standard8, Assigned: sshagarwal)

Tracking

(Blocks: 1 bug)

Trunk
Thunderbird 31.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=aceman][lang=C++])

Attachments

(2 attachments, 12 obsolete attachments)

8.56 KB, patch
Magnus Melin
: review+
neil@parkwaycc.co.uk
: review+
aceman
: feedback+
Details | Diff | Splinter Review
48.57 KB, patch
sshagarwal
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
nsComposeStrings.h has a lot of defines in it, and I think some can be removed or simplified.

There are two basic categories that I think we can address:

- Unused definitions
-- e.g. NS_MSG_CANT_POST_TO_MULTIPLE_NEWS_HOSTS
--- This isn't used anywhere, however it also needs its associated string (12507) removing from composeMsgs.properties
-- I think there are several in this category.

- String-only definitions
-- e.g. NS_MSG_ASSEMB_DONE_MSG
--- This needs the define removing, the call to GetStringFromID changing to GetStringFromName, and then the number (12508) replacing by an actual string, e.g. msgAssemblyDoneAlert
- I think the ones that get passed to UpdateStatus could also be changed from a number to a name as well.

Once we've addressed those, we can then think about the other values in that file, but we can do them in a follow-up bug.
(Reporter)

Updated

6 years ago
Blocks: 783526

Updated

5 years ago
Whiteboard: [good first bug][mentor=aceman][lang=C++]

Comment 1

5 years ago
For the second part where the string IDs need to be changed, see how it is done in bug 858238. Also check if this does not clash with the changes in that bug (meaning you would both change the same spots). If yes, you will need to wait until bug 858238 is checked in.

Comment 2

5 years ago
Hi. I would like to take this and have a patch in line for the first part of the bug.

Updated

5 years ago
Assignee: nobody → m.o.m.o

Comment 3

5 years ago
Created attachment 755433 [details] [diff] [review]
bug_802266_patch_1_removes_unused_definitions.patch
Attachment #755433 - Flags: feedback?(acelists)

Updated

5 years ago
Attachment #755433 - Attachment description: Patch 1 → bug_802266_patch_1_removes_unused_definitions.patch

Comment 4

5 years ago
Created attachment 755435 [details] [diff] [review]
bug_802266_patch_2_removes_superfluous_definitions.patch
Attachment #755435 - Flags: feedback?(acelists)

Updated

5 years ago
Attachment #755435 - Attachment description: Patch 2 → bug_802266_patch_2_removes_superfluous_definitions.patch

Comment 5

5 years ago
Ok, the patches must be applied in the order as specified (patch 1 and patch 2).
Status: NEW → ASSIGNED

Comment 6

5 years ago
Comment on attachment 755433 [details] [diff] [review]
bug_802266_patch_1_removes_unused_definitions.patch

Review of attachment 755433 [details] [diff] [review]:
-----------------------------------------------------------------

Please remove r=aceman, I am not a reviewer. I just mentor you to create the patch and then we send it to a real reviewer. But it seems you managed to do the patches alone, so good work there! ;)

Can you also remove the same strings from suite/locales/en-US/chrome/mailnews/compose/composeMsgs.properties ?
Attachment #755433 - Flags: feedback?(acelists) → feedback+

Comment 7

5 years ago
Comment on attachment 755435 [details] [diff] [review]
bug_802266_patch_2_removes_superfluous_definitions.patch

Review of attachment 755435 [details] [diff] [review]:
-----------------------------------------------------------------

Can you also change the same strings in suite/locales/en-US/chrome/mailnews/compose/composeMsgs.properties ?

I have run TB tests on this and they do pass.
The changes are fine but there are some small changes to be done yet.

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +165,5 @@
> +genericFailureExplanation=Please verify that your Mail & Newsgroups account settings are correct and try again.
> +
> +## @name NS_MSG_UNDISCLOSED_RECIPIENTS
> +## LOCALIZATION NOTE: this string must be using only US_ASCII characters
> +undisclosedRecipients=undisclosed-recipients

Can you remove all the '@name' lines from the strings you change?

@@ +237,5 @@
> +smtpDeliveringMail=Delivering mail…
> +smtpMailSent=Mail sent successfully
> +
> +assemblingMailInformation=Assembling mail information…
> +gatheringAttachment=Attaching %s…

Can you add a '## LOCALIZATION NOTE' describing what the %s is/will become?

@@ +240,5 @@
> +assemblingMailInformation=Assembling mail information…
> +gatheringAttachment=Attaching %s…
> +creatingMailMessage=Creating mail message…
> +
> +copyMessageStart=Copying message to %S folder…

Can you add a '## LOCALIZATION NOTE' describing what the %S is/will become?

@@ +255,5 @@
> +saveDraftErrorTitle=Save Draft Error
> +saveTemplateErrorTitle=Save Template Error
> +
> +failureOnObjectEmbeddingWhileSaving=There was a problem including the file %.200s in the message. Would you like to continue saving the message without this file?
> +failureOnObjectEmbeddingWhileSending=There was a problem including the file %.200s in the message. Would you like to continue sending the message without this file?

Can you add a '## LOCALIZATION NOTE' describing what the %.200s is/will become?

@@ +398,5 @@
>  ## LOCALIZATION NOTE(stopShowingUploadingNotification): This string is used in the Filelink
>  ## upload notification bar to allow the user to dismiss the notification permanently.
>  stopShowingUploadingNotification.accesskey=N
>  stopShowingUploadingNotification.label=Never show this again
> +

Why adding this line?

::: mailnews/compose/src/nsMsgAttachmentHandler.cpp
@@ +1045,2 @@
>      else
> +      bundle->GetStringFromName(NS_LITERAL_STRING("failureOnObjectEmbeddingWhileSending").get(), getter_Copies(msg));

Please wrap long lines to not cross 80 chars.
You can wrap like this (align arguments after bracket):
long_expression_and_function(argument1, argument2
                             argument3, ...)

And if that is not possible then (2 space indent):
long_expression_and_function(argument1, argument2
  argument3, ...)

::: mailnews/compose/src/nsMsgSend.cpp
@@ +3591,5 @@
>      GetNotificationCallbacks(getter_AddRefs(callbacks));
>  
>      // Tell the user we are sending the message!
>      nsString msg;
> +    mComposeBundle->GetStringFromName(NS_LITERAL_STRING("Sending message…").get(), getter_Copies(msg));

The string ID here, not the real text.

::: mailnews/compose/src/nsMsgSendReport.cpp
@@ +214,5 @@
>  NS_IMETHODIMP nsMsgSendReport::DisplayReport(nsIPrompt *prompt, bool showErrorOnly, bool dontShowReportTwice, nsresult *_retval)
>  {
> +  NS_NAMED_LITERAL_STRING(genericFailureExplanationMsg, "genericFailureExplanation");
> +  NS_NAMED_LITERAL_STRING(sendErrorTitleMsg, "sendMessageErrorTitle");
> +

Why are you adding these variables? In the previous files you put the strings directly into GetStringFromName() . And you do the same later on.

@@ +385,2 @@
>      bundle->GetStringFromID(NS_ERROR_GET_CODE(preStrId),
>                              getter_Copies(dialogMessage));

Good, we will have a separate bug for converting the rest of the strings.

::: mailnews/compose/src/nsSmtpProtocol.cpp
@@ +434,5 @@
>  /////////////////////////////////////////////////////////////////////////////////////////////
>  // End of nsIStreamListenerSupport
>  //////////////////////////////////////////////////////////////////////////////////////////////
>  
> +void nsSmtpProtocol::UpdateStatus(const char* aStatusMsg)

'*Msg' may mislead into thinking this should be the text to display (as in UpdateStatusWithString). Try aStatusID or aStatusName.
Attachment #755435 - Flags: feedback?(acelists)
(Assignee)

Comment 8

5 years ago
Moritz, are you working, or will you be getting back to this?
Flags: needinfo?(m.o.m.o)

Comment 9

5 years ago
Yes, definitely. Except a revised patch some time this weekend.
Flags: needinfo?(m.o.m.o)
(Assignee)

Comment 10

5 years ago
Moritz are you still working on this?
Flags: needinfo?(m.o.m.o)

Comment 11

5 years ago
(In reply to :aceman from comment #7)
> ::: mailnews/compose/src/nsMsgSendReport.cpp
> @@ +214,5 @@
> >  NS_IMETHODIMP nsMsgSendReport::DisplayReport(nsIPrompt *prompt, bool showErrorOnly, bool dontShowReportTwice, nsresult *_retval)
> >  {
> > +  NS_NAMED_LITERAL_STRING(genericFailureExplanationMsg, "genericFailureExplanation");
> > +  NS_NAMED_LITERAL_STRING(sendErrorTitleMsg, "sendMessageErrorTitle");
> > +
> 
> Why are you adding these variables? In the previous files you put the
> strings directly into GetStringFromName() . And you do the same later on.

I could not find the quoted changes in the patch.

> @@ +385,2 @@
> >      bundle->GetStringFromID(NS_ERROR_GET_CODE(preStrId),
> >                              getter_Copies(dialogMessage));
> 
> Good, we will have a separate bug for converting the rest of the strings.
> 
> ::: mailnews/compose/src/nsSmtpProtocol.cpp
> @@ +434,5 @@
> >  /////////////////////////////////////////////////////////////////////////////////////////////
> >  // End of nsIStreamListenerSupport
> >  //////////////////////////////////////////////////////////////////////////////////////////////
> >  
> > +void nsSmtpProtocol::UpdateStatus(const char* aStatusMsg)
> 
> '*Msg' may mislead into thinking this should be the text to display (as in
> UpdateStatusWithString). Try aStatusID or aStatusName.

So keep as was?
> void nsSmtpProtocol::UpdateStatus(int32_t aStatusID)
>    bundle->GetStringFromID(aStatusID, getter_Copies(msg));

Comment 12

5 years ago
Created attachment 8337306 [details] [diff] [review]
bug802266-1-cleanup-strings
Assignee: m.o.m.o → buchner.johannes
Attachment #755433 - Attachment is obsolete: true
Attachment #8337306 - Flags: review?(acelists)
Flags: needinfo?(m.o.m.o)

Comment 13

5 years ago
Created attachment 8337307 [details] [diff] [review]
bug802266-2-by-name
Attachment #755435 - Attachment is obsolete: true
Attachment #8337307 - Flags: review?(acelists)

Comment 14

5 years ago
Created attachment 8337319 [details] [diff] [review]
bug802266-1-cleanup-strings
Attachment #8337306 - Attachment is obsolete: true
Attachment #8337306 - Flags: review?(acelists)
Attachment #8337319 - Flags: review?(acelists)

Comment 15

5 years ago
Created attachment 8337320 [details] [diff] [review]
bug802266-2-by-name

Nevermind the second part of my question in Comment 11.
Attachment #8337307 - Attachment is obsolete: true
Attachment #8337307 - Flags: review?(acelists)
Attachment #8337320 - Flags: review?(acelists)

Comment 16

5 years ago
Comment on attachment 8337319 [details] [diff] [review]
bug802266-1-cleanup-strings

Review of attachment 8337319 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this looks fine to me. Let's forward to real reviewers.
Attachment #8337319 - Flags: review?(neil)
Attachment #8337319 - Flags: review?(mkmelin+mozilla)
Attachment #8337319 - Flags: review?(acelists)
Attachment #8337319 - Flags: feedback+

Comment 17

5 years ago
Comment on attachment 8337320 [details] [diff] [review]
bug802266-2-by-name

Review of attachment 8337320 [details] [diff] [review]:
-----------------------------------------------------------------

This also looks fine to me. I see you didn't touch IDs like 12503 that are used as both error numbers and string IDs. I'm OK if that is done later (may be a new bug) after this patch is checked in, as those need more massaging.

I give preliminary f+ (if you fix the nits below) but I haven't yet compiled this so I'll confirm that later.

Also please remove r=aceman from the patch header and nice that you keep the original author mentioned.

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +243,5 @@
> +## LOCALIZATION NOTE: argument is file name/URI of attachment
> +gatheringAttachment=Attaching %s…
> +creatingMailMessage=Creating mail message…
> +
> +## LOCALIZATION NOTE: argument is folder name

Please write "argument %S is folder name".

@@ +262,5 @@
> +## LOCALIZATION NOTE: argument is file name/URI of object to be embedded
> +failureOnObjectEmbeddingWhileSaving=There was a problem including the file %.200s in the message. Would you like to continue saving the message without this file?
> +## LOCALIZATION NOTE: argument is file name/URI of object to be embedded
> +failureOnObjectEmbeddingWhileSending=There was a problem including the file %.200s in the message. Would you like to continue sending the message without this file?
> +returnToComposeWindowQuestion=Would you like to return to the compose window?

These same changes need to be done in the suite composeMsgs.properties file too.

::: mailnews/compose/src/nsMsgSend.cpp
@@ +570,5 @@
>  
>    NS_ASSERTION (m_attachment_pending_count == 0, "m_attachment_pending_count != 0");
>  
> +  mComposeBundle->GetStringFromName(NS_LITERAL_STRING("messageAssembling").get(),
> +		  getter_Copies(msg));

Please align getter_Copies under NS_LITERAL_STRING (after bracket). See comment 7.

@@ +920,5 @@
>    }
>  
>    // Tell the user we are creating the message...
> +  mComposeBundle->GetStringFromName(
> +    NS_LITERAL_STRING("creatingMailMessage").get(), getter_Copies(msg));

You seem to use different alignment in the same file for the same function call (mComposeBundle->GetStringFromName). Could you consolidate it into a single way?

@@ +963,5 @@
>      }
>    }
>  
> +  mComposeBundle->GetStringFromName(NS_LITERAL_STRING("messageAssemblingDone").get(),
> +		  getter_Copies(msg));

Please align getter_Copies under NS_LITERAL_STRING (after bracket).
Attachment #8337320 - Flags: review?(acelists) → feedback+

Comment 18

5 years ago
Thanks for trying to finish this patch!
Severity: normal → minor
Keywords: helpwanted

Comment 19

5 years ago
Created attachment 8337650 [details] [diff] [review]
bug802266-2-by-name

My motivation to work on this bug is decreasing ... it's not particularly interesting.
Attachment #8337320 - Attachment is obsolete: true
Attachment #8337650 - Flags: review?(neil)
Attachment #8337650 - Flags: review?(mkmelin+mozilla)

Comment 20

5 years ago
Comment on attachment 8337650 [details] [diff] [review]
bug802266-2-by-name

Review of attachment 8337650 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, this is not a very interesting bug :) But it is easy as a starting bug.

The changes in suite/locales/en-US/chrome/mailnews/compose/composeMsgs.properties should have been the same as to 
mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties, not adding different localization notes.
Comment on attachment 8337319 [details] [diff] [review]
bug802266-1-cleanup-strings

rs=me assuming this compiles.
Attachment #8337319 - Flags: review?(neil) → review+

Comment 22

5 years ago
Applying both patches does compile for me.

Comment 23

5 years ago
Comment on attachment 8337650 [details] [diff] [review]
bug802266-2-by-name

Review of attachment 8337650 [details] [diff] [review]:
-----------------------------------------------------------------

::: suite/locales/en-US/chrome/mailnews/compose/composeMsgs.properties
@@ +306,1 @@
>  mailnews.reply_header_authorwrote=%s wrote

there's no $s there ;)
(same thing with the one below)
Attachment #8337650 - Flags: review?(mkmelin+mozilla) → review+

Updated

5 years ago
Attachment #8337319 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 8337650 [details] [diff] [review]
bug802266-2-by-name

>+  mComposeBundle->GetStringFromName(NS_LITERAL_STRING("messageAssembling").get(),
>+		                            getter_Copies(msg));
Nit: Always align the arguments i.e. the g should be below the N.
Note: Compiler support has improved to the point where we might be able to use MOZ_UTF16("messageAssembling") instead of NS_LITERAL_STRING(...).get() although I posted a request to dev-platform just to be sure. (The advantage is that it's just a string literal so you can do stuff like assign it to a const char16_t* pointer.)

>-    bundle->GetStringFromID(titleID, getter_Copies(dialogTitle));
>+    bundle->GetStringFromName(title.get(), getter_Copies(dialogTitle));
>     bundle->GetStringFromID(NS_ERROR_GET_CODE(preStrId),
>                             getter_Copies(dialogMessage));
Any reason you changed the title but not the message?
Comment on attachment 8337650 [details] [diff] [review]
bug802266-2-by-name

>+    void UpdateStatus(const char * aStatusName);
Looks like MOZ_UTF16 is good to go these days, in which case you can also fix this to be const char16_t *.
(Assignee)

Comment 26

4 years ago
There are just a few changes left (I think). Are you going to complete this?
Flags: needinfo?(buchner.johannes)

Comment 27

4 years ago
Sorry, no. I stowed away my Moz-dev directory and am involved in other things. Feel free to take it.
Status: ASSIGNED → NEW
Flags: needinfo?(buchner.johannes)
Comment on attachment 8337650 [details] [diff] [review]
bug802266-2-by-name

In that case, setting review- so this doesn't get accidentally checked in.
Attachment #8337650 - Flags: review?(neil) → review-
(Assignee)

Comment 29

4 years ago
Created attachment 8389246 [details] [diff] [review]
But802266 by name

(In reply to neil@parkwaycc.co.uk from comment #24)
> >-    bundle->GetStringFromID(titleID, getter_Copies(dialogTitle));
> >+    bundle->GetStringFromName(title.get(), getter_Copies(dialogTitle));
> >     bundle->GetStringFromID(NS_ERROR_GET_CODE(preStrId),
> >                             getter_Copies(dialogMessage));
> Any reason you changed the title but not the message?

preStrId is nsresult, so changing it will further require changing a few other function calls and definitions and return types as well.
E.g. http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCopy.cpp#181

Should I change it too?
Attachment #8337650 - Attachment is obsolete: true
Attachment #8389246 - Flags: feedback?(acelists)

Comment 30

4 years ago
I think this patch is already large enough.
We ultimately want to convert all numeric IDs in composeMsgs.properties to string IDs. However, those that are used as a return value and also as an error message ID need different conversions. So I think those can be done in a followup bug as standard8 suggests in comment 0.
Neil, would you be fine with that?
Flags: needinfo?(neil)
Comment on attachment 8389246 [details] [diff] [review]
But802266 by name

>-    int32_t titleID;
>+    nsString title;
const char16_t* perhaps?

(In reply to Suyash Agarwal from comment #29)
> (In reply to comment #24)
> > >-    bundle->GetStringFromID(titleID, getter_Copies(dialogTitle));
> > >+    bundle->GetStringFromName(title.get(), getter_Copies(dialogTitle));
> > >     bundle->GetStringFromID(NS_ERROR_GET_CODE(preStrId),
> > >                             getter_Copies(dialogMessage));
> > Any reason you changed the title but not the message?
> 
> preStrId is nsresult

Ah, sorry, I'd overlooked that.

(In reply to aceman from comment #30)
> Neil, would you be fine with that?

Yes, that's fine.
(Assignee)

Comment 32

4 years ago
Created attachment 8389648 [details] [diff] [review]
Bug802266 by name v1.2
Attachment #8389246 - Attachment is obsolete: true
Attachment #8389246 - Flags: feedback?(acelists)
Attachment #8389648 - Flags: feedback?(acelists)
(Assignee)

Comment 33

4 years ago
Comment on attachment 8389648 [details] [diff] [review]
Bug802266 by name v1.2

Review of attachment 8389648 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/compose/src/nsMsgSendReport.cpp
@@ +378,5 @@
>  
>      switch (mDeliveryMode)
>      {
>        case nsIMsgCompDeliverMode::Later:
> +        title = MOZ_UTF16("sendLaterErrorTitle");

While compiling, I got warnings that this is a deprecated conversion. Is there another way by which I can convert const char* to const char16_t* ?

Comment 34

4 years ago
Comment on attachment 8389648 [details] [diff] [review]
Bug802266 by name v1.2

Review of attachment 8389648 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like this is heavily bitrotted. It didn't apply to any of the files.
Attachment #8389648 - Flags: feedback?(acelists)
(Assignee)

Comment 35

4 years ago
(In reply to :aceman from comment #34)
> Comment on attachment 8389648 [details] [diff] [review]
> Bug802266 by name v1.2
> 
> Review of attachment 8389648 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks like this is heavily bitrotted. It didn't apply to any of the files.

It applies cleanly for me on the updated tree.
I also applied "cleanup strings" patch before working on this patch, so maybe that's why it failed for you. Applying both the patches works.
Please let me know if that fails too.

Thanks.
Flags: needinfo?(neil)

Comment 36

4 years ago
Comment on attachment 8389648 [details] [diff] [review]
Bug802266 by name v1.2

Review of attachment 8389648 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, sorry, I missed the first patch :)

The patch looks fine to me, but I wonder about the missing Seamonkey conversion...

::: mailnews/compose/src/nsMsgCompUtils.cpp
@@ +630,5 @@
>            rv = stringService->CreateBundle("chrome://messenger/locale/messengercompose/composeMsgs.properties", getter_AddRefs(composeStringBundle));
>            if (NS_SUCCEEDED(rv))
>            {
>              nsString undisclosedRecipients;
> +            rv = composeStringBundle->GetStringFromName(MOZ_UTF16("undisclosedRecipients"), 

Trailing space.

::: mailnews/compose/src/nsMsgSend.cpp
@@ +3976,2 @@
>    else
> +    mComposeBundle->GetStringFromName(MOZ_UTF16("copyMessageFailed"), getter_Copies(msg));

In the first string you preserved the word "start", in the second you didn't. Shouldn't it be consistent?

::: mailnews/compose/src/nsMsgSendReport.cpp
@@ +300,5 @@
>        mAlreadyDisplayReport = true;
>        return NS_OK;
>      }
>  
> +    bundle->GetStringFromName(MOZ_UTF16("sendErrorTitleMsg"), 

Trailing space.

::: mailnews/compose/src/nsSmtpProtocol.cpp
@@ +442,5 @@
>      nsresult rv = bundleService->CreateBundle("chrome://messenger/locale/messengercompose/composeMsgs.properties", getter_AddRefs(bundle));
>      if (NS_FAILED(rv)) return;
>      nsString msg;
> +    bundle->GetStringFromName(aStatusName,
> +                              getter_Copies(msg));

Didn't this fit on a single line?

::: suite/locales/en-US/chrome/mailnews/compose/composeMsgs.properties
@@ +57,3 @@
>  
>  ## @name SMTP_DELIV_MAIL
>  12521=Delivering mail…

So Seamonkey does not get the conversion to string names? This looks wrong.
Attachment #8389648 - Flags: feedback?(acelists)
(Assignee)

Comment 37

4 years ago
Created attachment 8391063 [details] [diff] [review]
Bug802266 by name v2

Oh sorry, I didn't look at that, I thought the patch was almost complete :P
Attachment #8389648 - Attachment is obsolete: true
Attachment #8389648 - Flags: feedback?(acelists)
Attachment #8391063 - Flags: feedback?(acelists)

Comment 38

4 years ago
Comment on attachment 8391063 [details] [diff] [review]
Bug802266 by name v2

Review of attachment 8391063 [details] [diff] [review]:
-----------------------------------------------------------------

Looks better now :)
Attachment #8391063 - Flags: review?(neil)
Attachment #8391063 - Flags: feedback?(acelists)
Attachment #8391063 - Flags: feedback+
Comment on attachment 8391063 [details] [diff] [review]
Bug802266 by name v2

>Bug 802266 - Removes definitoins with ids (numbers) in nsComposeStrings.h that don't need it. Instead, the messages are now
>accessed by name which makes them far more convenient to use.
Typo: definitions
Also, note that Mercurial outputs the first line of a commit message in short logs so you should wrap at a more appropriate place (i.e. at the end of a sentence).

>+genericFailureExplanation=Please verify that your Mail & Newsgroups account settings are correct and try again.
The code that refers to this string uses the wrong name.

>+messageAssemblingDone=Assembling message…Done
>+messageAssembling=Assembling message…
The original #define contained MSG twice which may have been confusing; these should be switched to assemblingMessage[Done].

>+sendMessageErrorTitle=Send Message Error
The code that refers to this string uses the wrong name.

Comment 40

4 years ago
(In reply to neil@parkwaycc.co.uk from comment #39)
> >+genericFailureExplanation=Please verify that your Mail & Newsgroups account settings are correct and try again.
>
> >+sendMessageErrorTitle=Send Message Error
> The code that refers to this string uses the wrong name.

Unfortunate side effect of using strings instead of defines - it does not catch typos :(

We could use some script to check whether all string names in .properties files are properly referenced in the whole tree. Is there such a tool already? If not, I could prepare one.
(Assignee)

Comment 41

4 years ago
Created attachment 8391099 [details] [diff] [review]
Bug802266 by name v2.2

Fixed the typos.
Assignee: buchner.johannes → syshagarwal
Attachment #8391063 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8391063 - Flags: review?(neil)
Attachment #8391099 - Flags: review?(neil)

Updated

4 years ago
Blocks: 952493
Comment on attachment 8391099 [details] [diff] [review]
Bug802266 by name v2.2

I'm going to give this patch r+ because it seems to be correct, but I would appreciate it if you would consider the following points:

>+gatheringAttachment=Attaching %s…
I just noticed this isn't a proper formatted string (using %S and FormatStringFromName), is there any chance you could convert this at the same time, as this would avoid having to rename the string again? (Or, assuming you check the patch in after the uplift, give this a temporary name now, and file a follow-up bug to convert it.)

>+copyMessageStartComplete=Copy complete.
Please would you rename this to copyMessageComplete everywhere. (Sorry for overlooking it last time.)

>+## LOCALIZATION NOTE (failureOnObjectEmbeddingWhileSaving): argument %.200s is file name/URI of object to be embedded
>+failureOnObjectEmbeddingWhileSaving=There was a problem including the file %.200s in the message. Would you like to continue saving the message without this file?
>+## LOCALIZATION NOTE (failureOnObjectEmbeddingWhileSending): argument %.200s is file name/URI of object to be embedded
>+failureOnObjectEmbeddingWhileSending=There was a problem including the file %.200s in the message. Would you like to continue sending the message without this file?
(These could do with being formatted as well, although I don't know offhand whether %.200S works.)

>+## LOCALIZATION NOTE (undisclosedRecipients): this string must be using only US_ASCII characters
Nit: please fix this to say "this string must use only US_ASCII characters"
Attachment #8391099 - Flags: review?(neil) → review+
(Assignee)

Comment 43

4 years ago
Created attachment 8391753 [details] [diff] [review]
Bug802266 by name for check-in

Fixed the nits. Carrying over review from Neil.
Attachment #8391099 - Attachment is obsolete: true
Attachment #8391753 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #8391753 - Flags: feedback+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
Comment on attachment 8391753 [details] [diff] [review]
Bug802266 by name for check-in

>+gatheringAttachment=Attaching %s…
You forgot to change the %s to %S.

>+## LOCALIZATION NOTE (failureOnObjectEmbeddingWhileSaving): argument %.200s is file name/URI of object to be embedded
>+failureOnObjectEmbeddingWhileSaving=There was a problem including the file %.200s in the message. Would you like to continue saving the message without this file?
>+## LOCALIZATION NOTE (failureOnObjectEmbeddingWhileSending): argument %.200s is file name/URI of object to be embedded
>+failureOnObjectEmbeddingWhileSending=There was a problem including the file %.200s in the message. Would you like to continue sending the message without this file?
(Also I still don't know whether %.200S works or not.)

>+      params.Assign("?");
This calls strlen, use either AssignLiteral or '?' instead.

>+      nsAutoString params = NS_ConvertUTF8toUTF16(m_attachments[i]->m_realName);
NS_ConvertUTF8toUTF16 params(m_attachments[i]->m_realName);
Keywords: checkin-needed
(Assignee)

Comment 45

4 years ago
Created attachment 8391809 [details] [diff] [review]
Patch v3

Sorry, made the changes.

(In reply to neil@parkwaycc.co.uk from comment #44)

> >+## LOCALIZATION NOTE (failureOnObjectEmbeddingWhileSaving): argument %.200s is file name/URI of object to be embedded
> >+failureOnObjectEmbeddingWhileSaving=There was a problem including the file %.200s in the message. Would you like to continue saving the message without this file?
> >+## LOCALIZATION NOTE (failureOnObjectEmbeddingWhileSending): argument %.200s is file name/URI of object to be embedded
> >+failureOnObjectEmbeddingWhileSending=There was a problem including the file %.200s in the message. Would you like to continue sending the message without this file?
> (Also I still don't know whether %.200S works or not.)

I couldn't test this.
So, shall we get this in as is and see if someone experiences any errors when running into this?
Attachment #8391753 - Attachment is obsolete: true
Attachment #8391809 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
Doesn't apply to comm-central tip. Please rebase.
Keywords: checkin-needed
(Assignee)

Comment 47

4 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #46)
> Doesn't apply to comm-central tip. Please rebase.

Can you please re-try with both the patches?
bug802266-1-cleanup-strings followed by
Patch v3

(In reply to Suyash Agarwal (:sshagarwal) from comment #45)
> > >+failureOnObjectEmbeddingWhileSending=There was a problem including the file %.200s in the message. Would you like to continue sending the message without this file?
> > (Also I still don't know whether %.200S works or not.)
> 
> I couldn't test this.
> So, shall we get this in as is and see if someone experiences any errors
> when running into this?

I think we can check this in as it was this way before too.

Thanks.
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/1ab1fff092a5
https://hg.mozilla.org/comm-central/rev/cf87c5c76627
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 31.0
Backed out for bustage.
https://hg.mozilla.org/comm-central/rev/13a79c6de877

https://tbpl.mozilla.org/php/getParsedLog.php?id=36666940&tree=Thunderbird-Trunk
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Thunderbird 31.0 → ---

Comment 50

4 years ago
(In reply to neil@parkwaycc.co.uk from comment #25)
> Comment on attachment 8337650 [details] [diff] [review]
> bug802266-2-by-name
> 
> >+    void UpdateStatus(const char * aStatusName);
> Looks like MOZ_UTF16 is good to go these days, in which case you can also
> fix this to be const char16_t *.

Yeah, looks like this change didn't arrive into the last patch version.
But it seems only OS X compiler is picky enough to fail on it so we didn't notice;)
(Assignee)

Comment 51

4 years ago
So, since this is an issue with OS X, should we wait for MOZ_UTF16() to work on OSX or should I revert the function definition to const char* instead of char16_t* ?
(Assignee)

Comment 52

4 years ago
Created attachment 8396598 [details] [diff] [review]
Patch v3.1 fixed OSX build failure cause
Attachment #8391809 - Attachment is obsolete: true
Attachment #8396598 - Flags: review+
(Assignee)

Comment 53

4 years ago
build passes on try server.
https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=8a178dce265e
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/bf99513cdccd
https://hg.mozilla.org/comm-central/rev/30d91b2583f1
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 31.0
You need to log in before you can comment on or make changes to this bug.