86233 - [RFE] Show account name in status bar when downloading messages from POP servers (Getting mail from multiple accounts: confusing status messages)

Status: RESOLVED FIXED

(MailNews Core :: Networking: POP, enhancement)

Description

[Erich 'Ricky' Iseli]

I have 2 pop accounts ("home" and "work"). When getting mail from both of them,
I get alternating messages like:
Receiving: message 3 of 15
Receiving: message 2 of 24
Receiving: message 4 of 15
Receiving: message 3 of 24
and so on.

I'd suggest using this instead:
Receiving mail from home account: message 3 of 15
Receiving mail from work account: message 2 of 24

It could even be a good idea to fetch the mails of one account after the other,
instead of checking all accounts at the same time. This could be more work to
implement and would maybe even need another bug report.

Comment 1

[Alfonso Martinez]

*** Bug 94405 has been marked as a duplicate of this bug. ***

Comment 2

[Eyal Lior]

Change the status to "too lazy to fix" and there you go....

Comment 3

[Brant Gurganus]

[RFE] is deprecated in favor of severity: enhancement.  They have the same meaning.

Comment 4

[rob cherny]

i really don't think there's good feedback right now on getting mail messages
from multiple accounts.  in both eudora and outlook there's a progress dialog
and download status meters (at least there's a meter in eudora) but right now, i
never know which account is doing what, nor can i cancel the checking of a
specific account mid-check -- all i can do is click stop. Stop what?

Comment 5

[Aleksey Nogin]

*** Bug 225591 has been marked as a duplicate of this bug. ***

Comment 6

[Aleksey Nogin]

From dup bug 225591:

> When more than one account is used, there is no way to see the status of
> messages being downloaded, e.g. status bar says "There are no new messages on
> server." after checking first account, but there are more accounts to check!
[...]
> Expected Results:  
> Show a message like: "4 of 7 accounts checked, 15 of 17 messages downloaded",
> or show a pop-up window with more detailed information.

I like the suggested status message (but hate the popup idea), not sure how hard
it will be to implement something like that.

Comment 7

[Alex Maneu]

Additionally, I think that no message box should appear when an account could
not be accessed. It is really annoying having to click a message box and not
being able to browse your messages.

I propose a solution similar to what apple mail does: 
- Use the statusbar to show from which account are mails being downloaded.
- Have the possibility to open a window to indicate progress (with a progressbar
or some similar UI element). This window would not open every time the mail
accounts are checked, thus not disturbing the user.
- Show a warning icon or message when an account could not be accessed, but do
not pop up a message box every time mozilla tries to connect to that account.

Comment 8

[Bernard Levy]

With multiple e-mail accounts, upon starting mailnews with 3 or more accounts
with new mail get only one account in notification (biff) msg, then when pass
cursor over envelope shows at most 2 accounts.  Then need to look at green arrows.

Would like biff and/or passover to show more accounts with new mail.

Comment 9

[Michel Meyers]

As always I would suggest to make it an option. I, for one, would like to get a
popup when TB fails to access an account but wouldn't want the status window to
pop up everytime I check mail. I can imagine some people would like the exact
opposite.

Comment 10

[rob cherny]

The Eudora model is a collapsible window pane which optionally is shown. You can
see activity, and if you want more information you can view it -- even stop an
individual account from checking by right-clicking.

I think that's probably the best thing to do, because users not interested in
additional feedback aren't forced to stare at a pop-up, such as in Outlook. This
way the user is shown feedback if they would like to see it.

Another option might be to come up with a way to incorporate it into the list of
accounts / folders list. Some type of animated icon next to or underneath the
account in question (similar to the icon on the Tabs in FB). Perhaps clicking on
each item w/ the account would change the status bar messages to the appropriate
account's information (downloading 1 of 3, 2 of 3, etc).

Comment 11

[louis bennett]

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

Comment 12

[:aceman]

So going by the original description, we can use this bug to add the account name to the status bar messages for POP3 accounts. Bug 66860 is getting crowded so I separate the tasks to do into separate bugs.

Comment 13

[:aceman]

This is still valid and to do. It seems the "downloading x of Y messages" status was already changed to "downloading x of Y messages to <account name>". Not sure if that is intentional or is should be updated to the new common format "account name: message" .

Comment 14

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v1

This patch makes the status messages conform with the other status messages:
<accountName> : downloading x message of y

Comment 15

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v1.5

Made a few more changes, tried to cover most of the messages.

Comment 16

[:aceman]

Comment on attachment 8391733 [details] [diff] [review]
Patch v1.5

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

Now I like the string changes. You removed %3$S where it contained account name, but kept places where host name was put. That is good.
I tested this and the proper strings show up when downloading from POP3. I just propose some code shuffling below.

::: mail/locales/en-US/chrome/messenger/localMsgs.properties
@@ +8,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

It seems you actually never use this.
You fetch statusMessage from messenger.properties, not from here (in FormStatusMessage). But I wonder whether we should fetch this one using already initialized mLocalBundle instead of always creating the messenger.properties bundle...

@@ +32,3 @@
>  # Place the word %1$S where the number of messages downloaded so far should appear.
>  # Place the word %2$S where the total number of messages to receive should appear;
> +receivingMsgs=Downloading message %1$S of %2$S

Please append ellipsis (…) here to be in line with the other texts describing operations in progress.

@@ +35,3 @@
>  
>  # Status - connecting to host
> +hostContacted=Connect: Host contacted, sending login information…

Unfortunately, you now need to change all these touched strings IDs :( Also in the Seamonkey file.

@@ +44,3 @@
>  # %1$S will receive the number of messages received
>  # %2$S will receive the total number of messages
> +receivedMessages=Received %1$S of %2$S messages

Unfortunately, you now need to change all these touched strings IDs :(

::: mailnews/local/src/nsPop3Protocol.cpp
@@ +3181,5 @@
>        NS_ASSERTION(NS_SUCCEEDED(rv), "couldn't format string");
> +      if (m_statusFeedback) {
> +        nsString statusMessage;
> +        FormStatusMessage(finalString.get(), statusMessage);
> +        m_statusFeedback->ShowStatusString(statusMessage);

I wonder why this is not using UpdateStatusWithString?
I see this uses m_statusFeedback->ShowStatusString() and UpdateStatusWithString would call mProgressEventSink->OnStatus(). But why the difference? This is the only call to m_statusFeedback->ShowStatusString in this file. Maybe Neil would know?

@@ +3850,5 @@
>      case POP3_FINISH_OBTAIN_PASSWORD_BEFORE_USERNAME:
> +      mLocalBundle->GetStringFromName(MOZ_UTF16("hostContacted"),
> +                                      getter_Copies(statusString));
> +      FormStatusMessage(statusString.get(), statusMessage);
> +      UpdateStatusWithString(statusMessage.get());

Somehow I do not like all this duplication.
Can you keep UpdateStatus and make it do these 3 calls?

@@ +4033,5 @@
>                                   m_pop3ConData->really_new_messages,
>                                   statusString);
> +        if (NS_SUCCEEDED(rv)) {
> +          FormStatusMessage(statusString.get(), statusMessage);
> +          UpdateStatusWithString(statusMessage.get());

Could UpdateStatusWithString call FormStatusMessage internally?

Actually I imagine this:
UpdateStatus(stringID) {
  return UpdateStatusWithString(mlocalBundle->GetString(stringID))
}

UpdateStatusWithString(string) {
  retrieve "statusMessage"
  and format the message="account : <string>"

}

So FormStatusMessage would be inside UpdateStatusWithString, not a separate function.

::: suite/locales/en-US/chrome/mailnews/localMsgs.properties
@@ +8,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

Unused for now.

@@ +32,3 @@
>  # Place the word %1$S where the number of messages downloaded so far should appear.
>  # Place the word %2$S where the total number of messages to receive should appear;
> +receivingMsgs=Downloading message %1$S of %2$S

Please append ellipsis (…) here to be in line with the other texts describing operations in progress.

Comment 17

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2

(In reply to :aceman from comment #16)
> ::: mail/locales/en-US/chrome/messenger/localMsgs.properties
> @@ +8,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

> It seems you actually never use this.
> You fetch statusMessage from messenger.properties, not from here (in
> FormStatusMessage). But I wonder whether we should fetch this one using
> already initialized mLocalBundle instead of always creating the
> messenger.properties bundle...

Ya, or we remove this and use the one in messenger.properties. That will be helpful in maintaining consistency I think.

> ::: mailnews/local/src/nsPop3Protocol.cpp
> @@ +3181,5 @@
> >        NS_ASSERTION(NS_SUCCEEDED(rv), "couldn't format string");
> > +      if (m_statusFeedback) {
> > +        nsString statusMessage;
> > +        FormStatusMessage(finalString.get(), statusMessage);
> > +        m_statusFeedback->ShowStatusString(statusMessage);
> 
> I wonder why this is not using UpdateStatusWithString?
> I see this uses m_statusFeedback->ShowStatusString() and
> UpdateStatusWithString would call mProgressEventSink->OnStatus(). But why
> the difference? This is the only call to m_statusFeedback->ShowStatusString
> in this file. Maybe Neil would know?

IIRC, m_statusFeedback->ShowStatusString() makes the status message persist until
another status message appears whereas OnStatus() simply shows it once and leaves (or vice-versa).

> Could UpdateStatusWithString call FormStatusMessage internally?
I think not, because of the above case of ShowStatusString().

Thanks.

Comment 18

[:aceman]

Comment on attachment 8398550 [details] [diff] [review]
Patch v2

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

Yes, I like this much better. Thanks.

::: mail/locales/en-US/chrome/messenger/localMsgs.properties
@@ +8,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

This is still unused. Depends on how we solve the caching in FormStatusMessage.

@@ +27,5 @@
>  # user name should appear, and %2$S where the host name should appear.
>  pop3PreviouslyEnteredPasswordIsInvalidPrompt=Please enter a new password for user %1$S on %2$S:
>  
> +# Status - Downloading message n of m
> +#LOCALIZATION NOTE (receivingMessages): Do not translate %1$S or %2$Sin the following lines.

Space before "in".

@@ +44,3 @@
>  # %1$S will receive the number of messages received
>  # %2$S will receive the total number of messages
> +receivedMsg s=Received %1$S of %2$S messages

Is there really a space inside the ID 'receivedMsg s' ? :)

::: mailnews/local/src/nsPop3Protocol.cpp
@@ +647,5 @@
> +  nsCOMPtr<nsIStringBundleService> sbs =
> +    mozilla::services::GetStringBundleService();
> +  nsCOMPtr<nsIStringBundle> bundle;
> +  rv = sbs->CreateBundle(MSGS_URL,
> +                         getter_AddRefs(bundle));

So I wonder how we could cache this to not recreate the messenger.properties bundle everytime. Maybe Neil would know a clever way (short of adding a member variable like mLocalBundle already is).

@@ +650,5 @@
> +  rv = sbs->CreateBundle(MSGS_URL,
> +                         getter_AddRefs(bundle));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  const char16_t *params[] = { accountName.get(), aStatusString };
> +  bundle->FormatStringFromName(MOZ_UTF16("statusMessage"),

rv = bundle->FormatStringFromName(...) otherwise there is no point returning rv later on as we already escaped in NS_ENSURE_SUCCESS(rv, rv); if there was an error.

::: suite/locales/en-US/chrome/mailnews/localMsgs.properties
@@ +27,5 @@
>  # user name should appear, and %2$S where the host name should appear.
>  pop3PreviouslyEnteredPasswordIsInvalidPrompt=Please enter a new password for user %1$S on %2$S:
>  
> +# Status - Downloading message n of m
> +#LOCALIZATION NOTE (receivingMessages): Do not translate %1$S or %2$Sin the following lines.

Space before "in".

Comment 19

[[e-mail]]

Comment on attachment 8398550 [details] [diff] [review]
Patch v2

>+#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
Not used?

>-receivedMessages=%3$S Received %1$S of %2$S messages
>+receivedMsg s=Received %1$S of %2$S messages
Bogus space  ^
Also, you changed the number of parameters, but you didn't update the caller.

>-        rv = mProgressEventSink->OnStatus(this, m_channelContext, NS_OK, aStatusString);      // XXX i18n message
>+        rv = mProgressEventSink->OnStatus(this, m_channelContext, NS_OK, statusMessage.get());      // XXX i18n message
Isn't mProgressEventSink an nsMsgStatusFeedback object? In which case OnStatus should already be prepending the pretty name...

>+      if (m_statusFeedback) {
>+        nsString statusMessage;
>+        rv = FormStatusMessage(finalString.get(), statusMessage);
>+        NS_ASSERTION(NS_SUCCEEDED(rv), "couldn't format string");
>+        m_statusFeedback->ShowStatusString(statusMessage);
Once the progress event sink call works, we can use it for this too.

Comment 20

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v3

Made the suggested changes.

Comment 21

[:aceman]

Comment on attachment 8406641 [details] [diff] [review]
Patch v3

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

::: mail/locales/en-US/chrome/messenger/localMsgs.properties
@@ +33,2 @@
>  
>  # Status - no messages to download 

Trailing space?

::: mailnews/local/src/nsPop3Protocol.h
@@ -300,5 @@
>    nsresult FormatCounterString(const nsString &stringName,
>                                 uint32_t count1,
>                                 uint32_t count2,
>                                 nsString &resultString);
> -

What is this change? If it is just whitespace you can probably drop it from this patch as you otherwise do not touch this place.

Is m_statusFeedback still used anywhere?

Comment 22

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v3.1

(In reply to :aceman from comment #21)
> Comment on attachment 8406641 [details] [diff] [review]
> Patch v3
> @@ +33,2 @@
> >  
> >  # Status - no messages to download 
> 
> Trailing space?
Fixed.
> @@ -300,5 @@
> >    nsresult FormatCounterString(const nsString &stringName,
> > -
> What is this change? If it is just whitespace you can probably drop it from
> this patch as you otherwise do not touch this place.
Dropped.

> Is m_statusFeedback still used anywhere?
Not in nsPop3Protocol*
Can we remove it?

Thanks.

Comment 23

[:aceman]

(In reply to Suyash Agarwal (:sshagarwal) from comment #22)
> > Is m_statusFeedback still used anywhere?
> Not in nsPop3Protocol*
> Can we remove it?
Try it. If it isn't even set anywhere then remove it.

Comment 24

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v1.4

Okay so I have removed m_statusFeedback altogether from nsPop3Protocol*, the patch still works as usual.

Thanks.

Comment 25

[:aceman]

Comment on attachment 8406760 [details] [diff] [review]
Patch v1.4

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

Yes, seems to work for me. Let's get it reviewed quickly before TB31 cutoff.

::: suite/locales/en-US/chrome/mailnews/localMsgs.properties
@@ +33,2 @@
>  
>  # Status - no messages to download 

The trailing space again. The changes of localMsgs.properties in /mail and /suite are slightly different, please sync them.

Comment 26

[[e-mail]]

Comment on attachment 8406760 [details] [diff] [review]
Patch v1.4

>     count2String.get(),
Trailing comma?

> void nsPop3Protocol::UpdateStatus(const nsString &aStatusName)
[This would have been an ideal time to change this to a const char16_t*]

>+        NS_ASSERTION(NS_SUCCEEDED(rv), "couldn't format string");
Wrong error message, perhaps you meant dropping error result?

Comment 27

[[e-mail]]

Comment on attachment 8406760 [details] [diff] [review]
Patch v1.4

r=me with the above nits fixed.

Comment 28

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2

Carrying over review from Neil.
Changed UpdateStatus's parameter to char16_t*. Requesting feedback to make sure I didn't mess it up.

Thanks.

Comment 29

[:aceman]

Comment on attachment 8410197 [details] [diff] [review]
Patch v2

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

Still works :)

Comment 30

[Ryan VanderMeulen [:RyanVM]]

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