Closed Bug 221592 Opened 16 years ago Closed 6 years ago

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

Categories

(MailNews Core :: Networking: POP, enhancement)

enhancement
Not set

Tracking

(thunderbird24-)

RESOLVED FIXED
Thunderbird 25.0
Tracking Status
thunderbird24 - ---

People

(Reporter: ermannob, Assigned: sshagarwal)

References

(Blocks 1 open bug)

Details

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

Attachments

(2 files, 12 obsolete files)

34.24 KB, image/png
Details
14.82 KB, patch
sshagarwal
: review+
sshagarwal
: ui-review+
sshagarwal
: feedback+
Details | Diff | Splinter Review
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.
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 "joe@someisp.com" 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.
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.
Whiteboard: DUPEME
Note that the wording suggested in comment 1 would regress bug 212937, which
avoids confusion over username/server combos that end up looking like
user@domain.com@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...
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?
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.  
yup, dupe

*** This bug has been marked as a duplicate of 187406 ***
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → DUPLICATE
Whiteboard: DUPEME
sorry... similar but not dupe
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Attached patch patch (obsolete) — Splinter Review
I don't have VC++, so I can only hope this compiles
Attachment #132934 - Flags: review?(dmose)
Depends on: 66860
Product: MailNews → Core
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).
Attachment #132934 - Flags: review?(dmose) → review-
OS: Windows XP → All
Hardware: PC → All
danielwang@mozillanews.org appears to no longer be available. need someone to
pick up where he left off.
Keywords: helpwanted
Summary: The "bad password" alert should containt also the name of the account → The "bad password" alert should contain also the name of the account
Summary: The "bad password" alert should contain also the name of the account → The "bad password" alert should contain also the name of pop account
Whiteboard: unlovedpatch
sorry for the spam.  making bugzilla reflect reality as I'm not working on these bugs.  filter on FOOBARCHEESE to remove these in bulk.
Assignee: sspitzer → nobody
Status: REOPENED → NEW
QA Contact: esther → networking.pop
Whiteboard: unlovedpatch → unlovedpatch [good first bug]
This applies for IMAP too, please change title to reflect this
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.
Product: Core → MailNews Core
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)
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)
Bryan ^^^  which one would you like to see implemented ?
Duplicate of this bug: 479777
Duplicate of this bug: 689999
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.
Whiteboard: unlovedpatch [good first bug] → [patchlove][needs new assignee][good first bug]
Needs decision from UI people, CCing bwinton.
Assignee: nobody → acelists
Keywords: uiwanted
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)
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.
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 bwinton@mozilla.com (bwinton) on mozilla.com.
which seems like a lot of needlessly duplicated information.)
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.
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!
Assignee: acelists → syshagarwal
Blocks: 66860
Depends on: 858238
No longer depends on: 66860
Attached patch Patch (obsolete) — Splinter Review
Sir,

I have tried to implement what is suggested in the comments. Please let me know what needs to be changed in this.
Attachment #132934 - Attachment is obsolete: true
Attachment #751017 - Flags: feedback?(acelists)
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.
Status: NEW → ASSIGNED
Keywords: helpwanted
Whiteboard: [patchlove][needs new assignee][good first bug] → [good first bug]
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).
Flags: needinfo?(bwinton)
Attached patch Patch (variant1) (obsolete) — Splinter Review
Sir,

