Closed Bug 768005 Opened 7 years ago Closed 7 years ago

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

Categories

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

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.16

People

(Reporter: philip.chee, Assigned: liuweiran.nus)

Details

(Whiteboard: [good first bug][mentor=IanN][lang=js/L10n][level=apprentice])

Attachments

(3 files, 5 obsolete files)

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.
>
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.
I'm new here, can you give me some guide about it pls :)
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 .
Thank u, I'll build seamonkey first:)
What platform are you on? Windows, Mac, or Linux?
it's Linux
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
OK Philip. I will take note of that.
Hi, I wish to start working with this bug, could you please assign it to me ? Thanks a lot.
Assignee: nobody → liuweiran.nus
Status: NEW → ASSIGNED
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
> 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();"/>
Attached patch Extend abDelete() (obsolete) — Splinter Review
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.
Attachment #678372 - Flags: review?(antoine.mechelynck)
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 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.
Attachment #678372 - Flags: review?(antoine.mechelynck) → review?(mbanner)
Comment on attachment 678372 [details] [diff] [review]
Extend abDelete()

Sorry, I don't generally do SM-only reviews these days.
Attachment #678372 - Flags: review?(mbanner) → review?(iann_bugzilla)
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.
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();"/>
> 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.
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 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.
Attachment #678372 - Flags: review?(iann_bugzilla) → review-
Attachment #680447 - Flags: review?(iann_bugzilla)
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.
Attachment #680447 - Flags: review?(iann_bugzilla) → review-
Attached file remove useless spaces (obsolete) —
Attachment #680961 - Flags: review?(iann_bugzilla)
Attached patch remove useless spaces (obsolete) — Splinter Review
Attachment #680961 - Attachment is obsolete: true
Attachment #680961 - Flags: review?(iann_bugzilla)
Attachment #680963 - Flags: review?(iann_bugzilla)
Hi Neal, is the latest patch in a correct format?
> # 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.
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.
So I simply remove the "extend abDelete()" from my patch. Is this version alright now?
Attachment #678372 - Attachment is obsolete: true
Attachment #681529 - Flags: review?(iann_bugzilla)
Attachment #680963 - Attachment is obsolete: true
Attachment #681529 - Attachment is obsolete: true
Attachment #680963 - Flags: review?(iann_bugzilla)
Attachment #681529 - Flags: review?(iann_bugzilla)
Attachment #681531 - Flags: review?(iann_bugzilla)
I'm sorry, I was confused by an old patch for my last two tries of submission.
This is the new patch.
Attachment #681531 - Attachment is obsolete: true
Attachment #681531 - Flags: review?(iann_bugzilla)
Attachment #681538 - Flags: review?(iann_bugzilla)
I think now the patch is alright :)
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.
Attachment #681538 - Flags: review?(iann_bugzilla) → review-
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.
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.
Attachment #681979 - Flags: review?(iann_bugzilla)
By the way, I put changes to both files in the same patch.
> By the way, I put changes to both files in the same patch.
That's how it's usually done. ;)
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.
Attachment #681979 - Flags: review?(iann_bugzilla) → review+
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.
Keywords: checkin-needed
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/73ac92981a1b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.16
You need to log in before you can comment on or make changes to this bug.