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

VERIFIED FIXED in mozilla1.0

Status

P2
normal
VERIFIED FIXED
17 years ago
8 years ago

People

(Reporter: nbaca, Assigned: shliang)

Tracking

Trunk
mozilla1.0
x86
Windows ME

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt2][ue wanted])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

17 years ago
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.
(Reporter)

Comment 1

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

Updated

17 years ago
Keywords: nsbeta1 → nsbeta1+
Priority: -- → P2
Whiteboard: [adt2]
Target Milestone: --- → mozilla1.0

Comment 2

17 years ago
Was this feature ever working and if so any idea when it broke?
Assignee: racham → varada
(Reporter)

Comment 3

17 years ago
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.
(Assignee)

Comment 4

17 years ago
taking
Assignee: varada → shliang
(Reporter)

Comment 5

17 years ago
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?
(Assignee)

Comment 6

17 years ago
Created attachment 81543 [details] [diff] [review]
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

17 years ago
Comment on attachment 81543 [details] [diff] [review]
patch

r=ssu
Attachment #81543 - Flags: review+
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?
Attachment #81543 - Flags: needs-work+
(Assignee)

Comment 9

17 years ago
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?
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.
 

Updated

17 years ago
Whiteboard: [adt2] → [adt2][ue wanted]
(Assignee)

Comment 11

17 years ago
Created attachment 84095 [details] [diff] [review]
patch
Attachment #81543 - Attachment is obsolete: true
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);
(Assignee)

Comment 13

17 years ago
Created attachment 84846 [details] [diff] [review]
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.
Attachment #84095 - Attachment is obsolete: true
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
(Assignee)

Comment 15

17 years ago
Created attachment 85281 [details] [diff] [review]
patch
Attachment #84846 - Attachment is obsolete: true
(Assignee)

Comment 16

17 years ago
Created attachment 85284 [details] [diff] [review]
patch
Attachment #85281 - Attachment is obsolete: true
(Assignee)

Comment 17

17 years ago
Created attachment 85286 [details] [diff] [review]
patch
Attachment #85284 - Attachment is obsolete: true
r=sspitzer, thanks for fixing those issues.
Status: NEW → ASSIGNED

Comment 19

17 years ago
sr=bienvenu
(Assignee)

Comment 20

16 years ago
checked into trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
(Reporter)

Comment 21

16 years ago
Trunk build 2002-07-08: WinMe, Linux RH 7.1,  Mac 10.1.3
Verified Fixed.
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey
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]
You need to log in before you can comment on or make changes to this bug.