189023 - Copy folder from IMAP or POP to Local folder only works once.

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Frank Harper]

User-Agent:       Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:1.3a) Gecko/20021212
Build Identifier: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:1.3a) Gecko/20021212

I'm copying folders from an IMAP server to Local Folders, using drag 'n drop.
This works for one folder, but when I do the next one nothing happens. If I quit
and restart Mozilla I can then copy another folder.

Reproducible: Always

Steps to Reproduce:
1. Drag one folder from IMAP server to Local Folder
2. Drag another folder from IMAP server to Local Folder
3. Check that contents of second folder have been copied.

Actual Results:  
No visible results.

Expected Results:  
Copied messages from IMAP folder to Local Folder.

Comment 1

[grylchan]

confirmed with current trunk
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3b) Gecko/20030113


worked in 7.01.

nominating

couldn't find any dupes. similar bugs:
bug 175819,bug 142448, bug 180229

Comment 2

[Samir Gehani]

Mail triage team: nsbeta1+/adt2

Comment 3

[esther]

Update, current behavior with build 20030326 on winxp and linux (haven't tested
Mac yet). 
1. Launch app and open Mail with IMAP account
2. DND an IMAP folder to top level of Local folder
Result: Folder and it's messages are copied to Local folders
3. DND another IMAP folder to top level of Local folder
Result: Folder does not show up in Local folders.
4. Copy or Move a message from any IMAP folder to any Local folder
Result: Message is moved AND the 2nd Folder that was dragged to Local Folders
now shows up with all it's messages.  
Note: Exiting and relaunching after step 3 does not display the 2nd folder only
a copy or move to any Local Folder will cause the 2nd folde to be displayed. 
This is regression and pretty bad. 

Comment 4

[esther]

I tested bug 175819 which I believe is using POP accounts and Local folders and
found the same problem.  Marking that a dup of this bug and changing summary to
reflect it happens with IMAP and POP.

Comment 5

[esther]

*** Bug 175819 has been marked as a duplicate of this bug. ***

Comment 6

[esther]

*** Bug 142448 has been marked as a duplicate of this bug. ***

Comment 7

[Cavin Song]

Taking.

Comment 8

[Cavin Song]

Attachment: Proposed patch, v1

The problem is that FindRequest() can't find the right request when copy
finishes. FindRequest() is called with 'dstFolder' points to the newly created
folder (for example, "Local Folders\Test1") and it's never the same as
copyRequest->m_dstFolder (for example, "Local Folders") which is the parent
folder of the copied folder. The check works fine for copy msgs operations
because the destination folders never change but it does not work for copy
folder operations since a new folder is created in this case.

Comment 9

[(not reading, please use [e-mail] instead)]

1)

I'd rather we didn't use filespec, which is obsolete, and I don't even think we
need to.

I think you can just use the folder name, right?

  nsXPIDLString folderName;
  rv = m_dstFolder->GetName(getter_Copies(folderName));

if so, instead of nsCString, use nsString for m_dstFolderName

also, when do you clear m_dstFolderLeafName?  shouldn't you be doing that when
you clear m_dstFolder?

2)

also, what about copying folders between imap servers?

Comment 10

[(not reading, please use [e-mail] instead)]

Comment on attachment 122496 [details] [diff] [review]
Proposed patch, v1

see my previous comments / questions.

Comment 11

[Cavin Song]

> I think you can just use the folder name, right?
> if so, instead of nsCString, use nsString for m_dstFolderName
>
Yes, good suggestion.

> also, when do you clear m_dstFolderLeafName?  shouldn't you be doing that when
> you clear m_dstFolder?
>
m_dstFolderLeafName is a member of CopyRequest and CopyRequest is created per
copy operation and destroyed when copy finishes so we don't have to clear
m_dstFolderLeafName.

> 2) also, what about copying folders between imap servers?
>
Copying folders between imap servers is not allowed. You can move an imap folder
underneath another within the same server but that does not use copy service code.

Comment 12

[Cavin Song]

Attachment: Proposed patch, v2

Incorporated comments.

Comment 13

[(not reading, please use [e-mail] instead)]

Comment on attachment 122598 [details] [diff] [review]
Proposed patch, v2

r/sr/a=sspitzer for 1.4 final.

one nit, no need for a new patch:

+    nsCOMPtr<nsIMsgFolder> srcFolder;
+    srcFolder = do_QueryInterface(aSupport, &rv);

just do

+    nsCOMPtr<nsIMsgFolder> srcFolder = do_QueryInterface(aSupport, &rv);

thanks for fixing this cavin.

Comment 14

[Cavin Song]

Fix checked in.

Comment 15

[Mike Cowperthwaite]

*** Bug 204927 has been marked as a duplicate of this bug. ***

Comment 16

[esther]

using trunk build 20030516 on winxp, macosx and linux this is fixed for IMAP,
but when I tested moving POP folders to Local it didn't work after the 1st move.
 From that point on the IMAP didn't work.  I'm reopening this to Cavin can fix
the POP case too. 

Comment 17

[Cavin Song]

The problem is that if the source folder is empty (ie, no msgs) then
notification is never received. In
nsMsgLocalMailFolder::CopyFolderAcrossServer() we should call OnCopyCompleted()
to clear the request. Normally, when a copy completion notification is received
it's from nsMailboxProtocol::OnStopRequest():

nsMsgCopyService::NotifyCompletion()
nsMsgLocalMailFolder::OnCopyCompleted()
nsMsgLocalMailFolder::EndCopy()
nsCopyMessageStreamListener::EndCopy()
nsCopyMessageStreamListener::OnStopRequest()
nsMsgProtocol::OnStopRequest()
nsMailboxProtocol::OnStopRequest()

Another problem I found is that if you try to copy a folder which already exists
in the destination folder we are not cleaning up the request either.

A patch is coming for the above problems. 

Comment 18

[Cavin Song]

Attachment: Proposed patch for empty folder copy problem, v1

Comment 19

[(not reading, please use [e-mail] instead)]

taking.  I'll test and review (and drive in) cavin's fix.

Comment 20

[David :Bienvenu]

This hasn't been checked in, has it? I have some dups of this, I think.

Comment 21

[David :Bienvenu]

*** Bug 180229 has been marked as a duplicate of this bug. ***

Comment 22

[David :Bienvenu]

fix checked in.