IMAP: Include account name and improve wording of status bar message "Downloading 1 of 10 in Inbox"

RESOLVED FIXED in Thunderbird 55.0

Status

MailNews Core
Networking: IMAP
--
enhancement
RESOLVED FIXED
10 months ago
7 months ago

People

(Reporter: Thomas D. (currently busy elsewhere), Assigned: gene smith)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {ux-userfeedback})

Trunk
Thunderbird 55.0
ux-userfeedback
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 13 obsolete attachments)

29.72 KB, patch
Jorg K (GMT+1)
: review+
gene smith
: feedback+
Details | Diff | Splinter Review
30.67 KB, patch
Jorg K (GMT+1)
: review+
Details | Diff | Splinter Review
13.88 KB, patch
Thomas D. (currently busy elsewhere)
: review+
Details | Diff | Splinter Review
(Reporter)

Description

10 months ago
+++ This bug was initially created as a clone of Bug #944526 +++
+++ This bug was initially created as a clone of Bug #66860 +++

I'm not sure if we tried to fix this in bug 944526 which currently claims to be News-only, but anyway, here's another scenario where account name is missing:

STR

0) have several IMAP accounts (including gmail)
1) When downloading new messages and/or automatic syncing, I'm seeing this status bar message:

Actual result

"Downloading 1 of 10 in Inbox" (sic)

Expected result

"[Account name]: Downloading message 1 of 10 in Inbox"

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

Updated

10 months ago
Component: Networking: NNTP → Networking: IMAP
(Reporter)

Comment 1

10 months ago
Sorry, BMO doesn't seem to allow changing dependencies when cloning bugs, so it needs manual intervention after the fact...
No longer blocks: 946643
(Reporter)

Comment 2

10 months ago
(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #0)

> Expected result
> 
> "[Account name]: Downloading message 1 of 10 in Inbox"

And as this is an ongoing action with potential waiting time, we should also add ellipsis at the end:

"[Account name]: Downloading message 1 of 10 in Inbox…"

Comment 3

10 months ago
I agree, this should be done, I just wish we had enough resources to do it.
(Assignee)

Comment 4

10 months ago
FWIW, when I setup and download a fairly large IMAP account and only want to store headers I see the "expected" status:

gds@charteir.net: INBOX Downloading message header 3393 of 3991...

But when I configure to store messages and then download, I see the abbreviated status that this bug points out:

Downloading 3393 of 3991 in Inbox

However, during normal operation I don't normally look at the download status since it occurs so fast so it's usually pretty hard to see. Maybe if you receives 100's of email in a batch you might be able to notice and read it.
(Assignee)

Comment 5

10 months ago
Created attachment 8850279 [details] [diff] [review]
1348762-account-in-dnld-stat.patch

This seems to fix it for me. Now when I setup an IMAP account and download full messages (not just headers) I see progress status message like these:

gds@cartier.com: Downloading 3393 of 3992 in Inbox...
gds@cartier.com: Downloading 1960 of 1962 in Sent...

Comment 6

10 months ago
Comment on attachment 8850279 [details] [diff] [review]
1348762-account-in-dnld-stat.patch

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

Thanks for finding the place.

::: mail/locales/en-US/chrome/messenger/activity.properties
@@ +37,5 @@
>  # Place the word %1$S in your translation where the number of the message being downloaded should appear.
>  # Place the word %2$S in your translation where the total number of pending messages should appear.
>  # Place the word %3$S in your translation where the name of the folder being processed should appear.
> +# Place the word %4$S in your translation where the name of account being processed should appear.
> +autosyncProcessProgress=%4$S: Downloading %1$S of %2$S in %3$S...

You need to change the stringID when you change the value. Also please use the UTF-8 ellipsis character "…".
Attachment #8850279 - Flags: feedback+
(Reporter)

Comment 7

10 months ago
Gene, thanks for taking!

Please add the word "message" to the new string, as seen in comment 0.
Assignee: nobody → gds
Keywords: ux-userfeedback

Comment 8

10 months ago
Gene, thanks for taking this bug. Can you please do the following in general:

I don't know how you create those patches, we all use |hg qnew -m "commit message" xx.patch|. If you've got your mercurial.ini and your .hg/hgrc set up correctly, you'll automatically get, for example:

# HG changeset patch
# User Gene Smith <gds@chartertn.net>
# Date 2017-03-15 14:42
# Parent  04d93e773a776bb86827873ba9d7a7d1246c7458
Bug 1231592 - Implement mail.server.default.forceSelect for misbehaving IMAP servers. Use UidExpunge(). r=jorgk, sr=rkent

In the other bug I repeatedly re-added the header for you (without mentioning), but from the second bug onwards you need to do it yourself ;-) Also, please set
[diff]
git = 1
showfunc = 1
unified = 8
in your mercurial.ini so you get eight lines of context for every change which is essential for reviewing a patch.
(Reporter)

Comment 9

10 months ago
I also think we should also change the other string for message headers to follow the same structure:
> John.Doe@asdf.com: INBOX Downloading message header 3393 of 3991...
This should be changed to:
> John.Doe@asdf.com: Downloading message header 3393 of 3991 in Inbox...
Trailing position for folder name avoids more flickering when several folders are synchronized simultaneously, as only the last part of the message will change.
I don't think we need capital letters for the folder name (or do we?), so that formatting should also be removed.

Comment 10

