Closed Bug 156558 Opened 22 years ago Closed 22 years ago

Creating a bridge between mail and aim

Categories

(MailNews Core :: Backend, defect)

Product:
MailNews Core
Component:
Backend
Platform:
x86
Windows 2000
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: naving, Assigned: naving)

Details

Attachments

(2 files)

mozilla/mailnews part of the patch
12.51 KB, patch
cavin
: review+
sspitzer
: superreview+
Details | Diff | Splinter Review
patch for comment #46
988 bytes, patch
Bienvenu
: superreview+
Details | Diff | Splinter Review 
The basic idea is that if you get new mail and it happens to be from your buddy you
show some this in some way on aim window. There will be a bugscape version of this
bug dealing with aim issues. This bug covers what we need to do in mail for this.
QA Contact: gayatri → sheelar
This bridge could also be used for other services that aim needs from mail.
QA Contact: sheelar → gayatri
changing qa contact again.
QA Contact: gayatri → sheelar
Problem: The first thing that we are trying to solve here is aim will give us a
list of buddy email addresses and we will have to send aim info on what buddies
have sent new/unread mail to this acct. For start we are going to take the
default account. We are going to do this for unread mail in Inbox. unread mail
will cover new mail plus something more (cases where we file unread messages to
Inbox or mark msgs unread) but that is ok for demo

Proposed solution: Create a new service class nsMailBridgeService in
mozilla/mailnews/extensions/mailBridge

nsMailBridgeService will contain all the methods that aim can use to get what it
needs from mail. 

Right now we need....

GetUnreadMessagesForDefaultAccountInbox(in buddiesEmailAddress)
{
  Make a copy of buddy's email addresses.
  This will get the Inbox for default account and get the db and register 
itself as db listener. Check if there are any unread messages, if so iterate
through db's hdrs and then build a hashtable ( that has buddy emailaddress as
key and unread msg count as value) notify aim about it.

}
nsMailBridgeService will implement nsIDBChangeListener methods so that 
we are notified when there is a change in MSG_FLAG_READ. The count can change if
msg is read/deleted. We may not need to implement all the methods in this
interface. 

We will also need helper function to build hashtable and manipulate it. Some aim
interfaces may need to be implemented in case there is a buddy add/remove. We
could possibly use nsIObserver service to do that. 

But I think this is basic building block and everything can keep coming later on. 

David, can you review this first cut design ? thx.
Status: NEW → ASSIGNED
QA Contact: sheelar → gayatri
Navin, I can't really say if that design makes sense without knowing about the
aim side of things. If you need to keep aim instantly in sync with changes in
the inbox, then that "push" design makes sense. If, however, you only want to
update aim every so often, a pull design might be easier. You might also
consider writing this listener code in js instead of creating a whole dll for
this feature.
QA Contact: gayatri → sheelar
yes, we need to keep aim instantly in sync with mail. This is what the other
client does. 

I think doing it in js is a good idea. I'll probably move this bug over to
bugscape because js files will be added to aim code. 
cool, yes, then a listener in js sounds like the right approach. If you end up
needing more db interfaces available to js (i.e., scriptable), let me know.
Filters:
=========

So, I was thinking of extending this for filters. For filters we will have to
first get all folders which are destination for "move filter". We can do this
by enumerating filter list on incomingServer for the default account. Once we
get all the folders we will have to get the db for each folder and register 
nsIDBChangeListener already implemented in js. But I will have to change 
nsIDBChangeListener interface to pass msgHdr instead of msgKey. This is 
needed because in our aim js file we will not know which key belongs to which db
and we need hdr because we need to get "author" from it. Author is needed to
show which buddy has sent new messages.

So all these methods will pass msgHdr instead of msgKey.

    void OnKeyChange(in nsMsgKey aKeyChanged, in unsigned long aOldFlags, in 
unsigned long aNewFlags, 
                           in nsIDBChangeListener aInstigator);
    void OnKeyDeleted(in nsMsgKey aKeyChanged, in nsMsgKey aParentKey, in long 
aFlags, 
                            in nsIDBChangeListener aInstigator);
    void OnKeyAdded(in nsMsgKey aKeyChanged, in nsMsgKey aParentKey, in long 
aFlags, 
                          in nsIDBChangeListener aInstigator);
	void OnParentChanged (in nsMsgKey aKeyChanged, in nsMsgKey oldParent, 
in nsMsgKey newParent, in nsIDBChangeListener aInstigator); 

