Closed Bug 858238 Opened 11 years ago Closed 11 years ago

localMsgs.properties should use string based identifiers rather than numbers

Categories

(MailNews Core :: Backend, defect)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 24.0

People

(Reporter: aceman, Assigned: sshagarwal)

References

()

Details

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

Attachments

(1 file, 7 obsolete files)

localMsgs.properties currently uses numbers for its identifiers rather than strings. This makes it harder for localisers to work with, and means that if we need to tweak strings then it isn't easy to change the string identifier to notify the localisers.

We should work on replacing the numbers with strings.

This affects POP3 and Movemail protocols.

Coding hint:
FormatStringFromID should be replaced with FormatStringFromName, etc.
Hi, I'm new to firefox development and would like to take this as my first bug.
This bug is about Thunderbird or Seamonkey. Do you want to work on it?
I noticed that when I started to look for stuff. I'd still like to work on it though.
OK, do you have the Thunderbird build environment set up?
I do, and I've built Thunderbird. Are you on IRC at all?
Yes, I'll be on #maildev in 4 hours from now.
So, this needs to be done here:
1. in the file mail/locales/en-US/chrome/messenger/localMsgs.properties look for string definitions like this:
# Status - parsing folder
## @name LOCAL_STATUS_SELECTING_MAILBOX
## @loc None
#LOCALIZATION NOTE (4000): Do not translate %s in the following line.
# Place the word %s where the name of the mailbox should appear
4000=Building summary file for %S…

2A. Change it to having a string identifier, like this:
buildingSummary=Building summary file for %S…

2B. Do the same in the equivalent Seamonkey file suite/locales/en-US/chrome/mailnews/localMsgs.properties .

3. Search for the LOCAL_STATUS_SELECTING_MAILBOX constant in the tree: http://mxr.mozilla.org/comm-central/search?string=LOCAL_STATUS_SELECTING_MAILBOX&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

4. Remove the "#define LOCAL_STATUS_SELECTING_MAILBOX 4000"

5. Find all usages of LOCAL_STATUS_SELECTING_MAILBOX and replace with "buildingSummary". All functions where the new identifier is passed should have the argument changed from int to PRUnichar* or something (see how FormatStringFromName is used in the tree).
Also check whether the original numeric ID (4000) is used directly somewhere, not via the LOCAL_STATUS_SELECTING_MAILBOX define. Also change those occurrences to the new string identifier.

6. Where the original numeric ID (via the define) was passed to FormatStringFromID, change the function to FormatStringFromName.
It might be a good(?) idea to use the names as new key. (E.g. LOCAL_STATUS_SELECTING_MAILBOX=Building summary file for %S…) At least don't make the keys too short or too generic to find.
See Also: → 551919
Assignee: nobody → syshagarwal
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
Sir, 
Thanks, I have attached the patch. Looking forward for your comments.
Thanks, I will look at the patch soon, please wait for the comments.
Sorry, but this does not compile for me. I get tons of errors like this:
mailnews/local/src/nsPop3Protocol.cpp: In member function 'int32_t nsPop3Protocol::Error(PRUnichar*)':
mailnews/local/src/nsPop3Protocol.cpp:1260:21: warning: comparison with string literal results in unspecified behaviour [-Waddress]
mailnews/local/src/nsPop3Protocol.cpp:1260:21: error: comparison between distinct pointer types 'PRUnichar* {aka short unsigned int*}' and 'const char*' lacks a cast [-fpermissive]
mailnews/local/src/nsPop3Protocol.cpp:1287:115: error: no matching function for call to 'nsIStringBundle::FormatStringFromName(const char [16], const PRUnichar* [1], int, nsGetterCopies)'
mailnews/local/src/nsPop3Protocol.cpp:1287:115: note: candidate is:
In file included from /var/SSD/TB-hg/mailnews/local/src/nsPop3Protocol.h:18:0,
                 from /var/SSD/TB-hg/mailnews/local/src/nsPop3Protocol.cpp:28:
../../../mozilla/dist/include/nsIStringBundle.h:55:60: note: virtual nsresult nsIStringBundle::FormatStringFromName(const PRUnichar*, const PRUnichar**, uint32_t, PRUnichar**)
../../../mozilla/dist/include/nsIStringBundle.h:55:60: note:   no known conversion for argument 1 from 'const char [16]' to 'const PRUnichar* {aka const short unsigned int*}'