10 months ago
(In reply to :aceman from comment #6)
> You need to change the stringID when you change the value.
Sadly so. The localisers only localise *new* strings, so to make it new, we change the ID, so autosyncProcessProgress2 in this case.

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #9)
Gene stated the new message in comment #5 which is:
gds@cartier.com: Downloading 3393 of 3992 in Inbox...
where the first bit is the account name that Gene doesn't seem to have configured. The code quoted in comment #6 also reflects this order. Not sure where |gds@charteir.net: INBOX Downloading message header 3393 of 3991...| from comment #4 came from. Maybe that's a different code path that needs to change, too?
(Assignee)

Comment 11

10 months ago
Created attachment 8850868 [details] [diff] [review]
1348762--improve-prog-msgs.patch

This works for both headers and messages. However, my patch header is not quite right (but closer I think). Struggling with "hg" :(.

So this a tentative patch. I need to look closer at it tomorrow (actually, later today).
(Assignee)

Comment 12

10 months ago
I only see the user and date in patch header if I add -UD option, e.g. hg qnew -DU -m "commit message" xx.patch
But the date is "internal format" like this:

# HG changeset patch
# User Gene Smith <gds@chartertn.net>
# Date 1490409205 14400
# Parent  0cf73e610aa1cfa7a7b2d9dc6b13c59a70d21e55
Bug 1348762 Make message and header progress messages better. r=jorgk, sr=rkent

Is that OK?

My name and email are in comm-central/.hg/hgrc

[ui]
username = Gene Smith <gds@chartertn.net>

I can't find how to set the date format inside the patch header to conventional date like yours.
(I did the mozilla/mach mercurial-setup and even upgraded to latest stable hg.)

Also, the "Parent" shown is not the lastest (head or tip or whatever) since that wouldn't build. So I had to move down to an earlier version that does build (per treeherdered) and that's what the parent above points to. Is that OK?
(Reporter)

Comment 13

10 months ago
(In reply to Jorg K (GMT+1) from comment #10)

> (In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment
> #9)
> Gene stated the new message in comment #5 which is:
> gds@cartier.com: Downloading 3393 of 3992 in Inbox...
> where the first bit is the account name that Gene doesn't seem to have
> configured. The code quoted in comment #6 also reflects this order. Not sure
> where |gds@charteir.net: INBOX Downloading message header 3393 of 3991...|
> from comment #4 came from. Maybe that's a different code path that needs to
> change, too?

Jörg, I have no idea what exactly you are trying to say in this comment; my comment 9 seems clear enough and correctly points out that there's *another* string involving *message /headers/* which should be changed in analogy to the main change of this bug, the string about *messages*. On the latter, I think it's clear from my responses in comment 7 and comment 9 that I've correctly noted Gene's comment 5.

But Jörg's comment caused me to think of this: Someone should double-check if the main string we are changing here is really about messages, since that word was not in the original string so it's not clear what is being downloaded.
Looking at the string ID, I think it's reasonable to assume that our string here is about entire messages (not headers), but again, it's not 100% clear:

> - autosyncProcessProgress=Downloading %1$S of %2$S in %3$S
> + autosyncProcessProgress2=%4$S: Downloading message %1$S of %2$S in %3$S...
(Reporter)

Comment 14

10 months ago
Comment on attachment 8850868 [details] [diff] [review]
1348762--improve-prog-msgs.patch

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

Thanks Gene for the new patch! :)
This is going the right way but I've got some nits and questions, from looking at the patch only.

::: mail/components/activity/modules/autosync.js
@@ +286,4 @@
>                                                   [syncItem.totalDownloaded,
>                                                    syncItem.pendingMsgCount,
> +                                                  folder.prettiestName,
> +                                                  folder.server.prettyName], 4);

If server is the term used in surrounding code, and if this is really the server name, this is fine. Otherwise, I'm wondering if we should use "account", which is freeform so it's not identical with server.
Is account really a property of folder, or this is just a variable for account name?

Does this patch really work? I see nowhere where %4$S is being resolved to account name (same for the other message about message headers!). Those placeholders must NOT be replaced just by their order of occurence, because this will fail for other languages where the order might differ.

::: mail/locales/en-US/chrome/messenger/activity.properties
@@ +32,5 @@
>  failedToSendMessageWithSubject=Failed to send message: %S
>  # LOCALIZATION NOTE (failedToCopyMessageWithSubject): %S will be replaced by the subject of the message being sent.
>  failedToCopyMessageWithSubject=Failed to copy message: %S
>  
> +# LOCALIZATION NOTE (autosyncProcessProgress2): Do not translate the word "%1$S", "%2$S" and "%3$S" below.

Nit: Pls add %4$S in this comment.

@@ +37,4 @@
>  # Place the word %1$S in your translation where the number of the message being downloaded should appear.
>  # Place the word %2$S in your translation where the total number of pending messages should appear.
>  # Place the word %3$S in your translation where the name of the folder being processed should appear.
> +# Place the word %4$S in your translation where the name of account being processed should appear.

I think we should add an example here for better understanding:

# EXAMPLE: John@Doe.com: Downloading message 3 of 10 in Inbox…

@@ +37,5 @@
>  # Place the word %1$S in your translation where the number of the message being downloaded should appear.
>  # Place the word %2$S in your translation where the total number of pending messages should appear.
>  # Place the word %3$S in your translation where the name of the folder being processed should appear.
> +# Place the word %4$S in your translation where the name of account being processed should appear.
> +autosyncProcessProgress2=%4$S: Downloading message %1$S of %2$S in %3$S…

I am seeing unicode characters messed up in this patch review, so Ellipsis (…) looks like this "…". My browser's text encoding is on Unicode. I don't see this display problem on similar patches like attachment 8406304 [details] [diff] [review]. I'm not sure if this is an artefact or something wrong with UTF8 in the patch. Maybe Jörg can assist.

::: mail/locales/en-US/chrome/messenger/imapMsgs.properties
@@ +74,5 @@
>  imapGettingMailboxInfo=Getting Mailbox Configuration Info…
>  
>  imapEmptyMimePart=This body part will be downloaded on demand.
>  
> +# LOCALIZATION NOTE (imapReceivingMessageHeaders3): Do not translate the word "%S" or "%lu" below.

This and following comments will need adjustment after fixing placeholders below.

@@ +79,3 @@
>  # Place the word %S in your translation where the name of the server should appear.
>  # Place the word %lu where the number of headers should appear.
> +imapReceivingMessageHeaders3=Download message header %lu of %lu in %S…

replace "name of the server" with "name of the account" as that's what we're showing, isn't it? Account name is freeform so can be different from server.

This looks like a bug in the original string: 1 of 10 must be represented by *two* placeholders, as in the other string about downloading messages, like this: ...%$2 of %$3... We can't rely on other languages having the same order like English, maybe even RTL can affect this. So you also have to check and change accordingly the code where placeholders get parsed.

@@ +82,2 @@
>  
> +# LOCALIZATION NOTE (imapReceivingMessageFlags3): Do not translate the word "%S" or "%lu" below.

This and subsequent comments need adjustment after fixing placeholders below.

@@ +84,3 @@
>  # Place the word %S in your translation where the name of the server should appear.
>  # Place the word %lu where the number of flags should appear.
> +imapReceivingMessageFlags3=Downloading message flag %lu of %lu in %S…

Dito, existing bug wrt x of y placeholders, plus pls add account name here:

imapReceivingMessageFlags3=%1$S: Downloading message flag %2$S of %3$S in %4$S…

@@ +104,5 @@
>  # LOCALIZATION NOTE (imapCopyingMessage): Do not translate the word "%S" below.
>  # Place the word %S in your translation where the name of the folder should appear.
>  imapCopyingMessage=Copying message to %S…
>  
> +# LOCALIZATION NOTE (imapFolderReceivingMessageOf3): Do not translate the word "%S" or "%lu" below.

adjust these comments per below

@@ +109,3 @@
>  # Place the word %S in your translation where the name of the folder should appear.
>  # Place the word %lu where the number of headers should appear.
> +imapFolderReceivingMessageOf3=- Downloading message %lu of %lu in %S…

Dito, add account name, remove "- ", and fix placeholder ids, as above, and ensure replacement routines work after adjusting, as above.

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +5203,5 @@
>      if (NS_SUCCEEDED(rv))
>      {
>        // ### should convert mailboxName to char16_t and change %s to %S in msg text
>        progressString = nsTextFormatter::smprintf(m_progressString.get(),
> +                                ++m_progressIndex, m_progressCount, unicodeMailboxName.get());

Jörg, can you comment if this is the right way of doing it? I'm a bit surprised to see two variables aside with a function which will be repeated on each iteration. Looks stylistically odd at least, not sure about functionally.

::: suite/locales/en-US/chrome/mailnews/imapMsgs.properties
@@ +79,5 @@
>  
>  # Status - empty mime part
>  imapEmptyMimePart=This body part will be downloaded on demand.
>  
> +# LOCALIZATION NOTE (imapReceivingMessageHeaders3): Do not translate the word "%S" or "%lu" below.

Dito, adjust comments per below.

@@ +84,3 @@
>  # Place the word %S in your translation where the name of the server should appear.
>  # Place the word %lu where the number of headers should appear.
> +imapReceivingMessageHeaders3=Downloading message header %lu of %lu in %S…

Dito, add account name, adjust placeholders and their parsing.

@@ +87,2 @@
>  
> +# LOCALIZATION NOTE (imapReceivingMessageFlags3): Do not translate the word "%S" or "%lu" below.

Dito, adjust comments.

@@ +89,3 @@
>  # Place the word %S in your translation where the name of the server should appear.
>  # Place the word %lu where the number of flags should appear.
> +imapReceivingMessageFlags3=Downloading message flag %lu of %lu in %S…

Dito, add account name, adjust placeholders and their parsing.

@@ +109,5 @@
>  # LOCALIZATION NOTE (imapCopyingMessage): Do not translate the word "%S" below.
>  # Place the word %S in your translation where the name of the folder should appear.
>  imapCopyingMessage=Copying message to %S…
>  
> +# LOCALIZATION NOTE (imapFolderReceivingMessageOf3): Do not translate the word "%S" or "%lu" below.

Dito, comments.

@@ +114,3 @@
>  # Place the word %S in your translation where the name of the folder should appear.
>  # Place the word %lu where the number of headers should appear.
> +imapFolderReceivingMessageOf3=- Downloading message %lu of %lu in %S…

Dito, account / placeholders.
Attachment #8850868 - Flags: review-

Comment 15

10 months ago
(In reply to gene smith from comment #12)
> But the date is "internal format" like this:
> ...
> Is that OK?
Yes. The User line and the commit comment need to be right. The date format is a mystery to me. And the parent seems to record your local HG parent and changes when you refresh (hg qref) and will be mostly ignored.

> So I had to move down to an earlier version that does build (per treeherdered).
Normally everything builds, that's my job here in fact. If in doubt, check https://treeherder.mozilla.org/#/jobs?repo=comm-central and the status information (click in the (i) next to comm-central), in particular https://mzl.la/2lrQTHC to see current failures.

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #13)
> Jörg, I have no idea what exactly you are trying to say in this comment; ...
If in doubt, assume I made a mistake ;-) - I simply missed "other string".
On Wednesday and Thursday I turned over 122 and 146 bugmails, so sadly don't have the time to study every single one in detail. Please excuse me. It's a very tough job and I'm doing my best. The tree hasn't been closed for more than a few hours for months now, so all the developers can work without impediment. Before we had closures/bustages for stretches of weeks.

Comment 16

10 months ago
Answering comment #14:
I don't know where the garbled characters come from, the patch is UTF-8 and I see:
imapReceivingMessageHeaders3=Download message header %lu of %lu in %S…
when opening it locally.

The example should read:
# EXAMPLE: John's account: Downloading message 3 of 10 in Inbox…
to show that it's the account name and not the e-mail address.

Aceman and Thomas are the specialists on placeholders, I mostly defer to them, but the pre-existing %lu doesn't look right.

Finally:
> progressString = nsTextFormatter::smprintf(m_progressString.get(),
> +   ++m_progressIndex, m_progressCount, unicodeMailboxName.get());
is absolutely fine. .get() is an accessor macro to access the raw string pointer in an nsAuto[C]String.

Have I missed anything before moving onto the next bug ;-)
(Reporter)

Comment 17

10 months ago
(In reply to Jorg K (GMT+1) from comment #16)
> Answering comment #14:

Thanks.

> The example should read:
> # EXAMPLE: John's account: Downloading message 3 of 10 in Inbox…
> to show that it's the account name and not the e-mail address.

Good catch!

> Aceman and Thomas are the specialists on placeholders, I mostly defer to
> them, but the pre-existing %lu doesn't look right.

Aceman, do we need plural forms here? (I'd think not because it's always about a single message out of many, which is different from our typical plural case of "1 message" vs. "2 messages").
"John's account: Downloading message 1 of 10 in Inbox…"
"John's account: Downloading message 3 of 10 in Inbox…"
 
> Finally:
> > progressString = nsTextFormatter::smprintf(m_progressString.get(),
> > +   ++m_progressIndex, m_progressCount, unicodeMailboxName.get());
> is absolutely fine. .get() is an accessor macro to access the raw string
> pointer in an nsAuto[C]String.

Thanks for explaining. Noting that m_progressCount is set as a variable before we get here, I was wondering if we need to run that accessor macro again for every single message because all messages will be from same account, or if we can run the macro once outside the loop then just use a variable inside the loop. But I haven't checked how surrounding code looks or works, so I wouldn't know.

> Have I missed anything before moving onto the next bug ;-)

You may move on, sir :)

Comment 18

10 months ago
As far as I understand it, as you said, we don't need plural forms, at least no one complained so far.

The accessor macro is just accessing a member of an object directly. You can't gain any CPU cycles by moving that access outside the loop. Let's not try to optimise where there is nothing to optimise. Bringing this text to the screen via XPCOM and JS is two million more CPU instructions than accessing the string pointer in the surrounding object.

Comment 19

10 months ago
(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #17)
> Aceman, do we need plural forms here? (I'd think not because it's always
> about a single message out of many, which is different from our typical
> plural case of "1 message" vs. "2 messages").
> "John's account: Downloading message 1 of 10 in Inbox…"
> "John's account: Downloading message 3 of 10 in Inbox…"

I think we don't need them as we are referring to the one message being downloaded. If we had "of 10 messages", that would be more dangerous (for plural forms). But also we don't have plural forms support for smprintf()...

Also please do not overdo it here ;) If gene was interested in the one autosync string, do not force him to dive into C++ and get TB building;)

