397811 - Enable Mac OS X system address book per default and add UI

Status: VERIFIED FIXED

(MailNews Core :: Address Book, enhancement)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

While bug 203927 is fixed we could enable the system-wide address book on Mac OS X per default for the upcoming Thunderbird release. I know it's read-only but it would be a big step forward and will make a lot of users happy. If we do so, then we should add a note to the Release Notes that it's read-only for the moment.

It would be great to have access within a fresh created profile for Tb3.

Following lines we could add:

user_pref("ldap_2.servers.osx.description", "System Address Book");
user_pref(”ldap_2.servers.osx.dirType”, 3);
user_pref(”ldap_2.servers.osx.uri”, “moz-abosxdirectory:///”);

David and Scott, are you also looking forward to it?

Comment 1

[Mark Banner (:standard8)]

This should be a core bug, not Thunderbird specific.

I'm guessing as we haven't had many new bugs filed yet, then either people aren't using it, or its fairly stable. So its probably a good idea to add some kind of default setup/enabling function.

IMHO rather than adding those three lines, we should create some kind of UI to "enable/add" connection to the Mac OS X address book.

Even if we do add those lines by default, then we should still have some form of UI to add them. Reason being if the user deletes the address book (which they are allowed to) they won't be able to get it back again (except via asking how, which would be silly).

Still, I'd like to know what David and Scott think.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #1)
> IMHO rather than adding those three lines, we should create some kind of UI to
> "enable/add" connection to the Mac OS X address book.
> Even if we do add those lines by default, then we should still have some form
> of UI to add them. Reason being if the user deletes the address book (which
> they are allowed to) they won't be able to get it back again (except via asking
> how, which would be silly).

It's the first time we are using a system wide address book. Should the users really be able to delete it? It could be visible all the time. In that case we don't need an UI. Otherwise the UI menu entry will mainly be grayed out/hidden, due to more than one system address book cannot be added. If you prefer the UI part we could have a look for an added system address book when opening the appropriate sub menu "New". If none is available the menu entry can be displayed and the user can add the system address book.

Comment 3

[Mark Banner (:standard8)]

