Closed Bug 551919 Opened 11 years ago Closed 7 years ago

imapMsgs.properties should used string based identifiers rather than numbers

Categories

(MailNews Core :: Networking: IMAP, defect)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 25.0

People

(Reporter: standard8, Assigned: sshagarwal)

References

()

Details

(Keywords: addon-compat, Whiteboard: [good first bug][mentor=aceman][lang=c++])

Attachments

(1 file, 8 obsolete files)

imapMsgs.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.
Whiteboard: [good first bug][mentor=aceman][lang=c++]
CC'ing myself because I use the numbers in an extension and will need to make modifications if/when this bug lands.
Sir, I would love to work on this bug, but as this will be my first bug fix, please guide me.
Do you have a Thunderbird build environment set up?

The description what needs to be done here is the same as bug 858238 comment 7, just work on the file imapMsgs.properties.
See Also: → 858238
Yes, I have built thunderbuild and firefox both.
*thunderbird
Sir, I have almost completed my work, but I have few doubts regarding:
1. Return type of Error() function under nsPop3Protocols.cpp,
2. GetStringFromID() function under nsCOMPtr.h

Can you please help, and assign this bug to me?
(In reply to Suyash Agarwal from comment #6)
> Sir, I have almost completed my work, but I have few doubts regarding:
Sure, please write your questions.

> 1. Return type of Error() function under nsPop3Protocols.cpp,
You can keep the return value of Error() as is, unchanged.
Only inside the function, change it to use GetStringFromName instead of GetStringFromID.

> 2. GetStringFromID() function under nsCOMPtr.h
I can't see usage of this function in that file...

> Can you please help, and assign this bug to me?
Yes, done.
Assignee: nobody → syshagarwal
Status: NEW → ASSIGNED
Keywords: helpwanted
Sir,
Thank you for assigning this bug to me :)  
There are two types of string definitions:
1. # Status - no messages to download 
## @name noMessages
## @loc None
#LOCALIZATION NOTE : Do not translate %1$S in the following line.
# Place %1$S where the account name should appear
noMessages=There are no new messages for %1$S.

2. ## @name COPYING_MSGS_STATUS
## @loc None
4027=Copying %S of %S messages to %S

i.e., the ones with the starting "Status" and the ones without it, so, should I change the type 2 string definitions also?
You should only change the string definitions having a number before the '=' character. So type 2. in your question. Do not touch the string definitions of type 1.

I your example, you can change the block to:
## @loc None
COPYING_MSGS_STATUS=Copying %S of %S messages to %S

As described in bug 858238 comment 7.
Sir,
I have finished with the tasks mentioned in bug 858238. What should I do now?
Thanks, attach the patch here using the "add an attachment" link.
Sir,
Is there a way so that I can test/compile if there is anything left to be changed?
And about the patch!, what should it be like? Am I supposed to attach all the files I have made changes in, or something else?
Of course you compile/build whole Thunderbird to see if nothing is broken.

Creating a patch is described here:
https://developer.mozilla.org/en-US/docs/Creating_a_patch

If you do not use 'hg' (mercurial) you need to find out how to produce the patch using the diff command in your environment.

You could attach a zip file with all the changed files, and I could produce the patch, but that would be inefficient in case there is some problem and you would need to make some more changes.
Attached patch Patch (obsolete) — Splinter Review
Sir, 
I have attached the patch.
(In reply to Suyash Agarwal from comment #14)
> Created attachment 736480 [details] [diff] [review]
> Patch
> 
> Sir, 
> I have attached the patch.

Nice work! Just one thing though. This bug is for IMAP strings. Your patch is for POP3. You should post this patch to bug 858238. Then when you are done there, you can come back and fix this bug too!
Attached patch Patch for localMsgs.properties (obsolete) — Splinter Review
Sir,
I forgot to attach localMsgs.properties to the previous patch, so this one is only for localMsgs.properties, the previous one is for all other changes. Sorry for the inconvenience.
Oops...
Sir, what should I do now? I was assigned this bug and I think, I created a patch for fixing bug 858238. Can you please help!
Yes, both patches are actually for bug 858238...
So then you should probably join that bug too and attach the patches there :)
Okay, so Would you assign that bug to me? I will try to finish patch for this as soon as possible.
Please finish one bug first (bug 858238). I'll test the patch tomorrow and see if it works and send you comments on what to change.

