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

RESOLVED FIXED in Thunderbird 31.0

Status

MailNews Core
Networking: POP
--
enhancement
RESOLVED FIXED
17 years ago
a year ago

People

(Reporter: Erich 'Ricky' Iseli, Assigned: sshagarwal)

Tracking

(Blocks: 1 bug)

Trunk
Thunderbird 31.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

17 years ago
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

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

Updated

17 years ago
QA Contact: esther → sheelar

Comment 2

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

Comment 3

16 years ago
[RFE] is deprecated in favor of severity: enhancement.  They have the same meaning.
Severity: normal → enhancement

Updated

16 years ago
QA Contact: sheelar → stephend

Comment 4

15 years ago
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

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

Comment 6

15 years ago
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

14 years ago
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

14 years ago
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

14 years ago
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

14 years ago
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).

Product: Browser → Seamonkey

Comment 11

13 years ago

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

Comment 12

5 years ago
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.
Assignee: sspitzer → syshagarwal
Blocks: 66860
Status: RESOLVED → REOPENED
Component: MailNews: Message Display → Networking: POP
OS: Windows 98 → All
Product: SeaMonkey → MailNews Core
Hardware: x86 → All
Resolution: DUPLICATE → ---
Summary: [RFE] Getting mail from multiple accounts: confusing status messages → [RFE] Show account name in status bar when downloading messages from POP servers (Getting mail from multiple accounts: confusing status messages)

Updated

5 years ago
QA Contact: stephend

Comment 13

5 years ago
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" .
(Assignee)

Comment 14

4 years ago
Created attachment 8391711 [details] [diff] [review]
Patch v1

This patch makes the status messages conform with the other status messages:
<accountName> : downloading x message of y
Attachment #8391711 - Flags: feedback?(acelists)
(Assignee)

Comment 15

4 years ago
Created attachment 8391733 [details] [diff] [review]
Patch v1.5

Made a few more changes, tried to cover most of the messages.
Attachment #8391711 - Attachment is obsolete: true
Attachment #8391711 - Flags: feedback?(acelists)
Attachment #8391733 - Flags: feedback?(acelists)

Comment 16

4 years ago
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.
Attachment #8391733 - Flags: feedback?(acelists) → feedback+
(Assignee)

Comment 17

4 years ago
Created attachment 8398550 [details] [diff] [review]
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.
Attachment #8391733 - Attachment is obsolete: true
Attachment #8398550 - Flags: feedback?(acelists)

Comment 18

4 years ago
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".
Attachment #8398550 - Flags: feedback?(neil)
Attachment #8398550 - Flags: feedback?(acelists)
Attachment #8398550 - Flags: feedback+
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.
Attachment #8398550 - Flags: feedback?(neil) → feedback-
(Assignee)

Comment 20

4 years ago
Created attachment 8406641 [details] [diff] [review]
Patch v3

Made the suggested changes.
Attachment #8398550 - Attachment is obsolete: true
Attachment #8406641 - Flags: feedback?(acelists)

Comment 21

4 years ago
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?
(Assignee)

Comment 22

4 years ago
Created attachment 8406674 [details] [diff] [review]
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.
Attachment #8406641 - Attachment is obsolete: true
Attachment #8406641 - Flags: feedback?(acelists)
Attachment #8406674 - Flags: feedback?(acelists)

Comment 23

4 years ago
(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.
(Assignee)

Comment 24

4 years ago
Created attachment 8406760 [details] [diff] [review]
Patch v1.4

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

Thanks.
Attachment #8406674 - Attachment is obsolete: true
Attachment #8406674 - Flags: feedback?(acelists)
Attachment #8406760 - Flags: feedback?(acelists)

Comment 25

4 years ago
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.
Attachment #8406760 - Flags: review?(neil)
Attachment #8406760 - Flags: feedback?(acelists)
Attachment #8406760 - Flags: feedback+
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 on attachment 8406760 [details] [diff] [review]
Patch v1.4

r=me with the above nits fixed.
Attachment #8406760 - Flags: review?(neil) → review+
(Assignee)

Comment 28

4 years ago
Created attachment 8410197 [details] [diff] [review]
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.
Attachment #8406760 - Attachment is obsolete: true
Attachment #8410197 - Flags: review+
Attachment #8410197 - Flags: feedback?(acelists)

Comment 29

4 years ago
Comment on attachment 8410197 [details] [diff] [review]
Patch v2

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

Still works :)
Attachment #8410197 - Flags: feedback?(acelists) → feedback+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/cab0a07d474c
Status: REOPENED → RESOLVED
Last Resolved: 14 years ago4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 31.0
You need to log in before you can comment on or make changes to this bug.