display account name for all messages in status bar when checking mail

RESOLVED FIXED in Thunderbird 27.0

Status

MailNews Core
Networking
--
enhancement
RESOLVED FIXED
15 years ago
5 years ago

People

(Reporter: Seairth Jacobs, Assigned: sshagarwal)

Tracking

(Blocks: 1 bug)

Trunk
Thunderbird 27.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

15 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5) Gecko/20030925
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5) Gecko/20030925

When checking mail for multiple accounts, it is not obvious which account is
currently being checked.  A "connecting" message sometimes show very briefly in
the status bar (depending on how slow the server responds), then is replaced
with a message about sending of login information.  Instead, it would be nice to
see the associated account at all times.  For intance:

account_name: Contacting...
account_name: Host contact, sending login information...
account_name: etc.

Reproducible: Always

Steps to Reproduce:

Comment 1

14 years ago

*** This bug has been marked as a duplicate of 66860 ***
Status: UNCONFIRMED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → DUPLICATE
Product: Browser → Seamonkey

Comment 2

5 years ago
I've noticed that even after bug 214854, when connecting to IMAP server these 2 messages still are not prefixed with the account name. It is possible these connection messages are shown by some shared code that is used by more account types (e.g. POP and IMAP). This needs to be investigated so I propose to split this task out of the already crowded bug 66860.
Assignee: sspitzer → syshagarwal
Blocks: 66860
Status: RESOLVED → REOPENED
Component: MailNews: Message Display → Networking
Ever confirmed: true
OS: Windows XP → All
Product: SeaMonkey → MailNews Core
Hardware: x86 → All
Resolution: DUPLICATE → ---

Updated

5 years ago
QA Contact: esther
(Assignee)

Comment 3

5 years ago
Created attachment 816744 [details] [diff] [review]
Patch v1

This patch completes all of the status messages for imap and covers most of the pop3 and nntp status messages.
Attachment #816744 - Flags: feedback?(acelists)

Comment 4

5 years ago
Comment on attachment 816744 [details] [diff] [review]
Patch v1

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

This works great for me. It covers the pop, imap and news "connecting to server" messages that are common. That is all that is needed for this bug.

::: mail/locales/en-US/chrome/messenger/messenger.properties
@@ +7,5 @@
> +
> +#LOCALIZATION NOTE(statusMessage): Do not translate the words
> +# $1S and $2S below. Place the word $1S where account name should appear and $2S
> +# where the status message should appear.
> +statusMessage=%1$S: %2$S

Can this be shared with the statusMessage string you have created for the IMAP specific strings? If it does not mean pulling whole messenger.properties into the IMAP module.
We probably want to keep this string as single to keep the format the same across all messages in all server types.

::: mailnews/base/util/nsMsgProtocol.cpp
@@ +749,5 @@
> +  nsCOMPtr<nsIStringBundle> bundle;
> +  nsCOMPtr<nsIStringBundleService> sbs =
> +    mozilla::services::GetStringBundleService();
> +  NS_ENSURE_TRUE(sbs, NS_ERROR_UNEXPECTED);
> +  rv = sbs->CreateBundle(MSGS_URL, getter_AddRefs(bundle));

Do we need to create this bundle for each string? Isn't that slow. Can't it be taken from other place or at least cached?
Attachment #816744 - Flags: feedback?(acelists) → feedback+
(Assignee)

Updated

5 years ago
Attachment #816744 - Flags: review?(neil)

Comment 5

5 years ago
Comment on attachment 816744 [details] [diff] [review]
Patch v1

>+NS_IMETHODIMP
This is only used for methods declared in IDL files.

>+nsMsgProtocol::FrameStatusString(nsCOMPtr<nsIMsgIncomingServer> server,
>+                                 nsresult aStatus, nsAutoCString &aStatusMessage)
This doesn't belong here (if it needs to be a separate function, then it would probably go in nsMsgUtils.cpp).
Use pointers for interface arguments.
Use nsA(C)String for string arguments where possible, otherwise ns(C)String.

>+  rv = sbs->FormatStatusMessage(
>+    aStatus, NS_ConvertUTF8toUTF16(aStatusMessage).get(),
>+    getter_Copies(partMessage));
nsMsgStatusHandler::OnStatus is still calling FormatStatusMessage, so you're now duplicating effort (maybe this code belongs there.)

>+  nsCOMPtr<nsIMsgIncomingServer> server = nullptr;
nsCOMPtr already defaults to null.

>+  nsImapProtocol imp;
This is just wrong.
Attachment #816744 - Flags: review?(neil) → review-
(Assignee)

Comment 6

5 years ago
Created attachment 817276 [details] [diff] [review]
Patch v2

Thanks :)
nsMsgUtils.cpp was the file I was looking for.
Attachment #816744 - Attachment is obsolete: true
Attachment #817276 - Flags: review?(neil)

Comment 7

