Bugzilla

This from looking at `nsLocalMailFolder::CopyMessage()`, but I think applies to other folder types too:

`nsIMsgCopyService.copyMessages()` takes a `nsIMsgCopyServiceListener` param, and passes it on to the folder's `CopyMessages()` fn. This is used for updating progress (via `OnProgress()`), and for some message key magic during file copies (`SetMessageKey()`, and possibly some Message ID magic).

But the nsIMsgCopyServiceListener is for outside observers of the copyservice, _not_ for internal synchronisation. The listener passed in to the folder is _not_ used by the folder to tell the copyservice that the copy has completed, even though that's what you'd expect from a casual reading.

Instead, when the folder has completed it's copy operation, it calls `nsIMsgCopyService.NotifyCompletion()` to tell the copyservice it's done.
The copyservice is responsible for sending out the OnStartCopy()/OnStopCopy() notifications to any copy listeners.

Also, it's really important to pass in an `nsISupports` to NotifyCompletion() rather than `nsIMsgFolder`, as the copyservice might not be able to match up with the original copy request (see bug 1728495 for rectifying this).

This is all quite confusing.

Recommendation: The folder-specific `CopyMessages()` functions  should never be exposed to `nsIMsgCopyServiceListener`.

Simple(ish) fix:
- Dont pass the `nsIMsgCopyServiceListener` into the folder `CopyMessages()` fns.
- Instead add a `nsIMsgCopyService.NotifyProgress()` for the folders to handle progress updates.
- Figure out exactly what `nsIMsgCopyServiceListener.SetMessageKey()` and `nsIMsgCopyServiceListener.GetMessageID()` are for, and replace them with another mechanism.

Better fix:
use a more specific async update mechanism (a custom listener maybe), shared between copyservice and folders. It needs to handle passing back:
- progress updates ("x out of y completed")
- Copy completed (or failed)
- whatever is required to handle what SetMessageKey()/GetMessageID() currently provide (but hopefully we can do it more cleanly elsewhere).

bonus points: the copyservice could use a local listener object, and use closures to handle the callbacks. This'd keep all the code in one place and much more readable.
There are soooo many places where classes derive from listener classes to handle async things which should be hidden internally. I want to avoid adding any more of this.
(eg folders deriving from `nsIUrlListener`)

This from looking at `nsLocalMailFolder::CopyMessages()`, but I think applies to the other folder CopyMessages() too:

`nsIMsgCopyService.copyMessages()` takes a `nsIMsgCopyServiceListener` param, and passes it on to the folder's `CopyMessages()` fn. This is used for updating progress (via `OnProgress()`), and for some message key magic during file copies (`SetMessageKey()`, and possibly some Message ID magic).

But the nsIMsgCopyServiceListener is for outside observers of the copyservice, _not_ for internal synchronisation. The listener passed in to the folder is _not_ used by the folder to tell the copyservice that the copy has completed, even though that's what you'd expect from a casual reading.

Instead, when the folder has completed it's copy operation, it calls `nsIMsgCopyService.NotifyCompletion()` to tell the copyservice it's done.
The copyservice is responsible for sending out the OnStartCopy()/OnStopCopy() notifications to any copy listeners.

Also, it's really important to pass in an `nsISupports` to NotifyCompletion() rather than `nsIMsgFolder`, as the copyservice might not be able to match up with the original copy request (see bug 1728495 for rectifying this).

This is all quite confusing.

Recommendation: The folder-specific `CopyMessages()` functions  should never be exposed to `nsIMsgCopyServiceListener`.

Simple(ish) fix:
- Dont pass the `nsIMsgCopyServiceListener` into the folder `CopyMessages()` fns.
- Instead add a `nsIMsgCopyService.NotifyProgress()` for the folders to handle progress updates.
- Figure out exactly what `nsIMsgCopyServiceListener.SetMessageKey()` and `nsIMsgCopyServiceListener.GetMessageID()` are for, and replace them with another mechanism.

Better fix:
use a more specific async update mechanism (a custom listener maybe), shared between copyservice and folders. It needs to handle passing back:
- progress updates ("x out of y completed")
- Copy completed (or failed)
- whatever is required to handle what SetMessageKey()/GetMessageID() currently provide (but hopefully we can do it more cleanly elsewhere).

bonus points: the copyservice could use a local listener object, and use closures to handle the callbacks. This'd keep all the code in one place and much more readable.
There are soooo many places where classes derive from listener classes to handle async things which should be hidden internally. I want to avoid adding any more of this.
(eg folders deriving from `nsIUrlListener`)