384691 - Bookmarks from Apple Address Book should display company / organization where Company option is selected

Status: RESOLVED FIXED

(Camino Graveyard :: Bookmarks, enhancement)

Description

[Jim Killock]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.10) Gecko/20070228 Camino/1.0.4
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.10) Gecko/20070228 Camino/1.0.4

The Address Book 'Company' field should be displayed instead of first name, last name in the title column where the 'Company' option is selected on the Address Book vcard. Otherwise the information can fairly unintelligble, especially where Address Book cards have the 'Company' option ticked but an individual is still named.

Reproducible: Always

Steps to Reproduce:
1. Tick  organization option on an Apple Address Book card
2. Add a URL field as well as a first and last name of the company contact
3. Check Camino > Bookmarks > Show All Bookmarks > Address Book; card cannot be found by company name

Comment 1

[froodian (Ian Leue)]

Confirming.

Comment 2

[Chris Lawson (gone)]

I can see how this could be potentially problematic, too, though -- we probably ought to fall back on Company if Name isn't populated, but using Company for Name when both are present could lead to (for example) a bunch of "Google" or "Apple" contacts with non way I see to differentiate between them.

Comment 3

[Jim Killock]

Hi Chris,

No, only if the "company" flag is checked should the "company" field be used. This flag is to say "this card represents a company" rather than an individual. therefore, the company flag should be used. This is also how Safari's Address Book bookmarks behaves.

Hope this clarifies

Jim

Comment 4

[Chris Lawson (gone)]

Right, that makes much more sense. Thanks, Jim.

cl

Comment 5

[Chris Lawson (gone)]

From AddressBookManager.h/.m:

/*
 * Lets us have address book code and still run on 10.1.  When/if 10.1
 * support goes away, merge this into SmartBookmarkManger class.
 */

We're far past that point now. Should we do the merge as part of fixing this bug, or should I file a separate bug for that?

Taking for now, as I should have time to look into this more in the next day or two.

Comment 6

[Chris Lawson (gone)]

Looking into this a bit more...

The default Address Book has an "Apple Computer, Inc." card with the Company flag checked, but no first or last name. That shows up in Camino as "Apple Computer, Inc.", just as it does in Safari. Fine so far.

The problem shows up when I populate the first and last name fields (in my test, with "John Doe"). In Safari, it still shows up as "Apple Computer, Inc.", while in Camino, it now shows up as "John Doe".

We need to be looking at the company flag before we start looking for a name to fill in. Patch coming shortly.

Comment 7

[Chris Lawson (gone)]

I filed bug 462782 on integrating AddressBookManager into KindaSmartFolderManager (comment 5).

Comment 8

[Chris Lawson (gone)]

Attachment: fix v1.1

This oughta do it.

Comment 9

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Two things:

Chris and I talked about perhaps postpending "(Companyname)" to the display for these people, so that you know they're your contact for Companyname.

Also, "<No Name>" never appears in Localizable.strings, so it needs to be added, and this is as good a bug as any.

Comment 10

[Chris Lawson (gone)]

Jim, could you explain why you'd want to have a card with an individual's name on it but the "Company" box checked? I'm having trouble understanding the use-case here, especially why you'd do that instead of just having an individual card with a company name entered on it (but no "Company" box checked).

Thanks.


Another thing Smokey and I talked about on IRC is this edge case:

* Card "Foo" has Company box checked.
* "Foo" has no company name.
* "Foo" has a first name, last name, or both.

The patch falls back on "<No Name>" in that case. I can't imagine this situation would be arrived at intentionally, but there might be more "useful" fallbacks we could do. My argument for doing it the way I did it was that <No Name> is an obvious "hey, you should fix this thing you probably didn't mean to do" flag.

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #9)
> Chris and I talked about perhaps postpending "(Companyname)" to the display for
> these people, so that you know they're your contact for Companyname.

Ignore that; I had this bug backwards when I suggested that (I thought we were supposed to be switching to displaying the Personname, which seemed a bit silly).

Comment 12

[Chris Lawson (gone)]

Another idea Smokey tossed out that I really like is, IMO, an improvement on what Safari does: display both the person's name and the Company name, and use the Company flag to determine the order.

So, for instance, with no "Company" flag set: "Steve Jobs (Apple Inc.)"

With the "Company" flag set: "Apple Inc. (Steve Jobs)"

That way it's very clear what's going on.

Comment 13

[Chris Lawson (gone)]

Comment on attachment 345982 [details] [diff] [review]
fix v1.1

Clearing review request; I'm going to respin this with the fix for bug 462785 and better overall logic.

Comment 14

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

The drawback of doing it with no company flag sent is that you might have a lot of people in your Address Book who are personal contacts but for whom you also have, say, a work phone number or email and the company name as part of the work info.  You might also get an organization for anyone imported from an ldap server where that organization may not be relevant.  In both of these cases, you don't really care about the company name at all, at least not in the way that you care about it in the Company case (where Bar is your contact at FooCorp).  In perusing my Address Book, I happen to have a lot of cards where I've got my friends' work info and company names.

So I might have a card:

Mike Pinkerton
Google, Inc.
http://homepage.mac.com/mikepinkerton/
pinkmail@
pinkworkmail@

I really don't care where pink works in the Address Book collection, and "Mike Pinkerton (Google, Inc.) http://homepage.mac.com/mikepinkerton/" seems wrong in a way that Contact Name + Company (for a Company card) does not.  (OTOH, for a *Company* card, it seems wrong not to have the company display *at all* there.)

Mike, Stuart, can we get some guidance as to what you'd prefer here?

Comment 15

[Stuart Morgan]

I'd vote against showing the company for normal contacts.

Comment 16

[Jim Killock]

Chris asks:

