133402 - Sidebar Address Book: Select card and it does not auto fill correctly in composer

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Address Book & Contacts, defect, P2)

Description

[Ninoschka Baca]

Trunk build 2002-03-25: WinMe
haven't tried linux or mac yet.

Overview: From the Sidebar AB, select card and it does not auto fill correctly 
in composer.

Steps to reproduce:
1. Address Book window
a. Select a card in the Sidebar AB
b. Select the Compose button

Actual Results: It autofills with the email address selected in the AB results 
pane instead of choosing the card selected in the Sidebar AB.

2. 3pane window
a. Select a card in the Sidebar AB
b. Select the Compose button 

Actual Results: It does not autofill with any email address.

Expected Results: In both cases the Compose window's address area should 
automatically display the email address of the card selected in the Sidebar AB.

Comment 1

[Ninoschka Baca]

Marking nsbeta1 since this feature is not useful unless a new message/compose 
window is autofilled with the correct email address.

Comment 2

[varada]

Was this feature ever working and if so any idea when it broke?

Comment 3

[Ninoschka Baca]

Additional Expected Results:
- If a list is selected then the list should autofill in a compose window.

I need to try some builds to see when this occured.

Comment 4

[shliang]

taking

Comment 5

[Ninoschka Baca]

In the 3pane if an entry is selected from the Sidebar Address Book, select the
New Message button and the compose window opens but it is not addressed to the
currently selected card. This bug was written specifically with the Address Book
window open so should I log a new bug for the 3pane or will this fix the 3pane
problem as well?

Comment 6

[shliang]

Attachment: patch

in addressbook and mail, if card(s) are selected in the sidebar panel and the
sidebar panel has focus, clicking compose will prefill the to: field with
selected email addresses

Comment 7

[Sean Su]

Comment on attachment 81543 [details] [diff] [review]
patch

r=ssu

Comment 8

[(not reading, please use [e-mail] instead)]

Comment on attachment 81543 [details] [diff] [review]
patch

needs work.  I don't think this is the right fix.

here's some problems I have with this:

1)  why does mailCommands.js have to know about addressbook?  I don't think it
does.

2)  the code added to mailCommands.js looks copied and pasted from existing ab
code.  If we were going to do it this way, we should have moved the common js
and then included it.

what do you think about something like this:

from the code that handles "new message" (mailCommon.js and abCommon.js, I
think), do this:

find out what has focus, and if that element has a getselectedaddresses
attribute, evaluate it.  if not, do what we did before.

so in our xul for the ab sidebar, we'd add:

getselectedaddresses="GetSelectedAddresses()"

this way, all the code for determining the selected addresses would live in our
the js for the addressbook sidebar.

comments?

Comment 9

[shliang]

the code that handles new message is in mailCommands.js - it's ComposeMessage() 
isn't it? that's where i added the stuff about getting the selected addresses.

[find out what has focus, and if that element has a getselectedaddresses
attribute, evaluate it.  if not, do what we did before.
so in our xul for the ab sidebar, we'd add:
getselectedaddresses="GetSelectedAddresses()"]

this doesn't work because GetSelectedAddresses() is called from mail and 
although it can see that the focused window has that attribute it can't 
evaluate the function - i tried it just to make sure and i get an error saying 
GetSelectedAddresses is not defined.

i could move the common js somewhere else and include it from both files to 
take care of (2) but i'm not sure what to do about (1) - what do you think?

Comment 10

[(not reading, please use [e-mail] instead)]

when we get the focused element, what do we have?  an nsIXULElement?  is there
anyway we can get to the tree, and then get the view? 

if we can get the nsIAbView, (I think we can if we can get the tree / outliner,
then get the view for the tree, and then QI) move the code that determines the
selected addresses from JS into C++, and then call it on the view.
 

Comment 11

[shliang]

Attachment: patch

Comment 12

[(not reading, please use [e-mail] instead)]

I like this approach much better.

Here are some minor comments, suggestions and questions with your last patch:

1)

why'd you have to add the try and catch here?
It looks like you are expecting to get into the catch.  
If so, please add a comment about what scenario will cause you to get there.

 function AbNewCard(abListItem)
 {
+  try {
   var selectedAB = GetSelectedAddressBookDirID(abListItem);
-
   goNewCardDialog(selectedAB);
 }
+  catch(ex) {
+    goNewCardDialog(GetAbViewURI());
+  }
+}

2)

this comment is incomplete:

+  // if sidebar is open and addressbook panel is open and focused
+  const abPanelUrl =
"chrome://messenger/content/addressbook/addressbook-panel.xul";

how about:

+  // if sidebar is open and addressbook panel is open and focused
+  // then use the ab view from the sidebar
+  const abPanelUrl =
"chrome://messenger/content/addressbook/addressbook-panel.xul";

3)

doesn't gCurFrame get defined in sidebarOverlay.js?  Seems like that's worth a
comment
and seems like you'd get js warnings about gCurFrame being undefined from
everything except the addressbook sidebar usage of abCommon.js
Are you seeing those warnings?

+  if (document.getElementById("sidebar-box")) {
+    if (gCurFrame && 
+        gCurFrame.getAttribute("src") == abPanelUrl &&
+        document.commandDispatcher.focusedWindow ==
gCurFrame.contentDocument.defaultView) 
+    {
+      abView = gCurFrame.contentDocument.defaultView.gAbView;
+    }
+  }

4)
 
