759328 - Add instant messaging fields to the address book cards

Status: RESOLVED FIXED

(Thunderbird :: Address Book, defect)

Description

[Florian Quèze [:florian]]

Attachment: WIP

I think the attached patch follows Blake's recommendations.
It adds a "Chat" tab to address book cards, where it's possible to set a username for: Google Talk, AIM, Yahoo, Skype, QQ, MSN, ICQ, Jabber.
In the first ("Contact") tab, the old Screen Name field is gone, and at the same place there's a "Chat information" link that when clicked selects the "Chat" tab.

Requesting feedback from Blake (to check that the UI is what he expects) and David (to check that the way this touches the address book code isn't going to cause significant pains).


David, I saw that all the current fields are listed at http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/public/nsIAbCard.idl#365 Do I need to add all the new IM fields in that list? I don't have a clear understanding of how this list is needed.

* I won't pretend to understand all the usages of kScreenNameProperty, except http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/src/nsAbAddressCollector.cpp#278 (and we can probably do the same for @gmail.com @googlemail.com, @hotmail, @msn, and @live addresses)

* Are AIM screen names stored in ldap? http://mxr.mozilla.org/comm-central/source/mailnews/mailnews.js#311 Do we need something similar for other IM services?

* I also noticed http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/src/nsAbOSXUtils.mm#136 that seems to be using Apple-defined constants to import things from the OS X address book. There are more similar constants documented at https://developer.apple.com/library/mac/#documentation/UserExperience/Reference/AddressBook/C/ABPersonRef/Reference/reference.html#//apple_ref/doc/c_ref/kABAIMInstantProperty and apparently OS X 10.7 supports even more: https://developer.apple.com/library/mac/#documentation/UserExperience/Reference/AddressBook/C/ABPersonRef/Reference/reference.html#//apple_ref/c/data/kABInstantMessageServiceAIM


Blake, the current address book shows the Screen Name in the card summary that's visible in the bottom part of the address book window when selecting a card. Do we need to change this to show all the IM fields that have a value?

There's currently an "Instant Message" context menu items on contacts (and it's also a toolbar button; hidden by default though) that just takes the AIM screen name and loads an aim:goim?screenname=<screenname> url through the external protocol service. I assume we want to change that, but what's the new expected behavior? Do we take into account which accounts the user has configured? The presence information we have of the contact? I don't think there's a known typical URL for all the IM services I have listed in the new "Chat" tab that this patch adds.

Comment 1

[Florian Quèze [:florian]]

Attachment: Screenshot of the "Contact" tab with the "Chat information" link

Comment 2

[Florian Quèze [:florian]]

Attachment: Screenshot of the "Chat" tab

Comment 3

[David :Bienvenu]

Comment on attachment 627950 [details] [diff] [review]
WIP

does copying a card with these attributes set from one address book to another work? I.e., if you set the gtalk screen name on a card, shut down TB, restart, then copy that card to an other personal address book, does the scren name get copied? From poking around the address book code, it looks like it might, which means that adding properties to cards is rather trivial, despite all the haters out there :-)

Comment 4

[Florian Quèze [:florian]]

(In reply to David :Bienvenu from comment #3)
> Comment on attachment 627950 [details] [diff] [review]
> WIP
> 
> does copying a card with these attributes set from one address book to
> another work?

I can't find any UI to copy a card (the "Copy" item in the Edit menu is disabled), but moving from an address book to another with a drag and drop works fine, and the gtalk address is still there after a restart.

> adding properties to cards is rather
> trivial, despite all the haters out there :-)

Right, it's trivial. Actually doing something useful with the new properties maybe be more difficult. Last time I looked at the code, I wasn't completely sure I could find notifications fired from the back-end when a property value changes that actually worked; but that was months ago so I don't recall very clearly. I guess if I can't find anything that works, I can still fire a notification from the UI when saving changes to a card, and use that instead of a back-end notification; that would be a relatively good approximation.

Do you have some feedback about what should be done for all the things that currently use the (aim) screenname property? Do I need to add all the new properties in each list where the screenname field was present?

Comment 5

[David :Bienvenu]