(In reply to comment #2)
> It's the first time we are using a system wide address book. Should the users
> really be able to delete it?

Yes. If I've always used Thunderbird or SeaMonkey (or whatever its called), and never used the system address book (or even moved everything across from the system address book), I shouldn't have to have my UI cluttered by a link to a system address book that I'm not going to use.

> Otherwise the UI menu entry will mainly be grayed out/hidden,
> due to more than one system address book cannot be added.
> If you prefer the UI
> part we could have a look for an added system address book when opening the
> appropriate sub menu "New". If none is available the menu entry can be
> displayed and the user can add the system address book.

The only other option I've thought of is to have a preference option somewhere of the type "enable/disable system address book". Of course this would have to be tidied into the new/delete address book routines which would be a bit more complicated.

So maybe the most reasonable option is to just have the menu entry for "New", and allow the delete.

I don't think the overhead for finding out if the address book already exists or not would be that much. If I remember correctly, the rdf resource uri is moz-abosxdirectory:/// which would be pretty simple to look up. If not, you'd have to iterate through the address books looking for one where the .uri matched to moz-abosxdirectory:/// (once bug 397202 is fixed).

Comment 4

[Mark Banner (:standard8)]

I spoke to Garry about this bug earlier. The Mac OS X address book has been on trunk for about 6 months now. Various folk know about it and I've just posted to the newsgroup to try and get some feedback.

I think we should want this for 3.0a1, and block 3.0 on it. I say want for 3.0a1 because I'm not sure if it will easily be possible. I'll be taking a look at it in the next day or so.

Comment 5

[Bryan Clark (DevTools PM) [:clarkbw]]

Ok, need to make sure these assumptions are clear and then we can come up with some kind of interface that supports it.

* We want this address book on by default
* We want people to be able to disable (remove,delete) the address book connection
* We want people to be able to enable (add, create) the address book connection

The properties option could work, making the address book similar to the personal address book such that it couldn't be deleted but the properties dialog could have an [ ] Enable section.

Though I think having a separate menu item will probably work out better for people discovering how to remove the address book if they don't want it.  As well as learning how to add it back in.  

With the menu item New -> Mac OS X Address Book, I'd separate it out from the others with a separator since it's different behavior.  No questions asked creation, just land the address book in the left pane and disable the menu item while it exists.

Longer term we'll need a better solution than those, however I think they are simple enough for now and our current goal for testing is to turn it on by default.  I think we'll have to ignore the read-only aspect for now and try to tell people who are testing.

Comment 6

[David Ascher (:davida)]

Clarifying flags.  Not going to make 3.0a1, but we need a solution for tb3.

Comment 7

[Bryan Clark (DevTools PM) [:clarkbw]]

As per Standard8's ping on IRC, using an "Enable OS X Address Book" in the File menu between New and Close should work well for mac users.  When the OS X address book is enabled a reverse "Disable ..." should be available as well which removes the address book from the list.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Doesn't it clutter the menu? When users will be using this entry? I think it will be rarely often. Can't we put an item in the New sub menu? The item should only be displayed when the system address book isn't already added. The deletion of the system address book can be done like for all the other address books.

Comment 9

[Mark Banner (:standard8)]

(In reply to comment #8)
> Doesn't it clutter the menu? When users will be using this entry? I think it
> will be rarely often. Can't we put an item in the New sub menu? The item should
> only be displayed when the system address book isn't already added. The
> deletion of the system address book can be done like for all the other address
> books.
> 
If we put it under the new menu, we have to have something that is appearing and disappearing. I'm also concerned that if the menu says "New" -> "OS X Address Book" the user is going to think they are creating a new address book rather than linking to the existing one.

So, maybe we have File -> "Enable OS X Address Book Link" or File -> New -> "OS X Address Book Link"

Even then I'd be concerned that with the New option they would think they could make more than one.

Just thinking about the option again, how about calling it "System Address Book Link" or is that simplifying it too far?

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #9)
> If we put it under the new menu, we have to have something that is appearing
> and disappearing. I'm also concerned that if the menu says "New" -> "OS X
> Address Book" the user is going to think they are creating a new address book
> rather than linking to the existing one.

Good points.

> So, maybe we have File -> "Enable OS X Address Book Link" or File -> New -> "OS
> X Address Book Link"

First one sounds good. Probably in conjunction with a checked menu item like "Use System Address Book". Personally I wouldn't use "OS X" inside this entry because if other OS will also support it, the items will be named differently. Further this string is shorter and shouldn't be too long for other locales.

Comment 11

[Bryan Clark (DevTools PM) [:clarkbw]]

I wouldn't simplify it too much, we're going to be coming back to this item later for a longer term solution.

[ ] OS X Address Book

A checkbox in the File menu for on/off and since we don't support other system adress books so I'm ok with making it obvious that this is mac only.  

I'm wondering if where we indicate that this is read-only... Not likely in the menu item since that would seem to convey changing it between read-only and write.  Is there something about the address book in the list that will indicate this?

Comment 12

[Mark Banner (:standard8)]

(In reply to comment #11)
> I'm wondering if where we indicate that this is read-only... Not likely in the
> menu item since that would seem to convey changing it between read-only and
> write.  Is there something about the address book in the list that will
> indicate this?

There's nothing in the address book list that will indicate read-only at the moment. The OS X address book won't appear in menus which would require a writable book, and if you open up the cards, then they will only have an OK button and you can't edit any of the fields. You won't be able to drag anything to the OS X address book either (it'll give you a denied feedback).

This is the same as how the LDAP address books currently work.

Comment 13

[Thorsten Hamann]

I must say I don't understand why TB must implement an own interface to the OSX system address book at all. Why not launch the OSX address book application itself when the user has chosen to use that as a default? This way the user can edit the address book in the appropriate (system) application, and the good guys working on TB don't have to worry about anything but reading from the system AB when new messages are composed.

Also, I'd "bury" the option to use the OSX system AB somewhere in the general TB configuration and launch a one-time wizard when the feature is rolled out that goes "Step 1: Thunderbird can now use your OSX Address book. Do you want to enable using it by default? (yes/no)" andm if yes, "Step 2: Do you want to export your current TB address book into your OSX system address book? (yes/no)". That way users on other platforms would never be bothered and the menus wouldn't be cluttered.

Comment 14

[Mark Banner (:standard8)]

Attachment: WIP Patch

This patch basically works, the main thing it doesn't do yet is to enable the directory by default.

There is one problem that I know of, when the user "removes" the address book by clicking the menu item, the message says do you want to remove the selected address book. I think I want to rework this menu, but if I do that, I'll cover it in a different bug.

Comment 15

[Bryan Clark (DevTools PM) [:clarkbw]]

If you just wanted to skip the dialog all together I think that will work out well.  The operation of disabling the OSX system address book is non-destructive and easy to undo/redo so I don't think we need to warn people at all.

Comment 16

[Mark Banner (:standard8)]

(In reply to comment #15)
> If you just wanted to skip the dialog all together I think that will work out
> well.  The operation of disabling the OSX system address book is
> non-destructive and easy to undo/redo so I don't think we need to warn people
> at all.

That's a good point. The only thing I would be slightly cautious about is when the OS X directory becomes writable, then we should prompt if it is the book address collections are made to, but I can cover that with a comment in the code for now.

Comment 17

[Mark Banner (:standard8)]

Attachment: Screenshot of menu option

I'm actually still working on a couple of bits on the patch, but I'm not intending this bit to change.

Comment 18

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 322076 [details]
Screenshot of menu option

Looks good!  It'll be a lot easier than following those about:config instructions.

Comment 19

[Mark Banner (:standard8)]

Attachment: The fix

I think this does everything right. My main concern with this now is that when running unit tests, if they fail, we get lots of:

2008-05-22 13:36:38.194 xpcshell[65363:10b] *** _NSAutoreleaseNoPool(): Object 0x43bfa0 of class NSCFArray autoreleased with no pool in place - just leaking
Stack: (0x9070312f 0x9060fec2 0x90acda96 0x92aceb02 0x13eb24d 0x13e600b 0x13a6943 0x13a726f 0x13a84a3 0x135e522 0x292d48 0x5a4bc0 0x5ad5f8 0x154af1 0x154d5b 0x154f08 0x15f64d 0x166126 0x1406a7 0x153665 0x104bc2 0x3340 0x154af1 0x1408dd 0x153665 0x104bc2 0x3e31 0x3eb5 0x5d7b 0x277c 0x26a9)

Which is going to make it really hard to debug address book related unit tests on mac. David, any idea what might be going on here? It seems to be happening during the running of the unit test and during shutdown.

Comment 20

[David Ascher (:davida)]

(In reply to comment #17)
> Created an attachment (id=322076) [details]
> Screenshot of menu option
> 

I wonder about changing the text to "Use OS X Address book", as I think that may be more transparent.

Comment 21

[Josh Aas]

+<!ENTITY osxAddressBook.label                           "OS X Address Book">

The proper name is "Mac OS X", not "OS X". Should probably change that.

Comment 22

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #20)
> I wonder about changing the text to "Use OS X Address book", as I think that
> may be more transparent.

Yeah, and looking now I see that most check box labels are using an action tone.  It would probably be good to keep that consistent tone.

So the new full name would become:

   [ ] Use Mac OS X Address Book

A little bit long, but hopefully it's as clear as possible.

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So why we want to show this entry within the menu? As Bryan said its a bit too long and normally it wont be touched too often. IMO it would better fit under "Preferences | Composition | Addressing". It could be placed between Local Address Books and Directory Server. Wouldn't this be a better option? Also in that way everything can be found in one tab and isn't scattered around over the UI.

Comment 24

[Mark Banner (:standard8)]

(In reply to comment #23)
> So why we want to show this entry within the menu?

I think the original suggestion that I (?) made for it to be under that menu was because of the New Address Book options being there as well. So if a user removed it, and wanted to get it back later it'd be in the "obvious" position.

Hiding it away in preferences could be a idea, but I'm not sure if "Preferences | Composition | Addressing" would be the best place - IMO its not just about Addressing messages.

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ok, you are right. It's not a good option.

Comment 26

[Neil]

Why not both?

Comment 27

[Mark Banner (:standard8)]

Attachment: The fix v2

This patch fixes the naming issues that were mentioned, also fixes a crash that could occur when deleting/removing the Mac AB interface.

The memory leak issues that I mentioned before are only at the xpcshell unit test level due to a problem with xpcshell running on the mac. Bug 435853 covers the issues and includes a fix.

Comment 28

[Mark Banner (:standard8)]

Attachment: [checked in] The fix v2a

Fixes the AB name in the unit test as well

Comment 29

[David :Bienvenu]

Comment on attachment 322638 [details] [diff] [review]
[checked in] The fix v2a

if I'm reading the diff correctly, in DIR_SetLocalizedStringPref
, the defaultValue arg is never used, presumably because the default value is localized. So can that arg be removed?

Comment 30

[David :Bienvenu]

Comment on attachment 322638 [details] [diff] [review]
[checked in] The fix v2a

I meant to provisionally r/sr this, module the comment about defaultValue.

Comment 31

[Mark Banner (:standard8)]

Comment on attachment 322638 [details] [diff] [review]
[checked in] The fix v2a

Checked in without the default parameter (as mentioned by David), and also fixed the unit tests so that OS X interface detection worked correctly cross-platform.

Comment 32

[Mark Banner (:standard8)]

Thunderbird & Core changes are in. I'm going to mark this as fixed; any volunteers to do the SM port (just the js/xul changes are needed)?

Comment 33

[Justin Wood (:Callek)]

I know I'm late to the party so to speak here.

But as a non-mac user, I do think "Use system..." instead of "Use OS X..." makes sense. In the chance that another OS ends up having a system addressbook and that we end up supporting it. (That and being on OS X already, "use OSX addressbook" just seems odd to me)

Comment 34

[Henrik Skupin [:whimboo][⌚️UTC+1]]

That's what I have already said in comment 10. But it seems that it was lost. Mark, shall we file a new bug for that naming thing? If we don't call it "System Address Book" we have to use different strings for each operating system.

Comment 35

[Mark Banner (:standard8)]

(In reply to comment #34)
> That's what I have already said in comment 10. But it seems that it was lost.
> Mark, shall we file a new bug for that naming thing? If we don't call it
> "System Address Book" we have to use different strings for each operating
> system.
> 

My argument here would be how do you define "System Address Book" on anything other than OS X? For windows, users could think of the address book built in to windows, or what their address book in outlook sees? Are they always the same thing? For Linux, I think there's various different address books we could hook into and I'm sure that KDE and Gnome have their own versions and hence I don't think it would make sense.

Comment 36

[Justin Wood (:Callek)]

My main point, that as long as OS X has only one "System address book" we should use the term "System" not "OS X".

For other OS's we can define and choose what a "System Address Book" is whenever we approach enabling this for them.  I'd argue on windows Outlook would be an import, and if Vista has a "System address book" then we could use that.  It would depend on the OS.  If we need disambiguous strings for each OS, so be it. But we should still make the mac case a bit less so when there is nothing else to select from on this scale.

Comment 37

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre) Gecko/2008060803 Thunderbird/3.0a2pre ID:2008060803

For the remaining naming question I would like to file a new bug. So we can move the discussion there.