Why is imapFolderReceivingMessageOf2 changing?
-imapFolderReceivingMessageOf2=%S - Downloading message %lu of %lu…
+imapFolderReceivingMessageOf3=- Downloading message %lu of %lu in %S…

Yeah I see %S here is folder name so you probably do not want it in front of the message where we try to have account name consistently. So then, are you going to put account name in front?

I see Thomas has already said that :) But then do not use '-' but ':' as account name separator (as is done in POP3) and also your new autosync change.
(Reporter)

Comment 20

10 months ago
(In reply to Jorg K (GMT+1) from comment #18)
> As far as I understand it, as you said, we don't need plural forms
(In reply to :aceman from comment #19)
> I think we don't need them

So if Aceman, Jörg, and me agree that we don't need plural forms, then we really don't ;-) Good.

> The accessor macro is just accessing a member of an object directly. You
> can't gain any CPU cycles by moving that access outside the loop. Let's not
> try to optimise where there is nothing to optimise.

I think it's always worth *trying* to optimize, but I understand from your technical explanation that "unfortunately this can't be optimized, but thanks for trying...". Thanks for educating me about some of the deeper details.

(In reply to gene smith from comment #12)
> # HG changeset patch [...]
> Bug 1348762 Make message and header progress messages better. r=jorgk,
> sr=rkent

Gene, in the patch header, please update the bug summary as follows to be more specific:

Bug 1348762 Improve status bar progress messages when downloading messages, headers, and flags via IMAP. ui-r=thomasd, r=jorgk, sr=rkent

(In reply to :aceman from comment #19)
> Why is imapFolderReceivingMessageOf2 changing?
> -imapFolderReceivingMessageOf2=%S - Downloading message %lu of %lu…
> +imapFolderReceivingMessageOf3=- Downloading message %lu of %lu in %S…
> 
> Yeah I see %S here is folder name so you probably do not want it in front of
> the message where we try to have account name consistently. So then, are you
> going to put account name in front?
> 
> I see Thomas has already said that :) But then do not use '-' but ':' as
> account name separator (as is done in POP3) and also your new autosync
> change.

Let's also make sure that our new placeholders are actually replaced correctly and safely...

Comment 21

10 months ago
(In reply to :aceman from comment #19)
> Also please do not overdo it here ;) If gene was interested in the one
> autosync string, do not force him to dive into C++ and get TB building;)
Gene has excellent C++ skills, see bug 1231592 ;-) He can cope with a challenge, but sure, never overdo it ;-)
(Personally I find that an unsolicited r- was not warranted on a WIP (work in progress).)

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #20)
> Bug 1348762 Improve status bar progress messages when downloading messages,
> headers, and flags via IMAP. ui-r=thomasd, r=jorgk, sr=rkent
Great, not sure who will review here, Aceman or me, but please lose the super-review, that's a hang-over from bug 1231592 where I asked Kent to look over a patch ... and in the end, we landed something completely different, haha.
(Assignee)

Comment 22

10 months ago
Created attachment 8851320 [details] [diff] [review]
1348762--improve-prog-msgs.patch

Here's another attempt at a patch. I think I addressed most of the comments except maybe this by Thomas:

>This looks like a bug in the original string: 1 of 10 must be represented by *two* placeholders, as in the other string about downloading messages, like this: ...%$2 of %$3... We can't rely on other languages having the same order like English, maybe even RTL can affect this. So you also have to check and change accordingly the code where placeholders get parsed.

Right now, the order of the %ul "placeholders" is still fixed. The problem seems to be that the string expansion occurs in smprintf() in the .cpp file. I don't have a good feel for how this can be made so order doesn't matter by using the %1$S, %5$S type placeholders. When I tried them with smprintf(), TB crashed.

As a workaround, I have included in the comments that the translated strings must keep the parameters in the same order. I don't know how existing translations look but I would think this has always been a requirement that wasn't too hard to meet. 

But if this in not acceptable, any suggestions on how to fix up the smprint() call to use the indexed placeholders would be appreciated.

Otherwise, I have tried to make sure the strings print OK when I do mailbox downloads with and without headers and copying from online to local folders. The only message I don't know how to produce is the one for downloading "flags" only. How does a user cause that to happen?  (I know imap flags are downloaded with fetch and I see that happen but don't see a status message, or it happens so fast I can't see it.)
Attachment #8850279 - Attachment is obsolete: true
Attachment #8850868 - Attachment is obsolete: true

Comment 23

10 months ago
Thanks for the update. Sorry, none of the people commenting previously (Aceman, Thomas, myself) gave any response, maybe they only react to NI or f? or r?

I'm sadly not an expert on those "placeholder" replacements. I've just reviewed something and there they used FormatStringFromName():
https://hg.mozilla.org/comm-central/rev/92ed8653017e#l1.72
Also here where I coded something myself:
https://hg.mozilla.org/comm-central/rev/3a7dac0fdaca#l1.24

Aceman, what's the way forward here?
Flags: needinfo?(acelists)

Comment 24

10 months ago
Gene, if you're interested, we have another bug like this one here: Bug 1348769.

Comment 25

