1348769 - POP3: Add account name and improve status bar message "checking Inbox for new messages"

Status: RESOLVED FIXED

(MailNews Core :: Networking: POP, enhancement)

Description

[Thomas D. (:thomas8)]

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

With multiple POP3 accounts, I find this lack of information annoying every time, especially on slow connections and/or big inboxes (10.000+ messages):

STR

0) have several POP accounts (worse for slow connections and/or big inboxes)
1) When checking for new messages, I'm seeing this status bar message:

Actual result

"checking Inbox for new messages" (sic)

Expected result

"[Account Name]: Checking Inbox for new messages…"

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

I'm not sure how often these status bar messages get updated, we might want to consider flickering when it co-occurs for several accounts. If flickering is too bad, we could consider adding account name at the end of the string, so that only the last part flickers, but it would be inconsistent with other status bar messages where we always have it in front, and it wouldn't help much when alternating with other types of status bar messages which we can't predict. Trailing account name would also be odd when checking single account, which would then flicker from leading to trailing position. Generally speaking (for another bug), our status bar message management may need some improvement, and some sort of links with activity manager, along the lines of my bug 532646, comment 3.

Comment 1

[gene smith]

Attachment: pop-ck-for-new-msg.patch (informal, 1st cut)

This addresses the simple 1st part of this bug. It puts the account name in front as requested. It is kind of hard to test since I can barely see this message appear before it it replaced with "...there are no new messages...". I had to set up a pop3 server at charter which they basically now leave undocumented, but I did find the right port/server name.

This doesn't address the 2nd part of the bug about "flickering" which I don't 100% understand and haven't completely perused yet.

Comment 2

[Jorg K (CEST = GMT+2)]

Comment on attachment 8864752 [details] [diff] [review]
pop-ck-for-new-msg.patch (informal, 1st cut)

No C++ changes required?

I think "flickering" means that multiple accounts are downloaded "interleaved" and the status message is updated in fast succession so it becomes illegible. I'm not sure what we can do about it. Perhaps a sophisticated scheme with a timer that makes sure each message is at least displayed for 500ms or so.

Comment 3

[Jorg K (CEST = GMT+2)]

Comment on attachment 8864752 [details] [diff] [review]
pop-ck-for-new-msg.patch (informal, 1st cut)

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

Surely this is the right way. Let me ask again, no C++ changes. Also, where is this message displayed?

Clearing f? for now.

::: mail/locales/en-US/chrome/messenger/activity.properties
@@ +45,5 @@
>  # LOCALIZATION NOTE (autosyncContextDisplayText): %S will be replaced by the account name
>  autosyncContextDisplayText=Synchronizing: %S
>  
> +# LOCALIZATION NOTE (pop3EventStartDisplayText): %S will be replaced by the folder name
> +pop3EventStartDisplayText=%2$S:Checking %1$S for new messages…

Hmm, shouldn't this be pop3EventStartDisplayText2 now? Also, can you please document %1$S and %2$S.

Comment 4

[Jorg K (CEST = GMT+2)]

Any news on this bug?

Comment 5

[gene smith]

