226270 - Rationalize folder listeners

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[[e-mail]]

Currently we generate the following events:
NotifyBoolPropertyChanged nsMsgFolder, DefaultServer
[NotifyPropertyFlagChanged nsIMsgDBHdr, Status - not used internally]
[NotifyPropertyFlagChanged nsIMsgDBHdr, Flagged - not used internally]
NotifyUnicharPropertyChanged nsMsgFolder, Name
NotifyIntPropertyChanged nsMsgFolder, UnreadMessages
NotifyIntPropertyChanged nsMsgFolder, TotalMessages
NotifyBoolPropertyChanged nsMsgFolder, Synchronize
NotifyBoolPropertyChanged nsMsgFolder, Open
NotifyIntPropertyChanged nsMsgFolder, FolderSize
NotifyPropertyFlagChanged nsMsgFolder, BiffState
NotifyPropertyChanged nsMsgFolder, NewMessages
NotifyItemAdded nsMsgFolder, nsIMsgFolder
NotifyItemRemoved nsMsgFolder, nsIMsgFolder
[NotifyItemAdded nsMsgFolder, nsIMsgDBHdr - not used internally]
NotifyItemRemoved nsMsgFolder, nsIMsgDBHdr

(1) NotifyItemRemoved is always called with a first parameter of
(nsISupports*)this, this parameter should be implied.

(2) NotifyItemAdded is almost always called with a first parameter of
(nsISupports*)this, there is one caller that does not do this and I believe it
should, the fix is trivial. The parameter could then be implied.

(3) In the case of the item to be added or deleted, this may be declared as an
nsISupports* and the item passed directly in; any interested listeners would
have to do_QueryInterface anyway.

(4) In the case of NotifyPropertyFlagChanged, this may be declared as in (3)
above. Additionally a second method with an implied parameter may be used.

(5) In the case of OnBoolPropertyChanged, OnUnicharPropertyChanged,
OnIntPropertyChanged, OnItemAdded and OnItemRemoved the first parameter is
always used as an nsIRDFResource, and occasionally as an nsIMsgFolder, not the
nsISupports that they are declared as.

(6) Search sessions forward folder events to their listeners. Any listener
attached to a search session could simply be directly registered on the object
that the search session was registered on. Note that searchBar.js registers the
folder listener on an unregistered search session!

(7) The windows integration listener does not need to listen to default server
and unread count changes if it's not storing them.

(8) Folders usually don't have their own listeners, so that the listener array
could be created as required, instead of being automatically constructed.

(9) Biff listeners have to take care to add themselves to root folders to avoid
getting flooded by the property flag changes. It would be possible to fire biff
as a bool property change, thus making them easier to filter. This would also
allow them to filter out other root folder notifications (currently they receive
all root folder notifications).

David, can you let me know which points (1)-(9) you agree with, and whether you
think you could review them all at once? Thanks.

Comment 1

[[e-mail]]

Apparently dbradley doesn't think that 3) will work for JS listeners :-(

Comment 2

[[e-mail]]

Heh, he's since changed his mind :-)

Comment 3

[David :Bienvenu]

NotifyItemRemoved is really NotifyItemDeleted, right?. Other than that, I agree
with 1.

Re 2, are we talking about nsMsgLocalMailFolder::Rename? Would you call
parent->NotifyItemAdded()?

3+4) isn't the param for the item to be added/deleted already declared as an
nsISupports? Or are you talking about the callers?

5) If true, what do you propose to do about it?

6,7, and 9, I need to look at.

(8) We're talking about nsMsgDBFolder::nsVoidArray mListeners, right? You could
do that, but then you'd have to add checks for null mListeners in about 10
places. Still, you'd save 12 bytes per folder, so it's probably worthwhile.


10) See bug 132450 for more cleanup that should be done at the same time,
viewString param is not used, I don't think.

Comment 4

[[e-mail]]

(In reply to comment #3)
>NotifyItemRemoved is really NotifyItemDeleted, right?. Other than that, I
>agree with 1.
Whoops, yes, it calls OnItemRemoved, I had confused the two, sorry :-(

>Re 2, are we talking about nsMsgLocalMailFolder::Rename? Would you call
>parent->NotifyItemAdded()?
Exactly.

>3+4) isn't the param for the item to be added/deleted already declared as an
>nsISupports? Or are you talking about the callers?
The callers use QueryInterface(NS_GET_IID(nsISupports),
getter_AddRefs(folderSupports)); to get an nsISupports to pass to the method,
instead of something along the lines of (nsISupports *)(nsRDFResource *)this.

>5) If true, what do you propose to do about it?
By changing the idl to expect an nsIRDFResource the listeners who need a
resource don't need to QI it back.