10 months ago
Comment on attachment 8851320 [details] [diff] [review]
1348762--improve-prog-msgs.patch

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

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +5202,5 @@
>                                     unicodeMailboxName);
>      if (NS_SUCCEEDED(rv))
>      {
>        // ### should convert mailboxName to char16_t and change %s to %S in msg text
>        progressString = nsTextFormatter::smprintf(m_progressString.get(),

To get rid of the %lu placeholders you would need to use FormatStringFromName() here. But then you can't use m_progressString but m_progressStringName here from SetProgressString(). Do you want to try that or should I do it in a separate patch?

::: suite/locales/en-US/chrome/mailnews/imapMsgs.properties
@@ +86,5 @@
> +# Place the word %S in your translation where the name of the folder downloaded should appear,
> +# and after the %lu words!
> +# Note: The account name is automatically encoded first in the displayed string.
> +# Example: "Joe's Account: Downloading message header 100 or 1000 in Drafts…"
> +imapReceivingMessageHeaders3=Download message header %lu of %lu in %S…

Don't we want the account name at the beginning?

@@ +95,5 @@
> +# Place the word %S in your translation where the name of the folder downloaded should appear,
> +# and after the %lu words!
> +# Note: The account name is automatically encoded first in the displayed string.
> +# Example: "Jim's Account: Downloading message flag 100 or 1000 in INBOX…"
> +imapReceivingMessageFlags3=Downloading message flag %lu of %lu in %S…

Don't we want the account name at the beginning?

@@ +124,5 @@
> +# Place the word %S in your translation where the name of the folder downloaded should appear,
> +# and after the %lu words!
> +# Note: The account name is automatically encoded first in the displayed string.
> +# Example: "Juan's Account: Downloading message 100 or 1000 in Sent…"
> +imapFolderReceivingMessageOf3=Downloading message %lu of %lu in %S…

Don't we want the account name at the beginning?
(Assignee)

Comment 26

10 months ago
(In reply to :aceman from comment #25)
> 
> To get rid of the %lu placeholders you would need to use
> FormatStringFromName() here. But then you can't use m_progressString but
> m_progressStringName here from SetProgressString(). Do you want to try that
> or should I do it in a separate patch?

I'll give it a try but I'm a complete novice on all this ns-string stuff. If I can't figure it out, I'll let you know ASAP. Thanks for the tips.

> > +# Note: The account name is automatically encoded first in the displayed string.
> 
> Don't we want the account name at the beginning?

I added the note to explain that the existing code magically/automatically puts the account name followed by colon in front for all three strings. Account name doesn't have a placeholder like the other 3 items.
(Assignee)

Comment 27

10 months ago
Created attachment 8853262 [details] [diff] [review]
1348762--improve-prog-msgs.patch

This now works with the %1$S style placeholders for strings processed with SetProgressString() in nsImapProtocol.cpp. The 3 placeholders can now be placed in any order when translations are made. However, and I don't know if this is a problem, the "server/account" followed by colon is *not* one of the %X$S items. It is still in a fixed position at the beginning of the string (i.e., on the left) and displayed by completely separate code somewhere I have yet to find.

Another problem I observer, even before I made any changes, is that if sync options is set such that headers and messages are stored, when a fairly large mailbox is downloaded the count of the number of headers currently downloaded frequently resets back to zero before it reached the total number. The reason for this is because headers and messages are both downloaded in an interleaved manner (with header leading the way). However there is only 1 variable that keeps track of the count of items downloaded, m_progressIndex, and it gets set to zero when the download activity changes between headers and messages. I fixed this by making m_progressIndex and m_progressCount arrays (hardcoded to [3]) so that headers, messages and flags have their own separate counts.

Note: if only headers are stored (not the default setting), a reset of of the count of downloaded headers does not occur since there is no message downloads to cause the count to reset. However, the arrays are still used. 
Also, it might be noted, that there is a third string selection for downloading "flags". I can't find a path in the code where this ever occurs so I haven't been able to test the "flag" download feature but I have treated it just like the other two.

I expect you may want me to make the above fix for resetting header counts a separate bug and patch. However, for now, I am showing both the "%X$S" feature addition and the resetting header count bug fix in the same patch.
Attachment #8851320 - Attachment is obsolete: true
(Assignee)

Comment 28

10 months ago
Created attachment 8853278 [details] [diff] [review]
1348762--improve-prog-msgs.patch

Fixed patch. (Sorry, forgot to include file changes in /suite area.) Comments with previous/obsolete patch still apply.
Attachment #8853262 - Attachment is obsolete: true

Comment 29

10 months ago
Thanks for the updated patch.

(In reply to gene smith from comment #27)
> I expect you may want me to make the above fix for resetting header counts a
> separate bug and patch. However, for now, I am showing both the "%X$S"
> feature addition and the resetting header count bug fix in the same patch.
I don't think that will be necessary. We can fix "side-issues" while we're there.

You added some trailing spaces in the patch, which you can see clicking on "Splinter review".

May I suggest that you add three defines for the three indices:
#define IMAP_STRING_INDEX_HEADERS 0
#define IMAP_STRING_INDEX_FLAGS 1
#define IMAP_STRING_INDEX_MESSAGES 2

I think m_progressIndex[3] and m_progressCount[3] might not be the best variable names. But I can't blame you, the names were bad to start with. One is the current "index" you're up to and the other one is the total. Well, m_progressIndex[m_stringIndex], reads just terrible.

So he have "total (count)" and "currently doing message X". Maybe:
m_progressTotalCount[3]
m_progressCurrent[3], that avoids using "index" or "count" in the variable name.

And please don't spread a comment that explains something else over three lines. Put the comment explaining the array size at the front before the defines and the array definitions.

Thanks for the patience, we're very picky.

Comment 30

10 months ago
Comment on attachment 8853278 [details] [diff] [review]
1348762--improve-prog-msgs.patch

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

Over all this looks good with the cosmetic issues I mentioned fixed, one more below.

::: mailnews/imap/src/nsImapProtocol.h
@@ +653,5 @@
>    bool m_closeNeededBeforeSelect;
>    bool m_retryUrlOnError;
>    bool m_preferPlainText;
>    bool m_forceSelect;
> +  nsCOMPtr<nsIStringBundle> m_bundle;

Please move this up after m_lastProgressTime;

Comment 31

10 months ago
(In reply to gene smith from comment #27)
> This now works with the %1$S style placeholders for strings processed with
> SetProgressString() in nsImapProtocol.cpp. The 3 placeholders can now be
> placed in any order when translations are made. However, and I don't know if
> this is a problem, the "server/account" followed by colon is *not* one of
> the %X$S items. It is still in a fixed position at the beginning of the
> string (i.e., on the left) and displayed by completely separate code
> somewhere I have yet to find.

Yes, it would be ideal if also the account name has a placeholder as in all the other strings.
 
> Another problem I observer, even before I made any changes, is that if sync
> options is set such that headers and messages are stored, when a fairly
> large mailbox is downloaded the count of the number of headers currently
> downloaded frequently resets back to zero before it reached the total
> number. The reason for this is because headers and messages are both
> downloaded in an interleaved manner (with header leading the way). However
> there is only 1 variable that keeps track of the count of items downloaded,
> m_progressIndex, and it gets set to zero when the download activity changes
> between headers and messages. I fixed this by making m_progressIndex and
> m_progressCount arrays (hardcoded to [3]) so that headers, messages and
> flags have their own separate counts.

Thanks, good catch.
 
> I expect you may want me to make the above fix for resetting header counts a
> separate bug and patch. However, for now, I am showing both the "%X$S"
> feature addition and the resetting header count bug fix in the same patch.

I also think we can do it here as it makes the status message be correct.
Flags: needinfo?(acelists)

Comment 32

10 months ago
(In reply to gene smith from comment #27)
> However, and I don't know if
> this is a problem, the "server/account" followed by colon is *not* one of
> the %X$S items. It is still in a fixed position at the beginning of the
> string (i.e., on the left) and displayed by completely separate code
> somewhere I have yet to find.
We've just done something similar for compaction here:
https://hg.mozilla.org/comm-central/rev/92ed8653017e7e7ae522997a1d9b0607282b4bfb#l1.64

DRX is always your friend ;-)
https://dxr.mozilla.org/comm-central/search?q=server-%3EGetPrettyName&redirect=false

And I'd be my hat that this is done for absolutely all reporting on any progress, be it POP, IMAP, etc.
https://dxr.mozilla.org/comm-central/rev/bee0697a1d3709df1db982f8320e7cac4a0b62c5/mailnews/base/src/nsMsgStatusFeedback.cpp#273

Comment 33

10 months ago
Oh, I forgot to say: That makes it undesirable as well as impossible to have a placeholder for the account in the message. Your patch is fine, please just fix the cosmetic issues.

Comment 34

10 months ago
There is also another solution we use sometimes. If you have one function producing string1 and another function producing string2, you can concatenate them using a third string, not just as string1+" "+string2. The string3 would be e.g. "%1$S: %2$S" where string1 and string2 would be replaced for the placeholders.
(Assignee)

Comment 35

10 months ago
Created attachment 8853633 [details] [diff] [review]
1348762--improve-prog-msgs.patch

This address the comments on last, now obsolete, patch:
Added #defines for number of strings and string indices.
"Improved" variable names for current number and expected number for items downloaded.
Removed multi-line to the right comments.
Moved m_bundle

Made one change in functionality in that the "expected number" for each items does not need an array of 3 but can share a int variable, m_progressExpectedNumber since it is set correctly with the expected number when mode changes between downloading headers and messages. Only m_progressCurrentNumber needs to be an array of 3 since, when the mode switches to download messages while previously downloading headers, the current number starts at zero and the expected number is a fairly small number like 200 and downloads this small range very fast (hard to see the counts on the screen). Anyhow, since the currentNumber for messages always resets to zero, a separate array elements is needed for it to avoid resetting the header current number back to zero if it was shared.
Attachment #8853278 - Attachment is obsolete: true

Comment 36

10 months ago
Comment on attachment 8853633 [details] [diff] [review]
1348762--improve-prog-msgs.patch

Excellent, we'll get it reviewed ;-)
Attachment #8853633 - Flags: review?(jorgk)
Attachment #8853633 - Flags: review?(acelists)
(Assignee)

Comment 37

10 months ago
Reviewing it myself, I see a #include error that I made. It doesn't cause problems in compiling or running.

I had to #include "nsImapStringBundle.h" in nsImapProtocol.h. It had to be there because I made m_bundle a class member variable and nsImapProtocol.h is also included in at least one other .cpp file other than nsImapProtocol.cpp. I failed to remove the #include "nsImapStringBundle.h" from nsImapProtocol.cpp so it's doubly included. I will fix it if you want or if I have to make other changes in response to reviews.

Also, not sure why I added the #include under the "// UI Thread proxy helper" comment in nsImapProtocol.h. It should probably just be moved to be the last #include in the file.

Made these changes in my local tree and still builds and runs like before.

Comment 38

10 months ago
Thanks for the attention to detail. Let's fix this in the next round if you want. Or you can upload a new patch, entirely up to you.

Comment 39

9 months ago
Comment on attachment 8853633 [details] [diff] [review]
1348762--improve-prog-msgs.patch

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

This works fine for me, thanks.

I still couldn't see where the account name is prepended to the message. But you do not need to bother with that.

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +2529,5 @@
>          uint32_t msgCount = 0;
>          bool more;
> +        m_imapMailFolderSink->GetMsgHdrsToDownload(&more,
> +                                                &m_progressExpectedNumber,
> +                                                &msgCount, &msgIdList);

Looks like this indenting is off by 2 spaces.

@@ +4312,5 @@
>  {
>    // lets worry about this progress stuff later.
>    switch (fields) {
>    case kHeadersRFC822andUid:
> +    SetProgressString("imapReceivingMessageHeaders3", IMAP_HEADERS_STRING_INDEX);

What about just passing the index number aroung? For each index there is a specific string to show.
Like set m_progressStringIndex in SetProgressString(), which would be one of the 1-3 constants. nullptr would be represented by an invalid index like IMAP_NUMBER_OF_PROGRESS_STRINGS . The exact string name would be determined in ShowProgress().

@@ +5198,5 @@
>      const char *mailboxName = GetServerStateParser().GetSelectedMailboxName();
>      nsString unicodeMailboxName;
>      nsresult rv = CopyMUTF7toUTF16(nsDependentCString(mailboxName),
>                                     unicodeMailboxName);
>      if (NS_SUCCEEDED(rv))

When you are rewriting the whole function, you can use "ENSURE_SUCCESS_VOID(rv))" here that just returns on error, and you save one level of indent.

@@ +5200,5 @@
>      nsresult rv = CopyMUTF7toUTF16(nsDependentCString(mailboxName),
>                                     unicodeMailboxName);
>      if (NS_SUCCEEDED(rv))
>      {
> +      nsAutoString progressCurrentNumberString;

The variable names are a bit too long for my taste :)

@@ +5207,5 @@
> +      nsAutoString progressExpectedNumberString;
> +      progressExpectedNumberString.AppendInt(m_progressExpectedNumber);
> +
> +      #define NUM_FMT_STRINGS 3
> +      const char16_t *formatStrings[NUM_FMT_STRINGS] = {

we use to do '*formatStrings[] =' and then just put the count into the FormatStringFromName call. Then the define is not needed.

@@ +5214,5 @@
> +        unicodeMailboxName.get()
> +      };
> +
> +      rv = m_bundle->FormatStringFromName(
> +                      NS_ConvertUTF8toUTF16(m_progressStringName).get(),

Can you change SetProgressString() to take 'const char16_t' to save this conversion? You then call it as SetProgressString(u"imapFolderReceivingMessageOf3", ...) (notice the 'u').

@@ +5217,5 @@
> +      rv = m_bundle->FormatStringFromName(
> +                      NS_ConvertUTF8toUTF16(m_progressStringName).get(),
> +                      formatStrings, NUM_FMT_STRINGS, getter_Copies(progressString));
> +
> +      if (NS_SUCCEEDED(rv) && progressString.get())

!progressString.IsEmpty() should be better. PercentProgressUpdateEvent can handle nullptr first argument, but we probably don't want to call it with an empty string. I'm not sure an empty nsString is always represented by nsString.get()==nullptr.
Attachment #8853633 - Flags: review?(acelists) → review+

Comment 40

9 months ago
(In reply to :aceman from comment #39)
> I still couldn't see where the account name is prepended to the message. But
> you do not need to bother with that.
Read comment #32 ;-) - Quote:

And I'd bet my hat that this is done for absolutely all reporting on any progress, be it POP, IMAP, etc.
https://dxr.mozilla.org/comm-central/rev/bee0697a1d3709df1db982f8320e7cac4a0b62c5/mailnews/base/src/nsMsgStatusFeedback.cpp#273
(Assignee)

Comment 42

9 months ago
Created attachment 8857770 [details] [diff] [review]
1348762--review-changes1.patch

Here is a patch in response to aceman's review in Comment 39. I tried to address each of his issues with this. I also included my review from Comment 37.
This patch is against my previous patch for this bug and not from before any changes were made by me. Not sure if that is right, but, if not, I'm sure you'll let me know :).

Updated

9 months ago
Attachment #8857770 - Flags: review?(jorgk)
Attachment #8857770 - Flags: review?(acelists)

Comment 43

9 months ago
Comment on attachment 8853633 [details] [diff] [review]
1348762--improve-prog-msgs.patch

I'll review the other patch.
Attachment #8853633 - Flags: review?(jorgk)

Comment 44

9 months ago
Created attachment 8858337 [details] [diff] [review]
1348762-merged.patch

Thanks, we prefer a single patch for the whole feature, even if a partial path got review already. We migrate the r+ or do another review for the extended full patch.

I have merged the patches for you :)

I think this looks good now.
Attachment #8853633 - Attachment is obsolete: true
Attachment #8857770 - Attachment is obsolete: true
Attachment #8857770 - Flags: review?(jorgk)
Attachment #8857770 - Flags: review?(acelists)
Attachment #8858337 - Flags: review+

Updated

9 months ago
Status: NEW → ASSIGNED

Updated

9 months ago
Attachment #8858337 - Flags: review?(jorgk)

Comment 45

9 months ago
Comment on attachment 8858337 [details] [diff] [review]
1348762-merged.patch

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

Nice!

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +5173,5 @@
>    m_lastPercent = -1;
>    m_lastProgressStringName.Truncate();
>  }
>  
> +void nsImapProtocol::SetProgressString(uint32_t aStringIndex)

This needs to be int32_t if we make the "no index" -1.

::: mailnews/imap/src/nsImapProtocol.h
@@ +133,5 @@
> +#define IMAP_NUMBER_OF_PROGRESS_STRINGS 3
> +#define IMAP_HEADERS_STRING_INDEX       0
> +#define IMAP_FLAGS_STRING_INDEX         1
> +#define IMAP_MESSAGES_STRING_INDEX      2
> +#define IMAP_NO_PROGRESS_STRING  IMAP_NUMBER_OF_PROGRESS_STRINGS

Nit: I think this is unfortunate. Make that -1, please
Attachment #8858337 - Flags: review?(jorgk) → review+

Comment 46

9 months ago
I can't import the patch, something wrong:
Parsing...Patch id=8850868 desc="1348762--improve-prog-msgs.patch" diff data were discarded:
UnicodeDecodeError: 'utf8' codec can't decode bytes in position 5609-5610: invalid continuation byte

That's why the ... shows up wrong in the review, I assume. Let me fix that and change the nits from comment #45, if you don't mind.

Comment 47

9 months ago
Created attachment 8858550 [details] [diff] [review]
1348762-merged.patch + minor tweaks

Any objection to landing this?
Attachment #8858550 - Flags: review+

Comment 48

9 months ago
Something very funny going on here with the ... .

The "support lines" in the patch, like:
@@ -105,20 +111,23 @@ imapMovingMessage=Moving message to %S
had broken unicode characters as the end. As a consequence, the patch couldn't be imported and the ... don't show in the review. After removing those broken characters, the review looks good, but the interdiff still doesn't work.

Anyway, the only thing I've changed is:
#define IMAP_NO_PROGRESS_STRING         -1
since I think that's better then putting "max index" there.

Comment 49

9 months ago
Hmm, perhaps not ready for prime time just yet.

I've compiled and run it and it crashes here:
nsImapProtocol.cpp:4252:
  m_progressCurrentNumber[m_stringIndex] = 0;  <====
  m_runningUrl->SetMoreHeadersToDownload(more);

Sadly my debugger broke so badly that I couldn't get any values.

I guess I missed making this signed:
  uint32_t      m_stringIndex = IMAP_HEADERS_STRING_INDEX;
but the initialisation needs to go into the constructor, right?

Comment 50

9 months ago
Created attachment 8858554 [details] [diff] [review]
imapProgressStrings.patch + more tweaks.

With this patch I didn't crash :-)

Are you OK with my changes?
Attachment #8858337 - Attachment is obsolete: true
Attachment #8858550 - Attachment is obsolete: true
Attachment #8858554 - Flags: review+
Attachment #8858554 - Flags: feedback?(gds)

Comment 51

9 months ago
(In reply to Jorg K (GMT+2) from comment #48)
> Something very funny going on here with the ... .
> 
> The "support lines" in the patch, like:
> @@ -105,20 +111,23 @@ imapMovingMessage=Moving message to %S
> had broken unicode characters as the end. As a consequence, the patch
> couldn't be imported and the ... don't show in the review. After removing
> those broken characters, the review looks good, but the interdiff still
> doesn't work.

Yes I noticed that too, but it didn't prevent importing the patch.
(Assignee)

Comment 52

9 months ago
Comment on attachment 8858554 [details] [diff] [review]
imapProgressStrings.patch + more tweaks.

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

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +2639,5 @@
>            {
>              // multiple messages, fetch them all
> +            SetProgressString(IMAP_MESSAGES_STRING_INDEX);
> +
> +            m_progressCurrentNumber[m_stringIndex] = 0;

This worries me some. If this gets set when m_stringIndex is -1 or other out of range index it might crash. You said you saw a crash. I also saw a crash a few times but the next time I would start tb it was OK. Never could pin it down. Only saw it shortly after startup. Did a full pull/update and clean build seemed to stop it, but not sure now. 
I will look closer at these usages and see if there is a problem.

@@ +3067,5 @@
>            nsresult rv = m_runningUrl->GetListOfMessageIds(messageIdString);
>            if (NS_SUCCEEDED(rv))
>            {
> +            SetProgressString(IMAP_MESSAGES_STRING_INDEX);
> +            m_progressCurrentNumber[m_stringIndex] = 0;

Same potential problem here.

@@ +4252,2 @@
>                                                   &msgCount, &msgIdList);
> +      m_progressCurrentNumber[m_stringIndex] = 0;

Ditto

@@ +4287,5 @@
>        // while we're dumping the messages, and then restore the setting.
>        bool wasStoringOffline;
>        m_runningUrl->GetStoreResultsOffline(&wasStoringOffline);
>        m_runningUrl->SetStoreResultsOffline(true);
> +      m_progressCurrentNumber[m_stringIndex] = 0;

Ditto

@@ +5226,5 @@
> +
> +    if (NS_SUCCEEDED(rv) && !progressString.IsEmpty())
> +    {
> +      PercentProgressUpdateEvent(progressString.get(),
> +          m_progressCurrentNumber[m_stringIndex], m_progressExpectedNumber);

I think this is OK since only called when string is not empty, so m_stringIndex is not -l.

::: mailnews/imap/src/nsImapProtocol.h
@@ +646,3 @@
>    
> +  nsString      m_progressStringName;
> +  int32_t       m_stringIndex;

I think my original initialization of m_stringIndex here would fail for "legacy" c++ but would work for c++11. Since it didn't cause a compile error it must be OK with my compiler, gcc 5.3.1. However, I don't know other compilers you use that might not support c++11 so better to move the init to the constructor like you did.

Comment 53

9 months ago
(In reply to gene smith from comment #52)
> This worries me some. If this gets set when m_stringIndex is -1 or other out
> of range index it might crash.
With your initial "out of range by one" (since you set it to 3) you were covering errors and introducing hard-to-find problems by overwriting allocated memory. At least with -1 you'll hear it crash ;-).

> You said you saw a crash. I also saw a crash
> a few times but the next time I would start tb it was OK.
How about you run with the modified patch for a while. I got the crash after start up when clicking on a folder with new messages. Or check the index with MOZ_ASSERT(0 <== m_stringIndex && m_stringIndex < IMAP_NUMBER_OF_PROGRESS_STRINGS).

> I think my original initialization of m_stringIndex here would fail for
> "legacy" c++ but would work for c++11. Since it didn't cause a compile error
> it must be OK with my compiler, gcc 5.3.1. However, I don't know other
> compilers you use that might not support c++11 so better to move the init to
> the constructor like you did.
Yes. Let's not introduce a new concept to old code here.
(Assignee)

Comment 54

9 months ago
Created attachment 8860590 [details] [diff] [review]
1348762--avoid-crash.patch

This avoids a crash due to writing outside the array. I left in the MOZ_ASSERT that you suggested. Not sure if you want them left in.

However, I did notice some other serious problems when downloading mailboxes from the imap server (again, mobile.charter.net). This has nothing to do with the changes in the attached patch (as far as I can tell). This probably warrants a new bug report but I will go ahead and describe it here for now.

I have been running with a direct ethernet connection to the wifi router until just recently. My wifi router was moved to another room and on my tb dev/test computer I am now using wifi via a usb dongle. The wifi works OK for normal web browsing it seems. However, problems arise when I delete all my email folders (using a file manager with tb shutdown) and then start tb and let them all re-download.
The main folders are inbox and sent with theses sizes in bytes:
358582991 Apr 21 20:34 INBOX
  2734987 Apr 21 21:31 INBOX.msf
 82619936 Apr 21 20:26 Sent
  1181517 Apr 21 21:25 Sent.msf
After downloading headers for a while, the server seems to stop responding and server messages are sent by tb like this (wireshark summary line):
1062	22:33:19.517358000	192.168.1.11	68.114.188.72	TLSv1.2	119	[TCP Retransmission] Encrypted Alert
A "retry" of this message is sent every few minutes and after maybe 10 or so of these the header download resumes. When these occur I also see this at tb's stdout:
2017-04-18 19:49:05        autosyncActivities      ERROR   OnDownloadError: Sent of gds@charter.net
These usually occur when headers are being downloaded and once they finish, the messages download occurs with no visible errors. 
Sometimes all the folders download OK (eventually) and have the expected size. However, more often there are problems with the download when it finishes. The problems are
1) Sometimes one or rarely 2 messages are listed in tb with no date, only a time. This may be a random email from years ago but it appears in the mailbox as the newest message. Inside the email, date and time are both shown.
2) Occasionally the download seems to finish with no problems and emails are displayed OK but the download never stops. I have seen this only on Sent mailbox. The Sent folder just keeps getting bigger (looking with the file mgr) and IMAP/TLS activity continues as seen with wireshark. The Sent folder, which is normally about 80Mbytes grew to 426Mbytes before I finally shutdown tb. After restarting tb, no more download occurred. "Compressing" the Sent folder did not make it smaller. In the Sent file I could see the same email numerous times.
3) A few times I have seen the download complete correctly with no further network activity like in 2) and with correct file sizes but tb registers at a constant 107% cpu in top after all download finishes. Normally it is around 3%. Restarting tb fixes the 107% activity so it goes back to around 3% and all is then well.

After returning to a direct ethernet connection to the router (not wifi) I have not observed any of the above problems after doing many full downloads from imap server. Apparently something about the wifi latency is causing problems with imap on tls? Maybe a known problem? I haven't searched bugzilla for this.

Since I saw no other problems with wifi I wasn't sure for a while that it wasn't caused by the fixes for this bug. But since it works OK with all the fixes when using ethernet I am pretty sure it is not caused by my recent changes.

Comment 55

9 months ago
Created attachment 8860599 [details] [diff] [review]
1348762--avoid-crash.patch (broken UTF-8 characters removed)
Attachment #8860590 - Attachment is obsolete: true

Comment 56

9 months ago
Comment on attachment 8860599 [details] [diff] [review]
1348762--avoid-crash.patch (broken UTF-8 characters removed)

Sigh, I wish the interdiff worked :-(
Attachment #8860599 - Flags: review?(jorgk)

Comment 57

9 months ago
Comment on attachment 8860599 [details] [diff] [review]
1348762--avoid-crash.patch (broken UTF-8 characters removed)

Thanks, I think we can go with this and move on to doing the same in bug 1348769.

I'll get this landed and also add full stops at the end of comments where they are missing ;-)

As for the other problems you describe. Yes, they need go to another bug, CC: Wada, he's got a lot of IMAP experience.
Attachment #8860599 - Flags: review?(jorgk) → review+

Comment 58

9 months ago
https://hg.mozilla.org/comm-central/rev/8ac8055cd42c05612a26558e684b9aa763ff36c2
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 55.0
(Reporter)

Comment 59

9 months ago
Thanks everyone, esp. Gene for taking this! Great turnaround time (


Sorry for not following this very closely; I've got some questions...

(In reply to Jorg K (GMT+2) from comment #33)
> Oh, I forgot to say: That makes it undesirable as well as impossible to have
> a placeholder for the account in the message.

1) Then why do we still have an account placeholder in the patch?

> # EXAMPLE: Ted's account: Downloading message 334 of 1008 in Inbox…
> autosyncProcessProgress2=%4$S: Downloading message %1$S of %2$S in %3$S…

2) Also, technically there seem to be 4 types of messages even though autosyncProcessProgress2 and imapFolderReceivingMessageOf3 happen to have the same wording. In the processing function, I see reference to only 3 messages - is that a problem?

> 130 // There are 3 types of progress strings for items downloaded from IMAP servers.
> 131 // An index is needed to keep track of the current count of the number of each
> 132 // item type downloaded. The IMAP_EMPTY_STRING_INDEX means no string displayed.
> 133 #define IMAP_NUMBER_OF_PROGRESS_STRINGS 4
> 134 #define IMAP_HEADERS_STRING_INDEX       0
> 135 #define IMAP_FLAGS_STRING_INDEX         1
> 136 #define IMAP_MESSAGES_STRING_INDEX      2
> 137 #define IMAP_EMPTY_STRING_INDEX         3

Finally, some questions on "parallel processing" of multiple accounts (for lack of a better word):

3) With more than 1 Imap account, when they are polled at the same time, I assume we are actually processing both of them in a somewhat parallel / intertwined fashion (i. e. we don't wait for 1st account to finish before starting 2nd account), right?

4) That's called asynchronous, right?

5) So for this asynchronous (parallel) processing of more than 1 Imap account, is the code written so that the status messages don't get confused with message counts etc. from the other account?

6) Do we have tests for that?

7) I see the same function handing status messages about headers, flags, messages. For processing a *single* account, maybe these flavors can never cooccur, depending on our design. What if both parrallel-processed accounts need e.g. status messages of type "header" at the same time, does that work? (Not sure if that's the same question like 5), but here I'm trying to ask if you're keeping headers, flags, messages strictly apart so that count of headers can never be confused with count of flags etc..., while still getting 5) right)?

8) Do we have tests for 7)?

9) (In reply to :aceman from comment #34)
> There is also another solution we use sometimes. If you have one function
> producing string1 and another function producing string2, you can
> concatenate them using a third string, not just as string1+" "+string2. The
> string3 would be e.g. "%1$S: %2$S" where string1 and string2 would be
> replaced for the placeholders.

Can someone point me the actual location in code where we prepend the account name and the colon? I tried to find it but failed.
I think if we want to be fully localizable, we must use this meta-pattern described by Aceman.
E.g., we can't hardcode the separator (colon), and I have doubts if we can even hardcode the order of account name and separator, and to just prepend that to the left of the string (not sure of implications for RTL languages).

Comment 60

9 months ago
(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #59)
> Can someone point me the actual location in code where we prepend the
> account name and the colon?
Comment #32, comment #40 and for the message:
https://dxr.mozilla.org/comm-central/rev/5e4e889f13eb1fd5091f60921a1566c661f2c630/mail/locales/en-US/chrome/messenger/messenger.properties#11
(Reporter)

Comment 61

9 months ago
Another nit for localization notes:

Current comments - compare:

a) Running count (wrong for headers, flags, and received messages):
> # Place the word %1$S in your translation where the *number of /headers/ being downloaded* should appear.

b) Total count ("correct", but odd wording):
> # Place the word %2$S in your translation where the *total number of pending headers* should appear.

c) Running count (correct for synced messages, # LOCALIZATION NOTE (autosyncProcessProgress2)):
> # Place the word %1$S in your translation where the *number of the /message/ being downloaded* should appear.

The wording of comment (a) is erroneous and confusing, because we have accidentally used plural (headers/flags/messages being downloaded) instead of singular (header/flag/message being downloaded). Then, unfortunatley, the meaning of "number of headers being downloaded" is pretty much the same as "total number of pending headers". If translators are confused about this, they might end up positioning {current/running count} vs. {total count} the wrong way round, which can mess up the message completely.

For the current/running count of {headers|flags|received messages|synced messages}, I suggest that we use the following wording pattern instead:
*running number of the {header|flag|message|message} currently being downloaded*

Proposal:
> a) # Place the word %1$S in your translation where the *running number* of the header currently being downloaded should appear.
* Pls change all 4 flavors including c) above).
* Pls double-check to use *singular* forms of header/flag/message/message.
* I suggest that we use starred emphasis ("*running number*") exactly as in the proposal.