The first argument of nsPop3Protocol::Error probably can't be PRUnichar* so easily.
We need to find out what the type should be so that it accepts plain char*. There is no other ::Error method where we could look how it is done. Neil?
Should we make it nsACString and send the string identifiers as NS_LITERAL_CSTRING("string")? But that may be overkill.
(In reply to :aceman from comment #11)
> The first argument of nsPop3Protocol::Error probably can't be PRUnichar* so
> easily.
> We need to find out what the type should be so that it accepts plain char*.
> There is no other ::Error method where we could look how it is done. Neil?

NS_LITERAL_STRING("").get() is how you call those sorts of methods.
Sir,
So, what should I do then? I mean, what should PRUnichar* be replaced with, and yes, at some places, strings are compared, so should I use the c function strcmp() ? 
And, what is "Neil"? Its your last mentioned word in your comment #11
(In reply to Suyash Agarwal from comment #14)
> Sir,
> So, what should I do then? I mean, what should PRUnichar* be replaced with,
> and yes, at some places, strings are compared, so should I use the c
> function strcmp() ? 
Keep the PRUnichar* but replace the strings with NS_LITERAL_STRING("").get() as jcranmer says. So e.g.:
if (err_code != "downloadError")
change to:
if (err_code != NS_LITERAL_STRING("downloadError").get())

Change
Error("pop3ServerError")
to
Error(NS_LITERAL_STRING("pop3ServerError").get())

And please try to compile the patch after you do this. It seems you haven't done it the first time before uploading.

> And, what is "Neil"? Its your last mentioned word in your comment #11
That is the name of one of our gurus :)
jcranmer, thanks for the help. Shouldn't we then make Error take nsString and only call the .get() when calling FormatStringFromName ?
(In reply to :aceman from comment #15)
> Keep the PRUnichar* but replace the strings with NS_LITERAL_STRING("").get()
> as jcranmer says. So e.g.:
> if (err_code != "downloadError")
> change to:
> if (err_code != NS_LITERAL_STRING("downloadError").get())

That code is very wrong--you cannot compare string literals with ==/!= in C++. This makes me think there may be some more pernicious things going on around that call, but I haven't read most of the patch.
(In reply to aceman from comment #15)
> if (err_code != "downloadError")
> change to:
> if (err_code != NS_LITERAL_STRING("downloadError").get())
No! don't do either of those!

(In reply to aceman from comment #16)
> jcranmer, thanks for the help. Shouldn't we then make Error take nsString
> and only call the .get() when calling FormatStringFromName ?
In theory it's easier for the compiler to optimse if you pass the PRUnichar.
OK, some version of err_code.Equals(NS_LITERAL_STRING("downloadError")) ?
Oh okay sir. But why can't I use strcmp() to compare strings?? And, what does this NS_LITERAL_STRING ("").get() actually do? Does it produce an equivalent integer to be compared?

And, what should I do in situations of the terenary operators, and few comparisons of the form: err_code != "downloadError"? Here, should I replace this "downloadError" also with NS_LITERAL_STRING("downloadError").get() or, strcmp()? And same for the error for FormatStringFromName(), should its parameter also be replaced with NS_LITERAL_STRING("").get() ??
There's a similar problem in nsMsgMailboxParser::UpdateStatusText, which happens to insert the folder name if the string is the selecting mailbox string. As there are only the two strings to worry about, perhaps a different approach is called for in that case.
Attached patch patch v2 (obsolete) — Splinter Review
Neil, jcranmer, is this what you meant? I fixed up nsPop3Protocol.cpp so that is compiles now.
Attachment #736513 - Attachment is obsolete: true
Attachment #736978 - Flags: feedback?(neil)
Attached patch Patch (obsolete) — Splinter Review
Sir,
I have attached a patch, and this compiles and builds properly.
Waiting for the comments.
I wonder how your patch is so much smaller than mine. But I'll check it out.
Comment on attachment 737422 [details] [diff] [review]
Patch

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

Good work now! The patch compiles and I have tested the "Building summary file" string that it does appear on the status line when needed.

I've just marked some style problems, as written below. I will not comment on the chosen names of the strings, that will be decided by the real reviewer.

::: mailnews/local/src/nsLocalMailFolder.cpp
@@ +845,1 @@
>                                     getter_Copies(deleteFolderDialogTitle));

Please add 2 spaces so that 'getter' aligns below 'NS_'. This happens in several places.

@@ +850,1 @@
>                                     getter_Copies(deleteFolderButtonLabel));

E.g. here.

@@ +3165,1 @@
>                                                 getter_Copies(finalString));

Can 'getter' be aligned below 'status' ?

::: mailnews/local/src/nsLocalStrings.h
@@ +47,2 @@
>  
>  #endif /* _nsLocalStrings_H__ */

Can you remove the whole file altogether and remove its inclusion where it is referenced?

::: mailnews/local/src/nsParseMailbox.cpp
@@ +119,5 @@
>              // the size of the mailbox file is our total base line for measuring progress
>              m_graph_progress_total = (uint32_t) fileSize;
> +            //UpdateStatusText(LOCAL_STATUS_SELECTING_MAILBOX);
> +	    UpdateStatusText(NS_LITERAL_STRING("buildingSummary").get());
> +	    //UpdateStatusText(NS_LITERAL_STRING("buildingSummary").get());	

Can you leave only the correct line and remove the commented code?

@@ +177,5 @@
>      // be sure to clear any status text and progress info..
>      m_graph_progress_received = 0;
>      UpdateProgressPercent();
> +//    UpdateStatusText(LOCAL_STATUS_DOCUMENT_DONE);
> +    UpdateStatusText (NS_LITERAL_STRING("done").get());

Remove the commented out line.

@@ +301,1 @@
>                                        getter_Copies(finalString));