5 years ago
Comment on attachment 817276 [details] [diff] [review]
Patch v2

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

::: mailnews/base/util/nsMsgProtocol.cpp
@@ +733,5 @@
>    return NS_OK;
>  }
>  
> +#define MSGS_URL    "chrome://messenger/locale/messenger.properties"
> +

Still needed here?

::: mailnews/base/util/nsMsgUtils.cpp
@@ +182,5 @@
> +  nsCOMPtr<nsIStringBundle> bundle;
> +  nsCOMPtr<nsIStringBundleService> sbs =
> +    mozilla::services::GetStringBundleService();
> +  NS_ENSURE_TRUE(sbs, NS_ERROR_UNEXPECTED);
> +  rv = sbs->CreateBundle(MSGS_URL, getter_AddRefs(bundle));

What about the cache?

::: mailnews/base/util/nsMsgUtils.h
@@ +5,5 @@
>  
>  #ifndef _NSMSGUTILS_H
>  #define _NSMSGUTILS_H
>  
> +#include "nsIMsgIncomingServer.h"

Still needed?

Comment 8

5 years ago
(In reply to comment #5)
> (From update of attachment 816744 [details] [diff] [review])
> >+  rv = sbs->FormatStatusMessage(
> >+    aStatus, NS_ConvertUTF8toUTF16(aStatusMessage).get(),
> >+    getter_Copies(partMessage));
> nsMsgStatusHandler::OnStatus is still calling FormatStatusMessage, so you're
> now duplicating effort (maybe this code belongs there.)
I meant to say, you could possibly move this code into OnStatus.
You only have an nsIRequest so you need to QI that to nsIChannel.
You can then use GetURI to get an nsIURI.
You then need to QI that to an nsIMsgMailNewsUrl etc.
(Assignee)

Comment 9

5 years ago
Created attachment 817448 [details] [diff] [review]
Patch v3

Thanks :)
Attachment #817276 - Attachment is obsolete: true
Attachment #817276 - Flags: review?(neil)
Attachment #817448 - Flags: review?(neil)
Comment on attachment 817448 [details] [diff] [review]
Patch v3

>+  if (!str.IsEmpty() && PL_strstr(NS_ConvertUTF16toUTF8(str).get(),
>+       NS_ConvertUTF16toUTF8(accountName).get()) == nullptr)
Did you mean !accountName.IsEmpty()?
You probably wanted to use str.Find(accountName) == kNotFound.

Do you know which messages already include the account name?

>-        NS_LITERAL_STRING("imapStatusMessage").get(),
>+        NS_LITERAL_STRING("statusMessage").get(),
I'm not sure this will work, you moved the message to a different bundle.
(Assignee)

Comment 11

5 years ago
Created attachment 819380 [details] [diff] [review]
Patch v4

(In reply to neil@parkwaycc.co.uk from comment #10)
> Comment on attachment 817448 [details] [diff] [review]
> Patch v3
> 
> >+  if (!str.IsEmpty() && PL_strstr(NS_ConvertUTF16toUTF8(str).get(),
> >+       NS_ConvertUTF16toUTF8(accountName).get()) == nullptr)
> Did you mean !accountName.IsEmpty()?
No I wanted to check str itself, while writing I observed status messages with only account name, that showed that there were a few blank status messages too, so I thought to check them here.
> 
> Do you know which messages already include the account name?
Ya, there were a few from bug 214854, but I think I fixed that in this bug, still I am keeping this check for the case if I missed something.
Attachment #817448 - Attachment is obsolete: true
Attachment #817448 - Flags: review?(neil)
Attachment #819380 - Flags: review?(neil)
Comment on attachment 819380 [details] [diff] [review]
Patch v4

>       DisplayStatusMsg(imapUrl, progressString);
I should have looked at this harder. It looks like DisplayStatusMsg calls OnStatus, which means that this code doesn't need to prefix the account name now that OnStatus does. Should we remove it?
(Assignee)

Comment 13

5 years ago
Created attachment 819427 [details] [diff] [review]
Patch v4.1

Okay, left everything in OnStatus().
Still there are a few status messages (I observed one in POP3 that has account name already in the status message) so I have left the account name check in OnStatus().
Attachment #819380 - Attachment is obsolete: true
Attachment #819380 - Flags: review?(neil)
Attachment #819427 - Flags: review?(neil)
Comment on attachment 819427 [details] [diff] [review]
Patch v4.1

Please double-check for any redundant account names in other status messages and file bugs if you find any. Thank you.
Attachment #819427 - Flags: review?(neil) → review+
(Assignee)

Comment 15

5 years ago
Okay, Thanks.

If possible, please check this in on the 1st of November (its 10th anniversary) :)
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/797bb5052d35
Status: REOPENED → RESOLVED
Last Resolved: 14 years ago5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 27.0
You need to log in before you can comment on or make changes to this bug.