While we are here, the wording of the total count comment should also be improved, because strictly speaking "number of pending headers" is NOT the total number of headers, but the remaining number of headers still pending for download.
I suggest using this wording pattern instead:
*total number of {headers|flags|messages|messsages} to be downloaded*

Proposal:
> b) # Place the word %2$S in your translation where the *total number* of headers to be downloaded should appear.

Comment 62

9 months ago
Created attachment 8860715 [details] [diff] [review]
1348762-notes.patch

Thomas, you're very late with your post-landing comments. However, you've got a point. I don't think the wording "running number" is necessary.
Attachment #8860715 - Flags: review?(bugzilla2007)

Comment 63

9 months ago
Created attachment 8860718 [details] [diff] [review]
1348762-notes.patch

Oops, "the" missing in two cases.
Attachment #8860715 - Attachment is obsolete: true
Attachment #8860715 - Flags: review?(bugzilla2007)
Attachment #8860718 - Flags: review?(bugzilla2007)
(Reporter)

Comment 64

9 months ago
Midair-collision, will look into Jörg's new patches after this comment...
Sorry for coming late to the party, you were fixing this too fast ;)
Better late than never... ;)

Nit #3:

> # LOCALIZATION NOTE (autosyncProcessProgress2): Do not translate the word "%1$S", "%2$S", "%3$S" and "%4$S" below.

