221592 - The "bad password" alert should contain also the name of pop account

Status: RESOLVED FIXED

(MailNews Core :: Networking: POP, enhancement)

Description

[Ermanno Baschiera]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; it-IT; rv:1.5) Gecko/20030925
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; it-IT; rv:1.5) Gecko/20030925

I have several mail accounts and I also have an Antivirus (Avast!) that acts as
a mail proxy for checking emails. So I have to configure MailNews for connecting
to the proxy (the antivirus), that is on the local machine. 
So when the password is wrong, MailNews shows me this alert: "Sending of
password did not succeed. Mail server 127.0.0.1 responded: Bad Login".
Here comes the problem: I cannot understand which account has bad password.

Reproducible: Always

Steps to Reproduce:



Expected Results:  
It would be better if the alert message contains also which account MailNews
can't connect to.

Comment 1

[Robert Accettura [:raccettura]]

I agree.  To most, the server name means nothing.  Their ISP gives them the data
to enter in the fields (once).  Never again do they touch it, or remember it.

It should display something like:

"Sending of password did not succeed for account "[e-mail]" because Mail
server 127.0.0.1 responded: Bad Login".


Don't think this has been filed before, or at least I can't find it.

Comment 2

[Robert Accettura [:raccettura]]

Wish I would be CC'd on bugs by default if I comment... I keep forgetting to add
myself... spamming everyone in the process.

Sorry.

Comment 3

[Joe Infla]

Note that the wording suggested in comment 1 would regress bug 212937, which
avoids confusion over username/server combos that end up looking like
[e-mail]@mail.domain.com.  Anyone providing a patch for this bug please
bear that in mind.  Not sure if that bug is the one timeless thought this would
dupe to...

Comment 4

[Christian Eyrich]

Robert, your message from comment #1 is nice. Joe, the old alert concatenated
two informations (loginname and servername) and resulted in a string which the
user didn't know (and with confusing two @).
If the account name is displayed, I guess the user knows what this is (it's
displayed in the folder pane). Apart from this, account names can also look like
"chris' account" or "Account 1".