Align 'string' and 'getter'.

::: mailnews/local/src/nsParseMailbox.h
@@ +168,5 @@
>  
>    void  UpdateDBFolderInfo();
>    void  UpdateDBFolderInfo(nsIMsgDatabase *mailDB);
> +//  void  UpdateStatusText (uint32_t stringID);
> +  void UpdateStatusText (const PRUnichar* stringName);

Remove commented line.

::: mailnews/local/src/nsPop3Protocol.h
@@ +45,4 @@
>   */
>  
>  #define MK_OUT_OF_MEMORY -207
> +// #define MK_POP3_OUT_OF_DISK_SPACE -321

Remove the line altogether.
Oh, the review was produced strangely. I'll write the first comments again if they can't be understood.

Also, please do not use tabs in the code. Use normal spaces instead.
--- a/mailnews/local/src/nsLocalMailFolder.cpp<>Mon Apr 08 00:56:01 2013 +0100
+++ b/mailnews/local/src/nsLocalMailFolder.cpp<>Mon Apr 15 21:31:15 2013 +0530
@@ -841,17 +841,17 @@
       const PRUnichar *formatStrings[1] = { folderName.get() };
.
       nsAutoString deleteFolderDialogTitle;
-      rv = bundle->GetStringFromID(POP3_DELETE_FOLDER_DIALOG_TITLE,
+      rv = bundle->GetStringFromName(NS_LITERAL_STRING("deleteFolderDialog").get(),
                                    getter_Copies(deleteFolderDialogTitle));

The first 3 remarks wanted to write that you should use spaces to align "getter_Copies" under "NS_LITERAL_STRING".
Blocks: 856577
No longer depends on: 856577
Attached patch Patch (indented) (obsolete) — Splinter Review
Sir, the patch is modifies as instructed. Waiting for the comments.
Attachment #737422 - Attachment is obsolete: true
Comment on attachment 738119 [details] [diff] [review]
Patch (indented)

Yes, this looks good now! You still left in some tabs, so please remove them in the next version of the patch, that you create when the real reviewer writes his comments. I'm forwarding to Neil for next review.
Attachment #738119 - Flags: review?(neil)
Attachment #738119 - Flags: feedback+
Attachment #736978 - Attachment is obsolete: true
Attachment #736978 - Flags: feedback?(neil)
Comment on attachment 738119 [details] [diff] [review]
Patch (indented)

>-4043=Unable to establish TLS connection to POP3 server. The server may be down or may be incorrectly configured. Please verify the correct configuration in the Server Settings for your mail server in the Account Settings window and try again.
>+COULD_NOT_CONNECT_VIA_TLS=Unable to establish TLS connection to POP3 server. The server may be down or may be incorrectly configured. Please verify the correct configuration in the Server Settings for your mail server in the Account Settings window and try again.
[Someone (not me!) needs to come up with some better names...]

>       (void) ThrowAlertMsg((rv == NS_MSG_FOLDER_EXISTS) ?
>-                            "folderExists" : "folderRenameFailed", msgWindow);
>+                           "folderExists" : 
>+			   "folderRenameFailed",
>+			   msgWindow);
Why did this change?