That sounds like an ungrammatical singular to me, pls replace "word" to become "words" (apart from eleminating %4$S if my comment 59, #1 is correct).
So this should become:

Proposal:
> # LOCALIZATION NOTE (autosyncProcessProgress2): Do not translate the words "%1$S", "%2$S", and "%3$S" below.

Accordingly for flags, headers, and received messages (word -> words).


Nit #4 (that's a proper bug, actually, with significant potential to break all account-related status messages; not caused by this patch):

https://dxr.mozilla.org/comm-central/rev/5e4e889f13eb1fd5091f60921a1566c661f2c630/mail/locales/en-US/chrome/messenger/messenger.properties#11
> #LOCALIZATION NOTE(statusMessage): Do not translate the words
> # $1S and $2S below. Place the word $1S where account name should appear and $2S

Really? If translators follow that instruction, they'll break status messages completely, because the *placeholder names in the comment are wrong* (assuming that they are right in the string):
In the comment, please substitute:

$1S -> %1$S (yes, these things are really cryptic, for TB.next maybe this could be simplified a bit...)
$2S -> %2$S

Due the instruction to put a wrong placeholder, I feel someone should check all localizations if there are any using the wrong placeholders. Is there any simple method from translation tools to see the translation of a given string into all locales at a glance? (Otherwise, we would have to change the string ID, which however might create more error potential than it would eliminate).

> # where the status message should appear.

I'm suprised that this important string pattern does not come with an example, let's add it please...

> statusMessage=%1$S: %2$S

Many languages won't touch this important string, and they shouldn't, and we should let them know in more detail what this is all about. Hence:

Proposal:

# LOCALIZATION NOTE(statusMessage):
# Do not translate the words %1$S and %2$S below. Place the word %1$S where the
# account name should appear and %2$S where the status message should appear.
# Note: This abstract model string is used to add the account name to status
# messages, typically in front. Translators should only touch this if account-
# related status messages (like the following example) have the account name
# in the wrong position, or if the colon (":") doesn't work as a separator in
# their local language.
# Example of a generated message (do not translate):
# Jim's Account: Downloading messages...
statusMessage=%1$S: %2$S

With that, we could even change the string ID to something more unique and descriptive, like accountStatusMessagePrototype or statusMessageAccountPrototype?

Btw, do we have non-account-related status messages?
Just my 2 cents.

Comment 65

9 months ago
Created attachment 8860723 [details] [diff] [review]
1348762-notes.patch

word --> words.
Fixed comment for statusMessage.
Now let's not fix all wrong translators' notes this Sunday morning :-(
Attachment #8860718 - Attachment is obsolete: true
Attachment #8860718 - Flags: review?(bugzilla2007)
Attachment #8860723 - Flags: review?(bugzilla2007)
(Reporter)

Comment 66

9 months ago
Comment on attachment 8860718 [details] [diff] [review]
1348762-notes.patch

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

Thanks for the rapid new patch! :)
On the downside, being too rapid may overtake ongoing scrutiny missions of previous patches and require more iterations... ;)
r+ with some more nits fixed.