(In reply to Florian Quèze from comment #4)

> I can't find any UI to copy a card

cntrl drag doesn't do a copy? In any case, testing move is sufficient, I believe.
> 
> Right, it's trivial. Actually doing something useful with the new properties
> maybe be more difficult. Last time I looked at the code, I wasn't completely
> sure I could find notifications fired from the back-end when a property
> value changes that actually worked; but that was months ago so I don't
> recall very clearly. I guess if I can't find anything that works, I can
> still fire a notification from the UI when saving changes to a card, and use
> that instead of a back-end notification; that would be a relatively good
> approximation.

No, the address book doesn't seem to send notifications on any property changes, from what I can see.

> 
> Do you have some feedback about what should be done for all the things that
> currently use the (aim) screenname property? Do I need to add all the new
> properties in each list where the screenname field was present?

those mostly seem to have to do with import/export/vcard kind of things. None of that seems relevant, though eventually I would think that the various screen names should survive an export as ldif and subsequent import.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

(In reply to David :Bienvenu from comment #5)
> (In reply to Florian Quèze from comment #4)
> 
> > I can't find any UI to copy a card
> 
> cntrl drag doesn't do a copy? In any case, testing move is sufficient, I
> believe.
> > 
> > Right, it's trivial. Actually doing something useful with the new properties
> > maybe be more difficult. Last time I looked at the code, I wasn't completely
> > sure I could find notifications fired from the back-end when a property
> > value changes that actually worked; but that was months ago so I don't
> > recall very clearly. I guess if I can't find anything that works, I can
> > still fire a notification from the UI when saving changes to a card, and use
> > that instead of a back-end notification; that would be a relatively good
> > approximation.
> 
> No, the address book doesn't seem to send notifications on any property
> changes, from what I can see.

Adding an nsIAbListener to the nsIAbManager doesn't work?  (http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/public/nsIAbListener.idl), (http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/public/nsIAbManager.idl#73)

According to the documentation for nsIAbListener, its for

"receiv[ing] notifications of address book items being added, removed or changed with loaded address books."

Comment 7

[David :Bienvenu]

(In reply to Mike Conley (:mconley) from comment #6)
> (In reply to David :Bienvenu from comment #5)
> > (In reply to Florian Quèze from comment #4)
> > 
> > > I can't find any UI to copy a card
> > 
> > cntrl drag doesn't do a copy? In any case, testing move is sufficient, I
> > believe.
> > > 
> > > Right, it's trivial. Actually doing something useful with the new properties
> > > maybe be more difficult. Last time I looked at the code, I wasn't completely
> > > sure I could find notifications fired from the back-end when a property
> > > value changes that actually worked; but that was months ago so I don't
> > > recall very clearly. I guess if I can't find anything that works, I can
> > > still fire a notification from the UI when saving changes to a card, and use
> > > that instead of a back-end notification; that would be a relatively good
> > > approximation.
> > 
> > No, the address book doesn't seem to send notifications on any property
> > changes, from what I can see.
> 
> Adding an nsIAbListener to the nsIAbManager doesn't work? 
> (http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/public/
> nsIAbListener.idl),
> (http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/public/
> nsIAbManager.idl#73)
> 
> According to the documentation for nsIAbListener, its for
> 
> "receiv[ing] notifications of address book items being added, removed or
> changed with loaded address books."

If the property is changed as a result of ModifyCard/EditCard, yes, you're right. Programatically tweaking the card (e.g., setting a property) won't send a notification, but I think Florian only cares about the UI editing...

Comment 8

[Mike Conley (:mconley) (:⚙️)]

(In reply to David :Bienvenu from comment #7)
> If the property is changed as a result of ModifyCard/EditCard, yes, you're
> right. Programatically tweaking the card (e.g., setting a property) won't
> send a notification, but I think Florian only cares about the UI editing...

Ah, good point.

A question on this patch for Florian: in the mockup you sent to Blake and I for feedback, you had UI for adding multiples of each account type. Has that plan been axed?

Comment 9

[Florian Quèze [:florian]]

(In reply to Mike Conley (:mconley) from comment #8)

> A question on this patch for Florian: in the mockup you sent to Blake and I
> for feedback, you had UI for adding multiples of each account type. Has that
> plan been axed?

Yes. Blake didn't like it because it was too different from the other parts of the existing address book. I think Blake wanted to have instead a comma separated list in each field, but the question of how this could be discoverable for users hasn't been solved (yet?) so I'm not pushing too much on this right now.

Comment 10

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 627950 [details] [diff] [review]
WIP

Yeah, this is mostly what I was thinking.  I do think that we need some layout and wording changes, but aside from those I'm pretty happy with it, so f=me.

Now, the changes:
"Chat information" seems a little strange.  Since we use "Screen Name" in the summary, what do you think about
_Screen Name:_ [the first filled in field from the chat tab]
where "Screen Name:" is a link that will take you to the chat tab?

As for the chat tab, I think the fields should be over on the left, like the first column on the Contact tab.  And I'm not sure whether adding arbitrary fields is in scope for this bug or not, but if it is, I'ld like to see a mockup of that.

Thanks,
Blake.

Comment 11

[Florian Quèze [:florian]]

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #10)

> Now, the changes:
> "Chat information" seems a little strange.  Since we use "Screen Name" in
> the summary, what do you think about
> _Screen Name:_ [the first filled in field from the chat tab]
> where "Screen Name:" is a link that will take you to the chat tab?

(I'm assuming you are talking about the "Contact" tab of the card edit dialog.)

So you think that we should keep the "Screen Name" term (which is a typical AIM term) for all possible chat usernames?

I'm not sure of what "[the first filled in field from the chat tab]" represents in your proposal. Is it a textbox field like the one we currently have? Should it be disabled/readonly (and a click on it would select the chat tab)?

I'm still waiting for an answer to the questions in the last 2 paragraphs of comment 0.

> And I'm not sure whether adding arbitrary
> fields is in scope for this bug or not, but if it is, I'ld like to see a
> mockup of that.

Let's decide it's not :).

Comment 12

[Blake Winton (:bwinton) (:☕️)]

I think that whatever we use in the summary should be used in the contact tab.

But I also think that if we have the space when displaying the card summary (and it seems like we do), then displaying all the info we have about the person seems reasonable to me, yes.  So perhaps "Screen Name" isn't the right label for that piece.

I think it could be a read-only textbox or a label.  Clicking on it to go to the chat tab and select the contents of the appropriate field would be good.

Of course, if we're not using "Screen Name", then not having the read-only field would be a good choice.  In that case, I propose a clickable label "Chat Names", lined up with the other labels.

> There's currently an "Instant Message" context menu items on contacts (and it's
> also a toolbar button; hidden by default though) that just takes the AIM screen
> name and loads an aim:goim?screenname=<screenname> url through the external
> protocol service. I assume we want to change that, but what's the new expected
> behavior?

I think clicking on it should do something different, yeah.

> Do we take into account which accounts the user has configured?

If we did, then we could open up the new chat in Thunderbird, which seems like a win.

> The presence information we have of the contact?

Perhaps.  I can imagine a neat set of decisions as to which protocol to use, but I'm going to let you come up with the algorithm.

> I don't think there's a known typical URL for all the IM services I have listed
> in the new "Chat" tab that this patch adds.

No, I think as a final fall-back, we can go with the current behaviour and the aim: protocol.

Thanks,
Blake.

Comment 13

[Florian Quèze [:florian]]

Attachment: Patch

This patch:
1. adds chat fields to the edit card dialog.
2. removes the "Screen Name" line of the card summary (bottom right part of the main address book window) and adds a new "Chat" section at the bottom.
3. replaces the "Screen Name" column of the address book tree view (top right part of the main address book window) with a "Chat Name" column. The Chat Name value is automatically generated, based on whatever chat property isn't empty on the card.
4. removes the "Screen Name" line from the print version of address book cards, and instead adds a "Chat" section. This is for Thunderbird only (the same code is shared with SeaMonkey, but I ifdef'ed my changes).
5. Same change as 3. but in the bottom part of the "Advanced address book search" dialog.

I can't find a way to make the "advanced address book search" dialog search on the "Chat Name" instead of the (AIM) "Screen Name". The code at http://mxr.mozilla.org/comm-central/source/mail/base/content/ABSearchDialog.js#155 seems to only be able to search on one (real) property at once.


Requesting the review mostly from David, but I think Mike may want to look at this too.

Comment 14

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 629237 [details] [diff] [review]
Patch

(I'm actually going to re-direct the ui-r to Mike, too, since he'll be looking at the code anyways…  ;)

Comment 15

[David :Bienvenu]

Comment on attachment 629237 [details] [diff] [review]
Patch

switching my review to sr, since you'll need one of those.

Comment 16

[David :Bienvenu]

Comment on attachment 629237 [details] [diff] [review]
Patch

I'm wondering if there's a way to avoid or lessen the #ifdef MOZ_THUNDERBIRD's - I realize SM doesn't have this UI, but unless the code actually breaks SM, it's OK for SM to have the code compiled in.

Clicking on the chat field in the main tab taking you to the chat tab is a bit jarring, though that's obviously up to Blake...

Comment 17

[Florian Quèze [:florian]]

(In reply to David :Bienvenu from comment #16)

> I'm wondering if there's a way to avoid or lessen the #ifdef
> MOZ_THUNDERBIRD's - I realize SM doesn't have this UI, but unless the code
> actually breaks SM, it's OK for SM to have the code compiled in.

I tried to be conservative, ifdef'ing all my changes so that I don't have to test SM at all.
If you really dislike them, we can probably safely remove the ifdef in mailnews/addrbook/src/nsAbView.cpp and in mailnews/addrbook/src/nsAbCardProperty.cpp the one around CHAT_ATTRS_ARRAY and the one inside nsAbCardProperty::GenerateChatName.

Removing the others would change SM's behavior.

> Clicking on the chat field in the main tab taking you to the chat tab is a
> bit jarring, though that's obviously up to Blake...

I implemented that as Blake requested it (or at least it was my understanding that this is what he wanted). It's surprising the first time, but after the initial surprise has passed, it seemed usable to me. It's obviously not the best UI, but I dislike almost everything in that dialog, so this specific point doesn't really bother me.

Note for Mike: the style="-moz-user-focus: ignore;" that I added at the last minute before attaching the patch doesn't have the intended effect, I'll remove it at the next iteration.

Comment 18

[David :Bienvenu]

(In reply to Florian Quèze from comment #17)
> I tried to be conservative, ifdef'ing all my changes so that I don't have to
> test SM at all.
> If you really dislike them, we can probably safely remove the ifdef in
> mailnews/addrbook/src/nsAbView.cpp and in
> mailnews/addrbook/src/nsAbCardProperty.cpp the one around CHAT_ATTRS_ARRAY
> and the one inside nsAbCardProperty::GenerateChatName.

Yeah, I really do dislike them. Maybe someone on SM can test with your changes to make sure nothing breaks in SM.

Comment 19

[David :Bienvenu]

Comment on attachment 629237 [details] [diff] [review]
Patch

clearing sr request for new patch (though other than what I've already commented on, the patch looks OK)

Comment 20

[Florian Quèze [:florian]]

Attachment: Patch v2

New version of the patch including the changes described in comment 17.

Comment 21

[Ian Neal]

Comment on attachment 629727 [details] [diff] [review]
Patch v2

>+++ b/mailnews/addrbook/public/nsIAbCard.idl
>+++ b/mailnews/addrbook/src/nsAbAddressCollector.cpp

>+  if (domain.IsEmpty())
>+    return;
>   // username in 
>   // [e-mail] (America Online)
>   // [e-mail] (Compuserve)
>   // [e-mail] (Netscape webmail)
>   // are all AIM screennames.  autocollect that info.
>+  if (domain.Equals("aol.com") || domain.Equals("cs.com") ||
>+      domain.Equals("netscape.net"))
>     aCard->SetPropertyAsAUTF8String(kScreenNameProperty, Substring(aEmail, 0, atPos));
Don't you want to be putting this in kAIMProperty for Thunderbird?

>+#ifdef MOZ_THUNDERBIRD
>+  else if (domain.Equals("gmail.com") || domain.Equals("googlemail.com"))
>+    aCard->SetPropertyAsAUTF8String(kGtalkProperty, Substring(aEmail, 0, atPos));
>+#endif
> }

Comment 22

[Florian Quèze [:florian]]

(In reply to Ian Neal from comment #21)

> >+  if (domain.Equals("aol.com") || domain.Equals("cs.com") ||
> >+      domain.Equals("netscape.net"))
> >     aCard->SetPropertyAsAUTF8String(kScreenNameProperty, Substring(aEmail, 0, atPos));
> Don't you want to be putting this in kAIMProperty for Thunderbird?

Both kAIMProperty and kScreenNameProperty are defined to the same "_AimScreenName" value, so it doesn't really matter.

> >+#ifdef MOZ_THUNDERBIRD
> >+  else if (domain.Equals("gmail.com") || domain.Equals("googlemail.com"))
> >+    aCard->SetPropertyAsAUTF8String(kGtalkProperty, Substring(aEmail, 0, atPos));
> >+#endif
> > }

Would setting address book card fields that don't have a UI be a problem for SeaMonkey? If not I can get rid of that ifdef too.

Thanks for looking at this patch!

Comment 23

[Ian Neal]

(In reply to Florian Quèze from comment #22)
> (In reply to Ian Neal from comment #21)
> 
> > >+  if (domain.Equals("aol.com") || domain.Equals("cs.com") ||
> > >+      domain.Equals("netscape.net"))
> > >     aCard->SetPropertyAsAUTF8String(kScreenNameProperty, Substring(aEmail, 0, atPos));
> > Don't you want to be putting this in kAIMProperty for Thunderbird?
> 
> Both kAIMProperty and kScreenNameProperty are defined to the same
> "_AimScreenName" value, so it doesn't really matter.
Is it worth looking at where kScreenNameProperty is used to see if:
1/ Just use kAIMProperty instead
2/ There are other places where we should be using the extended list of chat properties (maybe import/export)

> > >+#ifdef MOZ_THUNDERBIRD
> > >+  else if (domain.Equals("gmail.com") || domain.Equals("googlemail.com"))
> > >+    aCard->SetPropertyAsAUTF8String(kGtalkProperty, Substring(aEmail, 0, atPos));
> > >+#endif
> > > }
> 
> Would setting address book card fields that don't have a UI be a problem for
> SeaMonkey? If not I can get rid of that ifdef too.
I don't see a problem with that. Just rebuilding at the moment without that ifdef to see if there is any issues.

Comment 24

[Ian Neal]

Any reason why there's not a field for IRC?

Comment 25

[Florian Quèze [:florian]]

(In reply to Ian Neal from comment #23)
> (In reply to Florian Quèze from comment #22)

> > Both kAIMProperty and kScreenNameProperty are defined to the same
> > "_AimScreenName" value, so it doesn't really matter.
> Is it worth looking at where kScreenNameProperty is used to see if:
> 1/ Just use kAIMProperty instead
> 2/ There are other places where we should be using the extended list of chat
> properties (maybe import/export)

Remaining use cases seem to be mostly import/export code. And yes, I would like to make these pieces of code work with more IM protocols if possible, but probably in a different bug (I don't expect that change to require any localizable string).

Comment 26

[Florian Quèze [:florian]]

(In reply to Ian Neal from comment #24)
> Any reason why there's not a field for IRC?

IRC is difficult for at least 2 reasons:
1. A nickname is meaningless without the name of the network, and I don't think users would want to write "[e-mail]" for each nick.
2. It's not always obvious to determine if someone connected with a nick is the same person as the person the user has seen with that nick.

The next step (hopefully happening very soon) is to use the information available in the address book cards and the presence information available through the Chat code to determine if it's possible to chat with someone (display presence information next to contact names in the email headers for example). Both the issues mentioned above make this unreliable for IRC (to solve 1. we would need to have in the address book card a link to a chat account; which afaik we don't want).

Comment 27

[David :Bienvenu]

Comment on attachment 629727 [details] [diff] [review]
Patch v2

yeah, I don't see why this #ifdef would be needed...

+#ifdef MOZ_THUNDERBIRD
+  else if (domain.Equals("gmail.com") || domain.Equals("googlemail.com"))
+    aCard->SetPropertyAsAUTF8String(kGtalkProperty, Substring(aEmail, 0, atPos));
+#endif

other than that, sr=me.

Comment 28

[Mike Conley (:mconley) (:⚙️)]

Florian:

A few issues:

1. When I go to the Advanced Address Book Search, I'm still seeing "Screen Name" as opposed to "Chat Name" in the field list.

2. The "Screen Name" field doesn't seem to map to the Chat Name at all - if I have a contact with an ICQ # of 12345 (and no other chat fields), searching for a "Screen Name" of 12345 returns no results.

3. Exporting to LDIF does not seem to include the chat fields.

Are the above known / expected?

-Mike

Comment 29

[Florian Quèze [:florian]]

(In reply to Mike Conley (:mconley) from comment #28)
> Florian:
> 
> A few issues:
> 
> 1. When I go to the Advanced Address Book Search, I'm still seeing "Screen
> Name" as opposed to "Chat Name" in the field list.
> 
> 2. The "Screen Name" field doesn't seem to map to the Chat Name at all - if
> I have a contact with an ICQ # of 12345 (and no other chat fields),
> searching for a "Screen Name" of 12345 returns no results.
> 
> 3. Exporting to LDIF does not seem to include the chat fields.
> 
> Are the above known / expected?

Yes.

I mentioned 1. and 2 in comment 13:

> I can't find a way to make the "advanced address book search" dialog search
> on the "Chat Name" instead of the (AIM) "Screen Name". The code at
> http://mxr.mozilla.org/comm-central/source/mail/base/content/ABSearchDialog.
> js#155 seems to only be able to search on one (real) property at once.


3. import/export of chat fields isn't implemented yet. David's comment 5 seemed to indicate that he was OK with us handling that in a follow-up:

> (In reply to Florian Quèze from comment #4)
[...]
> > Do you have some feedback about what should be done for all the things that
> > currently use the (aim) screenname property? Do I need to add all the new
> > properties in each list where the screenname field was present?
> 
> those mostly seem to have to do with import/export/vcard kind of things.
> None of that seems relevant, though eventually I would think that the
> various screen names should survive an export as ldif and subsequent import.

Comment 30

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 629727 [details] [diff] [review]
Patch v2

Okie doke, since those issues are known, I'm otherwise happy with the UI.

Comment 31

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 629727 [details] [diff] [review]
Patch v2

Review of attachment 629727 [details] [diff] [review]:
-----------------------------------------------------------------

Overall, this looks pretty good. Just a few nits / questions.

::: mail/base/content/ABSearchDialog.xul
@@ +97,2 @@
>                     hidden="true"
> +                   persist="hidden ordinal width sortDirection"  flex="1" label="&ChatName.label;"/>

nit: extra space after persist attribute

::: mail/components/addrbook/content/abCardOverlay.js
@@ +889,5 @@
> + * the first field if none of them has a value.
> + */
> +function showChat()
> +{
> +  document.getElementById('abTabPanels').parentNode.selectedTab =

Why .parentNode instead of just setting document.getElementById('abTabPanels').selectedTab?

::: mail/components/addrbook/content/abCardOverlay.xul
@@ +401,5 @@
> +            <label control="Skype" value="&Skype.label;"
> +                   accesskey="&Skype.accesskey;"/>
> +            <hbox class="CardEditWidth">
> +              <textbox id="Skype" flex="1" onchange="updateChatName();"/>
> +          </hbox>

Nit: misaligned </hbox> here.

::: mail/components/addrbook/content/abCardViewOverlay.js
@@ +317,5 @@
>  
> +    // Chat section
> +    visible = cvSetNodeWithLabel(data.cvGtalk, zGtalk,
> +                                 card.getProperty("_GoogleTalk"));
> +    visible = cvSetNodeWithLabel(data.cvAIM, zAIM,

This looks a bit funny until I see the || visible at the very end of the line.

I think I'd prefer:

visible |= cvSetNodeWithLabel(...

to make it easier to know immediately what is going.

@@ +482,2 @@
>  
> +  node.textContent = text;

What about the case where !node.hasChildNodes()? Is this an unlikely case to get into? Or is this an extension point that we're eliminating?

Comment 32

[Florian Quèze [:florian]]

(In reply to Mike Conley (:mconley) from comment #31)

> ::: mail/components/addrbook/content/abCardOverlay.js
> @@ +889,5 @@
> > + * the first field if none of them has a value.
> > + */
> > +function showChat()
> > +{
> > +  document.getElementById('abTabPanels').parentNode.selectedTab =
> 
> Why .parentNode instead of just setting
> document.getElementById('abTabPanels').selectedTab?

Is there a selectedTab property on the tabpanels XUL element? https://developer.mozilla.org/en/XUL/tabpanels doesn't document one. Setting selectedPanels changes only the selected panel and keeps the previously selected tab selected.

What I really wanted is the tabbox, but it doesn't have an id: http://mxr.mozilla.org/comm-central/source/mail/components/addrbook/content/abCardOverlay.xul#22

So if you really dislike the .parentNode usage, the only other solution I see is to add an id on the tabbox node.

> ::: mail/components/addrbook/content/abCardViewOverlay.js
> @@ +317,5 @@
> >  
> > +    // Chat section
> > +    visible = cvSetNodeWithLabel(data.cvGtalk, zGtalk,
> > +                                 card.getProperty("_GoogleTalk"));
> > +    visible = cvSetNodeWithLabel(data.cvAIM, zAIM,
> 
> This looks a bit funny until I see the || visible at the very end of the
> line.
> 
> I think I'd prefer:
> 
> visible |= cvSetNodeWithLabel(...
> 
> to make it easier to know immediately what is going.

I tried hard to be consistent with the existing code. The visible = ... || visible; pattern is used in lots of places in that function. As much as I dislike it, I think introducing something different for a single section wouldn't improve readability but decrease it.

> @@ +482,2 @@
> >  
> > +  node.textContent = text;
> 
> What about the case where !node.hasChildNodes()? Is this an unlikely case to
> get into? Or is this an extension point that we're eliminating?

And this is the only case where I really couldn't resist and rewrote the whole function :).
The itch I was scratching when I started to rewrite that function is that the existing code returns "undefined" when the node doesn't exist.
Then I noticed that the rest of the code of the function is just filling in the textContent of the node (textContent is a relatively new addition to the DOM, I think it didn't exist at the time the address book was written; so at the time the code had to either create a DOM Text node if there wasn't one already, or find the existing one and update its content).

That said, if you dislike this hunk, I can just revert it; it's not really useful for fixing this bug, it was just painful to read while debugging.

Comment 33

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 629727 [details] [diff] [review]
Patch v2

Review of attachment 629727 [details] [diff] [review]:
-----------------------------------------------------------------

Naw, I don't want to slow this thing down. Nothing I found is setting off alarm bells, just mildly raised eyebrows. ;)

Your arguments are solid though, the code looks good, and the tests pass.  Fix the nits, and r=me.

Comment 34

[Florian Quèze [:florian]]

Attachment: Patch v3 (for check-in)

Removed the ifdef (comment 27) and fixed the 2 nits from comment 31.

Comment 35

[Florian Quèze [:florian]]

Thanks for the reviews!

https://hg.mozilla.org/comm-central/rev/329adfbe8317

Comment 36

[Frank]

Hi,
thanks for your job.
I linked my contacts book with my LDAP and works fine.
But I need to know how can match the one LDAP field (i.e. wWWHomePage) with the "ID Jabber" field for the Autocomplete in Contacts Book. So I can use the Contacts Book to send Instant Messages!
Can you tell me, how and what modify?
Thanks a lot.
Frank

Comment 37

[Frank]

RESOLVED BY MYSELF!
Added this in TB prefs.js.
user_pref("ldap_2.servers.default.attrmap._JabberId", "wWWHomePage");

Thanks
:D

Comment 38

[Frank]

Hi.... again.
I matched the field _JabberID with wWWHomePage and now I've the nickname but I can't send IM from the contacts book. "Instant Message" is disabled.
What's the matter?
Thx