331560 - "Order Received" reversed when copying messages from Local to IMAP account

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Gili Tzabari]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1

I want to migrate from POP3 to IMAP. So I create a new IMAP account, select all my old messages in my old Inbox and drag-n-drop them into the Inbox of my new IMAP account.

The problem? The "Order Received" in the destination Inbox is reverse of what it was in the source Inbox. I am expecting "Order Received" to be retained because I sort against this column and now all my emails sort in reverse order.

Reproducible: Always

Comment 1

[Felipe]

I have this problem, too, on TB 1.5.0.4.

It doesn’t reverse the order when I just copy from one mailbox to another in the same account, but when I copy all messages in a mailbox to a mailbox on another account, the order gets reversed.

Comment 2

[Gary Kwong [:gkw] [:nth10sd]]

Reporter, does this issue still occur in the latest supported 2.0.0.x / Shredder trunk nightlies?

(1.5.0.x is now end-of-life, and the latest supported 2.0.0.x is 2.0.0.16)

Comment 3

[Gili Tzabari]

Yes, I can reproduce the problem in 2.0.0.16

Comment 4

[Wayne Mery (:wsmwk)]

after copying, you are unable to resort to get the order you want?


beta 1 of Thunderbird 3 will be a available in a few days.  If you have a good backup of your data, please try http://www.mozillamessaging.com/en-US/thunderbird/early_releases/ and report results.

Comment 5

[Gili Tzabari]

Wayne,

The problem is that when I copy messages from one account to another their "order received" number is reversed. For example, if I have messages with order id:

First: 1
Second: 2

and I copy them, I end up with:

First: 2
Second: 1

Changing the sort order is pointless because the underlying data you are sorting over is corrupt.

Comment 6

[Wayne Mery (:wsmwk)]

I understand the first part. 

I'm guessing this may be closed invalid.  Yes, the order changes and it gets a new value for order received.  Mike can prob give an explanation.  Until then, have a look at some of these, http://tinyurl.com/6gvkht, especially the ones marked invalid.

Comment 7

[Gili Tzabari]

Your link gives me HTTP 403 forbidden.

As for marking this invalid, I disagree. I fully expect for Thunderbird to assign the items new IDs. But... it should retain the relative order of those IDs.

The use-case is rather simple: I have two accounts that are sorted according to increasing "order received". I want to copy messages from one account to another, without changing the sorting order, and expect the relative ordering to be retained. Given:

Account1
Item1
Item2
Item3

Account2
Item4
Item5
Item6

If I copy/paste Items 1, 2, 3 from account 1 to account 2 I expect to get:

Account2
Item4
Item5
Item6
Item1 (has new id but relative ordering is the same)
Item2 (has new id but relative ordering is the same)
Item3 (has new id but relative ordering is the same)

Comment 8

[Wayne Mery (:wsmwk)]

better I hope http://tinyurl.com/5qgtay

Comment 9

[WADA:World Anti-bad-Duping Agency]

Test result with Tb trunk(2008/12/04 build), on MS Win, with Gmail IMAP.

(Step-0) Initial status
> Account1 (Dummy POP3 account. "Order Received"==Offset in mail folder file)
>   Folder-1
>     Mail-1  Order Received=0       Size=L1      
>     Mail-2  Order Received=0+L1    Size=L2
>     Mail-3  Order Received=0+L1+L2 Size=L3 
> Account2 (IMAP account. "Order Received"==UID of mail)
>   Folder-2
>     Mail_IMAP_1  Order Received=UID=1
>   (IMAP Folder-2 is newly created, and a mail is copied to IMAP Folder-2)

(Step-1) When mails are sorted by "Order Received"/Ascending at local Folder-1.
         Select All at local Folder-1, and Copy to IMAP Folder-2.
> Account2 (IMAP account. "Order Received"==UID of mail)
>   Folder-2
>     Mail_IMAP_1  Order Received=UID=1
>     Mail-3       Order Received=UID=1+1=2
>     Mail-2       Order Received=UID=1+2=3
>     Mail-1       Order Received=UID=1+3=4
>     (Highest_UID==4 at this step) 

(Step-2) Delete Mail-1/Mail-2/Mail-3 in Folder-2 of IMAP.
         (This step is required, because Gmail IMAP ignores duplicated mail)   

(Step-3) When mails are sorted by "Order Received"/Descending at local Folder-1.
         Select All at local Folder-1, and Copy to IMAP Folder-2.