Next patch, please also include fix for wrong localization comment of statusMessage model string (not caused by this patch), which is very error-prone (see comment 64, nit #4.) With that, I propose changing that string ID to accountStatusMessagePrototype or statusMessageAccountPrototype.

::: mail/locales/en-US/chrome/messenger/activity.properties
@@ +27,5 @@
>  failedToSendMessageWithSubject=Failed to send message: %S
>  # LOCALIZATION NOTE (failedToCopyMessageWithSubject): %S will be replaced by the subject of the message being sent.
>  failedToCopyMessageWithSubject=Failed to copy message: %S
>  
>  # LOCALIZATION NOTE (autosyncProcessProgress2): Do not translate the word "%1$S", "%2$S", "%3$S" and "%4$S" below.

Per my above comment 64 (which came in after Jörg's patch): word -> words.

Per my above comment 59 (which came in before this patch): Why are we still keeping an account placeholder %4$S here, whereas in all other instances we auto-prepend? Sorry if I overlooked an existing comment on this...

If it can be removed, please add improved note about auto-adding account name, as seen below (nit #5).

@@ +29,5 @@
>  failedToCopyMessageWithSubject=Failed to copy message: %S
>  
>  # LOCALIZATION NOTE (autosyncProcessProgress2): Do not translate the word "%1$S", "%2$S", "%3$S" and "%4$S" below.
>  # Place the word %1$S in your translation where the number of the message being downloaded should appear.
> +# Place the word %2$S in your translation where the total number of messages should appear.

Jörg prefers minimalist/contextual, me prefers explicit (which total number of messages?); would add "to be downloaded" context here:

# Place the word %2$S in your translation where the total number of messages to be downloaded should appear.

OT: For the avoidance of doubt, I'm repeating this not to insist on my proposal, but since you haven't explicitly commented that or why you removed this part of my proposed wording, I can't tell if it's just by accidental omission or deliberate. I guess it's deliberate, but guessing isn't good enough. Perfectionists at work ;)

@@ +34,2 @@
>  # Place the word %3$S in your translation where the name of the folder being processed should appear.
>  # Place the word %4$S in your translation where the name of account being processed should appear.

dito, wrt remove account placeholder?

@@ +34,4 @@
>  # Place the word %3$S in your translation where the name of the folder being processed should appear.
>  # Place the word %4$S in your translation where the name of account being processed should appear.
>  # EXAMPLE: Ted's account: Downloading message 334 of 1008 in Inbox…
>  autosyncProcessProgress2=%4$S: Downloading message %1$S of %2$S in %3$S…

dito, remove account placeholder?

::: mail/locales/en-US/chrome/messenger/imapMsgs.properties
@@ +74,5 @@
>  imapGettingMailboxInfo=Getting Mailbox Configuration Info…
>  
>  imapEmptyMimePart=This body part will be downloaded on demand.
>  
>  # LOCALIZATION NOTE (imapReceivingMessageHeaders3): Do not translate the word "%1$S", "%2$S", and "%3$S" below.

word -> words

@@ +76,5 @@
>  imapEmptyMimePart=This body part will be downloaded on demand.
>  
>  # LOCALIZATION NOTE (imapReceivingMessageHeaders3): Do not translate the word "%1$S", "%2$S", and "%3$S" below.
> +# Place the word %1$S in your translation where the number of the header being downloaded should appear.
> +# Place the word %2$S in your translation where the total number of headers should appear.

headers to be downloaded?

@@ +81,2 @@
>  # Place the word %3$S in your translation where the name of the folder being processed should appear.
>  # Note: The account name and colon is automatically encoded first in the displayed string.

Nit #5 (not caused by this patch):
Encoded first? Displayed string? This sounds pretty tech-gibberish to me, and may be factually incorrect as we only know the default position of account name, which might differ for other locales, per language-specific statusMessage model string.

Proposal:
# Note: The account name and localized separators (e.g. colon, space) will be automatically added to the status message.

@@ +84,5 @@
>  imapReceivingMessageHeaders3=Downloading message header %1$S of %2$S in %3$S…
>  
>  # LOCALIZATION NOTE (imapReceivingMessageFlags3): Do not translate the word "%1$S", "%2$S", and "%3$S" below.
> +# Place the word %1$S in your translation where the number of the flag being downloaded should appear.
> +# Place the word %2$S in your translation where the total number of flags should appear.

flags to be downloaded?

@@ +89,2 @@
>  # Place the word %3$S in your translation where the name of the folder being processed should appear.
>  # Note: The account name and colon is automatically encoded first in the displayed string.

dito, improve account name note (see above, nit #5)

@@ +110,5 @@
>  # LOCALIZATION NOTE (imapCopyingMessage): Do not translate the word "%S" below.
>  # Place the word %S in your translation where the name of the folder should appear.
>  imapCopyingMessage=Copying message to %S…
>  
>  # LOCALIZATION NOTE (imapFolderReceivingMessageOf3): Do not translate the word "%1$S", "%2$S", and "%3$S" below.

word -> words

@@ +112,5 @@
>  imapCopyingMessage=Copying message to %S…
>  
>  # LOCALIZATION NOTE (imapFolderReceivingMessageOf3): Do not translate the word "%1$S", "%2$S", and "%3$S" below.
> +# Place the word %1$S in your translation where the number of the message being downloaded should appear.
> +# Place the word %2$S in your translation where the total number of messages should appear.

messages to be downloaded?

@@ +117,2 @@
>  # Place the word %3$S in your translation where the name of the folder being processed should appear.
>  # Note: The account name and colon is automatically encoded first in the displayed string.

dito, improve account name note (see above, nit #5)

::: suite/locales/en-US/chrome/mailnews/imapMsgs.properties
@@ +79,5 @@
>  
>  # Status - empty mime part
>  imapEmptyMimePart=This body part will be downloaded on demand.
>  
>  # LOCALIZATION NOTE (imapReceivingMessageHeaders3): Do not translate the word "%1$S", "%2$S", and "%3$S" below.

word -> words

@@ +81,5 @@
>  imapEmptyMimePart=This body part will be downloaded on demand.
>  
>  # LOCALIZATION NOTE (imapReceivingMessageHeaders3): Do not translate the word "%1$S", "%2$S", and "%3$S" below.
> +# Place the word %1$S in your translation where the number of the header being downloaded should appear.
> +# Place the word %2$S in your translation where the total number of headers should appear.

headers to be downloaded?

@@ +86,2 @@
>  # Place the word %3$S in your translation where the name of the folder being processed should appear.
>  # Note: The account name and colon is automatically encoded first in the displayed string.

dito, new note, nit #5

@@ +89,5 @@
>  imapReceivingMessageHeaders3=Downloading message header %1$S of %2$S in %3$S…
>  
>  # LOCALIZATION NOTE (imapReceivingMessageFlags3): Do not translate the word "%1$S", "%2$S", and "%3$S" below.
> +# Place the word %1$S in your translation where the number of the flag being downloaded should appear.
> +# Place the word %2$S in your translation where the total number of flags should appear.

flags to be downloaded?

@@ +94,2 @@
>  # Place the word %3$S in your translation where the name of the folder being processed should appear.
>  # Note: The account name and colon is automatically encoded first in the displayed string.

dito, new note, nit #5

@@ +115,5 @@
>  # LOCALIZATION NOTE (imapCopyingMessage): Do not translate the word "%S" below.
>  # Place the word %S in your translation where the name of the folder should appear.
>  imapCopyingMessage=Copying message to %S…
>  
>  # LOCALIZATION NOTE (imapFolderReceivingMessageOf3): Do not translate the word "%1$S", "%2$S", and "%3$S" below.

word -> words

@@ +117,5 @@
>  imapCopyingMessage=Copying message to %S…
>  
>  # LOCALIZATION NOTE (imapFolderReceivingMessageOf3): Do not translate the word "%1$S", "%2$S", and "%3$S" below.
> +# Place the word %1$S in your translation where the number of the message being downloaded should appear
> +# Place the word %2$S in your translation where the total number of messages should appear.

messages to be downloaded?

@@ +122,2 @@
>  # Place the word %3$S in your translation where the name of the folder being processed should appear.
>  # Note: The account name and colon is automatically encoded first in the displayed string.

dito, new note, nit #5
Attachment #8860718 - Attachment is obsolete: false

Comment 67

9 months ago
Created attachment 8860732 [details] [diff] [review]
1348762-notes.patch

Care to take another look. activity.properties is different, done in JS, you have %4$S there.
Attachment #8860718 - Attachment is obsolete: true
Attachment #8860723 - Attachment is obsolete: true
Attachment #8860723 - Flags: review?(bugzilla2007)
(Reporter)

Comment 68

9 months ago
Comment on attachment 8860732 [details] [diff] [review]
1348762-notes.patch

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

Perfect. :)

::: suite/locales/en-US/chrome/mailnews/imapMsgs.properties
@@ +116,5 @@
>  # Place the word %S in your translation where the name of the folder should appear.
>  imapCopyingMessage=Copying message to %S…
>  
> +# LOCALIZATION NOTE (imapFolderReceivingMessageOf3): Do not translate the words "%1$S", "%2$S", and "%3$S" below.
> +# Place the word %1$S in your translation where the number of the message being downloaded should appear

...except for this missing trailing dot! ;)
Attachment #8860732 - Flags: review+
(Assignee)

Comment 70

9 months ago
(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #59)
> Finally, some questions on "parallel processing" of multiple accounts (for
> lack of a better word):
> 
> 3) With more than 1 Imap account, when they are polled at the same time, I
> assume we are actually processing both of them in a somewhat parallel /
> intertwined fashion (i. e. we don't wait for 1st account to finish before
> starting 2nd account), right?
> 
> 4) That's called asynchronous, right?
> 
> 5) So for this asynchronous (parallel) processing of more than 1 Imap
> account, is the code written so that the status messages don't get confused
> with message counts etc. from the other account?
> 
> 6) Do we have tests for that?
> 
> 7) I see the same function handing status messages about headers, flags,
> messages. For processing a *single* account, maybe these flavors can never
> cooccur, depending on our design. What if both parrallel-processed accounts
> need e.g. status messages of type "header" at the same time, does that work?
> (Not sure if that's the same question like 5), but here I'm trying to ask if
> you're keeping headers, flags, messages strictly apart so that count of
> headers can never be confused with count of flags etc..., while still
> getting 5) right)?
> 
> 8) Do we have tests for 7)?

I had thought about it but I hadn't tried to download two accounts at the same time (after deleting all the "folders" for them). So I just tried it with charter and gmail downloading at the same time. These are the only accounts I have that contain a large number of messages in several folders so the download isn't instant. It seemed to work OK. 
In the status at the bottom it would switch between charter and gmail accounts and also between the different mailboxes (folders) within the accounts. Most of the "multitasking" occurs when downloading headers. Once a mailbox (folder) has all its headers it seems to stay with that mailbox until all the messages are downloaded, then moves to another mailbox in an account. It didn't seem to get confused as to the counts while downloading because each mailbox has a corresponding instance of nsImapProtocol class so the counts are separated by mailbox and don't get corrupted.

I think when you are initializing several mailboxes/accounts it would be better to have a separate display for this instead of just the single line at the bottom which is hard to read since it is often overwritten by different accounts, mailboxes, item types and counts. Each account and the mailboxes/item type within the account could have its own line in a separate pop-up display, e.g.,

Acct:   mbox/item type:  Num:  Total:
charter inbox/headers    100   4000
charter inbox/messages   50    4000
charter sent/headers     33    2000
charter sent/messages    15    2000
gmail   inbox/headers    300   5555
gmail   inbox/messages   50    5555
gmail   sent/headers     33    4324
gmail   send/messages    1     4324
:
:

This might appear in a new window if you double-click on the status at the bottom. Typically, only the "Num" values would increment and the other columns are mostly static. Would probably be a fairly major project...
(Assignee)

Updated

9 months ago
Attachment #8858554 - Flags: feedback?(gds) → feedback+

Updated

7 months ago
Depends on: 1373940
You need to log in before you can comment on or make changes to this bug.