Jim, could you explain why you'd want to have a card with an individual's name
on it but the "Company" box checked? 

Good question, I guess I would say that this is just the way my cards were arranged when i noticed this. Perhaps I had names added so I knew who the receptionist was, or who the main contact was.

The point is perhaps that users don't behave themselves as they *logically* ought, but they expect the software to behave as they indicated ;-)

If the 'company' flag is checked, then that I would say is an indication that the company name is the prime indentifier, not the person name, so that should display against a URL in Camino, because that (Company > URL) is more likely to be recognisable.

Comment 17

[Chris Lawson (gone)]

Attachment: fix v1.2

This oughta do it.

Note: strings change.

Comment 18

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

> Note: strings change.

(Probably) needs to be in by beta, then.

Comment 19

[Chris Lawson (gone)]

Attachment: fix v1.21

Whoops. The previous patch would put "<No Name>" in for Company cards. That's obviously not good :-p

Comment 20

[Stuart Morgan]

If this makes it before beta, great, but it's not going to block.

Comment 21

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #20)
> If this makes it before beta, great, but it's not going to block.

Moving this to the b2 flag for l10n tracking.

Comment 22

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 349930 [details] [diff] [review]
fix v1.21


>Index: camino/resources/localized/English.lproj/Localizable.strings.in
>+"<No Name>" = "<No Name>"; /* Used when no name is found for an Address Book smart bookmark */
>+"<No Company Name>" = "<No Company Name>"; /* Used when no company name is found for an Address Book smart bookmark */

Can you put the comments on the lines before the strings?  Also, can you improve the comment for Company Name, so that it's clear to the l10n team that they'll only be seeing it when they have a Company Card with no company name?

Comment 23

[Christopher Henderson]

Comment on attachment 349930 [details] [diff] [review]
fix v1.21

>+  NSString* homepage = nil;

Do we need to set homepage to nil, or can we just declare it?

>     // |kABHomePageProperty| is depricated on Tiger, look for the new property first and then
>     // the old one (as the old one is present but no longer updated by ABook).

Might as well fix the spelling of deprecated, and make the comment conform to proper grammar.

Comment 24

[Chris Lawson (gone)]

Attachment: fix v1.22

Addresses Smokey's and hendy's comments.

Comment 25

[Chris Lawson (gone)]

Attachment: Address Book archive of tests

Smokey reminded me I should document my tests somewhere, too, so here's what I used to test this patch:

1) a personal card with nothing in it but a URL (no first name, no last name, no company name)
2) a personal card for "Joe Person" with the company filled in
3) a personal card for "Joe Person" with the company filled in and the "last name first" flag set on the card ("Reorder Last Name Before First" in the Card menu)
4) a personal card for "John Individual" with no company
5) a personal card for "John Individual" with no company and the "last name first" flag set as in #3
6) a personal card for a nameless individual who works for "Employer Inc." (which Address Book will "helpfully" turn into a company card for you, so if you create one of these manually you'll need to go back to the card and manually un-check the company box) This displays in Address Book with the company name, which is really damned confusing to me, since the "company" box isn't checked.

7) a company card with nothing in it but a URL and the company box checked (no company name, no first or last name)
8) a company card for Foobar Inc. with no person name (the name ordering flag, as expected, makes no difference here)
9) a company card for Foobar Inc. with "John Companyman" as the person name
10) a company card for Barfoo Inc. with "John Companyman" as the person name and the "last name first" flag set as in #3
11) a company card for a nameless company with "Joe Employee" as the person name
12) a company card for a nameless company with "Joe Employee" as the person name and the "last name first" flag set as in #3

13) a personal card with only a first name
14) a personal card with only a last name
15) #13 with a company name set
16) #14 with a company name set
17) a company card with a company name and only a first name
18) a company card with a company name and only a last name

The attached zip file contains all of the above examples for anyone who wants to download it and test.

Comment 26

[Stuart Morgan]

Comment on attachment 352266 [details] [diff] [review]
fix v1.22

>+          name = [NSString stringWithFormat:@"%@ (%@)", company, name];

Does everyone use parens the way we do in English? This should probably be a localizable string, with "%1$@ (%2$@)" as the English version.

>-        if (!name)
>-          name = NSLocalizedString(@"<No Name>", nil);
>+        name = [NSString stringWithFormat:@"%@", (name ? name : NSLocalizedString(@"<No Name>", nil))];

This was better the way it was.

sr=smorgan with those changes.

Comment 27

[Stuart Morgan]

Comment on attachment 352266 [details] [diff] [review]
fix v1.22

Actually, one other thing:

>+          if (nameOrderFlag == kABFirstNameFirst)
>+            name = [NSString stringWithFormat:@"%@ %@", firstName, lastName];
>+          else if (nameOrderFlag == kABLastNameFirst)
>+            name = [NSString stringWithFormat:@"%@ %@", lastName, firstName];

Make this:
  if (nameOrderFlag == kABLastNameFirst)
    ...
  else
    ...

If there's ever a new/different flag, we don't want to stop showing names.

Comment 28

[Chris Lawson (gone)]

Attachment: fix v1.23

For checkin, addresses sr comments.

Comment 29

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Landed on cvs trunk.

Comment 30

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Additional test cards

I created these test cards when looking at this patch before checkin.

Chris also mentioned filing a rdar:// on getting the AB flags changed eventually to not use the ambiguous "FirstName" and "LastName".

Comment 31

[Chris Lawson (gone)]

(In reply to comment #30)
> Chris also mentioned filing a rdar:// on getting the AB flags changed
> eventually to not use the ambiguous "FirstName" and "LastName".

rdar://6440206 filed on deprecating the usage of "firstname" and "lastname" throughout the AddressBook framework, and on replacing them with "givenname" and "familyname" as applicable.