> Account2 (IMAP account. "Order Received"==UID of mail)
>   Folder-2
>     Mail_IMAP_1  Order Received=UID=1 
>     Mail-1       Order Received=UID=4+1=5
>     Mail-2       Order Received=UID=4+2=6
>     Mail-3       Order Received=UID=4+3=7

It looks that Thunderbird copies multiple mails in reversed-order of sorted-order at mail-list-pane(thread-pane).

To Gili(bug opener):
"Sort by 'Order Received/Descending' before Copy" can be a workaround?

Comment 10

[[:Aureliano Buendía]]

Gili could answer to comment #9, please?

Comment 11

[Gili Tzabari]

yes, workaround seems to work.

Comment 12

[Gili Tzabari]

On a related note, if you copy multiple emails at a time the destination copy will end up with an empty subject line. If you select the email the preview pane will display the subject line just fine, but the pane that lists all the emails you show an empty string instead. Restarting Thunderbird doesn't fix the problem either.

Comment 13

[Thomas D.]

Wada, is your comment 9 a confirmation of this bug?

I understand from comments that this only happens when copying into an IMAP account (pls correct me if wrong). Is "copy from POP to IMAP" necessary for reproducing or the same bug happens for IMAP to IMAP? Summary should be as precise as possible.

Comment 14

[WADA:World Anti-bad-Duping Agency]

Confirming, per my test results of comment #9.
Problem perhaps occurs when APPEND is needed to copy mails.
(a) From local folder(POP3) to IMAP folder
(b) From IMAP folder to IMAP folder of different IMAP account
If copy between IMAP folders of same IMAP account, "UID COPY" is used and Tb perhaps requests it in UID order, so this bug doesn't occur.

Comment 15

[WADA:World Anti-bad-Duping Agency]

This bug may be a result of "fetch(download) newest first" like logic in IMAP.

Comment 16

[David :Bienvenu]