In addition to this we will have to add Observer methods to filterList, so 
when we change/save filter list we would know in our js code to update our db 
list to keep it in sync. I mean the list will have only dbs that can get 
new messages. 

The reason we have to listen to db in the first place is because we need to 
update the aim window when a new msg is read/deleted (new msg count goes from 0 
to 1 or 1 to 0). 

David, can you review this? thx.
Navin, that makes it seem like you'd need to have all the db's open for all the
folders that could ever have messages filtered into them. That could produce an
undesirable memory bloat, either keeping all those db's open, or opening all
those db's every time we get new mail. A different approach would be to get
notified of the headers as they come into the inbox before they get filtered.
The problem is how will you know if you have read a new message in a filtered 
folder. Also we are already opening the db for filtering msgs to it right now. 
we only null out folder db when we switch folders. 

let me think some more. 
I might not be understanding what you had in mind. We only open target dbs for
folders if messages actually get filtered into them, and then we close the db.
It sounded like you wanted to keep a listener open on the db's of all possible
filter targets. And I'm not clear on what exactly you want to do. It seems to me
that if you're just interested in when a new message gets marked read, you just
want to have a db change listener open on the currently opened folder in the ui
so you'll then get notified if a new message gets read. So instead of having a
bunch of db change listeners around, you'd just have one for the open folder.
Then, when you get a notification, you know what the db is, and you don't have
to change all the notification methods.
open folder strategy may not work. What would happen if you were delete the new 
hdrs using advanced search window (you did an account level search).   
what if they moved all the new messages to another, non-filter related folder
from account search, w/o reading them, and then read them in the new folder?
Your approach wouldn't work in that situation (but it would work in mine :-) ) I
think they're both edge cases that are not worth worrying much about, but if
there's some efficient approach that will solve all the cases, that's great.

Here's another idea. I've recently added the ability to get notified in the js
when a message is read by the user (it's checked in on a branch). It seems like
you're interested in that event primarily. I guess you're also interested in the
user deleting a message w/o reading it, but maybe not so much (how often are you
going to delete mail from someone on your buddy list w/o reading it?). I know
for your demo you're only going to pay attention to the transition from unread
to read and that you think ultimately you're going to look at the new flag going
away. I'm not sure that's ultimately going to be the right approach, because we
clear the new flag when you select a folder and then select a different folder.
I think you might want to build up a list of all the message-ids for new
messages coming into the inbox (message-ids are more or less guaranteed to be
unique). Then, you'd only pay attention to when the user reads a msg with a msg
id you're keeping track of, no matter what folder they read it in. Anyway, just
a few thoughts.
If you move them all your new status will be lost in the original folder and
listener will notify us of that. So it is going to work in my case. 

I am doing this just for new, not for unread, for demo and demo is just for
Inbox. I thought (putterman asked me to look into it) of scaling for filters.
That other client doesn't do for filters, may be we can skip it altogether if
they agree. 


I think doing it just for new might not be the best idea. For example, I get 10
new messages in my inbox from my buddies, and notify aim. Then, I click on
another folder. Now, I have no new messages. Do you tell aim that I no longer
have any new messages from any of my buddies?
yes, because you have seen those msgs from your buddies (if not read them). you
may not want to read all your msgs from your buddies :)
Is that the way the other client works? That would surprise me very much.
It(other client) works for unread. So if you go and mark a hdr unread it(other
client) shows on the aim window (I don't think is correct either). 
We can do for unread&new if others also agree but for demo I think new is ok. I
guess you can argue either way. 
yeah, that's because the other client treats unread and new as the same concept,
so it's doing the right thing as far as it's concerned. We have a different
concept of new.
Since this is just for a demo, you shouldn't come up with a solution that's too
involved. I say this because I'd hate for you to do a lot of work that we just
have to throw away if it turns out people want it to work differently than you
think they'll want. Also, whatever you do should have no performance impact if
this feature is not turned on (for example, if a user isn't using this feature,
we don't want a lot of js notifications flying around, because they can slow us
down significantly).
The problem is there is no spec. It is being written/rewritten.

I talked to putterman, we agreed to do for new messages (atleast for now). 

we are not going to register the db listener if we are not using this feature so
it is not going to affect the performance in any way. 

So if mail is not running and aim is up and then we launch mail, we discussed
in the meeting that mail will need to notify aim that it is up. I was thinking 
of tying up the mail-loaded and mail-unloaded notifications to when the 
accounts first get loaded in nsMsgAccountManager::LoadAccounts() and when they 
get unloaded in nsMsgAccountManager::UnloadAccounts(). So the notification will 
be done using the ObserverService and we will register aim to listen for these 
notifications.

David, does this sound ok ? 
That's sounds OK, Navin. I couldn't find anything in the component mgr that
looked helpful.
wait, that's not going to work in a few scenarios like:

1. start mail.
2. close mail
3. start aim
4 start mail

Let me think about other ways of doing this.
ok, I assume you are having navigator running all the time in that example. We
could solve that case by checking for accounts loaded in account manager when we
start aim, something like IsAccountsLoaded() which will return m_accountsLoaded.

If we do that then I don't think we need to check if mail is running i.e
mail:3pane exists (in aim). 
Navin, you can't check anything in the account manager without causing it to
load all of mail news, so I don't think your idea will work.

I talked to Seth about this. He thinks we should have some sort of "persistent
notification", such that you can register this notification, and then anyone who
adds themselves as a listener after the notificiaton is registered can still get
the notification. That's probably for the future. I think for now, we should
just say that this only works when mail is running - there will be cases where
it will work when mail isn't running, but we won't "support" that.
m_accountsLoaded is a boolean in nsMsgAccountManager and we will just return the
boolean value. We are not going to do anything else in that function.

nsMsgAccountManager::AreAccountsLoaded(PRBool *aResult)
{
  *aResult = m_accountsLoaded;
  return NS_OK;
}

Navin, you can't call a method w/o loading the dll that implements the method.
ok, I was thinking we are just going to load msgbase.dll but not whole of
mailnews but there could be dependencies. 

I thought about it some more :-) There are two things we could do.

