Last Comment Bug 768005 - Add a "confirm on delete" option for address book entries.
: Add a "confirm on delete" option for address book entries.
Status: RESOLVED FIXED
[good first bug][mentor=IanN][lang=js...
:
Product: SeaMonkey
Classification: Client Software
Component: MailNews: Address Book & Contacts (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.16
Assigned To: Liu
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-25 08:20 PDT by Philip Chee
Modified: 2012-11-19 09:51 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Extend abDelete() (2.10 KB, patch)
2012-11-05 10:10 PST, Liu
iann_bugzilla: review-
Details | Diff | Review
modified according to the requirement (2.10 KB, patch)
2012-11-11 08:55 PST, Liu
iann_bugzilla: review-
Details | Diff | Review
remove useless spaces (2.09 KB, text/plain)
2012-11-12 23:25 PST, Liu
no flags Details
remove useless spaces (2.09 KB, patch)
2012-11-12 23:27 PST, Liu
no flags Details | Diff | Review
Add a "confirm on delete" option for address book entries (2.06 KB, patch)
2012-11-14 09:22 PST, Liu
no flags Details | Diff | Review
Add a "confirm on delete" option for address book entries (2.06 KB, patch)
2012-11-14 09:23 PST, Liu
no flags Details | Diff | Review
Add a "confirm on delete" option for address book entries (2.04 KB, patch)
2012-11-14 09:35 PST, Liu
iann_bugzilla: review-
Details | Diff | Review
Add a "confirm on delete" option for address book entries (3.15 KB, patch)
2012-11-15 06:55 PST, Liu
iann_bugzilla: review+
Details | Diff | Review

Description Philip Chee 2012-06-25 08:20:46 PDT
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 2012-06-25 11:17:35 PDT
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 2012-08-18 04:18:20 PDT
I'm new here, can you give me some guide about it pls :)
Comment 3 Philip Chee 2012-08-18 08:17:03 PDT
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 2012-08-18 08:20:30 PDT
Thank u, I'll build seamonkey first:)
Comment 5 Philip Chee 2012-08-18 08:47:00 PDT
What platform are you on? Windows, Mac, or Linux?
Comment 6 Liu 2012-08-18 08:55:00 PDT
it's Linux
Comment 7 Philip Chee 2012-08-18 20:15:03 PDT
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 2012-08-18 23:44:46 PDT
OK Philip. I will take note of that.
Comment 9 Liu 2012-11-02 09:03:25 PDT
Hi, I wish to start working with this bug, could you please assign it to me ? Thanks a lot.
Comment 10 Liu 2012-11-05 08:02:42 PST
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 2012-11-05 09:01:10 PST
> 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 2012-11-05 10:10:15 PST
Created attachment 678372 [details] [diff] [review]
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 2012-11-05 10:19:42 PST
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] 2012-11-05 17:41:07 PST
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) 2012-11-05 22:38:21 PST
Comment on attachment 678372 [details] [diff] [review]
Extend abDelete()

Sorry, I don't generally do SM-only reviews these days.
Comment 16 Liu 2012-11-06 01:30:48 PST
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 2012-11-06 05:26:50 PST
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 2012-11-06 11:31:00 PST
> 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 2012-11-06 22:26:00 PST
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 2012-11-10 16:53:06 PST
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 2012-11-11 08:55:35 PST
Created attachment 680447 [details] [diff] [review]
modified according to the requirement
Comment 22 Ian Neal 2012-11-12 14:17:12 PST
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 2012-11-12 23:25:54 PST
Created attachment 680961 [details]
remove useless spaces
Comment 24 Liu 2012-11-12 23:27:59 PST
Created attachment 680963 [details] [diff] [review]
remove useless spaces
Comment 25 Liu 2012-11-14 08:05:40 PST
Hi Neal, is the latest patch in a correct format?
Comment 26 Philip Chee 2012-11-14 08:47:19 PST
> # HG changeset patch
> # User liu weiran <liuweiran.nus@gmail.com>
> # 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 2012-11-14 08:57:02 PST
OK, I'll get a change right now

(In reply to Philip Chee from comment #26)
> > # HG changeset patch
> > # User liu weiran <liuweiran.nus@gmail.com>
> > # 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 2012-11-14 09:22:13 PST
Created attachment 681529 [details] [diff] [review]
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 2012-11-14 09:23:36 PST
Created attachment 681531 [details] [diff] [review]
Add a "confirm on delete" option for address book entries
Comment 30 Liu 2012-11-14 09:35:02 PST
Created attachment 681538 [details] [diff] [review]
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 2012-11-14 10:53:51 PST
I think now the patch is alright :)
Comment 32 Ian Neal 2012-11-14 14:07:56 PST
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 2012-11-14 23:27:41 PST
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 35 Liu 2012-11-15 06:55:17 PST
Created attachment 681979 [details] [diff] [review]
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 2012-11-15 18:40:48 PST
By the way, I put changes to both files in the same patch.
Comment 37 Philip Chee 2012-11-15 21:17:31 PST
> By the way, I put changes to both files in the same patch.
That's how it's usually done. ;)
Comment 38 Ian Neal 2012-11-18 09:37:05 PST
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 2012-11-19 01:48:51 PST
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 2012-11-19 09:51:39 PST
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/73ac92981a1b

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