About implementing this.
Internally we have two strings ("Sending of password did not succeed." and "Mail
server %S responded: ") which are concatenated and the placeholder filed with
the servers response.
Extending the second string with another placeholder and changing the code would
be easier than add one to the first string. To this this "Mail server %S of
account %S responded: " would add the account to every alert with server response.

What about this?

Comment 5

[Robert Accettura [:raccettura]]

I'd say put the account name first, and the server stuff later.

Reason being, people get confused fast.  

It's more friendly, when the first words are:

Mail account %S's mail server (%s) responded: " would add the account to every 
alert with server response.

Users don't like reading confusing messages.  Putting the point in the 
beginning makes it easier for them to read.

"I don't know server X!  What's server X?"... they don't read any further.

Now they are more likely to know their account name, so they will feel a bit 
more comfortable with it.  

Comment 6

[Daniel Wang]

yup, dupe

*** This bug has been marked as a duplicate of 187406 ***

Comment 7

[Daniel Wang]

sorry... similar but not dupe

Comment 8

[Daniel Wang]

Attachment: patch

I don't have VC++, so I can only hope this compiles

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 132934 [details] [diff] [review]
patch

Sorry for the absurdly long review delay.

>+    passwordTitle = nsTextFormatter::smprintf(titleTemplate, (const char *) acctName);
>+
>+    if (passwordPromptString && passwordTitle)
>     {
>-      if (passwordTitle)
>-        rv =  server->GetPasswordWithUI(passwordPromptString, passwordTitle.get(),
>+        rv =  server->GetPasswordWithUI(passwordPromptString, passwordTitle,
>                                         aMsgWindow, okayValue, aPassword);
>       nsTextFormatter::smprintf_free(passwordPromptString);
>+      nsTextFormatter::smprintf_free(passwordTitle);
>     }

If one but not both of these strings are null, then the other string will be
leaked here.  

Note that it's generally considered good form to have someone build and test a
fix before submitting it for review.  Generally #developers is a good place to
find help with that (and/or help with getting a build working with one of the
free compilers on Windows).

Comment 10

[Wayne Mery (:wsmwk)]

[e-mail] appears to no longer be available. need someone to
pick up where he left off.

Comment 11

[(not reading, please use [e-mail] instead)]

sorry for the spam.  making bugzilla reflect reality as I'm not working on these bugs.  filter on FOOBARCHEESE to remove these in bulk.

Comment 12

[Nikolay Shopik]

This applies for IMAP too, please change title to reflect this

Comment 13

[Wayne Mery (:wsmwk)]

irrc there are other bugs which address imap etc.  
if there's a common solution they can be duped. 
otherwise, probably want to leave this one as is.

Comment 14

[Edmund Wong (:ewong)]

Digging up this old bug and reading the comments, would the following suffice?

Now : Enter your password for %S on %S
      (parameters: username, servername)

     *user enter bad password and clicks on ok*

     Sending of password did not succeed.  Mail Server %S responded:
      Authentication failed (bad password?)
      (parameter: servername)

Proposing:

     Enter your password for mail account %S (%S) on %S.
     (parameter: account name, user name, server name)

     *user enter bad password and clicks on ok*

     Sending of password for mail account %S (%S) did not succeed.
      Mail Server %S responded: Authentication failed (bad password?)
     (parameter: account name, user name, server name)

Comment 15

[Edmund Wong (:ewong)]

I missed the repeat password entry prompt:

Now: Please enter a new password for user %S on %S:
     (user name, server name)


Proposing:  Please enter a new password for mail account %S (for user %S)
            on %S: 
            (account name, user name, server name)

Comment 16

[Ludovic Hirlimann [:Usul]]

Bryan ^^^  which one would you like to see implemented ?

Comment 19

[:aceman]

I am not sure why the patch changes the password dialog ("Enter you password). The original report was about some other error dialog. Maybe after that one the password dialog comes up.

Comment 20

[:aceman]

Needs decision from UI people, CCing bwinton.

Comment 21

[Blake Winton (:bwinton) (:☕️)]

I'm not entirely clear why people want the account name, instead of just the username and server name…

I think I would be fine with:

Enter your password for %S on %S
      (parameters: username, server name)

     *user enter bad password and clicks on ok*

Sending of password for %S on %S did not succeed.
Mail Server responded: Authentication failed (bad password?)
     (parameters: username, server name)

Please enter a new password for %S on %S:
     (parameters: username, server name)

Comment 22

[Rob]

Example of why the account name may be useful:

account1: My Email
user: u1234567890
server name: smtp.server.com

account2: My Spam Mail
user: u1234567891
server name: smtp.server.com

username is not always something intelligible, and the same server can accessed using several names, so people want something which they can recognize.

Comment 23

[Blake Winton (:bwinton) (:☕️)]

Thank you for the example, I can now see how it would be useful to distinguish between accounts sometimes, but the password seems to me to be more closely tied to the user and server, and fairly independent of the name the user has chosen for the account.

(Also, the suggested account name is <user>@<server>, which would leave us with:
  Enter your password for mail account [e-mail] (bwinton) on mozilla.com.
which seems like a lot of needlessly duplicated information.)

Comment 24

[Rob]

My mail accounts are:
Rob Work
Rob Telenet (both on smtp.telenet.be and with an unreadable username like u12346546984)
Rob Gmail (obvious)
(and then some shared accounts)

If I would use the suggested account name I'd be screwed ;)

I have no real vested interest in this issue, I just noticed it and explained why the requested info might be useful.

Comment 25

[Stéphane Ascoët]

Yet another bug that lasts since almost 10 years... It wouldn't be that complicated to change these damn messages patterns, avoiding confusion over people everyday in the world for such years!

I suppose that if I was used to read and modify Mozilla source code by myself, I would be able to easy do the search and replace with Edmund Wong proposal...

You have time to add a in instant messaging module in Thunderbird, but not to correct these dozens of tiny annoying bugs... Shame on you!

Comment 26

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch

Sir,

I have tried to implement what is suggested in the comments. Please let me know what needs to be changed in this.

Comment 27

[:aceman]

Yes, this looks good and works right for what we want to achieve.

I just think this duplicates a lot of code. Can the original (1 argument) Error() variant now call the new variant, like Error(err_code) { return Error(err_code, nullptr, 0); } ?
Also, can HandleNoUidListAvailable now call the new Error, without duplicating the alerting code? This would actually make this patch a gain in code size reduction.

Comment 28

[:aceman]

bwinton, what if we displayed the account name in the title of the dialog (for all pop3 error messages coming via the Error function)? Currently the title is empty (nullptr).

Comment 29

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch (variant1)

Sir,

I have made the changes, and tried to minimize the duplication. Please let me know if this needs to be changed.

Comment 30

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch (variant2)

Sir,

In this patch, I have tried to implement your idea of displaying the mail account name in the title of the prompt instead of Alert (comment 28). Please approve the variant you find appropriate.

Comment 31

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #28)
> bwinton, what if we displayed the account name in the title of the dialog
> (for all pop3 error messages coming via the Error function)? Currently the
> title is empty (nullptr).

I would suggest (also) including the server address, like "Private Account [pop3.example.com]". Esp. for tech-support situations, and other strange special situations that may be helpful.

Comment 32

[Suyash Agarwal (:sshagarwal)]

(In reply to Magnus Melin from comment #31)
 
> I would suggest (also) including the server address, like "Private Account
> [pop3.example.com]". Esp. for tech-support situations, and other strange
> special situations that may be helpful.

Sir,

The server address is displayed by my patches(both of them) in the Error message.

Comment 33

[Magnus Melin [:mkmelin]]

Comment on attachment 751339 [details] [diff] [review]
Patch (variant2)

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

::: mailnews/local/src/nsPop3Protocol.cpp
@@ +1857,5 @@
> +        {
> +          if (m_password_already_sent)
> +            return Error("pop3PasswordFailure", params, 2);
> +          else
> +            return Error("pop3UsernameFailure", params, 1);

no else after return please

Comment 34

[Magnus Melin [:mkmelin]]

(In reply to Suyash Agarwal (:sshagarwal) from comment #32)
> The server address is displayed by my patches(both of them) in the Error
> message.

Well not for the general case, i don't think. E.g. pop3PasswordFailure.
(Didn't actually try the patch though.)

Comment 35

[Suyash Agarwal (:sshagarwal)]

Attachment: Screen Shot

(In reply to Magnus Melin from comment #34)
Sir,

I think, I didn't understand about what part of the patch you were talking about. This is the screen shot of the Error dialog box from patch(variant2). Please let me know if something else is wrong.

Comment 36

[Magnus Melin [:mkmelin]]

I was responding to comment comment #28 about adding the account name to the alert *title* (which sounds like a good idea to me). 

It's indeed shown in the msg text for the case in your screenshot, but in a general case the server name is not necessarily there.

BTW, no need to Sir me, we're usually very informal in bugzilla :)

Comment 37

[:aceman]

Magnus, the "server x.x.x responded: xxx" is tacked automatically after each error message. It is part of the Error() function and can be seen in the 1st variant of the patch.

Comment 38

[:aceman]

Comment on attachment 751339 [details] [diff] [review]
Patch (variant2)

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

::: mailnews/local/src/nsPop3Protocol.cpp
@@ +1287,2 @@
>                if (m_pop3ConData->command_succeeded)  //not a server error message
> +                dialog->Alert(params[0], alertString.get());

params[0] does not exist (params is nullptr) for all the other messages except "pop3PasswordFailure". So this may crash. I think you have to fetch the account name here unconditionally for all messages. Use a new local "params array" and use that as the first argument to Alert().

@@ +1308,5 @@
>                  message.AppendLiteral(" ");
>                  message.Append(serverSaidPrefix);
>                  message.AppendLiteral(" ");
>                  message.Append(NS_ConvertASCIItoUTF16(m_commandResponse));
> +                dialog->Alert(params[0],message.get());

Ditto.

@@ +2545,2 @@
>      m_pop3ConData->next_state = POP3_GET_MSG;
> +    return 0;

Early return, very good.

@@ +2547,5 @@
>    }
> +  m_pop3ConData->next_state = POP3_SEND_QUIT;
> +  nsAutoString hostNameUnicode;
> +  nsCString hostName;
> +  CopyASCIItoUTF16(hostName, hostNameUnicode);

Looks like you also removed initialization of "hostName". What is its value?

Comment 39

[:aceman]

Comment on attachment 751337 [details] [diff] [review]
Patch (variant1)

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

So this would work (with the fix to HandleNoUidListAvailable as written at the other patch), but I'd like better if variant 2 is accepted.

Comment 40

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch (variant2)_v2

Sir,

I have tried to make the changes as you suggested in variant 2.

Comment 41

[:aceman]

Comment on attachment 751472 [details] [diff] [review]
Patch (variant2)_v2

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

::: mailnews/local/src/nsPop3Protocol.cpp
@@ +1265,5 @@
>      // so print out that error message!
>      nsresult rv = NS_OK;
> +    const PRUnichar* accountName = nullptr;
> +    if (params)
> +      accountName = params[0];

I would not depend on params[0] being the account name. Just remove this branch.

@@ +1269,5 @@
> +      accountName = params[0];
> +    else
> +    {
> +      nsCOMPtr<nsIMsgIncomingServer> server = do_QueryInterface(m_pop3Server);
> +      nsString account;

I'd rename this to accountName .

@@ +1272,5 @@
> +      nsCOMPtr<nsIMsgIncomingServer> server = do_QueryInterface(m_pop3Server);
> +      nsString account;
> +      rv = server->GetPrettyName(account);
> +      NS_ENSURE_SUCCESS(rv, -1);
> +      accountName = account.get();

And remove this assignment.

@@ +1298,2 @@
>                if (m_pop3ConData->command_succeeded)  //not a server error message
> +                dialog->Alert(params[0], alertString.get());

Change params[0] -> accountName.get() ?

@@ +1319,5 @@
>                  message.AppendLiteral(" ");
>                  message.Append(serverSaidPrefix);
>                  message.AppendLiteral(" ");
>                  message.Append(NS_ConvertASCIItoUTF16(m_commandResponse));
> +                dialog->Alert(params[0],message.get());

Change params[0] -> accountName.get() ?

@@ +2558,5 @@
>    }
> +  m_pop3ConData->next_state = POP3_SEND_QUIT;
> +  nsAutoString hostNameUnicode;
> +  nsCString hostName;
> +  CopyASCIItoUTF16(hostName, hostNameUnicode);

hostName is still empty...

Comment 42

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch (variant2)_v3

Sir,

I have tried to make the changes.

Comment 43

[:aceman]

Comment on attachment 751478 [details] [diff] [review]
Patch (variant2)_v3

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

::: mailnews/local/src/nsPop3Protocol.cpp
@@ +1283,5 @@
>                nsString alertString;
> +              // Format the alert string if parameter list isin't empty
> +              if (params)
> +                mLocalBundle->FormatStringFromName(NS_ConvertASCIItoUTF16(err_code).get(),
> +                                                   params+1, length, getter_Copies(alertString));

params + 1 ??? Don't send account name in params into the Error function and this can just be 'params'.

@@ +1849,5 @@
> +        // parameter list -> account, user
> +        nsString accountName;
> +        nsCString userName;
> +        nsCOMPtr<nsIMsgIncomingServer> server = do_QueryInterface(m_pop3Server);
> +        rv = server->GetPrettyName(accountName);

You don't need accountName now.

@@ +1854,5 @@
> +        NS_ENSURE_SUCCESS(rv, -1);
> +        rv = server->GetRealUsername(userName);
> +        NS_ENSURE_SUCCESS(rv, -1);
> +        NS_ConvertUTF8toUTF16 userNameUTF16(userName);
> +        const PRUnichar* params[] = { accountName.get(),

You don't need accountName now.

@@ +1860,4 @@
>          if (TestFlag(POP3_STOPLOGIN))
> +        {
> +          if (m_password_already_sent)
> +            return Error("pop3PasswordFailure", params, 2);

There is only %1$S for username in the string so no need to send also account name. params count will be 1.

@@ +1870,5 @@
>          {
>              PR_LOG(POP3LOGMODULE, PR_LOG_DEBUG,
>                 ("auth failure, setting password failed"));
> +            if (m_password_already_sent)
> +              Error("pop3PasswordFailure", params, 2);

params count = 1.

@@ +1872,5 @@
>                 ("auth failure, setting password failed"));
> +            if (m_password_already_sent)
> +              Error("pop3PasswordFailure", params, 2);
> +            else
> +              Error("pop3UsernameFailure", params, 1);

No params needed for "pop3UsernameFailure"...

@@ +1889,5 @@
>              PR_LOG(POP3LOGMODULE, PR_LOG_DEBUG, ("USER username failed"));
>              // if USER auth method failed before sending the password,
>              // the username was wrong.
>              // no fallback but return error
> +            return Error("pop3UsernameFailure", params, 1);

No params needed for "pop3UsernameFailure"...

@@ +1907,5 @@
>                 But if we're just checking for new mail (biff) then don't bother
>                 prompting the user for a password: just fail silently.
>              */
>              SetFlag(POP3_PASSWORD_FAILED);
> +            Error("pop3PasswordFailure", params, 2);

params count = 1.

Comment 44

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch (variant2) v4

Sir,

I have tried to clean up the stuff you mentioned.

Comment 45

[:aceman]

Comment on attachment 751507 [details] [diff] [review]
Patch (variant2) v4

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

This looks good now :)

::: mailnews/local/src/nsPop3Protocol.cpp
@@ +1265,5 @@
>      // so print out that error message!
>      nsresult rv = NS_OK;
> +    nsCOMPtr<nsIMsgIncomingServer> server = do_QueryInterface(m_pop3Server);
> +    nsString accountName;
> +    rv = server->GetPrettyName(accountName);

You can merge this with declaration of rv: nsresult rv = server->GetPrettyName(accountName);

@@ +1280,5 @@
>              rv = msgWindow->GetPromptDialog(getter_AddRefs(dialog));
>              if (NS_SUCCEEDED(rv))
>              {
>                nsString alertString;
> +              // Format the alert string if parameter list isin't empty

Please fix the 'isin't' typo .

@@ +1312,5 @@
>                  message.AppendLiteral(" ");
>                  message.Append(serverSaidPrefix);
>                  message.AppendLiteral(" ");
>                  message.Append(NS_ConvertASCIItoUTF16(m_commandResponse));
> +                dialog->Alert(accountName.get(),message.get());

Space after comma.

@@ +1822,5 @@
>   */
>  int32_t nsPop3Protocol::NextAuthStep()
>  {
>      PR_LOG(POP3LOGMODULE, PR_LOG_MAX, ("NextAuthStep()"));
> +    nsresult rv = NS_OK;

You probably do not need rv at this level.

@@ +1848,5 @@
>          // wrong credential -> stop login without retry or pw dialog, only alert
> +        // parameter list -> user
> +        nsCString userName;
> +        nsCOMPtr<nsIMsgIncomingServer> server = do_QueryInterface(m_pop3Server);
> +        rv = server->GetRealUsername(userName);

Declare rv at this line.

Comment 46

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch (variant2) v4

Sir,

I have made the changes suggested in your comment.

Comment 47

[:aceman]

Comment on attachment 752025 [details] [diff] [review]
Patch (variant2) v4

This looks great now :)

Maybe the window title should not be just plain account name but something like "Error on account <account name>". Let's first see how bwinton likes it.

Comment 48

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 752025 [details] [diff] [review]
Patch (variant2) v4

Yeah, I think I prefer "Error with account <accountname>", but other than that, it seems like an improvement to me.  ui-r=me with that changed.

Thanks,
Blake.

Comment 49

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch (variant2) v5

Sir,

I have made the required change. Please let me know if this needs to be changed.
Carrying over ui-review from bwinton.

Comment 50

[:aceman]

Comment on attachment 752707 [details] [diff] [review]
Patch (variant2) v5

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

I have not compiled this yet, but semantically the dialog title is constructed correctly. So good work Suyash!

::: mail/locales/en-US/chrome/messenger/localMsgs.properties
@@ +7,5 @@
>  #
>  
> +# LOCALIZATION NOTE(pop3ErrorWithAccountDialog): Do not translate the word "%S"
> +# below. Place the word %S where the account name should appear.
> +pop3ErrorWithAccountDialog=Error with account %S

Probably call this "pop3ErrorDialogTitle".

::: mailnews/local/src/nsPop3Protocol.cpp
@@ +1267,5 @@
> +    nsCOMPtr<nsIMsgIncomingServer> server = do_QueryInterface(m_pop3Server);
> +    nsString accountName;
> +    nsresult rv = server->GetPrettyName(accountName);
> +    NS_ENSURE_SUCCESS(rv, -1);
> +    const PRUnichar *errorParams[] = { accountName.get() };

Call it "titleParams".

@@ +1271,5 @@
> +    const PRUnichar *errorParams[] = { accountName.get() };
> +    nsString errorString;
> +    mLocalBundle->FormatStringFromName(
> +      NS_LITERAL_STRING("pop3ErrorWithAccountDialog").get(),
> +      errorParams, 1, getter_Copies(errorString));

This looks fine, just call the variable "dialogTitle", not "errorString", which it isn't.

::: suite/locales/en-US/chrome/mailnews/localMsgs.properties
@@ +66,1 @@
>  

Add the new string pop3ErrorDialogTitle to this file too.

Comment 51

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch (variant2) v5

Sir,

I have made the changes. Probably this bug finishes now.

Comment 52

[Suyash Agarwal (:sshagarwal)]

I forgot to mention. Carrying over review from bwinton.

Comment 53

[:aceman]

Comment on attachment 753258 [details] [diff] [review]
Patch (variant2) v5

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

UI review from bwinton only. We now go to code review ;)

Comment 54

[:aceman]

Neil, ping :)

Comment 55

[[e-mail]]

Comment on attachment 753258 [details] [diff] [review]
Patch (variant2) v5

>+# LOCALIZATION NOTE(pop3ErrorDialogTitle): Do not translate the word "%S"
>+# below. Place the word %S where the account name should appear.
>+pop3ErrorDialogTitle=Error with account %S
(This wording doesn't sound quite right to me but I can't work out what the correct preposition should be.)

> # Status - password failed
>-pop3PasswordFailure=Sending of password did not succeed.
>+#LOCALIZATION NOTE (pop3PasswordFailure): Do not translate "%1$S" below.
>+# Place the word %1$S where the user name should appear.
>+pop3PasswordFailure=Sending of password for user %1$S did not succeed.
Sorry, you have to change the string name at the same time otherwise localisers won't notice the change. (Just be glad that we're not still using numbers.)

> int32_t
> nsPop3Protocol::Error(const char* err_code)
> {
>+  // Since this Error() is a special case of the 3 parameter Error()
>+  // A call to that Error() is made here to avoid duplication.
>+  return Error(err_code, nullptr, 0);
Can we not just use default arguments here?

>+  nsAutoString hostNameUnicode;
>+  nsCString hostName;
>+  nsCOMPtr<nsIMsgIncomingServer> server = do_QueryInterface(m_pop3Server);
>+  nsresult rv = server->GetRealHostName(hostName);
>+  NS_ENSURE_SUCCESS(rv, -1);
>+  CopyASCIItoUTF16(hostName, hostNameUnicode);
NS_ConvertASCIItoUTF16 hostNameUnicode(hostName);

Comment 56

[Suyash Agarwal (:sshagarwal)]

(In reply to [e-mail] from comment #55)
> Comment on attachment 753258 [details] [diff] [review]
> Patch (variant2) v5
> 
> >+# LOCALIZATION NOTE(pop3ErrorDialogTitle): Do not translate the word "%S"
> >+# below. Place the word %S where the account name should appear.
> >+pop3ErrorDialogTitle=Error with account %S
> (This wording doesn't sound quite right to me but I can't work out what the
> correct preposition should be.)

What about just the account name in the title bar?

Comment 57

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch (variant2) v6

Made the suggested changes.

Comment 58

[[e-mail]]

Comment on attachment 779742 [details] [diff] [review]
Patch (variant2) v6

>-pop3PasswordFailure=Sending of password did not succeed.
>+pop3PasswordFailures=Sending of password for user %1$S did not succeed.
Adding an "s" is confusing as there is only one failure at a time.
There are a number of options here:
1. A numeric suffix (starting at 2) is fairly popular
2. You could use a synonym
3. You could use a verb instead of a noun e.g. pop3PasswordFailed

> int32_t
>-nsPop3Protocol::Error(const char* err_code)
>+nsPop3Protocol::Error(const char* err_code,
>+                      const PRUnichar **params = nullptr,
>+                      uint32_t length = 0)
This works, but it's very confusing, so please put the default arguments on the declaration instead.

Comment 59

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch (variant2) v6 (revised)

Okay, made the changes.

Thanks.

Comment 60

[:aceman]

Comment on attachment 780189 [details] [diff] [review]
Patch (variant2) v6 (revised)

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

I haven't run with this this time, but the code looks nice. Thanks.

Please fix the patch description line to say that all POP error dialogs will show an account name now.

::: mailnews/local/src/nsPop3Protocol.cpp
@@ +2542,5 @@
> +  nsresult rv = server->GetRealHostName(hostName);
> +  NS_ENSURE_SUCCESS(rv, -1);
> +  NS_ConvertASCIItoUTF16 hostNameUnicode(hostName);
> +  const PRUnichar *formatStrings[] = { hostNameUnicode.get() };
> +  return Error("pop3ServerDoesNotSupportUidlEtc", formatStrings, 1);  

I know you copied the formatStrings variable name, but I think it is misleading. The "pop3ServerDoesNotSupportUidlEtc" is the "format string" and hostNameUnicode.get() is the argument/parameter for it. Notice you call the variable "params" at other places so please do so here too.

Comment 61

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch (variant2) v6 (revisited)

Okay sir, done!

Comment 62

[Suyash Agarwal (:sshagarwal)]

carrying over r=Neil, ui-r=bwinton, f=aceman

Comment 63

[:aceman]

Thanks :)

Comment 64

[Mark Banner (:standard8)]

https://hg.mozilla.org/comm-central/rev/e2a13615d5d3

Comment 65

[Ludovic Hirlimann [:Usul]]

This looks like a nice fix for people doing support porting it to 24 would make people's life easier a year before 31 gets out.

Comment 66

[:aceman]

Yes, but it has string changes so the chances are probably low.

Comment 67

[Mark Banner (:standard8)]

Correct, this contains string changes, so we can't take it forward to 24.