There are several issues with maildir message move copying. Undo doesn't work, copying doesn't send the right done notification, and pop3 filters don't work with quarantining. I'm trying to work through the issues. This fixes a couple of the basic ones. I had to add the do_QueryInterface to the srcSupports line because the code that sets up the srcSupports also does a QI (in nsMsgCopyService::CopyMessages), and if I don't, they don't match.

Comment on attachment 608714 [details] [diff] [review] proposed fix > boolean copyMessages(in boolean isMove, > in nsIArray aHdrArray, > in nsIMsgFolder aDstFolder, >- in nsIMsgCopyServiceListener aListener); >+ in nsIMsgCopyServiceListener aListener, >+ out nsITransaction aUndoAction); I don't see any callers of this method in this patch? > NS_IMETHODIMP > nsMsgBrkMBoxStore::CopyMessages(bool isMove, nsIArray *aHdrArray, > nsIMsgFolder *aDstFolder, > nsIMsgCopyServiceListener *aListener, >+ nsITransaction **aUndoAction, > bool *aCopyDone) > { > NS_ENSURE_ARG_POINTER(aHdrArray); > NS_ENSURE_ARG_POINTER(aDstFolder); > NS_ENSURE_ARG_POINTER(aCopyDone); > *aCopyDone = false; > return NS_OK; Nit: Should zero out *aUndoAction if not throwing an exception. (Given this code and the maildir code, perhaps the failure to copy should be indicated by an exception, but I can't tell because I can't see a caller.) >- return NS_OK; >+ return CallQueryInterface(msgTxn, aUndoAction); msgTxn->forget(aUndoAction); would save an addref/release.

msgTxn is a refptr, so .forget doesn't work (should have said that I tried that when I attached the original patch). copying is "optional" - if the store doesn't do the copy, the backend code does it for the store, so that's not a failure per se.

(In reply to David Bienvenu from comment #2) > msgTxn is a refptr, so .forget doesn't work (should have said that I tried > that when I attached the original patch). Strange, my nsAutoPtr.h shows a template <typename I> void forget(I** rhs)

I must have done ->forget... or dreamt it all.

Comment on attachment 608924 [details] [diff] [review] ugh, you're right. >+ nsCOMPtr<nsITransaction> undoTxn; >+ rv = msgStore->CopyMessages(isMove, messages, this, listener, >+ getter_AddRefs(undoTxn), &storeDidCopy); > if (storeDidCopy) >+ { >+ if (msgWindow) >+ { >+ nsCOMPtr<nsITransactionManager> txnMgr; >+ msgWindow->GetTransactionManager(getter_AddRefs(txnMgr)); >+ if (txnMgr) >+ txnMgr->DoTransaction(undoTxn); >+ } > return rv; >+ } So, there must be a transaction if and only if the store did the copy?

(In reply to neil@parkwaycc.co.uk from comment #5) > Comment on attachment 608924 [details] [diff] [review] > ugh, you're right. > > >+ nsCOMPtr<nsITransaction> undoTxn; > >+ rv = msgStore->CopyMessages(isMove, messages, this, listener, > >+ getter_AddRefs(undoTxn), &storeDidCopy); > > if (storeDidCopy) > >+ { > >+ if (msgWindow) > >+ { > >+ nsCOMPtr<nsITransactionManager> txnMgr; > >+ msgWindow->GetTransactionManager(getter_AddRefs(txnMgr)); > >+ if (txnMgr) > >+ txnMgr->DoTransaction(undoTxn); > >+ } > > return rv; > >+ } > So, there must be a transaction if and only if the store did the copy? Yes, right.

(In reply to David Bienvenu from comment #6) > (In reply to comment #5) > > (From update of attachment 608924 [details] [diff] [review]) > > >+ nsCOMPtr<nsITransaction> undoTxn; > > >+ rv = msgStore->CopyMessages(isMove, messages, this, listener, > > >+ getter_AddRefs(undoTxn), &storeDidCopy); > > > if (storeDidCopy) > > >+ { > > >+ if (msgWindow) > > >+ { > > >+ nsCOMPtr<nsITransactionManager> txnMgr; > > >+ msgWindow->GetTransactionManager(getter_AddRefs(txnMgr)); > > >+ if (txnMgr) > > >+ txnMgr->DoTransaction(undoTxn); > > >+ } > > > return rv; > > >+ } > > So, there must be a transaction if and only if the store did the copy? > > Yes, right. (Sorry for the delay.) So, is there any point in returning both a transaction and a separate boolean that says "hey, check that I returned a transaction!"?

(In reply to neil@parkwaycc.co.uk from comment #7) > > (Sorry for the delay.) So, is there any point in returning both a > transaction and a separate boolean that says "hey, check that I returned a > transaction!"? Only if we wanted undo support to be optional, and I'd rather not do that. We could assert if there's no txn returned, though. Or we could figure out if there's a way to implement undo in a store-agnostic way, and then we could check for a null txn.

Comment on attachment 608924 [details] [diff] [review] ugh, you're right. Sorry, I didn't understand your previous answer, so I'll ask this instead: >+ nsCOMPtr<nsITransaction> undoTxn; >+ rv = msgStore->CopyMessages(isMove, messages, this, listener, >+ getter_AddRefs(undoTxn), &storeDidCopy); > if (storeDidCopy) Any reason not to use if (undoTxn) here?

(In reply to neil@parkwaycc.co.uk from comment #9) > Comment on attachment 608924 [details] [diff] [review] > ugh, you're right. > > Sorry, I didn't understand your previous answer, so I'll ask this instead: > >+ nsCOMPtr<nsITransaction> undoTxn; > >+ rv = msgStore->CopyMessages(isMove, messages, this, listener, > >+ getter_AddRefs(undoTxn), &storeDidCopy); > > if (storeDidCopy) > Any reason not to use if (undoTxn) here? No, you're right, we can do that - that was meant to be implicit in my suggestion of adding an assertion, since assertions are supposed to imply the handling of the case the assertion was warning about (though, sadly, that's not always the case). I'll attach a version that does that.

Comment on attachment 611328 [details] [diff] [review] fix with assertion for null undo txn >+ nsCOMPtr<nsITransaction> undoTxn; >+ rv = msgStore->CopyMessages(isMove, messages, this, listener, >+ getter_AddRefs(undoTxn), &storeDidCopy); > if (storeDidCopy) >+ { >+ NS_ASSERTION(undoTxn, "if store does copy, it needs to add undo action"); >+ if (msgWindow && undoTxn) Fair enough; as you think it would be confusing to assume the store did the copy based on the existence of an undo action then don't bother.

standard8, sr ping - I'd like to move maildir support forward, and this bug definitely blocks dogfooding of maildir.

the dogs are really hungry! :-)

fixed on trunk - http://hg.mozilla.org/comm-central/rev/26c2c8008156 the xpcshell tests revealed these issues, when I ran them with maildir turned on, so I'm going to mark this in testsuite+