768005 - Add a "confirm on delete" option for address book entries.

Status: RESOLVED FIXED

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

Description

[Philip Chee]

From Thunderbird Bug 526541:

> I often accidentally delete entries via the following scenario:
> - Open Address Book
> - Start typing a name to autoscroll to that person
> - Hit Delete key on Mac keyboard to delete the last letter I typed 
>   because it was a typo
> - Oops!  Whatever address book entry I had autoscrolled to silently 
>   vanishes.
> 
> This is very easy to do accidentally since so many apps these days
> do incremental searches like this and allow Delete or Backspace to 
> delete the last search char typed.  My fingers are well trained to
> make this mistake.
>

Comment 1

[Philip Chee]

Our abCommon.js code lives around here:
<http://mxr.mozilla.org/comm-central/search?string=function+AbDelete()&find=%2Fsuite%2F&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central>

Since we don't run Mozmill tests the test additions aren't relevant to this bug. Unless someone gets Mozmill running with SeaMonkey.

Comment 2

[Liu]

I'm new here, can you give me some guide about it pls :)

Comment 3

[Philip Chee]

First you need to be able to build seamonkey. Please read <https://developer.mozilla.org/en-US/docs/Simple_SeaMonkey_build>. Also you can join us on IRC: irc://moznet/seamonkey where it's easier to ask questions and get replies. For help in getting seamonkey to build it's also useful to ask in irc://moznet/introduction .

Comment 4

[Liu]

Thank u, I'll build seamonkey first:)

Comment 5

[Philip Chee]

What platform are you on? Windows, Mac, or Linux?

Comment 6

[Liu]

it's Linux

Comment 7

[Philip Chee]

In the section for Ubuntu in <https://developer.mozilla.org/en-US/docs/Simple_SeaMonkey_build>
Instead of:
  sudo apt-get build-dep seamonkey
Use:
  
sudo apt-get build-dep thunderbird

Comment 8

[Liu]

OK Philip. I will take note of that.

Comment 9

[Liu]

Hi, I wish to start working with this bug, could you please assign it to me ? Thanks a lot.

Comment 10

[Liu]

I have a few questions about code in abCommon.js :
in line 30, what does cmd_delete mean?
and what is the usage of function AbDelete()? It is never called by others

Comment 11

[Philip Chee]

> in line 30, what does cmd_delete mean?
This is the controller for the Delete command:

http://mxr.mozilla.org/comm-central/source/suite/mailnews/addrbook/addressbook.xul#88
See:
https://developer.mozilla.org/en-US/docs/XUL/command
https://developer.mozilla.org/en-US/docs/XUL_Tutorial/Commands

> and what is the usage of function AbDelete()? It is never called by others

It is called from addressbook-panel.xul :
<menuitem label="&deleteAddrBookCard.label;" accesskey="&deleteAddrBookCard.accesskey;" oncommand="AbDelete();"/>

Comment 12

[Liu]

Attachment: Extend abDelete()

The problem of abDelete() is that there is no confirm check for deleting address book entry/entries when users accidentally press delete. So I extended abDelete() and add confirm on deletion for address book entry/entries as well.

Comment 13

[Liu]

Thanks a lot :)
(In reply to Philip Chee from comment #11)
> > in line 30, what does cmd_delete mean?
> This is the controller for the Delete command:
> 
> http://mxr.mozilla.org/comm-central/source/suite/mailnews/addrbook/
> addressbook.xul#88
> See:
> https://developer.mozilla.org/en-US/docs/XUL/command
> https://developer.mozilla.org/en-US/docs/XUL_Tutorial/Commands
> 
> > and what is the usage of function AbDelete()? It is never called by others
> 
> It is called from addressbook-panel.xul :
> <menuitem label="&deleteAddrBookCard.label;"
> accesskey="&deleteAddrBookCard.accesskey;" oncommand="AbDelete();"/>

Comment 14

[Tony Mechelynck [:tonymec]]

Comment on attachment 678372 [details] [diff] [review]
Extend abDelete()

Sorry, I have no reviewer's powers. According to http://www.seamonkey-project.org/dev/project-areas the module owner for Address Book & Contacts is Mark Banner.

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 678372 [details] [diff] [review]
Extend abDelete()

Sorry, I don't generally do SM-only reviews these days.

Comment 16

[Liu]

Hello, could you give me an advice that who can I turn to ask for review for this bug? Thank you.
(In reply to Mark Banner (:standard8) from comment #15)
> Comment on attachment 678372 [details] [diff] [review]
> Extend abDelete()
> 
> Sorry, I don't generally do SM-only reviews these days.

Comment 17

[Liu]

Hi Philip,
Are you able to review this patch? If not, can you tell me who I can request for a review? Thanks

(In reply to Philip Chee from comment #11)
> > in line 30, what does cmd_delete mean?
> This is the controller for the Delete command:
> 
> http://mxr.mozilla.org/comm-central/source/suite/mailnews/addrbook/
> addressbook.xul#88
> See:
> https://developer.mozilla.org/en-US/docs/XUL/command
> https://developer.mozilla.org/en-US/docs/XUL_Tutorial/Commands
> 
> > and what is the usage of function AbDelete()? It is never called by others
> 
> It is called from addressbook-panel.xul :
> <menuitem label="&deleteAddrBookCard.label;"
> accesskey="&deleteAddrBookCard.accesskey;" oncommand="AbDelete();"/>

Comment 18

[Philip Chee]

> Are you able to review this patch? If not, can you tell me who I can request for a
> review? Thanks
Hi! The review request flag has already been set and it is currently pointing to IanN. Please be patient. Cheers.

Comment 19

[Liu]

Oh,sorry, I did not notice that. Thanks:)

(In reply to Philip Chee from comment #18)
> > Are you able to review this patch? If not, can you tell me who I can request for a
> > review? Thanks
> Hi! The review request flag has already been set and it is currently
> pointing to IanN. Please be patient. Cheers.

Comment 20

[Ian Neal]

Comment on attachment 678372 [details] [diff] [review]
Extend abDelete()

>+  if (types == kCardsOnly ){
Nit: please remove the space between kCardsOnly and ). The { should be on its own line.
>+    
>+     if (gAbView && gAbView.selection.count<2)
Nit: you need a space either side of the <

>   // If at least one mailing list is selected then prompt users for deletion.
>+  if (types != kCardsOnly){
Use else instead of this if statement, { should be on its own line.

>+  if (confirmDeleteMessage && Services.prompt.confirm(window, null, confirmDeleteMessage))
Nit: split this over two lines (after the &&), so it does not go over 80 characters in a line.

r- for the moment so that I can review the new patch.

Comment 21

[Liu]

Attachment: modified according to the requirement

Comment 22

[Ian Neal]

Comment on attachment 680447 [details] [diff] [review]
modified according to the requirement

>+  if (types == kCardsOnly)
>+  {
>+     if (gAbView && gAbView.selection.count < 2)
>+       confirmDeleteMessage = "Are you sure you want to delete the selected contact?";
>+     else 
>+       confirmDeleteMessage = "Are you sure you want to delete the selected contacts?";
>+  }
Sorry, didn't spot before you have indented by one space too many, so you have 5 and 7 rather than 4 and 6.

>+  {   
Nit: You have some spaces after the {, please remove them.
>+     if (types == kListsAndCards)
>+       confirmDeleteMessage = gAddressBookBundle.getString("confirmDeleteListsAndContacts");
>+     else if (types == kMultipleListsOnly)
>+       confirmDeleteMessage = gAddressBookBundle.getString("confirmDeleteMailingLists");
>+     else if (types == kSingleListOnly)
>+       confirmDeleteMessage = gAddressBookBundle.getString("confirmDeleteMailingList");
Again, didn't spot before you have indented by one space too many, so you have 5 and 7 rather than 4 and 6.

Almost there, r- so I can check the next patch.

Comment 23

[Liu]

Attachment: remove useless spaces

Comment 24

[Liu]

Attachment: remove useless spaces

Comment 25

[Liu]

Hi Neal, is the latest patch in a correct format?

Comment 26

[Philip Chee]

> # HG changeset patch
> # User liu weiran <[e-mail]>
> # Date 1352138705 -28800
> # Node ID baf659340921df3a0e24f19ab04d3477b58ba8ef
> # Parent  ed60a8f558aa4bc9329fef1d85db2ae4485db2c5
> extended abDelte()

Looks fine except for "extended abDelte()"
Normally we put the bug summary in the commit comment e.g.

Bug 768005 - Add a "confirm on delete" option for address book entries.

Comment 27

[Liu]

OK, I'll get a change right now

(In reply to Philip Chee from comment #26)
> > # HG changeset patch
> > # User liu weiran <[e-mail]>
> > # Date 1352138705 -28800
> > # Node ID baf659340921df3a0e24f19ab04d3477b58ba8ef
> > # Parent  ed60a8f558aa4bc9329fef1d85db2ae4485db2c5
> > extended abDelte()
> 
> Looks fine except for "extended abDelte()"
> Normally we put the bug summary in the commit comment e.g.
> 
> Bug 768005 - Add a "confirm on delete" option for address book entries.

Comment 28

[Liu]

Attachment: Add a "confirm on delete" option for address book entries

So I simply remove the "extend abDelete()" from my patch. Is this version alright now?

Comment 29

[Liu]

Attachment: Add a "confirm on delete" option for address book entries

Comment 30

[Liu]

Attachment: Add a "confirm on delete" option for address book entries

I'm sorry, I was confused by an old patch for my last two tries of submission.
This is the new patch.

Comment 31

[Liu]

I think now the patch is alright :)

Comment 32

[Ian Neal]

Comment on attachment 681538 [details] [diff] [review]
Add a "confirm on delete" option for address book entries

I'm not sure how you created this patch, but it doesn't apply. Make sure you use diff rather than editing by hand.

>+      confirmDeleteMessage = "Are you sure you want to delete the selected contact?";
>+      confirmDeleteMessage = "Are you sure you want to delete the selected contacts?";
These two strings need to be added to addressBook.properties file, with IDs of confirmDeleteContact and confirmDeleteContacts - see http://mxr.mozilla.org/comm-central/source/mail/locales/en-US/chrome/messenger/addressbook/addressBook.properties#36 for how TB does it.
Then you can follow what TB did - see http://mxr.mozilla.org/comm-central/source/mail/components/addrbook/content/abCommon.js#267 onwards.

Comment 33

[Liu]

I'm not sure the location of addressBook.properties file of Seamonkey

(In reply to Ian Neal from comment #32)
> Comment on attachment 681538 [details] [diff] [review]
> Add a "confirm on delete" option for address book entries
> 
> I'm not sure how you created this patch, but it doesn't apply. Make sure you
> use diff rather than editing by hand.
> 
> >+      confirmDeleteMessage = "Are you sure you want to delete the selected contact?";
> >+      confirmDeleteMessage = "Are you sure you want to delete the selected contacts?";
> These two strings need to be added to addressBook.properties file, with IDs
> of confirmDeleteContact and confirmDeleteContacts - see
> http://mxr.mozilla.org/comm-central/source/mail/locales/en-US/chrome/
> messenger/addressbook/addressBook.properties#36 for how TB does it.
> Then you can follow what TB did - see
> http://mxr.mozilla.org/comm-central/source/mail/components/addrbook/content/
> abCommon.js#267 onwards.

Comment 34

[Philip Chee]

Ours is located here:
http://mxr.mozilla.org/comm-central/source/suite/locales/en-US/chrome/mailnews/addressbook/addressBook.properties

Comment 35

[Liu]

Attachment: Add a "confirm on delete" option for address book entries

I've modified both abCommon.js and addressBook.properties, so that the confirm messages of contact/contacts are directly invoked from the two new lines in addressBook.properties.

Comment 36

[Liu]

By the way, I put changes to both files in the same patch.

Comment 37

[Philip Chee]

> By the way, I put changes to both files in the same patch.
That's how it's usually done. ;)

Comment 38

[Ian Neal]

Comment on attachment 681979 [details] [diff] [review]
Add a "confirm on delete" option for address book entries

r=me
Thanks for your work on this.

Comment 39

[Liu]

You are welcome :)
Thanks for the guides & help

(In reply to Ian Neal from comment #38)
> Comment on attachment 681979 [details] [diff] [review]
> Add a "confirm on delete" option for address book entries
> 
> r=me
> Thanks for your work on this.

Comment 40

[Philip Chee]

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/73ac92981a1b