Comment 5

[[e-mail]]

Since NotifyItemDeleted calls OnItemRemoved is it safe to rename one?

Comment 6

[[e-mail]]

Attachment: First part

Comment 7

[[e-mail]]

Comment on attachment 145764 [details] [diff] [review]
First part

Is there anyone else that you would like me to get to cast their eyes over
this?

Comment 8

[David :Bienvenu]

Comment on attachment 145764 [details] [diff] [review]
First part

this looks good to me. I think while we're touching all this code, we might as
well remove all the viewString's, since they're not used, right? That would
involve changing the js listeners as well...or do you want to leave that to a
subsequent patch?

Comment 9

[[e-mail]]

It looks as if that's going in a separate patch ;-)

Comment 10

[David :Bienvenu]

OK, I can do that one then, it's no problem. I have a bug filed on it already.

Comment 11

[David :Bienvenu]

I'm seeing a bug where the new flag on folder in the folder pane doesn't get
cleared when I read or delete the last new message in the folder. It doesn't
happen to every folder. It's almost got to be this change that caused it - I'll
investigate.

Comment 12

[David :Bienvenu]

well, I don't think it was this change. I don't know why I just started seeing
it today...

Comment 13

[David :Bienvenu]

sorry to falsely blame this patch, Neil. The problem was mine, and it's been fixed.

Comment 14

[[e-mail]]

Attachment: Second part

This just changes the type of the first parameter of some of the notifications.

I thought that since this changes enough files already that I should leave the
data source and JS cleanup for a subsequent patch.

Comment 15

[[e-mail]]

Attachment: Second part, including data source and JS cleanup

Comment 16

[[e-mail]]

Comment on attachment 146985 [details] [diff] [review]
Second part, including data source and JS cleanup

...which makes it 64% bigger, so you might want to review attachment 146925 [details] [diff] [review] and
I'll rediff this patch.

Comment 17

[[e-mail]]

Attachment: Third part

Comment 18

[[e-mail]]

Comment on attachment 147152 [details] [diff] [review]
Third part

Does anyone know why search sessions are folder listeners? There's only one
consumer, and it could just as easily be added to the mail session as shown...

Comment 19

[David :Bienvenu]

Comment on attachment 147152 [details] [diff] [review]
Third part

yes, I think we can cut out the middle man and just use the mail session
directly...

Comment 20

[Scott MacGregor]

Comment on attachment 147152 [details] [diff] [review]
Third part

if David says so :)

Comment 21

[R.K.Aa.]

Neil: The checkins at 2004-04-28 05:48 caused bug 242098

Comment 22

[[e-mail]]

Attachment: Fourth part

This moves the biff notifications from flag changes to int changes.
This means that only message header notifications will have flag changes.

Comment 23

[[e-mail]]

Attachment: Fifth part

So all that remains is to change the flag notifications to use message headers.


Nobody listens to them anyway...

I don't think I can do anything about the last two uses of nsISupports.

Comment 24

[David :Bienvenu]

Comment on attachment 169442 [details] [diff] [review]
Fifth part

any js extensions out there that implement OnItemPropertyFlagChanged and qi the
item param to an nsIMsgDBHdr will be fine with this change, so r/sr=bienvenu

Comment 25

[[e-mail]]

Although not exactly what I originally envisaged this is just about done now.

Comment 26

[Frank Wein [:mcsmurf]]

Might it be that Bug 276372 was caused by this bug (by either the checkin from
22th or 23th of December 2004)? I'm not quite sure, regression timeframe fits
and this bug was the only checkin to MailNews in this timeframe, but the only
part that's broken is the new mail sound. The new mail popup still works. Or are
these two events rather independent from each other in code?

Comment 27

[[e-mail]]

It shouldn't do, I made corresponding changes to nStatusBarBiffManager (which
plays the sound) and nsMessengerWinIntegration (which shows the icon/alert).