Please attach the patches in the other bug. If possible, merge them into one patch.
Attachment #736480 - Attachment is obsolete: true
Attachment #736489 - Attachment is obsolete: true
Attached patch Patch (obsolete) — Splinter Review
Attachment #752634 - Flags: feedback?(acelists)
Comment on attachment 752634 [details] [diff] [review]
Patch

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

This looks very nice and is big work. Can you now remove the IMAP_* defines from mailnews/imap/src/nsImapStringBundle.h ?

Can you now remove the *ID variants of the functions? Or does some other code use them? You will probably find out by removing the constants above.

As a bonus, could you maybe check if all of the strings in the .properties file are actually used in code? It could be just some 'grep' over the source files...

I give f+ as this is mostly working and only needs the cleanup I mentioned above.

::: mail/locales/en-US/chrome/messenger/imapMsgs.properties
@@ +127,2 @@
>  
> +imapDone=

Is this intentionally blank? If so, we could probably add a comment saying so.

::: mailnews/imap/src/nsImapIncomingServer.cpp
@@ +1883,5 @@
> +  }
> +
> +  // Error condition
> +  message.AssignLiteral("String ID ");
> +  message.Append(NS_ConvertASCIItoUTF16(aMsgId).get());

Can you try .Adopt(aMsgId) or .Assign(aMsgId) here?

@@ +1920,5 @@
>      nullptr,
>      nullptr
>    };
>  
> +  const PRUnichar* msgID;

Could this be nsString?

@@ +1943,5 @@
>      aUrl->GetFolder(getter_AddRefs(folder));
>      if (folder)
>        folder->GetPrettyName(folderName);
>      numStrings = 3;
> +    msgID = NS_LITERAL_STRING("imapFolderCommandFailed").get();

Then without .get() here.

@@ +1958,5 @@
>    nsresult rv = GetStringBundle();
>    NS_ENSURE_SUCCESS(rv, rv);
>    if (m_stringBundle)
>    {
> +    rv = m_stringBundle->FormatStringFromName(msgID,

... and msgID.get() here.

@@ +2008,2 @@
>  {
> +  nsresult res = NS_OK;

Name this 'rv'.

@@ +2020,4 @@
>    }
> +  aString.AssignLiteral("String ID ");
> +  // mscott: FIX ME
> +  nsString tmpIntStr;

Unused variable?

@@ +2952,5 @@
> +    nsString tmpVal (aValue);
> +    const PRUnichar *formatStrings[] =
> +    {
> +      tmpVal.get(),
> +    };

Can this be condensed onto the same line as 'const PRUnichar *formatStrings' ?

::: mailnews/imap/src/nsImapMailFolder.cpp
@@ +5891,5 @@
>  {
>    NS_ENSURE_ARG(aFolderProps);
> +  const PRUnichar* folderTypeStringID;
> +  const PRUnichar* folderTypeDescStringID = nullptr;
> +  const PRUnichar* folderQuotaStatusStringID;

I'd feel safer if these would be a nsString instead of raw string pointer.

::: mailnews/imap/src/nsImapMailFolder.h
@@ +199,5 @@
>    NS_DECL_ISUPPORTS_INHERITED
>  
>    // nsIMsgFolder methods:
>    NS_IMETHOD GetSubFolders(nsISimpleEnumerator **aResult);
> +  

As you do no code change in this file, do not do this whitespace change.

::: mailnews/imap/src/nsImapProtocol.cpp
@@ -5063,5 @@
> -{
> -  if (m_imapMailFolderSink && aMsgId != m_lastProgressStringId)
> -  {
> -    m_imapMailFolderSink->ProgressStatus(this, aMsgId, nullptr);
> -    m_lastProgressStringId = aMsgId;

Why are you removing this logic? It is probably needed.

@@ +5238,5 @@
>      m_imapMailFolderSink->GetAclFlags(&aclFlags);
>  
>    if (aclFlags && !(aclFlags & IMAP_ACL_EXPUNGE_FLAG))
>      return;
> +  ProgressEventFunctionUsingId ("imapStatusExpungingMailbox");

Remove space before ( . There are several occurrences of this later on.

::: mailnews/imap/src/nsImapProtocol.h
@@ +240,4 @@
>    void AlertUserEventFromServer(const char * aServerEvent);
>  
> +  void ProgressEventFunctionUsingId(const char* aMsgId);
> +  void ProgressEventFunctionUsingIdWithString(const char* aMsgId, const char *

You renamed the *ID functions to *Name in all other places except here. Why?

::: mailnews/imap/src/nsImapStringBundle.cpp
@@ +29,5 @@
>  
>  nsresult
> +IMAPGetStringByName(const PRUnichar* stringName, PRUnichar **aString)
> +{
> +  nsresult res=NS_OK;

Please use 'rv' in new code.

::: mailnews/imap/src/nsImapStringBundle.h
@@ +9,5 @@
>  
>  PR_BEGIN_EXTERN_C
>  
>  nsresult      IMAPGetStringByID(int32_t stringID, PRUnichar **aString);
> +nsresult      IMAPGetStringByName(const PRUnichar* stringName, PRUnichar **aString);

Can this also become const char* instead of PRUnichar*? Or would it be hard to use?
Attachment #752634 - Flags: feedback?(acelists) → feedback+
Attached patch Patch v2 (obsolete) — Splinter Review
Sir,

I have made the changes you suggested. I didn't use Adopt(aMsgId) or .Assign(aMsgId) because the effect of "message.AssignLiteral("String ID ");" would be lost. But if that isn't the case, I can make the change.
Attachment #752634 - Attachment is obsolete: true
Attachment #753824 - Flags: feedback?(acelists)
Comment on attachment 753824 [details] [diff] [review]
Patch v2

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

Looks better now, thanks.

::: mailnews/imap/src/nsImapIncomingServer.cpp
@@ +1960,5 @@
>    if (m_stringBundle)
>    {
> +    rv = m_stringBundle->FormatStringFromName(msgName.get(),
> +      formatStrings, numStrings,
> +      getter_Copies(fullMessage));

Does the fullMessage fit on the previous line?

::: mailnews/imap/src/nsImapMailFolder.cpp
@@ +5891,5 @@
>  {
>    NS_ENSURE_ARG(aFolderProps);
> +  nsString folderTypeStringID;
> +  nsString folderTypeDescStringID;
> +  nsString folderQuotaStatusStringID;

Now when you could convert IMAPGetStringByName to const char* try to change these to nsCString too. It may save some of the ToNewUTF8String conversions.

@@ +5929,5 @@
>          }
>          else
>          {
>            // If not, there is no storage quota set on this folder
> +          folderQuotaStatusStringID = NS_LITERAL_STRING("imapQuotaStatusNoQuota");

Can you try folderQuotaStatusStringID.AssignLiteral("imapQuotaStatusNoQuota") here and all other places? I worry that assignment operator on ns*String does not do what you think.

@@ +5958,5 @@
>      {
>        // Hide quota data and show reason why it is not available
>        aFolderProps->ShowQuotaData(false);
>  
> +      rv = IMAPGetStringByName(ToNewUTF8String(folderQuotaStatusStringID),

If folderQuotaStatusStringID gets nsCString, does .get() work here?

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +5067,5 @@
> +  if (m_imapMailFolderSink && !NS_ConvertASCIItoUTF16(aMsgName).
> +                                 Equals(m_lastProgressStringId))
> +  {
> +    m_imapMailFolderSink->ProgressStatusString(this, aMsgName, nullptr);
> +    m_lastProgressStringId = NS_ConvertASCIItoUTF16(aMsgName);

If m_lastProgressStringId becomes nsCString, this could become .Adopt() or something.

::: mailnews/imap/src/nsImapProtocol.h
@@ +628,5 @@
>    nsString m_progressString;
>    int32_t       m_progressStringId;
>    int32_t       m_progressIndex;
>    int32_t       m_progressCount;
> +  nsString      m_lastProgressStringId;

This can probably be nsCString as we store a const char* into it. Then some of the conversions can be probably dropped.

::: mailnews/imap/src/nsImapStringBundle.cpp
@@ +29,5 @@
>  
>  nsresult
> +IMAPGetStringByName(const char* stringName, PRUnichar **aString)
> +{
> +  nsresult rv=NS_OK;

And spaces around = .
Attachment #753824 - Flags: feedback?(acelists) → feedback+
(In reply to :aceman from comment #24)
> > +          folderQuotaStatusStringID = NS_LITERAL_STRING("imapQuotaStatusNoQuota");
> 
> Can you try
> folderQuotaStatusStringID.AssignLiteral("imapQuotaStatusNoQuota") here and
> all other places? I worry that assignment operator on ns*String does not do
> what you think.

Ok, "= NS_LITERAL_STRING" is used throughout the tree so you can probably keep this if you want, just use "= NS_LITERAL_CSTRING".

> ::: mailnews/imap/src/nsImapProtocol.cpp
> @@ +5067,5 @@
> > +  if (m_imapMailFolderSink && !NS_ConvertASCIItoUTF16(aMsgName).
> > +                                 Equals(m_lastProgressStringId))

And if m_lastProgressStringId becomes nsCString, this may become some simpler .Equals() call as I am really scared by this expression :)
Attached patch Patch v3 (obsolete) — Splinter Review
Sir,

I have tried to make the changes suggested.
Attachment #753824 - Attachment is obsolete: true
Attachment #753908 - Flags: feedback?(acelists)
Comment on attachment 753908 [details] [diff] [review]
Patch v3

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

Yes, this seems fine now. You can fix the one nit I found in a new version after a real reviewer writes his comments too.
Just remember to remove also the *ID versions of the functions after a reviewer says this can more or less accepted.

::: mailnews/imap/src/nsImapStringBundle.cpp
@@ +31,5 @@
> +IMAPGetStringByName(const char* stringName, PRUnichar **aString)
> +{
> +  nsresult rv = NS_OK;
> +  nsCOMPtr <nsIStringBundle> sBundle;
> +  rv = IMAPGetStringBundle(getter_AddRefs(sBundle));

Sorry, I didn't notice, but you can merge this: nsresult rv = IMAP...
Attachment #753908 - Flags: review?(neil)
Attachment #753908 - Flags: feedback?(acelists)
Attachment #753908 - Flags: feedback+
Suyash, can you try using ns*String.AppendASCII(char variable) instead of ns*String.Append(NS_ConvertASCIItoUTF16(char variable) ?

Neil, can you look at this already?
Flags: needinfo?(neil)
Attached patch Patch v3 (revised) (obsolete) — Splinter Review
Covered the changes mentioned in comment 27 and 28.
Attachment #753908 - Attachment is obsolete: true
Attachment #753908 - Flags: review?(neil)
Attachment #763464 - Flags: review?(neil)
Comment on attachment 763464 [details] [diff] [review]
Patch v3 (revised)

>   void progressStatus(in nsIImapProtocol aProtocol, in unsigned long aMsgId, in wstring extraInfo);
>+  void progressStatusString(in nsIImapProtocol aProtocol, in string aMsgId, in wstring extraInfo);
Well, the obvious question is why didn't you remove the old methods?

>+  message.AssignLiteral("String ID ");
Is it an ID or is it a name?

>+    msgName = NS_LITERAL_STRING("imapFolderCommandFailed");
...
>+    msgName = NS_LITERAL_STRING("imapServerCommandFailed");
AssignLiteral?

>-           (!deleteNoTrash) ? IMAP_MOVE_FOLDER_TO_TRASH : IMAP_DELETE_NO_TRASH,
[I find !s confusing in ?: expressions]

>+  nsCString folderTypeStringID;
>+  nsCString folderTypeDescStringID;
>+  nsCString folderQuotaStatusStringID;
These might work better as const char*.

>-            SetProgressString(0);
>+            SetProgressString(nullptr);
[Note to self: figure out what this is supposed to do.]

>-  int32_t       m_progressStringId;
>-  int32_t       m_progressIndex;
>-  int32_t       m_progressCount;
>-  uint32_t      m_lastProgressStringId;
>-  int32_t       m_lastPercent;
>-  int64_t       m_lastProgressTime;
>+  int32_t        m_progressStringId;
>+  int32_t        m_progressIndex;
>+  int32_t        m_progressCount;
>+  nsCString      m_lastProgressStringId;
>+  int32_t        m_lastPercent;
>+  int64_t        m_lastProgressTime;
Why the extra space?
(In reply to neil@parkwaycc.co.uk from comment #30)
> Comment on attachment 763464 [details] [diff] [review]
> Patch v3 (revised)
> 
> >   void progressStatus(in nsIImapProtocol aProtocol, in unsigned long aMsgId, in wstring extraInfo);
> >+  void progressStatusString(in nsIImapProtocol aProtocol, in string aMsgId, in wstring extraInfo);
> Well, the obvious question is why didn't you remove the old methods?
Yes, we will remove them when the new versions are reviewed.
 
> >+  nsCString folderTypeStringID;
> >+  nsCString folderTypeDescStringID;
> >+  nsCString folderQuotaStatusStringID;
> These might work better as const char*.
These need to be PRUnichar*, but as they are being assigned different values later, I thought it would be safer with nsCString. See comment 22. Do you still want to change it back?
(In reply to aceman from comment #31)
> > >   void progressStatus(in nsIImapProtocol aProtocol, in unsigned long aMsgId, in wstring extraInfo);
> > >+  void progressStatusString(in nsIImapProtocol aProtocol, in string aMsgId, in wstring extraInfo);
> > Well, the obvious question is why didn't you remove the old methods?
> Yes, we will remove them when the new versions are reviewed.
If they're no longer needed then I'd prefer a patch that includes the removals.

> > >+  nsCString folderTypeStringID;
> > >+  nsCString folderTypeDescStringID;
> > >+  nsCString folderQuotaStatusStringID;
> > These might work better as const char*.
> These need to be PRUnichar*, but as they are being assigned different values
> later, I thought it would be safer with nsCString. See comment 22. Do you
> still want to change it back?
But IMAPGetStringByName takes a const char*...
Comment on attachment 763464 [details] [diff] [review]
Patch v3 (revised)

>+void nsImapProtocol::SetProgressString(const char * stringName)
>+{
>+  if (stringName && m_imapServerSink)
>+    m_imapServerSink->GetImapStringByName(stringName,
>+                                          m_progressString);
> }
> 
> void
> nsImapProtocol::ShowProgress()
> {
>   if (!m_progressString.IsEmpty() && m_progressStringId)
You now never set m_progressStringId so this condition always fails. But then once you fix that, SetProgressString never clears m_progressString, so the condition always passes. So you need to fix that too.

>+nsImapProtocol::ProgressEventFunctionUsingName(const char* aMsgName)
>+{
>+  if (m_imapMailFolderSink && !m_lastProgressStringId.Equals(aMsgName))
[I wonder how often we actually fail this check.]
Attachment #763464 - Flags: review?(neil) → review-
Attached patch Patch v4 (obsolete) — Splinter Review
Addressing:
 Comment 30 neil@parkwaycc.co.uk 2013-07-08 07:34:22 PDT 
>-  int32_t       m_progressStringId;
>-  int32_t       m_progressIndex;
>-  int32_t       m_progressCount;
>-  uint32_t      m_lastProgressStringId;
>-  int32_t       m_lastPercent;
>-  int64_t       m_lastProgressTime;
>+  int32_t        m_progressStringId;
>+  int32_t        m_progressIndex;
>+  int32_t        m_progressCount;
>+  nsCString      m_lastProgressStringId;
>+  int32_t        m_lastPercent;
>+  int64_t        m_lastProgressTime;
>Why the extra space?

These were previously aligned one under the other, so I aligned them too, if this isn't acceptable, I'll remove it

 Comment 33 neil@parkwaycc.co.uk 2013-07-08 11:39:27 PDT 
> nsImapProtocol::ShowProgress()
> {
>   if (!m_progressString.IsEmpty() && m_progressStringId)
>You now never set m_progressStringId so this condition always fails. But then >once you fix that, SetProgressString never clears m_progressString, so the >condition always passes. So you need to fix that too.

Sorry, made that change, but clearing of m_progressString wasn't done before too, so where should I clear it out now?
Attachment #763464 - Attachment is obsolete: true
Attachment #772316 - Flags: review?(neil)
(In reply to Suyash Agarwal from comment #34)
> (In reply to comment #30)
> >-  int32_t       m_progressStringId;
> >-  int32_t       m_progressIndex;
> >-  int32_t       m_progressCount;
> >-  uint32_t      m_lastProgressStringId;
> >-  int32_t       m_lastPercent;
> >-  int64_t       m_lastProgressTime;
> >+  int32_t        m_progressStringId;
> >+  int32_t        m_progressIndex;
> >+  int32_t        m_progressCount;
> >+  nsCString      m_lastProgressStringId;
> >+  int32_t        m_lastPercent;
> >+  int64_t        m_lastProgressTime;
> > Why the extra space?
> 
> These were previously aligned one under the other, so I aligned them too

Sure, the alignment isn't the problem, only the extra space.

> (In reply to comment #33)
> > nsImapProtocol::ShowProgress()
> > {
> >   if (!m_progressString.IsEmpty() && m_progressStringId)
> > You now never set m_progressStringId so this condition always fails. But then
> > once you fix that, SetProgressString never clears m_progressString, so the
> > condition always passes. So you need to fix that too.
> 
> Sorry, made that change, but clearing of m_progressString wasn't done before
> too, so where should I clear it out now?

The way you tried to fix it means that you don't need to.
Flags: needinfo?(neil)
Comment on attachment 772316 [details] [diff] [review]
Patch v4

>   message.AssignLiteral("String ID ");
[String Name here too please]

>+          folderQuotaStatusStringID = NULL;
nullptr please

>-  m_progressStringId = stringId;
>+  m_lastProgressStringId.Truncate();
Different string I'm afraid.

(I wonder whether it's worth filing a bug on finding out whether we need to save those string IDs or not.)

> nsresult      IMAPGetStringByID(int32_t stringID, PRUnichar **aString);
>+nsresult      IMAPGetStringByName(const char* stringName, PRUnichar **aString);
Do you still use IMAPGetStringByID?
Attached patch Patch v4 (revised) (obsolete) — Splinter Review
Sorry for making mistakes again and again & taking so much of time :(
I hope everything is fine this time.
Attachment #772316 - Attachment is obsolete: true
Attachment #772316 - Flags: review?(neil)
Attachment #772759 - Flags: review?(neil)
Comment on attachment 772759 [details] [diff] [review]
Patch v4 (revised)

Nearly there, though you used Length() instead of !IsEmpty() in a couple of places where you only wanted to do a boolean test. r=me with those fixed.

(But I still think we should investigate that last string cache...)
Attachment #772759 - Flags: review?(neil) → review+
Carrying over review from Neil.
And for the last string cache, I think it is needed here:
http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapProtocol.cpp#5064

Should I remove it or continue it with check-in?
Attachment #772759 - Attachment is obsolete: true
Attachment #772907 - Flags: review+
The check probably prevents setting the status to the same string several times in a row as it may flash while redrawing. We may be doing that in some loops like http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapServerResponseParser.cpp#3133 (at least that code looks like a potential loop).
Let's see if Neil agrees with comment 40.
Flags: needinfo?(neil)
Fair enough, I had only looked in nsImapProtocol.cpp itself, so I hadn't spotted that one.
Flags: needinfo?(neil)
I tested this again on an IMAP server and it seems to work.

Suyash, congrats to your first taken bug becoming finished :)
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/1164d550d225
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 25.0
Blocks: 906937
You need to log in before you can comment on or make changes to this bug.