>+      const PRUnichar* statusMsgName = ((mCopyState->m_isMove) ? NS_LITERAL_STRING("movingMessages")
>+		      					       : NS_LITERAL_STRING("copyingMessages")).get();
Sorry, but the NS_LITERAL_STRING gets deleted at the end of the statement, so you can't use .get() here. What would work though would be to write the ?: expression directly in the FormatStringFromName call.

>+    if (nsDependentString(stringName).EqualsLiteral("buildingSummary"))
I'm not that keen on using a string comparison here, but I couldn't think of a good way to avoid it.

>+    PR_LOG(POP3LOGMODULE, PR_LOG_ALWAYS, ("ERROR: %s", err_code));
Sorry, but PR_LOG only works on const char* strings. I suppose you could change err_code to be a char* and then convert it to UTF16 later (compare nsMsgDBFolder::GetStringWithFolderNameFromBundle).

>-    if (err_code != POP3_TMP_DOWNLOAD_FAILED && NS_SUCCEEDED(rv))
>+    // we handle "downloadError" earlier...
>+    if (nsDependentString(err_code).EqualsLiteral("downloadError") && NS_SUCCEEDED(rv))
I think you accidentally deleted the !
Also, I don't like the string comparison again, but I still don't have a better idea.

>-    // XXX Error() returns -1, which is not a valid nsresult
>-    return static_cast<nsresult>(Error(POP3_USERNAME_UNDEFINED));
>+    Error(NS_LITERAL_STRING("usernameUndefined").get());
>+    return NS_MSG_SERVER_USERNAME_MISSING;
Nice!
Attachment #738119 - Flags: review?(neil) → review-
Attached patch Patch (obsolete) — Splinter Review
Sir, 

I have tried to address all the issues mentioned by you.
Please review this patch.
Attachment #738119 - Attachment is obsolete: true
Attachment #744233 - Flags: review?(neil)
Comment on attachment 744233 [details] [diff] [review]
Patch

This is looking good. Has it been tested on the Try server? There are just a few things that I would like to resolve:

> ## @name MK_POP3_OUT_OF_DISK_SPACE
> ## @loc None
Sorry for not noticing before, but I think the patch probably makes all of these @name/@loc lines unnecessary, and you could remove them.

