131571 - [RFE] when clicking on a mail adress that is already in the adressbook the option should be "edit address book entry", not "add to address book"

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, enhancement)

Description

[Helge Hielscher]

when I click on a mail adress I get 4 options: "add to address book", "compose
mail to", "copy email address" and "create filter".

When I choose "add to address book" it adds a new entry in the address book even
if there is alreday a card for that address.

The better way would be to look up the addressbook first if there is already an
entry and when it is there prompt "edit address book entry" instead or
additional to "add to address book".

Comment 1

[Jason M. Abels]

This seems like a sensible RFE to me. Can't find any dupes under a few obvious
searches.

This may even be a bug. It seems obvious if an address is already in the book,
it shouldn't add another one.

Comment 2

[AJS]

Agreed.  In fact, I mentioned this in bug 310601 -- you never know 
whether "add to address book" is going to do anything, since there is no 
indication that a person (on the CC list, for example) is already in your 
address book.

Comment 3

[Eyal Rozenberg]

*** Bug 262100 has been marked as a duplicate of this bug. ***

Comment 4

[Eyal Rozenberg]

Attachment: partial wip against tb 1.5.0.4

This patch is _not_ working: It has all the UI stuff sorted out just fine, the edit address book card dialog opens up as it should, there's just one problem: I need to figure out how to lookup an address book card based on e-mail address and/or display name. Any ideas?

Standard8: Please review the patch modulo this issue.

If and when I can get this to work, I'll make the same changes in seamonkey as well.

Comment 5

[Mark Banner (:standard8)]

(In reply to comment #4)
> Created an attachment (id=229347) [edit]
> I need to figure out how to lookup an address book card based on e-mail address
> and/or display name. Any ideas?

Sorry for the delay, I've been needing to concentrate on other stuff for a while. I haven't looked at the entire patch yet, I think one way way to do this would be to copy the idea of the address collector (http://lxr.mozilla.org/seamonkey/source/mailnews/addrbook/src/nsAbAddressCollecter.cpp#132) - though that wouldn't work for LDAP directories. So the better alternative is to copy the quick search method and get the ab out of RDF with a URI supplied which includes the search information as per http://lxr.mozilla.org/seamonkey/source/mailnews/addrbook/resources/content/addressbook.js#709 and http://lxr.mozilla.org/seamonkey/source/mailnews/addrbook/src/nsAbView.cpp#222

I'd also like to see the option for searching multiple address books possibly. 

Comment 6

[Eyal Rozenberg]

(In reply to comment #5)

AFAICT all of that code is based on having an addressbook window with a  dirtree and what-not. It mixes UI and backend code in very weird ways I would really not want to get into. Just tell me what the relevant API is (if it exists, of course; otherwise we need a new bug blocking this one).

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 229347 [details] [diff] [review]
partial wip against tb 1.5.0.4

Sorry for not getting back to you much earlier on this

(In reply to comment #6)
> (In reply to comment #5)
> 
> AFAICT all of that code is based on having an addressbook window with a 
> dirtree and what-not. It mixes UI and backend code in very weird ways I would
> really not want to get into. Just tell me what the relevant API is (if it
> exists, of course; otherwise we need a new bug blocking this one).
> 

See Scott's patch on bug 357321 specifically attachment 245415 [details] [diff] [review] and the function allowRemoteContentForSender. That should give you all the information you need.

Comment 8

[Eyal Rozenberg]

Will not be able to work on this at all, at least for the next month or so; unassigning for now (although it doesn't look like a lot of work).

Comment 9

[Mark Banner (:standard8)]

This has been fixed for Thunderbird by bug 450724 attachment 336332 [details] [diff] [review]

Comment 10

[Ian Neal]

Attachment: Add ability to edit/view contacts that already have an addressbook card [Checked in: Comment 12]

This patch ports:
* Front end parts of Bug 450724 - Implement New/Edit Card inline features for message header display.
* The storing headerName parts (but not the "You" string bits) of Bug 481966.
* Front end helper parts of Bug 474721.
* Various other follow-up bugs to the above ones.

Comment 11

[Karsten Düsterloh]

Comment on attachment 630190 [details] [diff] [review]
Add ability to edit/view contacts that already have an addressbook card [Checked in: Comment 12]

Just some nits …

>+function AddContact(emailAddressNode)

Changing the function name is a good opportunity for correcting the parameter name to aEmailAddressNode.

>+function EditContact(emailAddressNode)

Idem.

>+  if (emailAddressNode.cardDetails.card) {

Brace on its own line, please.

>+// These are all the items that use a mail-multi-emailHeaderField widget and
>+// therefore may require updating if the address book changes.
>+const gEmailAddressHeaderNames = ["from", "reply-to",
>+                                  "to", "cc", "bcc"];

No need to wrap.

>+  // stash this so that the <mail-multi-emailheaderfield/> binding can

"Stash" should be capitalized here. (Mega minor dragon dinosaur præhistoric nit! *g*)

>+var AddressBookListener = {

Braces on their own line, please; ditto for all the new code following.

>+    OnAddressBookDataChanged(aItem instanceof nsIAbCard ?
>+                             nsIAbListener.directoryItemRemoved :
>+                             nsIAbListener.directoryRemoved,
>+                             aParentDir, aItem);

Maybe indent the two middle lines to highlight that the are parameters of the operator?

>+function UpdateEmailNodeDetails(aEmailAddress, aDocumentNode, aCardDetails)
> {
…
>+  var cardDetails = aCardDetails ? aCardDetails :
>+                                   getCardForEmail(aEmailAddress);

I think you mean:
  var cardDetails = aCardDetails || getCardForEmail(aEmailAddress);

>+  var displayName;
>+  if (condense && cardDetails.card)
>+    displayName = cardDetails.card.displayName;

displayName should be properly initialized to "".

>+  if (displayName) {
>+    aDocumentNode.setAttribute("tooltiptext", aEmailAddress);
>+  } else {
>+    aDocumentNode.removeAttribute("tooltiptext");
>+    displayName = aDocumentNode.getAttribute("fullAddress") ||
>+                  aDocumentNode.getAttribute("displayName");
>+  }

Braces on their own line, please; ditto for all the new code following.

>+      if (aItem instanceof Components.interfaces.nsIAbDirectory) {

Why no constant for Components.interfaces.nsIAbDirectory, but for nsIAbListener/nsIAbCard?

>+          // Just in case we have a bogus parent directory

Just "just".

>+            var cardDetails = { book: aParentDir, card: aItem };

Could use let instead of var here.

>+function setupEmailAddressPopup(aAddressNode)

Why is this function lowercased? TB compat? If not, please use uppercase as well.

>+function getCardForEmail(emailAddress)

Same question here?
Also, worth taking the chance of using aEmailAddress.


r=me with that explained/corrected, especially the braces. ;-)

Comment 12

[Ian Neal]

Comment on attachment 630190 [details] [diff] [review]
Add ability to edit/view contacts that already have an addressbook card [Checked in: Comment 12]

Checked in with points addressed:
http://hg.mozilla.org/comm-central/rev/0d2ec5a8ce0e