(In reply to WADA from comment #15)
> This bug may be a result of "fetch(download) newest first" like logic in
> IMAP.

That's not involved here - it's just the copying to imap doesn't sort the message headers by message keys first, so it uses the order in the selection. Copying messages to a local mail folder does sort the message headers - http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsLocalMailFolder.cpp#1589

so this would be fairly easy to fix. It might be easier to sort the keys in the higher level code where we convert selection indices to message keys.

Comment 17

[WADA:World Anti-bad-Duping Agency]

(In reply to David :Bienvenu from comment #16)
> It might be easier to sort the keys in the higher level code where we convert selection indices to message keys.

As seen in WORKAROUND(sort in descending order at thread pane), sort of copied mails is already done at thread pane level, and problem in "copy from local to IMAP" is following.
  Copy to IMAP looks  for(var nn=mails.length;nn>=0;nn--){append mails[nn];}
  instead of expected for(var nn=0;nn<mails.length;nn++) {append mails[nn];}
If "copy from local to local", I think "copy lowest offset first, highest offset last" by "internal sorting by offset" is desirable, and it seems current logic.

Comment 19

[Ulf Zibis]

(In reply to WADA from comment #9)
> It looks that Thunderbird copies multiple mails in reversed-order of
> sorted-order at mail-list-pane(thread-pane).

As this "feature" looks kinda useful in some cases, please don't throw it away completely, but change it to from-above-to-below order.

Comment 20

[Ulf Zibis]

I just tried the the workaround from comment 9 on an entire mail folder. It doesn't work, so maybe bug 760852 should be treated separately.

Comment 21

[Ulf Zibis]

Another workaround:
Use tool "IMAPSize" to upload local Mbox file to IMAP server.
- "Order Received" has been perfectly saved on my try
- X-Mozilla-Keys were uploaded, but TB didn't care about any flag after subscribing the folder, see bug 760856.

Comment 22

[Magnus Melin [:mkmelin]]

Attachment: bug331560_imap_copy_order.patch

Comment 23

[[e-mail]]

Comment on attachment 8530937 [details] [diff] [review]
bug331560_imap_copy_order.patch

>+nsresult nsMsgDBFolder::SortMessagesBasedOnKey(nsTArray<nsMsgKey> &aKeyArray,
>+  nsIMsgFolder *srcFolder, nsIMutableArray* messages)
[This may have done so in the past, but there's no sorting done here any more.]
[It really pains me to see the hdrs turned into keys and back into hdrs again...]
[The output should really be an nsCOMArray<nsIMsgDBHdr> but that's beyond the scope of this bug.]

>+
Nit: please remove the unnecessary whitespace changes.

>     nsTArray<nsMsgKey> srcKeyArray;
...
>+    nsTArray<nsMsgKey> keyArray(numMsgs);
...
>-    rv = BuildIdsAndKeyArray(messages, messageIds, srcKeyArray);
>+    rv = BuildIdsAndKeyArray(sortedMsgs, messageIds, srcKeyArray);
Since you already have the key array, you can just call AllocateUidStringFromKeys directly.

Comment 24

[Magnus Melin [:mkmelin]]

(In reply to [e-mail] from comment #23)
> Comment on attachment 8530937 [details] [diff] [review]
> bug331560_imap_copy_order.patch
> 
> >+nsresult nsMsgDBFolder::SortMessagesBasedOnKey(nsTArray<nsMsgKey> &aKeyArray,
> >+  nsIMsgFolder *srcFolder, nsIMutableArray* messages)
> [This may have done so in the past, but there's no sorting done here any
> more.]

I suppose it's more of a reordering yes.

> >+    rv = BuildIdsAndKeyArray(sortedMsgs, messageIds, srcKeyArray);
> Since you already have the key array, you can just call
> AllocateUidStringFromKeys directly.

Unfortunately can't do sortedMsgs->->Elements() so the parameter type difference makes it harder to do, so I'd just leave it for now.

Comment 25

[Magnus Melin [:mkmelin]]

Attachment: bug331560_imap_copy_order.patch

Comment 26

[[e-mail]]

(In reply to Magnus Melin from comment #24)
> (In reply to comment #23)
> > (From update of attachment 8530937 [details] [diff] [review])
> > >+nsresult nsMsgDBFolder::SortMessagesBasedOnKey(nsTArray<nsMsgKey> &aKeyArray,
> > >+  nsIMsgFolder *srcFolder, nsIMutableArray* messages)
> > [This may have done so in the past, but there's no sorting done here any more.]
> I suppose it's more of a reordering yes.
Not even that, it doesn't actually use the old array in any way, it builds a new array of message headers from the sorted key array.

> > >+    rv = BuildIdsAndKeyArray(sortedMsgs, messageIds, srcKeyArray);
> > Since you already have the key array, you can just call
> > AllocateUidStringFromKeys directly.
> Unfortunately can't do sortedMsgs->->Elements() so the parameter type
> difference makes it harder to do, so I'd just leave it for now.
Type difference? AllocateUidStringFromKeys takes an nsMsgKey* which is what sortedMsgs->Elements() gives you.

Comment 27

[Magnus Melin [:mkmelin]]

Yes, if nsIMutableArray had an Elements()...

Comment 28

[[e-mail]]

(In reply to comment #26)
> AllocateUidStringFromKeys takes an nsMsgKey* which is what
> sortedMsgs->Elements() gives you.
I meant keyArray->Elements(). Sorry for the confusion.

Comment 29

[Magnus Melin [:mkmelin]]

Attachment: bug331560_imap_copy_order.patch

Doh! Like this?

Comment 30

[[e-mail]]

Comment on attachment 8531998 [details] [diff] [review]
bug331560_imap_copy_order.patch

>+  nsCOMPtr <nsIMsgDBHdr> msgHdr;
(I know you just cut & pasted the code)
Nit: no space before <

>+  if (NS_SUCCEEDED(rv) && db) {
>+    for (uint32_t i = 0; i < numMessages; i++)
>+    {
Nit: inconsistent { placement

> }
>+
Nit: unnecessary blank line

>     nsTArray<nsMsgKey> srcKeyArray;
Nit: unused variable

>+    nsCOMPtr<nsIMutableArray> sortedMsgs(do_CreateInstance(NS_ARRAY_CONTRACTID));
[14 lines snipped]
>+
>+    rv = MessagesInKeyOrder(keyArray, srcFolder, sortedMsgs);
Nit: sortedMsgs doesn't need to precede those 15 lines, might as well move it to where it's first used.

The code that calls MessagesInKeyOrder in nsLocalMailFolder.cpp looks very similar, but I couldn't see an easy way to special-case a single message here.

Comment 31

[Magnus Melin [:mkmelin]]

https://hg.mozilla.org/comm-central/rev/fa298c74a1df -> FIXED