Last Comment Bug 307056 - Read-only address book cards should have just a ok button that doesn't try to update the card
: Read-only address book cards should have just a ok button that doesn't try to...
Status: VERIFIED FIXED
: fixed-seamonkey1.0, fixed-seamonkey1.1a, fixed1.8.1
Product: MailNews Core
Classification: Components
Component: Address Book (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Mark Banner (:standard8) (afk until 26th July)
:
Mentors:
Depends on:
Blocks: 64305
  Show dependency treegraph
 
Reported: 2005-09-04 14:15 PDT by Mark Banner (:standard8) (afk until 26th July)
Modified: 2008-07-31 04:30 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
SeaMonkey only version 1 (2.68 KB, patch)
2005-09-06 13:02 PDT, Mark Banner (:standard8) (afk until 26th July)
no flags Details | Diff | Splinter Review
SeaMonkey only version 2 (2.83 KB, patch)
2005-09-08 09:57 PDT, Mark Banner (:standard8) (afk until 26th July)
no flags Details | Diff | Splinter Review
SeaMonkey only version 3 (1.60 KB, patch)
2005-09-08 09:59 PDT, Mark Banner (:standard8) (afk until 26th July)
no flags Details | Diff | Splinter Review
SeaMonkey only version 3a (checked in, trunk and 1.8 and 1.8.0 branches) (2.05 KB, patch)
2005-09-16 05:39 PDT, Mark Banner (:standard8) (afk until 26th July)
neil: review+
neil: superreview+
Details | Diff | Splinter Review
Thunderbird version (checked in) (803 bytes, patch)
2005-09-17 12:11 PDT, Mark Banner (:standard8) (afk until 26th July)
mozilla: review+
mozilla: superreview+
mscott: approval1.8.1+
Details | Diff | Splinter Review
Follow up patch (checked in) (695 bytes, patch)
2005-09-20 11:54 PDT, Mark Banner (:standard8) (afk until 26th July)
mozilla: review+
mozilla: superreview+
Details | Diff | Splinter Review

Description Mark Banner (:standard8) (afk until 26th July) 2005-09-04 14:15:55 PDT
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) (afk until 26th July) 2005-09-06 13:02:19 PDT
Created attachment 195038 [details] [diff] [review]
SeaMonkey only version 1

This works, but I'm not sure its the best way to do it.
Comment 2 Mark Banner (:standard8) (afk until 26th July) 2005-09-08 09:57:55 PDT
Created attachment 195285 [details] [diff] [review]
SeaMonkey only version 2

Version 2 of the patch, this reuses the cancel button as a close button.
Comment 3 Mark Banner (:standard8) (afk until 26th July) 2005-09-08 09:59:21 PDT
Created attachment 195286 [details] [diff] [review]
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) (afk until 26th July) 2005-09-16 04:58:08 PDT
Comment on attachment 195285 [details] [diff] [review]
SeaMonkey only version 2

We're going for version 3a.
Comment 5 Mark Banner (:standard8) (afk until 26th July) 2005-09-16 05:39:50 PDT
Created attachment 196317 [details] [diff] [review]
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) (afk until 26th July) 2005-09-17 12:11:49 PDT
Created attachment 196448 [details] [diff] [review]
Thunderbird version (checked in)

Thunderbird version of the patch.
Comment 7 Mark Banner (:standard8) (afk until 26th July) 2005-09-17 12:13:04 PDT
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) (afk until 26th July) 2005-09-18 04:05:10 PDT
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) (afk until 26th July) 2005-09-19 05:00:52 PDT
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) (afk until 26th July) 2005-09-19 09:26:01 PDT
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) (afk until 26th July) 2005-09-20 11:54:27 PDT
Created attachment 196814 [details] [diff] [review]
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) (afk until 26th July) 2005-09-20 12:04:54 PDT
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] 2005-10-23 13:03:09 PDT
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) (afk until 26th July) 2005-12-03 04:27:17 PST
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) (afk until 26th July) 2005-12-03 04:29:01 PST
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 cst@andrew.cmu.edu cst@yecc.com] 2005-12-10 08:23:41 PST
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 2005-12-10 08:24:34 PST
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) (afk until 26th July) 2005-12-10 08:25:54 PST
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) (afk until 26th July) 2005-12-10 08:45:44 PST
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) (afk until 26th July) 2005-12-22 09:35:40 PST
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) (afk until 26th July) 2005-12-30 14:20:54 PST
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.

Note You need to log in before you can comment on or make changes to this bug.