trunk: Undo message moved from local folder to an imap folder is broken for non UIDPLUS servers
Categories
(MailNews Core :: Backend, defect)
Tracking
(Not tracked)
People
(Reporter: naving, Unassigned)
References
(Blocks 2 open bugs)
Details
(Keywords: dataloss, imap-interop)
Attachments
(1 file)
2.40 KB,
patch
|
Details | Diff | Splinter Review |
The fix for bug 129495 caused this regression because it prevented SetAppendMsgUid needed for undo.
Reporter | ||
Updated•22 years ago
|
Reporter | ||
Updated•22 years ago
|
Comment 1•22 years ago
|
||
actually, I don't believe that was working even without that change. The message is removed from the source, but not from the destination imap folder, with or w/o my change. I'll go verify it on the branch, but that was my experience.
Reporter | ||
Comment 2•22 years ago
|
||
This code is never executed because dstKeyArray size is always zero. if (m_dstKeyArray.GetSize() > 0) { nsCOMPtr<nsIMsgFolder> dstFolder = do_QueryReferent(m_dstFolder, &rv); if (NS_FAILED(rv) || !dstFolder) return rv; nsCOMPtr<nsIUrlListener> dstListener; dstListener = do_QueryInterface(dstFolder, &rv); if (NS_FAILED(rv)) return rv; // ** make sire we are in the selected state; use lite select folder // so we won't hit preformace hard rv = imapService->LiteSelectFolder(m_eventQueue, dstFolder, dstListener, nsnull); if (NS_FAILED(rv)) return rv; rv = imapService->AddMessageFlags(m_eventQueue, dstFolder, dstListener, nsnull, m_dstMsgIdString.get(), kImapMsgDeletedFlag, m_idsAreUids); }
Reporter | ||
Comment 3•22 years ago
|
||
If I back out your fix locally on trunk it fixes the problem for me.
Comment 4•22 years ago
|
||
OK, well, please read the comments in the other bug - we can't go back to the way it worked before - that approach was ridiculously slow.
Updated•22 years ago
|
Reporter | ||
Comment 5•22 years ago
|
||
How else are we going to know the new key of the msg we appended. We cannot just go and search for new key when the user tries to do undo because that can happen anytime and new hdrs could come into that folder. The only way not to slow down copy is to not execute that code when we are copying sent mail, probably we would have to add another imapAction nsIImapUrl::CopyingSentMail or something.
Comment 6•22 years ago
|
||
no, as I thought I said in my other comment, you can search by message-id, which should be unique.
Comment 7•22 years ago
|
||
also, we shouldn't just fix it for copying to the sent folder. The way the code was before, if you copied 100 messages from local to imap, we would issue a select and 100 searches based on message-id trying to find the appended messages, JUST IN CASE the user undid the move/copy. That price should obviously only be paid in the highly unlikely event that the user undoes the move/copy.
Comment 8•22 years ago
|
||
OK, I've rearranged the code so that everything works or doesn't work like it did before. This was working for UIDPLUS servers and not working for non UIDPLUS servers. With this patch, we'll be back where we were in terms of it working, but w/o the horrible performance penalty on non UIDPLUS servers. This has r/sr =sspitzer/
Comment 9•22 years ago
|
||
changing summary, removing regression keyword, adding interop, since it works like it did before now. It is, however, still broken on non UIDPLUS servers, like the UW server. The Netscape server supports UIDPLUS, but we should really work on all the servers that don't support UIDPLUS.
Reporter | ||
Comment 10•22 years ago
|
||
Actually I was working on adding some search code in imapUndo. I mean creating search session..... and searching for message-id. Let me see how it goes. I will add a patch if it works. Also I will be looking into passing m_srcMsgIdString instead of m_dstMsgIdString.get() here (from comment #2) with m_idsAreUids as false. I don't know if it will work. rv = imapService->AddMessageFlags(m_eventQueue, dstFolder, dstListener, nsnull, m_dstMsgIdString.get(), kImapMsgDeletedFlag, m_idsAreUids);
Comment 11•22 years ago
|
||
yes, you still need to do that, I think. It's only in the case of UIDPLUS, where we know right away what the UID of the newly appended message will be, that we don't need to issue a search later. It will make life a little more complicated in that the UNDO object will need to know if it already knows the UID, or if it needs to search based on the string message-id to find the UID.
Reporter | ||
Comment 12•22 years ago
|
||
How is this not a regression anymore ? It was working for non UIDPLUS servers before.
Comment 13•22 years ago
|
||
No, it was not. If you can show me an imap protocol log with a non-uid plus imap server with a 1.0 build where it worked, then I'm wrong, but my tests showed that it did not work - the msg was not deleted from the destination imap server. In any case, as it stands now, it's an interop bug because it only occurs on certain imap servers.
Reporter | ||
Comment 14•22 years ago
|
||
why 1.0 build ? I am saying this is a trunk regression. Do you have a uw-imap server I can test on ?
Comment 15•22 years ago
|
||
Are you saying it worked on the trunk, but not in Mozilla 1.0? I didn't realize it had been fixed on the trunk, but not in 1.0 - can you point me at a trunk checkin that made it work on the trunk? I was not aware of that, sorry.
Comment 16•22 years ago
|
||
Instead of 1.0, I should have said "1.1 build, or any trunk build before my checkin that introduced the regression".
Reporter | ||
Comment 17•22 years ago
|
||
I am unable to get such a server for testing. I had problems in setting up accts. Anyway, if you think it was not working before, then it is ok.
Comment 18•22 years ago
|
||
sorry, forgot - valjean.mcom.com is a UW server - Seth maintains it.
Updated•20 years ago
|
Comment 20•20 years ago
|
||
Possibly related: bug 59688.
Comment 21•17 years ago
|
||
sorry for the spam. making bugzilla reflect reality as I'm not working on these bugs. filter on FOOBARCHEESE to remove these in bulk.
Assignee | ||
Updated•16 years ago
|
Comment 23•2 years ago
|
||
Seems like we should be able to fix this.
Severity S1 to match bug 427007
Updated•2 years ago
|
Updated•4 months ago
|
Comment 25•4 months ago
|
||
Just about everybody supports UIDPLUS now, even UW server (re: https://imapwiki.org/Specs) so this bug as originally written is probably mostly N/A now.
However, I do see a problem when undo'ing a move from Local Folder to IMAP when the local folders are setup to use maildir. When I undo, the message seems to come right back but it won't open. It's says the file can't be found instead of showing the message content and references the expected specific maildir file. Seems to work fine when Local Folders are setup as mbox (the default) and also if I manually move the message from imap back to maildir Local Folders, that works too (and how I would normally do this since I don't rely on "undo").
I also tried undo-ing messages deleted from imap inbox to the imap trash (on same imap server). This works OK for a while but eventually the connection to Trash folder times out and the undo doesn't work again until I do something to re-establish the connection (which sometimes triggers the move back to Inbox) or if I restart TB which also seems to trigger the move.
Not sure how many users rely on "undo/ctrl-z" functionality like the user in the bug marked as a dup of this (bug 1752569, which is not really a duplicate!). I definitely don't so I don't notice these undo problems unless I'm asked or decide to test them.
Description
•