One is to use that AreAccountsLoaded() function because we are going to be 
loading just msgbase.dll and nothing else, right ?

The second thing we could do is aim handling accounts loaded notifications 
gracefully in the sense that multiple such notifications can be handled there.
I think that can be done.
sorry, I took a day off.

You may be looking at this as adding 3 lines of code to see if mail has been
loaded. I look at it as loading at least 30,000 lines of code (the number of
lines of code in msgbase) to see if mail has been started.
What about the second thing I suggested ?

>The second thing we could do is aim handling accounts loaded notifications 
>gracefully in the sense that multiple such notifications can be handled there.
>I think that can be done.
Sorry, I have no idea what that second thing means.  Maybe you could explain it
a little more?
So, whenever mail starts up we call LoadAccounts() (possibly multiple times).
When we call LoadAccounts(), we will send mail-loaded notification as I
described in comment #21. This will solve the problem you mentioned in comment
#23. There could be multiple calls to LoadAccounts() but it won't matter to aim
once we know that mail has started up. So aim would just listen to the first
notification and ignore the rest.
I don't see how that could possibly address my comment 23 - in that scenario,
aim is not running, so it can't be receiving any notifications. Unless you're
proposing that loading mail always forces AIM to get loaded, which is not
something we had in mind. The problem with the scenario in comment 23 is not
that mail starts up twice, but that it starts up and stops before aim even gets
loaded.
what forcing aim to get loaded ? huh

Here is what I am saying.

