66016 - Alert messages are hardcoded in nsMsgProtocol.cpp

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Jessica Blanco]

"Failed to connect to the server", "Connection refused to the server", 
"Connection to the server timed out", and "unknown error" are strings 
in nsMsgProtocol.cpp so they are unable to be translated.

Comment 1

[Asa Dotzler [:asa]]

over to mailnews product and mscott

Comment 2

[Doron Rosenberg (IBM)]

gonna try to patch this

Comment 3

[Jessica Blanco]

Attachment: Added reference numbers for error strings used in imapMsgs.properties

Comment 4

[Jessica Blanco]

Attachment: nsMsgProtocol.cpp - Using the errors defined in nsMsgProtocol.h to get the strings defined in imapMsgs.properties

Comment 5

[Doron Rosenberg (IBM)]

hmm, I talked to sspitzer about this and he said they should be moved to 
messenger.properties and we would change the code to read the imap errors from 
the messenger.properties as well.

btw, you would need "unknown error" defined in the bundle as well, that should 
be localisable as well.

Comment 6

[Jessica Blanco]

Are you saying that imapMsgs.properties should be removed and merged 
with messenger.properties?

Comment 7

[Doron Rosenberg (IBM)]

no, just perhaps the warning that are global to mailnews (not pop or imap
specific) should go into messenger.properties. cc: sspitzer

perhaps we should move the unknown error message to the default: switch as well?

Comment 8

[Jessica Blanco]

Attachment: Add common errors to the end of messenger.properties - others can be added. Take out common errors from files like ImapMsgs.properties

Comment 9

[Jessica Blanco]

Attachment: Would now use messenger.properties to get the error strings, setting default to Unknown

Comment 10

[Jessica Blanco]

Attachment: Define reference numbers for error strings used

Comment 11

[Håkan Waara]

Jessica, the code looks great. But I have some suggestions before I can stamp 
this with my r=;

+## @name UNKNOWN_ERROR
+## @loc None

As the other strings in that file don't use it, I think those (javadoc?) 
comments are useless.


+#define UNKNOWN_ERROR			  101

Remove the tabs used at that line.

Other than that, everything looks fine to me. Also, can you please run the cvs 
diff comment in the highest common ancestor directory next time? That way all 
these patches will fit in the same file.  

When you fix this and apply the new patch, r=hwaara.

Comment 12

[Jessica Blanco]

Attachment: attachment with changes. Should the imap errors be looked at as mentioned above by Doron?

Comment 13

[Håkan Waara]

I don't really understand your question, but I think your last patch looks good.
 It's up to the super-reviewer of this patch to answer your question; Seth, Scott?

Comment 14

[Alec Flett]

1) how old is your tree? nsIStringBundleService->CreateBundle no longer takes a
"locale" parameter
2) instead of giant nested if()'s, why don't you just return nsnull whenever
there is a failure?
3) you're leaking every string - you return with ToNewUnicode() but pass that
pointer directly to Alert()
4) You're using nsString as a local, automatic variable. use nsAutoString
5) resultString is declared within the scope of the function, rather than in the
place where it is used
6) you're using AssignWithConversion instead of using NS_LITERAL_STRING("foo").get()
7) you're only actually using resultString when ONE of the failure conditions
happens. Why not just return NULL from GetStringByID() and handle ITS failure by
setting up your [StringID###] string?
8) GetStringByID should be static, since it's not used anywhere else

Comment 15

[Jessica Blanco]

Alec, I have modified the code based on your comments. I 
don't quite understand comment #6. I did pass back null 
from the GetStringByID function and handle the failure 
before the alert. Also added in the unknown error (aStatus) 
after "Unknown Error".

Comment 16

[Jessica Blanco]

Attachment: modified nsMsgProtocol.cpp

Comment 17

[Alec Flett]

my comment #6 meant to replace this:
+         nsAutoString resultString;
+         resultString.AppendWithConversion("[StringID ");
+         resultString.AppendInt(errorID, 10);
+         resultString.AppendWithConversion("?]");

with:
+         nsAutoString resultString(NS_LITERAL_STRING("[StringID ");
+         resultString.AppendInt(errorID, 10);
+         resultString.Append(NS_LITERAL_STRING("?]"));

and similarly for errorString.

The other thing I forgot to mention is that you should be using nsCOMPtr<> for
the nsIStringBundle. On a side note, you should never call ->Release() directly,
you should always use NS_RELEASE() (though that will be a moot point when you
switch to nsCOMPtr)

Comment 18

[Jessica Blanco]

Attachment: made changes to nsMsgProtocol.cpp

Comment 19

[Alec Flett]

hate to say it, but you're still leaking an errorMsg, just noticed it - in the
handling for UNKNOWN_ERROR, you're dropping the old value when you assign a new one.

I wasn't going to comment on this, but since I'm here :) - why make an
additional char* pointer "propertyURL" to point to MSGS_URL? Why not just pass
in MSGS_URL directly to CreateBundle?

Comment 20

[Jessica Blanco]

Attachment: Thanks for the input, Alec.

Comment 21

[Alec Flett]

yay! sr=alecf

Comment 22

[Håkan Waara]

r=hwaara

Comment 23

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to 0.9.1 
(on behalf of driver)

Comment 24

[Mike Kaply [:mkaply]]

Fix checked in to branch and trunk.

Comment 25

[Jessica Blanco]

Verified - fixed