>-    if (stringID == LOCAL_STATUS_SELECTING_MAILBOX)
>+    if (nsDependentString(stringName).EqualsLiteral("buildingSummary"))
>     {
>       const PRUnichar * stringArray[] = { m_folderName.get() };
>-      rv = bundle->FormatStringFromID(stringID, stringArray, 1,
>-                                      getter_Copies(finalString));
>+      rv = bundle->FormatStringFromName(stringName, stringArray, 1,
>+                                        getter_Copies(finalString));
>     }
>     else
>-      bundle->GetStringFromID(stringID, getter_Copies(finalString));
>+      bundle->GetStringFromName(stringName, getter_Copies(finalString));
There was a suggestion on IRC that you could avoid the string comparison by always formatting the string (the strings that didn't need the format would just ignore it).

>+    PR_LOG(POP3LOGMODULE, PR_LOG_ALWAYS, 
>+          ("ERROR: %s", NS_LossyConvertUTF16toASCII(err_code).get()));
Seeing as you always need to do a conversion, it might be a better idea to use a const char* err_code instead, and then convert it to UTF16 when getting the string. That advantage is that you don't have to fiddle around with NS_LITERAL_STRING().get() all the time.

>+            return(Error(((m_password_already_sent)
>+                          ? NS_LITERAL_STRING("pop3PasswordFailure")
>+                          : NS_LITERAL_STRING("pop3UsernameFailure")).get()));
Another thing that I overlooked before is that I think the Mozilla coding style now prefers the ? and : operators at the end of the previous line. Also, I think earlier on in the patch you used a separate .get() for each NS_LITERAL_STRING; I prefer that version, so the code I quoted would end up something like this:
return Error(m_password_already_sent ?
             NS_LITERAL_STRING("pop3PasswordFailure").get() :
             NS_LITERAL_STRING("pop3UsernameFailure").get());
I think there is at least one other case of ( ? : ).get() that should be updated.
Attachment #744233 - Flags: review?(neil) → feedback+
Thanks, hopefully we are nearing finish here.
Severity: trivial → minor
Attached patch Patch v2 (obsolete) — Splinter Review
Sir,

I have made the changes suggested. Waiting for your comments.
Attachment #744233 - Attachment is obsolete: true
Attachment #745577 - Flags: review?(neil)
Blocks: 221592
Comment on attachment 745577 [details] [diff] [review]
Patch v2

This looks good, assuming all the tests pass. (Has anyone run them?)

(It would be nice of the @name and @loc lines could be removed from suite/locales/en-US/chrome/mailnews/localMsgs.properties too.)
Attachment #745577 - Flags: review?(neil) → review+
Attached patch Patch v3 (obsolete) — Splinter Review
Sir,
Sorry I left the suite/ file in the previous patch. I have made the changes in this one.
:aceman sir, please provide the feedback so that I can mark this for check-in.
Carrying over review from Neil.
Attachment #745577 - Attachment is obsolete: true
Attachment #749298 - Flags: review+
Attachment #749298 - Flags: feedback?(acelists)
Comment on attachment 749298 [details] [diff] [review]
Patch v3

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

Please update your tree as this patch surely will not apply cleanly due to checkin of bug 640371. Then please update the patch. As you are going to change the patch anyway I have some nits.

::: mailnews/local/src/nsMovemailService.cpp
@@ -114,5 @@
>    if (NS_FAILED(rv))
>      return;
>  
>    nsString errStr;
> -  // Format the error string if necessary

Why remove this comment?

::: mailnews/local/src/nsParseMailbox.cpp
@@ -176,5 @@
>      // be sure to clear any status text and progress info..
>      m_graph_progress_received = 0;
>      UpdateProgressPercent();
> -    UpdateStatusText(LOCAL_STATUS_DOCUMENT_DONE);
> -

Do not remove the empty line and do not add space before ( .

::: mailnews/local/src/nsParseMailbox.h
@@ -168,5 @@
>  
>    void  UpdateDBFolderInfo();
>    void  UpdateDBFolderInfo(nsIMsgDatabase *mailDB);
> -  void  UpdateStatusText (uint32_t stringID);
> -

Probably keep the blank line here.

::: mailnews/local/src/nsPop3Protocol.cpp
@@ +2343,5 @@
>        if (NS_FAILED(rv))
>        {
>          m_nsIPop3Sink->AbortMailDelivery(this);
>          if (rv == NS_MSG_FOLDER_BUSY)
> +          return Error("pop3MessageFolderBusy");

Can this be changed to the return Error(rv="xxx"? "error1" : "error2") style we already used elsewhere?

@@ +2985,5 @@
>            // Not enough disk space!
>  #ifdef DEBUG
>            printf("Not enough disk space! Raising error! \n");
>  #endif
> +          return Error("pop3OutOfDiskSpace");

This will need update due to the clash with bug 640371.
(In reply to neil@parkwaycc.co.uk from comment #35)
> This looks good, assuming all the tests pass. (Has anyone run them?)

Yes, I've run TB tests on linux (xpcshell and mozmill) with this.
Attached patch Patch v4Splinter Review
Sir,

I have made the changes you suggested.
Carrying over review from Neil.
Attachment #749298 - Attachment is obsolete: true
Attachment #749298 - Flags: feedback?(acelists)
Attachment #749481 - Flags: review+
Attachment #749481 - Flags: feedback?(acelists)
Comment on attachment 749481 [details] [diff] [review]
Patch v4

Great, everything is done:)
Attachment #749481 - Flags: feedback?(acelists) → feedback+
Let's drive this in finally, other bugs are waiting in queue for it to land :)
Congrats to your first taken bug being fixed. And it ended up to be quite big patch.
Keywords: checkin-needed
Thank you sir :)
Its my pleasure learning from you.
https://hg.mozilla.org/comm-central/rev/41a231cc4161
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 24.0
You need to log in before you can comment on or make changes to this bug.