Closed Bug 331560 Opened 14 years ago Closed 5 years ago

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

Categories

(MailNews Core :: Networking: IMAP, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 37.0

People

(Reporter: cowwoc2020, Assigned: mkmelin)

References

Details

(Whiteboard: [workaround comment #9])

Attachments

(1 file, 2 obsolete files)

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
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.
Assignee: mscott → nobody
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)
Whiteboard: closeme 2008-09-18
Yes, I can reproduce the problem in 2.0.0.16
Component: General → Mail Window Front End
Keywords: qawanted
QA Contact: general → front-end
Whiteboard: closeme 2008-09-18
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.
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.
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.
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)
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?
Gili could answer to comment #9, please?
Whiteboard: closeme 2009-11-13
yes, workaround seems to work.
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.
Whiteboard: closeme 2009-11-13 → [workaround comment #9]
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.
Summary: "Order Received" reversed when copying messages → "Order Received" reversed when copying messages to IMAP account
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Component: Mail Window Front End → Networking: IMAP
Product: Thunderbird → MailNews Core
QA Contact: front-end → networking.imap
Version: unspecified → Trunk
This bug may be a result of "fetch(download) newest first" like logic in IMAP.
(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.
(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.
Duplicate of this bug: 760852
(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.
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.
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.
Keywords: qawanted
OS: Windows XP → All
Hardware: x86 → All
Summary: "Order Received" reversed when copying messages to IMAP account → "Order Received" reversed when copying messages from Local to IMAP account
Attached patch bug331560_imap_copy_order.patch (obsolete) — Splinter Review
Assignee: nobody → mkmelin+mozilla
Status: NEW → ASSIGNED
Attachment #8530937 - Flags: review?(neil)
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.
Attachment #8530937 - Flags: review?(neil) → feedback+
(In reply to neil@parkwaycc.co.uk 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.
Attached patch bug331560_imap_copy_order.patch (obsolete) — Splinter Review
Attachment #8530937 - Attachment is obsolete: true
Attachment #8531595 - Flags: review?(neil)
(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.
Yes, if nsIMutableArray had an Elements()...
(In reply to comment #26)
> AllocateUidStringFromKeys takes an nsMsgKey* which is what
> sortedMsgs->Elements() gives you.
I meant keyArray->Elements(). Sorry for the confusion.
Doh! Like this?
Attachment #8531595 - Attachment is obsolete: true
Attachment #8531595 - Flags: review?(neil)
Attachment #8531998 - Flags: review?(neil)
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.
Attachment #8531998 - Flags: review?(neil) → review+
https://hg.mozilla.org/comm-central/rev/fa298c74a1df -> FIXED
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 37.0
You need to log in before you can comment on or make changes to this bug.