439225 - Revamp nsIMsgFolderNotificationService

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect, P2)

Description

[Rain [:rain1] (inactive)]

Attachment: patch

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 1

[Rain [:rain1] (inactive)]

Attachment: small comment fixes

Comment 2

[Rain [:rain1] (inactive)]

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.

Comment 3

[Rain [:rain1] (inactive)]

err, make that kind, not king.

Comment 4

[David :Bienvenu]

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.

Comment 5

[Rain [:rain1] (inactive)]

(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 ;) )

Comment 6

[David :Bienvenu]

(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?

Comment 7

[Rain [:rain1] (inactive)]

(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 8

[Mark Banner (:standard8)]

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.

Comment 9

[Rain [:rain1] (inactive)]

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

Comment 10

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 11

[Andrew Sutherland [:asuth] (he/him)]

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

Comment 12

[Rain [:rain1] (inactive)]

(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.

Comment 13

[Rain [:rain1] (inactive)]

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

Comment 14

[David :Bienvenu]

Comment on attachment 325097 [details] [diff] [review]
small comment fixes

r/sr=me, modulo turning off the test for now...

Comment 15

[Rain [:rain1] (inactive)]

Attachment: new patch, bitrot fixed in tests

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

Comment 16

[Mark Banner (:standard8)]

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.

Comment 17

[Rain [:rain1] (inactive)]

Attachment: [checked in] final patch to checkin

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.

Comment 18

[Mark Banner (:standard8)]

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 19

[Rain [:rain1] (inactive)]

Attachment: [checked in] Add notification bits

Comment 20

[David :Bienvenu]

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 21

[Rain [:rain1] (inactive)]

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.

Comment 22

[Rain [:rain1] (inactive)]

Attachment: test for single notifications

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

Comment 23

[Mark Banner (:standard8)]

Checked in, changeset id: 153:cfbcd52d91cc

Comment 24

[Mark Banner (:standard8)]

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.

Comment 25

[Rain [:rain1] (inactive)]

Attachment: Added support for removing listeners in the middle of notifications (+ test for single notifications)

Also contains the test for single notifications above.

Comment 26

[Rain [:rain1] (inactive)]

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.

Comment 27

[Rain [:rain1] (inactive)]

Attachment: [checked in] IMAP msgsMoveCopyCompleted should use isReallyMove instead of aMove

oops, this slipped through in the last checkin.

Comment 28

[Mark Banner (:standard8)]

Comment on attachment 334887 [details] [diff] [review]
[checked in] IMAP msgsMoveCopyCompleted should use isReallyMove instead of aMove

Checked in changeset id: 162:9b0bdd17dbd8

Comment 29

[David :Bienvenu]

anything more to do to mark this fixed?

Comment 30

[Rain [:rain1] (inactive)]

(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.

Comment 31

[Andrew Sutherland [:asuth] (he/him)]

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

Comment 32

[Andrew]

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?