This bug is to change the nsIMsgFolderNotificationService to make it cleaner and easier to use, for both callers and listeners. - Message and folder notifications have been split up - The folderRenamed notification has been enabled - nsParseMailbox::NotifyGlobalListeners, which was an itemAdded notification, has been removed and all its callers now directly call NotfyMsgAdded - doxygen comments and whitespace fixes The patch is very straightforward, a simple global search and replace. It doesn't fix any bugs, though. (the patch is over the one for bug 436880) There are a couple of bugs in the folderRenamed code, so I've commented out those tests until that's fixed.

Comment on attachment 325097 [details] [diff] [review] small comment fixes Mark, would you be so king as to review the changes in this test after reviewing the one in bug 436880? Thanks, and sorry for the trouble.

err, make that kind, not king.

Comment on attachment 325097 [details] [diff] [review] small comment fixes Thx, Sid - some nits: Notified after a folder has been deleted and its disk space removed. "disk space removed" isn't quite right. Maybe "corresponding file(s) deleted from disk" or "backing store(s) deleted". I prefer "for (" to "for(" I wonder if we want a folderAdded notification. In particular, we could discover a new imap folder. The user might create a new local folder, but until it has messages in it, indexers aren't going to care much. I also wonder if we want to do something similar to nsIFolderListener, which is allow the listener to register which events they're interested in, so we can avoid the overhead of going to c++ to js for cases where extensions don't care about particular events. Maybe that can be a spin-off bug.