start mail  (no aim no nothing)
close mail  (no aim no nothing)
start aim ( we register rdf observer saying we listen to mail-loaded and
mail-unloaded notifications)
now start mail ( OnLoad messenger will call LoadAccounts(), now m_accountsLoaded
is true, we send mail-loaded notification as I said earlier in comment #33.)

aim knows mail is up and it works, makes sense ?
ah, sorry, I didn't understand. I think you're assuming that we unload accounts
when you close the mail window and reload accounts when you re-open the mail
window. That's not true. Also, we call LoadAccounts a very large number of
times, but it usually doesn't do anything because of the following code at the
very start of this routine:

  // for now safeguard multiple calls to this function
  if (m_accountsLoaded)
    return NS_OK;

the code after that check only ever gets hit once, no matter how many times you
open and close the mail window.
Perhaps that's what your "perhaps multiple times" comment meant. There's no
perhaps about it - we call it a very large number of times, and continue to call
it for pretty much every operation the user performs.

You could find a place that really only does get called once (or a close
approximation of once), like OnLoadMessenger.
So I will send notifications here

OnLoadMessenger (in msgMail3PaneWindow.js) ---> mail-loaded
nsMsgAccountManager::UnloadAccounts ---> mail-unloaded 

sounds ok ?

That would work. It would be more symmetric to call unload from
OnUnloadMessenger - does that get called when the three pane closes down?
If you have multiple 3 pane windows open then onUnloadMessenger would get called
for every window that is closed. so it wouldn't work. 
This is the mozilla/mailnews part of the aim-mail feature. The rest of the
patch is in the ns tree and I will attach it to bugscape bug 19015 . As we
discussed when mail comes up we will have to notify that mail has been loaded
so that aim can start listening for mail from buddy. To do that I had to add
"defaultInboxLoadedOnStartup" notification. we could not do just with
"mail3pane" window loaded notification because there could be no mail accounts
when we start mail first time. Also to deal with invalid msf files/ or uid
validity check failing, it makes sense to notify when Inbox is completely
loaded because Inbox
db gets trashed in those cases. So I am using global vars to notify only when 
we launch a 3 pane window as you can see. For "View mail messages .." from
buddy 
we were asked to quick search messages from buddy and show them. So I had to
pass
in the buddy email address as arg and we launch a new 3 pane window and after
Inbox is loaded we do the quick search, seems a little slow, I was suggesting
we
could directly do an advanced search (something we can do later..)

Also the default mail account can change so I had to notify whenever it happens
though it is rare. I added mail-unloaded notification but I'm not using that
currently because mail is never unloaded until we File | Exit.

In nsMsgDatabase code I had to change the dbChangeListeners array from
nsVoidArray to nsISupportsArray because db changeListener I implemented in js
didn't like it. 

Cavin, David, can you review ? thx.
Comment on attachment 94895 [details] [diff] [review]
mozilla/mailnews part of the patch

r=cavin.
Attachment #94895 - Flags: review+
you've probably figured this out already, but what I was suggesting here was
changing the various notification methods like this:

	for (PRInt32 i = 0; i < m_ChangeListeners->Count(); i++)
	{
		nsIDBChangeListener *changeListener =
            (nsIDBChangeListener *) m_ChangeListeners->ElementAt(i);

to something like this
	for (PRInt32 i = 0; i < m_ChangeListeners->Count(); i++)
	{
		nsISupports *listenerSupports =
            (nsISupports *) m_ChangeListeners->ElementAt(i);
               nsCOMPtr <nsIDBChangeListener> changeListener = listenerSupports;

yes, I changed it but I am in middle of pulling commercial. What about other
things in the patch like the notifications ? etc
it looked OK to me at first glance - I'm going to ask Seth to take a look since
this is important and he might have some helpful suggestions.
can you use QueryElementAt here, like you are in the other places?

+  for (PRInt32 i = count - 1; i >= 0 ; i--)
+  {
+    nsCOMPtr<nsISupports> supports =
getter_AddRefs(m_ChangeListeners->ElementAt(i)); //addref because it could be
deleted from underneath us
+    nsCOMPtr<nsIDBChangeListener> changeListener = do_QueryInterface(supports);
+    nsresult rv;
+    if (changeListener)
+      rv = changeListener->OnAnnouncerGoingAway(this); 
I'll change it (if I didn't do it for a reason, seems fine to use QueryElementAt). 
I'll integrate this before merging on trunk. I am working on mailim branch
right now.
Attachment #95469 - Flags: superreview+
Comment on attachment 95469 [details] [diff] [review]
patch for comment #46

that looks fine.
Comment on attachment 94895 [details] [diff] [review]
mozilla/mailnews part of the patch

1)

remove or comment out this dump

+		dump ("gEmailAddress " + gEmailAddress + "\n");

2)


Can you add make this:

+function NotifyObservers(aSubject, aTopic, aData)
+{
+  var observerService =
Components.classes["@mozilla.org/observer-service;1"].getService(Components.int
erfaces.nsIObserverService);
+  observerService.notifyObservers(aSubject, aTopic, aData);
 }

and fix the code I added (in the same file for the extra toolbar) to use it?

	var observerService =
Components.classes["@mozilla.org/observer-service;1"].getService(Components.int
erfaces.nsIObserverService);
	observerService.notifyObservers(window, "mail:updateToolbarItems",
null);

address those issues, and sr=sspitzer

have you testing this code with multiple 3 panes?
Attachment #94895 - Flags: superreview+
fixed. will file new bugs for enhancements
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
This commit have added a "may be used unitialized" warning on brad TBox:

+mailnews/db/msgdb/src/nsMsgDatabase.cpp:574
+ `nsresult rv' might be used uninitialized in this function
QA Contact: sheelar → stephend
Someone (will/might?) take care of the verification on the commercial side of
this bug in http://bugscape.netscape.com/show_bug.cgi?id=19015

As for this Mozilla bug, I'm verifying based on LXR and my CVS tree.

Verified FIXED
Status: RESOLVED → VERIFIED
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: