17230 - Missing address book property menu item - can't rename AB name

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Address Book & Contacts, defect, P2)

Description

[pmock]

Build Date & Platform Bug Found:
 Win32 commercial seamonkey build 1999-10-25-16-m11 installed on P166 Win98
 Linux commercial seamonkey build 1999-10-25-16-m11 installed on P200 RedHat 6.1
 MacOS mozilla seamonkey build 1999-10-25-16-m11 installed on G3/400 OS 8.5.1

Overview Description:
 There is no way (that I can find) to be able to edit the Address Book name.  I
highlight the address book name and tried clicking on the toolbar edit button.
Nothing happen.  I select the edit menu looking for the property menu item.
It's not there.

Steps to Reproduce:
1) Start Messenger
2) From the task menu select 'address book'
   The address book opens
3) Select the File menu and choose New->Address Book
   The new address book dialog will appear
4) Enter a name and press OK
5) Highlight the newly created address book
6) Click on the Edit button
    Nothing happens
7) Select the Edit menu...
   Note, there is no property menu

Actual Results:

There's no menu item to edit the address book property

Expected Results:

There should be a way to change the address book name.

Additional Builds and Platforms Tested On:


Additional Information:
 I can not tell if the feature has been implemented to edit the Address Book.

Comment 1

[pmock]

This bug found trying to verify bug 10839 [FEATURE] New/Edit Address Book dialog

Comment 2

[selmer (gone)]

Not beta2 stopper.  Marking M18.  Please let me know if you disagree.

Comment 3

[hangas]

Mass move mailnews bugs to Putterman.  Ouch.

Comment 4

[lchiang]

Luis - can you try to reproduce this on all platforms?

Comment 5

[luis rincon]

build 2000-07-18-11-m17
win98, mac8.6 and linux5.2
when push edit button nothappends.

Comment 6

[scottputterman]

there's currently no way to rename an address book in the ui.  Moving to future 
milestone.

Comment 7

[laurel]

*** Bug 61578 has been marked as a duplicate of this bug. ***

Comment 8

[fenella]

Assign to myself.

Comment 9

[scottputterman]

reassigning to racham

Comment 10

[scottputterman]

*** Bug 32021 has been marked as a duplicate of this bug. ***

Comment 11

[scottputterman]

*** Bug 60370 has been marked as a duplicate of this bug. ***

Comment 12

[Ninoschka Baca]

*** Bug 108434 has been marked as a duplicate of this bug. ***

Comment 13

[Ninoschka Baca]

Marking nsbeta1 because you should be able to change the name of your address
book. Also, when importing address books they are automatically given a name
(i.e. "Outlook Addresses") and this often is not meaninful to the user.

Comment 14

[R.K.Aa.]

*** Bug 116127 has been marked as a duplicate of this bug. ***

Comment 15

[Michael Buckland]

Discussed at 2/19/02 bug meeting w/ Eng/PjctMg/Marketing: decided to move to
1.2, and change nsbeta+ to nsbeta-

Comment 16

[jglick]

Please reconsider minusing this bug. This bug has 5 dups so far. The Address 
Book has really improved for MachV. Users can add/edit/delete, including rename, 
LDAP directories now. They should be able to rename local ABs as well, a basic 
feature. Currently the "Properties" button and menu items are enabled when a 
local AB has focus, but nothing happens when user selects the button/menu item. 
This is bad. Also as nbaca wrote in comment #13, we can now import ABs from 
other apps; not letting users rename those imported ABs is bad.

Comment 17

[scottputterman]

reassigning to srilatha.  Seth mentioned that you are working near that code for
the ldap properties dialog.

Comment 18

[saari (gone)]

nsbeta1-, because it isn't a high priority

Comment 19

[saari (gone)]

marked as per ADT

Comment 20

[jglick]

Removing minus so this bug might be considered for rtm.

Comment 21

[Lori Kaplan]

reassigning. UI team wants this bug fixed for rtm. 

Comment 22

[Vladimir]

This only one bug in a set of related bugs.

In order to complete UI for the address book we also need

1. Enable/disable Edit|Copy, Edit|Paste and Edit|Cut depending on card selection.
2. In the menu and toolbar buttons tips indicate that the selected operation
   will be applied to the card or book.
3. There also should be a context menu for both panes: "Address Books"
   and card list.

Comment 23

[R.K.Aa.]

*** Bug 144974 has been marked as a duplicate of this bug. ***

Comment 24

[(not reading, please use [e-mail] instead)]

some comments:

At this time, certain addressbooks can't be renamed (or for that matter, 
deleted):  Personal Addressbook, Collected Addressbook.

shuehan, if you get to this after 
http://bugzilla.mozilla.org/show_bug.cgi?id=124007 (fix dir pane sorting), we 
might need make sure we do the right thing so that on rename causes the 
addressbook to update and change position immediately (instead of upon restart).


Also, don't forget about LDAP addressbooks.

Are we going to allow the user to rename mailing lists in the same way?  or 
should that be spun off to another bug?

Comment 25

[jglick]

>Are we going to allow the user to rename mailing lists in the same way?  or 
>should that be spun off to another bug?

Mailing Lists can already be renamed. Selected the mailing list and double click 
or select Properties. Mailing List dialog opens and lets you rename it.

Comment 26

[shliang]

Attachment: patch

Comment 27

[(not reading, please use [e-mail] instead)]

Comment on attachment 85304 [details] [diff] [review]
patch

1)

+	// the id of the directory used in prefs
+	attribute wstring dirId;

This should be:

attribute ACString dirId;

one, it will allow us to avoid allocations.  two, the pref ID is a const char
*, not a PRUnichar *,
and that will allow us to avoid conversions.

2)

-	   var directory = GetDirectoryFromURI(selectedDir);
-	   if ((directory.isMailList) || 
-	       (!(directory.operations & directory.opWrite)))
+	 if (selectedDir && 
+	    (selectedDir != kPersonalAddressbookURI) &&
+	    (selectedDir != kCollectedAddressbookURI)) {

There must be a problem with our addressbook xul or js.  Because looking at the
existing code,
button_edit (Properties Button) should be disabled when the dir tree has focus
and we've selected a mailing list, or an ldap addressbook.  but right now, that
button is always enabled.

I think we actually want the properties button (button_edit) to always be
enabled.  If we click it for the pab or the cab, it should just come up with
the name field disabled.  

we should really have:

case "button_edit":
  return true;

3)

+    else {
+      window.okCallback = sortDirectories;
+     
window.openDialog("chrome://messenger/content/addressbook/abEditAddressbook.xul
",
+			 "", 
+			 "chrome,modal=yes,resizable=no,centerscreen", 
+			 {abURI:mailingListUri, okCallback:okCallback});

I had to read the code to figure out that mailingListUri really isn't a mailing
list uri, it's the uri for the selected addressbook.

AbEditSelectedDirectory() needs some cleanup.  now that we're adding a
properties dialog for local addressbooks, the existing variable name is poorly
chosen.

var mailingListUri = GetSelectedDirectory();

that should be something like:

var selectedURI = GetSelectedDirectory();

can you clean up AbEditSelectedDirectory() since you are in it?

4)

+function sortDirectories()
+{
+  var xulSortService =
Components.classes["@mozilla.org/xul/xul-sort-service;1"].getService(Components
.interfaces.nsIXULSortService);
+  xulSortService.Sort(dirTree,
"http://home.netscape.com/NC-rdf#AddressbookSort", "ascending");
+}
+

How come we have to sort the tree manually, when the name of the mailing list
or addressbook changes?
Shouldn't our dir datasource be notifying listeners of a change?

5)

+	 style="width:28em;"> 

we should not have inline style.  this value probably looks great for en-US,
not other languages.
I think the prefered way of doing this is:

on the dialog tag, do this:

width="&dialog.width;"

and then in abEditAddressbook.dtd, define that entity as 28em

6)

since we are now allowing this properties dialog to come up for the pab and
cab, see my comment #2,
you'll have to add code that disables the textbox if the uri is one of those
two well know uris
kPersonalAddressbookURI and kCollectedAddressbookURI.

we may end up adding more to the properties dialog, so it should work for those
abs.

7)

once you make the idl change I suggested in #1, you'll have to fix these two.

+NS_IMETHODIMP nsAbDirProperty::GetDirId(PRUnichar **_retval)
+{
+  *_retval = ToNewUnicode(m_DirId);
+  return NS_OK;
+}
+
+NS_IMETHODIMP nsAbDirProperty::SetDirId(const PRUnichar *aDirId)
+{
+  m_DirId.Assign(aDirId);
+  return NS_OK;
+}

for example,

NS_IMETHODIMP nsAbDirProperty::GetDirId(nsACString & aDirId)
{
  aDirId = m_DirId;
  return NS_OK;
}


8)

+  nsString m_DirId;

this will now be nsCString m_DirId;

9)

+    rv = directory->SetDirId(ToNewUnicode(prefName));
     NS_ENSURE_SUCCESS(rv, rv);

yikes, this leaks.  ToNewUnicode() and ToNewCString() allocate.

after my other suggested fixes, I think this should become

+    rv = directory->SetDirId(prefName);
     NS_ENSURE_SUCCESS(rv, rv);

10)

+  SetDirName(aDirName);

you should check the rv of this method.

11)

+  PRUnichar *prefName;
+  GetDirId(&prefName);

you should check the rv of this.  note, this code leaks, since in your patch
GetDirId() allocates.
That will change once you make the other fixes I suggested, but you should
watch out for this.

12)

+    rv = prefs->GetBranch(nsnull, getter_AddRefs(prefBranch));

I'd add NS_ENSURE_SUCCESS(rv,rv); after this call.

13)

+  rv = prefBranch->SetCharPref(ToNewCString(nsDependentString(prefName) +
NS_LITERAL_STRING(".description")),
+				ToNewCString(nsDependentString(aDirName)));

I'd add NS_ENSURE_SUCCESS(rv,rv); after this call.

And, both of those calls to ToNewCString leak.
And, the second call to ToNewCString() will cause
you to lose any non-US-ASCII data.  (the dir name is a wstring, remember)

You should find out how we store the UCS2 (PRUnichar *) directory name is
prefs.
I think you need to use setComplextValue() [instead of SetCharPref].

Comment 28

[(not reading, please use [e-mail] instead)]

for #13, look at pref-directory.js, to see how srilatha (or dmose?) did it in 
js.

Comment 29

[shliang]

Attachment: patch

Comment 30

[(not reading, please use [e-mail] instead)]

Attachment: my preliminary review notes

here are some preliminary review notes.  

I call them preliminary because as I'm not sure what we want to do about the
issues and this bug in general.  All I do know is we want to allow users to
rename local addressbooks.

But I haven't fully thought out what the right fix should be.  Ugh, I hate the
addressbook.

We know we need to give the user a way to rename local addressbooks, we just
need to figure out the right way.

Comment 31

[(not reading, please use [e-mail] instead)]

Comment on attachment 86540 [details] [diff] [review]
patch

needs work.  I'll be out until 6-24.  either we'll resume this then, or another
reviewer can take over.

given how nasty the addressbook code is, I fear this will be waiting for me on
6-24

Comment 32

[jglick]

fyi, bug 76382 can be dumped once this is fixed.

Comment 33

[R.K.Aa.]

*** Bug 174376 has been marked as a duplicate of this bug. ***

Comment 34

[(not reading, please use [e-mail] instead)]

over to cavin.

Comment 35

[Cavin Song]

The backend fix is in bug #124059. The modifyAddressBook() method works for 
normal address books as well.

Comment 36

[(not reading, please use [e-mail] instead)]

Attachment: not really a patch, so don't try to apply this

Comment 37

[(not reading, please use [e-mail] instead)]

the reason for that second patch (not really a patch) is that for something 
cavin is doing (see bug #124059), he needs some of shuehan's changes.

but we don't want all of shuehan's last patch yet.

the fix for rename (once cavin lands #124059) will be to use the 
modifyAddressbook().

once #124059 lands, we can finish up this bug.

Comment 38

[(not reading, please use [e-mail] instead)]

cavin should be able to fix this now that 124069 has landed.

he's got a partial patch in his tree now.

Comment 39

[Cavin Song]

Attachment: Proposed patch, v1

UI patch of the bug. The backend patch/fix is in bug 124069.

Comment 40

[(not reading, please use [e-mail] instead)]

Comment on attachment 116369 [details] [diff] [review]
Proposed patch, v1

r/sr=sspitzer

Comment 41

[(not reading, please use [e-mail] instead)]

cavin, when you land your fix, can you log the spin off issue about the 
enabling/disabling of rename/delete/properties menu items/buttons?

Comment 42

[Cavin Song]

> cavin, when you land your fix, can you log the spin off issue about the 
> enabling/disabling of rename/delete/properties menu items/buttons?
>
It is bug 196123.

Comment 43

[Cavin Song]

UI fix checked in.

Comment 44

[(not reading, please use [e-mail] instead)]

Attachment: supplimental fix

cavin, see the attached supplimental fix.

I was running into the "can't rename" alert too frequently for the PAB and CAB
(on double click of addressbooks, on Edit | Properties, on the Properties
toolbar button.

Instead of "Rename Address Book", I made the title "Address Book Properties"
(For LDAP, the dialog is "Directory Server Properties")

if the dialog is opened for PAB and CAB, the text field is disabled.

I think this is a better experience. 

can you review, and if you agree, land the patch for me?

some open issues:

1)  Do we really need a "Rename Address Book..." menu item, if there are
already several ways to get to this dialog?  (maybe, we have "File | Rename
Folder" in mail).  (spin off bug)

2)  double clicking on a PAB with mailing lists opens the properties (rename)
dialog, and expands (or collapses) the twisty.	I was hoping that
event.preventBubble() would fix this, but it doesn't seem to do the trick. 
(spin off bug)

Comment 45

[(not reading, please use [e-mail] instead)]

3)  if we keep "Rename Address Book...", should be disable for PAB and CAB. 
(like "File | Rename Folder" is for special folders, like Inbox.  (spin off bug)

Comment 46

[Cavin Song]

Comment on attachment 116394 [details] [diff] [review]
supplimental fix

r=cavin.

Comment 47

[(not reading, please use [e-mail] instead)]

cavin has (or is) going to log some spin off bugs.

Comment 48

[Cavin Song]

> 2)  double clicking on a PAB with mailing lists opens the properties (rename)
> dialog, and expands (or collapses) the twisty.	I was hoping that
> event.preventBubble() would fix this, but it doesn't seem to do the trick. 
> (spin off bug)
>
It's bug 196135.

Comment 49

[Ninoschka Baca]

Trunk build 2003-03-06: Mac 10.1.5, WinXP
Verified Fixed, since the basics are working. 

There is a problem renaming imported address books (bug# 19622).

Comment 50

[jglick]

>I was running into the "can't rename" alert too frequently for the PAB and CAB
>(on double click of addressbooks, on Edit | Properties, on the Properties
>toolbar button. Instead of "Rename Address Book", I made the title "Address
>Book Properties" (For LDAP, the dialog is "Directory Server Properties"). If
>the dialog is opened for PAB and CAB, the text field is disabled.
>I think this is a better experience. 

I agree. Is this what is currently checked in?

>Do we really need a "Rename Address Book..." menu item, if there are
>already several ways to get to this dialog? 

I Agree. Please remove File: Rename Address Book.

Since the CAB isn't a special AB anymore (we don't even create it for new
profiles) why can't the user rename it?

Comment 51

[jglick]

>Since the CAB isn't a special AB anymore (we don't even create it for new
>profiles) why can't the user rename or delete it?
I filed bug 196926 for this.

Comment 52

[Stefan Borggraefe]

*** Bug 138983 has been marked as a duplicate of this bug. ***