307056 - Read-only address book cards should have just a ok button that doesn't try to update the card

Status: VERIFIED FIXED

(MailNews Core :: Address Book, defect)

Description

[Mark Banner (:standard8)]

Currently, when we display read-only cards (i.e. LDAP address books) we display
them with both OK and Cancel buttons. This should really be just a close button.

At the moment when we click ok, it's accepted but not acted on due to
nsAbLDAPCard
(http://lxr.mozilla.org/seamonkey/source/mailnews/addrbook/src/nsAbLDAPCard.cpp#52)
which returns just NS_OK for the edit function and doesn't do anything with it.

However, bug 64305 calls for processing on the OK button to check the email
address is valid. If it is not valid it will put a dialog up which in the case
of read-only ldap books, is pretty pointless and confusing to the user.

My proposal is to replace the ok & cancel buttons with close for a read-only
card and return NS_ERROR_NOT_IMPLEMENTED in nsAbLDAPCard. Whilst that
effectively makes nsAbLDAPCard redundant, I'm guess Dan (or whoever) may want to
use it when we implement writeable LDAP directories, and hence I think we'll
leave it in for now.

Comment 1

[Mark Banner (:standard8)]

Attachment: SeaMonkey only version 1

This works, but I'm not sure its the best way to do it.

Comment 2

[Mark Banner (:standard8)]

Attachment: SeaMonkey only version 2

Version 2 of the patch, this reuses the cancel button as a close button.

Comment 3

[Mark Banner (:standard8)]

Attachment: SeaMonkey only version 3

Version 3 - slightly different, drop the cancel button, keeping the ok, but
re-mapping it to do the same as cancel.

Take your pick or suggest a different method ;-)

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 195285 [details] [diff] [review]
SeaMonkey only version 2

We're going for version 3a.

Comment 5

[Mark Banner (:standard8)]

Attachment: SeaMonkey only version 3a (checked in, trunk and 1.8 and 1.8.0 branches)

We are going with the single ok button version (as discussed with Neil on irc).


Changes from previous version: removed unnecessary setting of default button
attribute, also changed setting of ondialogaccept to removal of that attribute
to avoid the one set in the xul.

Comment 6

[Mark Banner (:standard8)]

Attachment: Thunderbird version (checked in)

Thunderbird version of the patch.

Comment 7

[Mark Banner (:standard8)]

Updating summary from "Read-only address book cards should have just a close
button" to reflect what we are actually doing.

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 196448 [details] [diff] [review]
Thunderbird version (checked in)

/cvsroot/mozilla/mail/components/addrbook/content/abCardOverlay.js,v  <-- 
abCardOverlay.js
new revision: 1.9; previous revision: 1.8

Comment 9

[Mark Banner (:standard8)]

With this change we should also now change nsAbLDAPCard::EditCardToDatabase to 
return NS_ERROR_NOT_IMPLEMENTED instead of NS_OK 
(http://lxr.mozilla.org/seamonkey/source/mailnews/addrbook/src/nsAbLDAPCard.cpp#
54).

It was done because we were incorrectly trying to edit the card.

Now we don't do that we should return NS_ERROR_NOT_IMPLEMENTED until we get 
round to implementing writable LDAP directories. As we are planning on doing 
that then I don't see any real reason to remove nsAbLDAPCard at the moment.

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 196317 [details] [diff] [review]
SeaMonkey only version 3a (checked in, trunk and 1.8 and 1.8.0 branches)

/cvsroot/mozilla/mailnews/addrbook/resources/content/abCardOverlay.js,v  <-- 
abCardOverlay.js
new revision: 1.56; previous revision: 1.55
/cvsroot/mozilla/mailnews/addrbook/resources/content/abEditCardDialog.xul,v 
<--  abEditCardDialog.xul
new revision: 1.21; previous revision: 1.20

Comment 11

[Mark Banner (:standard8)]

Attachment: Follow up patch (checked in)

This follow up patch changes NS_OK to NS_ERROR_NOT_IMPLEMENTED in
nsAbLDAPCard::EditCardToDatabase. Now we're not trying to edit cards, we can
return the "correct" value for the current code. As I said before, I think we
should leave nsAbLDAPCard in until we get a better idea on what we are doing
about writeable ldap directories.

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 196814 [details] [diff] [review]
Follow up patch (checked in)

/cvsroot/mozilla/mailnews/addrbook/src/nsAbLDAPCard.cpp,v  <-- 
nsAbLDAPCard.cpp
new revision: 1.6; previous revision: 1.5

Comment 13

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using version 1.6a1 (20051022) of Thunderbird and SeaMonkey 1.1a;Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051023 Mozilla/1.0 with prefs:

Hostname: ads.iu.edu
Base DN: dc=ads,dc=iu,dc=edu
Port number: 3269
Bind DN: ads\stdonner
[X} Use secure connection (SSL) checked

Address book LDAP cards are read-only, and are presented with grayed-out fields, and a single "Close" button.

Great work, Mark!

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 196317 [details] [diff] [review]
SeaMonkey only version 3a (checked in, trunk and 1.8 and 1.8.0 branches)

Requesting approval for checkin on branch of SeaMonkey-only patch.

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 196317 [details] [diff] [review]
SeaMonkey only version 3a (checked in, trunk and 1.8 and 1.8.0 branches)

Opps, didn't mean to set 1.0.8?, DID mean to set 1.8.0.1?

Comment 16

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Comment on attachment 196317 [details] [diff] [review]
SeaMonkey only version 3a (checked in, trunk and 1.8 and 1.8.0 branches)

a=ctho, just need one more

Comment 17

[Robert Kaiser]

Comment on attachment 196317 [details] [diff] [review]
SeaMonkey only version 3a (checked in, trunk and 1.8 and 1.8.0 branches)

approval-seamonkey1.0=me

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 196317 [details] [diff] [review]
SeaMonkey only version 3a (checked in, trunk and 1.8 and 1.8.0 branches)

Cancelling obsolete review request, approval-seamonkey1.0 given by SeaMonkey Council (see above 2 comments).

Comment 19

[Mark Banner (:standard8)]

Comment on attachment 196317 [details] [diff] [review]
SeaMonkey only version 3a (checked in, trunk and 1.8 and 1.8.0 branches)

I just checked this in the 1.8 branch for SeaMonkey1.0

Comment 20

[Mark Banner (:standard8)]

Comment on attachment 196317 [details] [diff] [review]
SeaMonkey only version 3a (checked in, trunk and 1.8 and 1.8.0 branches)

Checked in to 1.8.0 branch:
Checking in mailnews/addrbook/resources/content/abCardOverlay.js;
/cvsroot/mozilla/mailnews/addrbook/resources/content/abCardOverlay.js,v  <--  abCardOverlay.js
new revision: 1.55.8.1; previous revision: 1.55
done
Checking in mailnews/addrbook/resources/content/abEditCardDialog.xul;
/cvsroot/mozilla/mailnews/addrbook/resources/content/abEditCardDialog.xul,v  <--  abEditCardDialog.xul
new revision: 1.20.30.1; previous revision: 1.20

Comment 21

[Mark Banner (:standard8)]

Comment on attachment 196448 [details] [diff] [review]
Thunderbird version (checked in)

Requesting approval for 1.8.1 branch. Simple patch to improve ldap address book card dialogs. This will be needed to allow bug 64305 & others into the branch that we'll want for trunk synchronisation for birthday & anniversary fields if we implement them.