(In reply to comment #4) > (From update of attachment 325097 [details] [diff] [review]) > Thx, Sid - some nits: > > Notified after a folder has been deleted and its disk space removed. > > > "disk space removed" isn't quite right. Maybe "corresponding file(s) deleted > from disk" or "backing store(s) deleted". Oh, I apologize for the slip-up, I meant to write "disk storage removed", but I guess corresponding file(s) deleted from disk" sounds better. What do you think about changing the logic to be before the files are deleted? > > I wonder if we want a folderAdded notification. In particular, we could > discover a new imap folder. The user might create a new local folder, but until > it has messages in it, indexers aren't going to care much. > OK, but I'd like to work on this on a lower priority, as if we discover a new IMAP folder, and there are messages in it, we get a msgAdded notification for those messages. > I also wonder if we want to do something similar to nsIFolderListener, which is > allow the listener to register which events they're interested in, so we can > avoid the overhead of going to c++ to js for cases where extensions don't care > about particular events. Maybe that can be a spin-off bug. > Part 2 of this bug will be fine. Sounds very reasonable, and AFAICT all the other notification services work this way. (It'll clean up a bit of the test code too ;) )

(In reply to comment #5) > > What do you think about changing the logic to be before the files are deleted? > I think that's OK, as long as the documentation is accurate. Does that work better for the vista search extension?

(In reply to comment #6) > > I think that's OK, as long as the documentation is accurate. Does that work > better for the vista search extension? > It doesn't matter for the extension, but it'd potentially be more useful to third parties that way.

Comment on attachment 325097 [details] [diff] [review] small comment fixes The changes here look reasonable, but I'd like to see an updated version once you've updated the patches for bug 436880 and this bug.

The new patch is ready, but I'm holding off on uploading it till the 436880 problem is fixed.

It sounds like the new patch should have folderAdded, which is good. The new javadoc/doxygen documentation is delightful and a great thing. Could the new patch also elaborate on the relationship to nsIFolderListener on the interface doc? It sounds like if you care about message addition/removal, nsIMsgFolderListener (with folderAdded) can do everything nsIFolderListener can do, but if you want to know about property changes, you need to use nsIFolderListener. Right now, I'd be suspicious as to whether I would need to use both or what. For 'part 2' with the desired notification bits, I would suggest planning to have separate bits for each of the types of objects the nsISupports aItem could conceivably hold. I guess that would be folders and messages.

Er, I meant to say message or folder addition/removal in my previous message.

(In reply to comment #10) > It sounds like the new patch should have folderAdded, which is good. > Actually, it doesn't. :) It only has David's comments addressed + bitrot fixes from bug 436880. > The new javadoc/doxygen documentation is delightful and a great thing. Could > the new patch also elaborate on the relationship to nsIFolderListener on the > interface doc? It sounds like if you care about message addition/removal, > nsIMsgFolderListener (with folderAdded) can do everything nsIFolderListener can > do, but if you want to know about property changes, you need to use > nsIFolderListener. Right now, I'd be suspicious as to whether I would need to > use both or what. I really don't know about nsIFolderListener, though if someone elaborates I'd be happy to fill the code in. itemEvent could possibly be used for notifying on certain specific events, eg "tagged". The changes shouldn't be too much trouble, but that'll have to hold off for a while. > > For 'part 2' with the desired notification bits, I would suggest planning to > have separate bits for each of the types of objects the nsISupports aItem could > conceivably hold. I guess that would be folders and messages. > The interface now has separate message and folder notifications, so it'll be done that way.

s/I'd be happy to fill the code in/I'd be happy to fill the documentation in/

Comment on attachment 325097 [details] [diff] [review] small comment fixes r/sr=me, modulo turning off the test for now...

The bitrot caused after the tests in the patch to bug 436880 were updated has been fixed.

Comment on attachment 326458 [details] [diff] [review] new patch, bitrot fixed in tests This has a slight bit of bitrot (nsLocalMailFolder.cpp) but I worked around that for testing purposes. r=me for the test changes, though I noticed a few whitespace issues in the idl files, so could you fix these as well please: >+ void msgsMoveCopyCompleted(in boolean aMove, >+ in nsIArray aSrcMsgs, > in nsIMsgFolder aDestFolder); ... >+ void folderMoveCopyCompleted(in boolean aMove, >+ in nsIMsgFolder aSrcFolder, >+ in nsIMsgFolder aDestFolder); ... >+ void notifyMsgsMoveCopyCompleted(in boolean aMove, >+ in nsIArray aSrcMsgs, > in nsIMsgFolder aDestFolder); ... >+ void notifyFolderMoveCopyCompleted(in boolean aMove, >+ in nsIMsgFolder aSrcFolder, >+ in nsIMsgFolder aDestFolder); These all have wrong indentations, the "in"s should be aligned to the one at the top of each function.

Fixed whitespace. I'm not sure why you were having bitrot troubles applying the patch -- it applied cleanly here on trunk. Maybe hg is more aggressive. I've regenerated the patch though -- hope that fixes it.

Checking in mailnews/base/public/Makefile.in; /cvsroot/mozilla/mailnews/base/public/Makefile.in,v <-- Makefile.in new revision: 1.95; previous revision: 1.94 done Checking in mailnews/base/public/nsIMsgFolderListener.idl; /cvsroot/mozilla/mailnews/base/public/nsIMsgFolderListener.idl,v <-- nsIMsgFolderListener.idl new revision: 1.4; previous revision: 1.3 done Checking in mailnews/base/public/nsIMsgFolderNotificationService.idl; /cvsroot/mozilla/mailnews/base/public/nsIMsgFolderNotificationService.idl,v <-- nsIMsgFolderNotificationService.idl new revision: 1.5; previous revision: 1.4 done Checking in mailnews/base/src/nsMsgCopyService.cpp; /cvsroot/mozilla/mailnews/base/src/nsMsgCopyService.cpp,v <-- nsMsgCopyService.cpp new revision: 1.63; previous revision: 1.62 done Checking in mailnews/base/src/nsMsgFolderNotificationService.cpp; /cvsroot/mozilla/mailnews/base/src/nsMsgFolderNotificationService.cpp,v <-- nsMsgFolderNotificationService.cpp new revision: 1.6; previous revision: 1.5 done Checking in mailnews/base/test/resources/msgFolderListenerSetup.js; /cvsroot/mozilla/mailnews/base/test/resources/msgFolderListenerSetup.js,v <-- msgFolderListenerSetup.js new revision: 1.2; previous revision: 1.1 done Checking in mailnews/base/test/unit/test_nsIMsgFolderListener.js; /cvsroot/mozilla/mailnews/base/test/unit/test_nsIMsgFolderListener.js,v <-- test_nsIMsgFolderListener.js new revision: 1.2; previous revision: 1.1 done Checking in mailnews/base/test/unit/test_nsIMsgFolderListenerLocal.js; /cvsroot/mozilla/mailnews/base/test/unit/test_nsIMsgFolderListenerLocal.js,v <-- test_nsIMsgFolderListenerLocal.js new revision: 1.2; previous revision: 1.1 done Checking in mailnews/base/util/nsMsgDBFolder.cpp; /cvsroot/mozilla/mailnews/base/util/nsMsgDBFolder.cpp,v <-- nsMsgDBFolder.cpp new revision: 1.348; previous revision: 1.347 done Checking in mailnews/db/msgdb/src/nsMsgDatabase.cpp; /cvsroot/mozilla/mailnews/db/msgdb/src/nsMsgDatabase.cpp,v <-- nsMsgDatabase.cpp new revision: 1.402; previous revision: 1.401 done Checking in mailnews/imap/src/nsImapMailFolder.cpp; /cvsroot/mozilla/mailnews/imap/src/nsImapMailFolder.cpp,v <-- nsImapMailFolder.cpp new revision: 1.821; previous revision: 1.820 done Checking in mailnews/local/src/nsLocalMailFolder.cpp; /cvsroot/mozilla/mailnews/local/src/nsLocalMailFolder.cpp,v <-- nsLocalMailFolder.cpp new revision: 1.592; previous revision: 1.591 done Checking in mailnews/local/src/nsParseMailbox.cpp; /cvsroot/mozilla/mailnews/local/src/nsParseMailbox.cpp,v <-- nsParseMailbox.cpp new revision: 1.305; previous revision: 1.304 done Checking in mailnews/local/src/nsParseMailbox.h; /cvsroot/mozilla/mailnews/local/src/nsParseMailbox.h,v <-- nsParseMailbox.h new revision: 1.96; previous revision: 1.95 done Checking in mailnews/local/src/nsPop3Sink.cpp; /cvsroot/mozilla/mailnews/local/src/nsPop3Sink.cpp,v <-- nsPop3Sink.cpp new revision: 1.149; previous revision: 1.148 done

Comment on attachment 334107 [details] [diff] [review] [checked in] Add notification bits cool,thx - you might also want to add a test that the flags actually work, e.g., if you sign up for msg deleted events only, you don't get other notifications.

Comment on attachment 334107 [details] [diff] [review] [checked in] Add notification bits I'll post a unit test with single notification listeners in a bit.

Assuming that bitwise if operations are used to check which notifications should be sent, this test looks like it's comprehensive.

Checked in, changeset id: 153:cfbcd52d91cc

Bustage fix follow up checked in, changeset id: 156:51e4bceebe77 This added a copy constructor (to ensure refcounts are correct) and moved the mListeners.RemoveElement call out of the NS_ASSERTION as that wasn't being applied in release mode.

Also contains the test for single notifications above.

Comment on attachment 334885 [details] [diff] [review] Added support for removing listeners in the middle of notifications (+ test for single notifications) Oh, this also has a macro, as changing repeating code was getting harder than it should be.

oops, this slipped through in the last checkin.

Comment on attachment 334887 [details] [diff] [review] [checked in] IMAP msgsMoveCopyCompleted should use isReallyMove instead of aMove Checked in changeset id: 162:9b0bdd17dbd8

anything more to do to mark this fixed?

(In reply to comment #29) > anything more to do to mark this fixed? > I wanted to add a folderAdded notification, but that can be spun off into a new bug I think.

I have logged bug 453763 to track the folderAdded notification desire. Marking fixed.

Sorry to bother everyone, but I was wondering if it is at all possible to get the equivalent of the itemMoveCopyCompleted and itemDeleted working for an extension in Thunderbird 2 with IMAP since I see this has only been fixed in Thunderbird 3?