(In reply to Jorg K (GMT+2) from comment #3)
> Comment on attachment 8864752 [details] [diff] [review]
> pop-ck-for-new-msg.patch (informal, 1st cut)
> 
> Review of attachment 8864752 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Surely this is the right way. Let me ask again, no C++ changes. Also, where
> is this message displayed?

Shows up in the status bar at the bottom for me.

> 
> Clearing f? for now.

f?  ?? -- no idea what you mean.

> 
> ::: mail/locales/en-US/chrome/messenger/activity.properties
> @@ +45,5 @@
> >  # LOCALIZATION NOTE (autosyncContextDisplayText): %S will be replaced by the account name
> >  autosyncContextDisplayText=Synchronizing: %S
> >  
> > +# LOCALIZATION NOTE (pop3EventStartDisplayText): %S will be replaced by the folder name
> > +pop3EventStartDisplayText=%2$S:Checking %1$S for new messages…
> 

> Hmm, shouldn't this be pop3EventStartDisplayText2 now? Also, can you please
> document %1$S and %2$S.

Ok, will document. Yes, you are right that it needs 2 suffix. Since the string name can also appear in C++ that might require a change too. I will check that. Also, sorry, somehow I never received (or accidentally deleted) email notifications regarding this that were supposedly sent over a month ago. Will submit another patch shortly.

Comment 6

[Jorg K (CEST = GMT+2)]

(In reply to gene smith from comment #5)
> > Clearing f? for now.
> f?  ?? -- no idea what you mean.
Well, f stands for feedback. In our jargon f?aceman means feedback request for Aceman, r=aceman means positive review from Aceman. Since you had asked for feedback, hence f?jorgk, I cleared the f?, that is, cancelled the feedback, since I noticed an issue that needed fixing before proceeding.

Also note that we no longer use "prettiestName", so this patch won't apply (I haven't tried).

Comment 7

[Thomas D. (:thomas8)]

(In reply to gene smith from comment #5)
> Will submit another patch shortly.

Gene, it would be great if you'd be able to continue here :)

Comment 8

[gene smith]

Attachment: 1348769-pop-check-for-new-message.patch

This puts the account name in front of the "checking [folder] for new messages..." pop3 status message. Not sure if this completely addresses the reporter's issues since I don't personally have a pop3 account that contain many messages.

Comment 9

[gene smith]

Jorg asked if there are c++ changes. I don't see any usage of the string variable pop3EventStartDisplayText in any c++ file per dxr and a full grep of comm-central. I also changed deprecated "prettiestName" to just "prettyName" and changed the string variable name to pop3EventStartDisplayText2 in the two files that contain that string and added the requested documentation. Also, I don't see this string used in "suite" for some reason. The patch also doesn't (yet?) contain any anti-flicker fixes.

Comment 10

[Jorg K (CEST = GMT+2)]

Comment on attachment 8894132 [details] [diff] [review]
1348769-pop-check-for-new-message.patch

Thomas is not a review peer. Either Aceman or I will review, in fact, I've seen this a few times. Thanks for finishing it off.

Comment 11

[gene smith]

Hang on! While testing this I see a failure in stdout/stderr. Even after reverting my patch I see the error too:

JavaScript error: resource:///modules/activity/pop3Download.js, line 65: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIStringBundle.formatStringFromName]

This is the function that prints the status string that I added account name to.

I see this when I click "Get new message" for pop account and also, of course, the status string doesn't print. Trying to determine what the problem is...

Comment 12

[gene smith]

Never mind, I think it was user error. I failed to do a build after applying the patch. I assumed that this javascript didn't need a re-build but apparently it does. 

The string is produced OK each time "get new mail" is clicked which I verified with a dump(displayText + "\n"); in the .js file. However just clicking "get new mail" doesn't cause the message to appear in the status area at the bottom. You have to actually have new messages on the pop3 server before the message

[e-mail]: Checking Inbox for new messages…

actually prints in the status area at the bottom. Does this seem correct?

Also, as I mentioned above, this message appears for so short a time that I can't really read it. I can read the next messages which is the progress count of messages downloaded. Maybe the "checking" message appears for a longer time with a slow connection as the reporter Thomas mentioned?

Comment 13

[:aceman]

Comment on attachment 8894132 [details] [diff] [review]
1348769-pop-check-for-new-message.patch

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

Yes, it seems some of the POP3 strings from the activity manager also appear in the status bar. I also didn't find any C++ callers so probably the string is forwarded automatically by some internals of nsActEvent.

::: mail/locales/en-US/chrome/messenger/activity.properties
@@ +45,4 @@
>  # LOCALIZATION NOTE (autosyncContextDisplayText): %S will be replaced by the account name
>  autosyncContextDisplayText=Synchronizing: %S
>  
> +# LOCALIZATION NOTE (pop3ProcessDisplayText2): Do not translate the words "%1$S" and "%2$S" below.

Looks like this string ID was copied incorrectly before your patch so please fix it now, it should be pop3EventStartDisplayText2 .

Comment 14

[Jorg K (CEST = GMT+2)]

I can fix that when landing the patch.

Comment 15

[Jorg K (CEST = GMT+2)]

No, in the current form checkin is NOT needed. I have the bug on the radar.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/ad7c18b07f87
Add account name in front of "Checking folderX for new messages" pop3 status message. r=aceman

Comment 17

[Jorg K (CEST = GMT+2)]

Fixed the localisation note, a trailing space and the indentation.