944526 - Include account name in status bar messages when sending/receiving/getting/downloading/fetching new News messages

Status: RESOLVED FIXED

(MailNews Core :: Networking: NNTP, enhancement)

Description

[:aceman]

+++ This bug was initially created as a clone of Bug #66860 +++

When you download new mail messages the account name should be mentioned in the status line, e.g.:

"There are no new messages on the server"	should be:
"account name: There are no new messages on the server for the account"

"Connect: Host connected, sending login information..."		should be:
"account name: Connect: Host connected, sending login information for..."

"Downloading message x of y"	should be:
"account name: Downloading message x of y for"

Especially if you have more than one account whose messages get downloaded at startup the current status messages aren't clear enough.

Comment 1

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v1

Possible fix.
Covers the ones mentioned in the description.

Comment 2

[:aceman]

Comment on attachment 8340647 [details] [diff] [review]
Patch v1

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

Seems to work for me nicely. Can you again push the constructing of the status line into SetProgressStatus?

Comment 3

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2

Okay, done!
And, this shows the account name associated. In case of news, aren't we interested in knowing the individual subscribed folders one is retrieving?
E.g. I have news.mozilla.org and that has "m.d.a.thunderbird" and "m.s.thunderbird" and others, so what would you like status messages to contain?
"news.mozilla.org..." or "m.d.a.thunderbird: ...."

Though, the downloading headers messages are formed this way:
"news.mozilla.org: downloading headers x of y for m.d.a.thunderbird"
Still, if we want all the status messages to mention the folder name, please let   me know.

Thanks.

Comment 4

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2.1

Sorry, forgot to hg qref.

Comment 5

[:aceman]

Comment on attachment 8342880 [details] [diff] [review]
Patch v2.1

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

I would love to see both account name and newsgroup name in the status message. But you will need a UI review for that. It may be quite long, but the idea is that the user will need to read the message only if the download takes too long (or hangs).

Comment 6

[Suyash Agarwal (:sshagarwal)]

(In reply to :aceman from comment #5)
> I would love to see both account name and newsgroup name in the status
> message. But you will need a UI review for that. It may be quite long, but
> the idea is that the user will need to read the message only if the download
> takes too long (or hangs).

So this can land and bug 946643 can be fixed after this? :)

Comment 7

[:aceman]

Yes, sure :) Get review from Neil and bwinton :)

Comment 8

[Suyash Agarwal (:sshagarwal)]

Comment on attachment 8342880 [details] [diff] [review]
Patch v2.1

Okay, thanks :)

Comment 9

[Suyash Agarwal (:sshagarwal)]

Attachment: Screenshot showing the status bar message.

Comment 10

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

Comment on attachment 8342880 [details] [diff] [review]
Patch v2.1

ui-r=me, based on the screenshot at http://imgur.com/03xeDOW

Comment 11

[Suyash Agarwal (:sshagarwal)]

Comment on attachment 8350626 [details]
Screenshot showing the status bar message.

Screenshot ui-reviewed by bwinton.

Comment 12

[[e-mail]]

Comment on attachment 8342880 [details] [diff] [review]
Patch v2.1

>-nsNNTPNewsgroupList::SetProgressStatus(const PRUnichar *message)
>+nsNNTPNewsgroupList::SetProgressStatus(const nsString aMessage)
Don't change this, you need the const PRUnichar* for the params. (Although it's const char16_t* by now.)

>     nsCOMPtr <nsIMsgStatusFeedback> feedback;
>     mailnewsUrl->GetStatusFeedback(getter_AddRefs(feedback));
> 
>     if (feedback) {
>-      feedback->ShowStatusString(nsDependentString(message));
>+      // prepending the account name to the status message.
[It's a shame we don't have a request we can pass to nsMsgStatusFeedback::OnStatus to avoid duplicating code.]

>+      nsresult rv;
>+      nsCOMPtr <nsIMsgFolder> folder = do_QueryInterface(m_newsFolder, &rv);
>+      NS_ENSURE_SUCCESS_VOID(rv);
>+      nsCOMPtr <nsIMsgIncomingServer> server;
>+      rv = folder->GetServer(getter_AddRefs(server));
Can you not get the server directly from the mailnewsUrl?

>+      rv = sbs->CreateBundle("chrome://messenger/locale/messenger.properties",
>+                             getter_AddRefs(bundle));
MSGS_URL?

>+      const PRUnichar *params[] = { accountName.get(), aMessage.get() };
>+      bundle->FormatStringFromName(NS_LITERAL_STRING("statusMessage").get(),
>+                                   params, 2, getter_Copies(statusMessage));
We also use MOZ_UTF16() instead of NS_LITERAL_STRING().get() these days.

Comment 13

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v3.5

Comment 14

[Richard Marti (:Paenglab)]

Comment on attachment 8362595 [details] [diff] [review]
Patch v3.5

I only checked how it looks. I see the account name in front of the message.

Comment 15

[Richard Marti (:Paenglab)]

Attachment: screenshot

Screenshot how it looks after polling. The only weird is, it says "No new messages on the server" but in screenshot you can see 4 new (unread) messages. On news I have only the headers downloaded. The body will be downloaded when I'm reading the messages. So the messages are still on the server. But this could be something for a new bug.

Comment 16

[Magnus Melin [:mkmelin]]

I don't know if that's what you saw, but new != unread. New is a message we didn't know about before.

Comment 17

[Richard Marti (:Paenglab)]

Okay, I subscribed to a new group. After "Get Messages" the last message I saw was "No new messages on the server" but it disappeared almost immediately. The group shows now 47 new messages. And I would say, I didn't know about them before.

Comment 18

[[e-mail]]

(In reply to aceman from comment #0)
> "Downloading message x of y"	should be:
> "account name: Downloading message x of y for"
> 
> Especially if you have more than one account whose messages get downloaded
> at startup the current status messages aren't clear enough.

Although, that's a bit of an edge case; you would need to be subscribed to the same newsgroup on two different servers.

Comment 19

[Suyash Agarwal (:sshagarwal)]

Thanks.

Comment 20

[Mark Banner (:standard8)]

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