again, why'd you have to add the try and catch here?
It looks like you are expecting to get into the catch.  
If so, please add a comment about what scenario will cause you to get there.
Also, if it's expected, remove the dump statement.
If unexpected, dump the exception.
 
 function SelectFirstAddressBook()
 {
+  try {
   dirTree.treeBoxObject.selection.select(0);
   ChangeDirectoryByURI(GetSelectedDirectory());
 }
+  catch(e) {
+    ChangeDirectoryByURI(kPersonalAddressbookURI);
+    dump("***in select first address book catch***\n");
+  }
+}
 
5)

same as #4 why'd you have to add the try and catch here?
It looks like you are expecting to get into the catch.  
If so, please add a comment about what scenario will cause you to get there.

 function AbNewList(abListItem)
 {
+  try {
   var selectedAB = GetSelectedAddressBookDirID(abListItem);
-
   goNewListDialog(selectedAB);
+  }
+  catch(ex) {
+    goNewListDialog(GetAbViewURI());
+  }
 }
 
6)

can you add a comment about how this if block makes it so:

// if the addressbook sidebar panel is open and has focus,
// we'll get the selected addresses from it on new message.

+    if
(document.commandDispatcher.focusedWindow.document.documentElement.hasAttribute("selectedaddresses"))
+    {

7)

remove the dump

+      dump("has selectedaddresses\n");

8)

+          params.composeFields = composeFields;
+          msgComposeService.OpenComposeWindowWithParams(null, params);

what about the other params, like type, format identity and msgWindow?

9)

It would be good to move all this code into a function so that we could
something like:

+    if
(document.commandDispatcher.focusedWindow.document.documentElement.hasAttribute("selectedaddresses"))
+      return NewMessageToSelectedAddresses();

Moving the code out keeps the "if (type == msgComposeType.New)" block from
getting out of control.

10)

To follow our conventions, please change

+
nsISupportsArray getSelectedAddresses();

to

+      [readonly] attribute nsISupportsArray selectedAddresses;

Your C++ should remain the same.  The js will change from:

+ var addresses = abView.getSelectedAddresses();
to
+ var addresses = abView.selectedAddresses;

11)

in ::GetSelectedAddresses(), you're using retval as a local variable.
This makes the code confusing.  Instead, declare a local for the selected cards:


+  nsCOMPtr<nISupportsArray> selectedCards;
+  nsresult rv = GetSelectedCards(getter_AddRefs(selectedCards));
+  NS_ENSURE_SUCCESS(rv,rv);

This allows you to change

+  (*_retval)->Count(&count);

to

+  selectedCards->Count(&count);

which is much more readable.

12)

+      char *mailListURI;
+      card->GetMailListURI(&mailListURI);

you're leaking mailListURI.  Instead, use a nsXPIDLCString

13)

+      rv = mailList->GetAddressLists(getter_AddRefs(mailListAddresses));

since you get rv, you should check it.


14)

You'll want to wrap:

+        nsCOMPtr<nsISupportsString>
supportsEmail(do_CreateInstance(NS_SUPPORTS_STRING_CONTRACTID));
+        supportsEmail->SetDataWithLength(primaryEmail.Length(),
ToNewCString(primaryEmail));
+        addresses->AppendElement(supportsEmail);

and 

+      nsCOMPtr<nsISupportsString>
supportsEmail(do_CreateInstance(NS_SUPPORTS_STRING_CONTRACTID));
+      supportsEmail->SetDataWithLength(primaryEmail.Length(),
ToNewCString(primaryEmail));
+      addresses->AppendElement(supportsEmail);

with:

if (!primaryEmail.IsEmpty()) {
}

It's possible for cards to not have email addresses.  If so, we should skip them.
(It may not be possible with mailing lists, but might as well be consistent)

15)

+  *_retval = addresses;  
+  NS_ADDREF(*_retval);

Instead do:

+  NS_ADDREF(*_retval = addresses);

Comment 13

[shliang]

Attachment: updated patch

about comments 1, 4, 5: the try/catch stuff doesn't belong to this bug. i took
them out of this patch...sorry about that.

Comment 14

[(not reading, please use [e-mail] instead)]

looks good. 

two last things:

1)

looking at nsIMsgComposeParams, we can set the msgwindow, but we can set 
identity.  can you pass that through to your new function 
[NewMessageToSelectedAddresses()] and set it on params?

2)  style nit,

in most of your patch, you do postfix, i++.  but in 
nsAbView::GetSelectedAddresses(), you do prefix, ++i, ++j.

Any reason for that?  I'd just keep it postfix.

fix those two things, and sr=sspitzer

Comment 15

[shliang]

Attachment: patch

Comment 16

[shliang]

Attachment: patch

Comment 17

[shliang]

Attachment: patch

Comment 18

[(not reading, please use [e-mail] instead)]

r=sspitzer, thanks for fixing those issues.

Comment 19

[David :Bienvenu]

sr=bienvenu

Comment 20

[shliang]

checked into trunk

Comment 21

[Ninoschka Baca]

Trunk build 2002-07-08: WinMe, Linux RH 7.1,  Mac 10.1.3
Verified Fixed.

Comment 22

[[e-mail]]

Comment on attachment 85286 [details] [diff] [review]
patch

>+    if (gCurFrame && 
>+        gCurFrame.getAttribute("src") == abPanelUrl &&
>+        document.commandDispatcher.focusedWindow == gCurFrame.contentDocument.defaultView) 
>+    {
>+      abView = gCurFrame.contentDocument.defaultView.gAbView;
>+    }
[.contentWindow is the same as .contentDocument.defaultView]