I have made the changes, and tried to minimize the duplication. Please let me know if this needs to be changed.
Attachment #751017 - Attachment is obsolete: true
Attachment #751017 - Flags: feedback?(acelists)
Attachment #751337 - Flags: feedback?(acelists)
Attached patch Patch (variant2) (obsolete) — Splinter Review
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.
Attachment #751339 - Flags: feedback?(acelists)
(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.
(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 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
(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.)
Attached image 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.
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 :)
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 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?
Attachment #751339 - Flags: feedback?(acelists) → feedback-
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.
Attachment #751337 - Flags: feedback?(acelists) → feedback+
Attached patch Patch (variant2)_v2 (obsolete) — Splinter Review
Sir,

I have tried to make the changes as you suggested in variant 2.
Attachment #751339 - Attachment is obsolete: true
Attachment #751472 - Flags: feedback?(acelists)
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...
Attachment #751472 - Flags: feedback?(acelists) → feedback-
Attached patch Patch (variant2)_v3 (obsolete) — Splinter Review
Sir,

I have tried to make the changes.
Attachment #751472 - Attachment is obsolete: true
Attachment #751478 - Flags: feedback?(acelists)
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.
Attachment #751478 - Flags: feedback?(acelists) → feedback-
Attached patch Patch (variant2) v4 (obsolete) — Splinter Review
Sir,

I have tried to clean up the stuff you mentioned.
Attachment #751478 - Attachment is obsolete: true
Attachment #751507 - Flags: feedback?(acelists)
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.
Attachment #751507 - Flags: feedback?(acelists) → feedback+
Whiteboard: [good first bug] → [good first bug][mentor=aceman][lang=c++]
Attached patch Patch (variant2) v4 (obsolete) — Splinter Review
Sir,

I have made the changes suggested in your comment.
Attachment #751507 - Attachment is obsolete: true
Attachment #752025 - Flags: feedback?(acelists)
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.
Attachment #752025 - Flags: ui-review?(bwinton)
Attachment #752025 - Flags: feedback?(acelists)
Attachment #752025 - Flags: feedback+
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.
Attachment #752025 - Flags: ui-review?(bwinton) → ui-review+
Flags: needinfo?(bwinton)
Attached patch Patch (variant2) v5 (obsolete) — Splinter Review
Sir,

I have made the required change. Please let me know if this needs to be changed.
Carrying over ui-review from bwinton.
Attachment #752025 - Attachment is obsolete: true
Attachment #752707 - Flags: ui-review+
Attachment #752707 - Flags: feedback?(acelists)
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.
Attachment #752707 - Flags: feedback?(acelists) → feedback+
Attached patch Patch (variant2) v5 (obsolete) — Splinter Review
Sir,

I have made the changes. Probably this bug finishes now.
Attachment #752707 - Attachment is obsolete: true
Attachment #753258 - Flags: review+
Attachment #753258 - Flags: feedback?(acelists)
I forgot to mention. Carrying over review from bwinton.
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 ;)
Attachment #753258 - Flags: ui-review+
Attachment #753258 - Flags: review?(neil)
Attachment #753258 - Flags: review+
Attachment #753258 - Flags: feedback?(acelists)
Attachment #753258 - Flags: feedback+
Neil, ping :)
Flags: needinfo?(neil)
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);
Attachment #753258 - Flags: review?(neil) → review-
(In reply to neil@parkwaycc.co.uk 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?
Flags: needinfo?(neil)
Flags: needinfo?(neil)
Attached patch Patch (variant2) v6 (obsolete) — Splinter Review
Made the suggested changes.
Attachment #751337 - Attachment is obsolete: true
Attachment #753258 - Attachment is obsolete: true
Attachment #779742 - Flags: ui-review+
Attachment #779742 - Flags: review?(neil)
Flags: needinfo?(neil)
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.
Attachment #779742 - Flags: review?(neil) → review+
Attached patch Patch (variant2) v6 (revised) (obsolete) — Splinter Review
Okay, made the changes.

Thanks.
Attachment #779742 - Attachment is obsolete: true
Attachment #780189 - Flags: ui-review+
Attachment #780189 - Flags: review+
Keywords: checkin-needed
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.
Attachment #780189 - Flags: feedback+
Okay sir, done!
Attachment #780189 - Attachment is obsolete: true
Attachment #780283 - Flags: ui-review+
Attachment #780283 - Flags: review+
Attachment #780283 - Flags: feedback+
carrying over r=Neil, ui-r=bwinton, f=aceman
Thanks :)
Keywords: uiwanted
https://hg.mozilla.org/comm-central/rev/e2a13615d5d3
Status: ASSIGNED → RESOLVED
Closed: 16 years ago6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 25.0
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.
Yes, but it has string changes so the chances are probably low.
Correct, this contains string changes, so we can't take it forward to 24.
Blocks: 592235
You need to log in before you can comment on or make changes to this bug.