The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in seamonkey2.16

Status

SeaMonkey
MailNews: Address Book & Contacts
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Philip Chee, Assigned: Liu)

Tracking

Trunk
seamonkey2.16

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

5 years ago
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.
>
(Reporter)

Comment 1

5 years ago
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.
(Assignee)

Comment 2

5 years ago
I'm new here, can you give me some guide about it pls :)
(Reporter)

Comment 3

5 years ago
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 .
(Assignee)

Comment 4

5 years ago
Thank u, I'll build seamonkey first:)
(Reporter)

Comment 5

5 years ago
What platform are you on? Windows, Mac, or Linux?
(Assignee)

Comment 6

5 years ago
it's Linux
(Reporter)

Comment 7

5 years ago
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
(Assignee)

Comment 8

5 years ago
OK Philip. I will take note of that.
(Assignee)

Comment 9

4 years ago
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
(Assignee)

Comment 10

4 years ago
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
(Reporter)

Comment 11

4 years ago
> 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();"/>
(Assignee)

Comment 12

4 years ago
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.
Attachment #678372 - Flags: review?(antoine.mechelynck)
(Assignee)

Comment 13

4 years ago
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)
(Assignee)

Comment 16

4 years ago
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.
(Assignee)

Comment 17

4 years ago
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();"/>
(Reporter)

Comment 18

4 years ago
> 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.
(Assignee)

Comment 19

4 years ago
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

4 years ago
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-
(Assignee)

Comment 21

4 years ago
Created attachment 680447 [details] [diff] [review]
modified according to the requirement
Attachment #680447 - Flags: review?(iann_bugzilla)

Comment 22

4 years ago
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-
(Assignee)

Comment 23

4 years ago
Created attachment 680961 [details]
remove useless spaces
Attachment #680961 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 24

4 years ago
Created attachment 680963 [details] [diff] [review]
remove useless spaces
Attachment #680961 - Attachment is obsolete: true
Attachment #680961 - Flags: review?(iann_bugzilla)
Attachment #680963 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 25

4 years ago
Hi Neal, is the latest patch in a correct format?
(Reporter)

Comment 26

4 years ago
> # 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.
(Assignee)

Comment 27

4 years ago
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.
(Assignee)

Comment 28

4 years ago
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?
Attachment #678372 - Attachment is obsolete: true
Attachment #681529 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 29

4 years ago
Created attachment 681531 [details] [diff] [review]
Add a "confirm on delete" option for address book entries
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)
(Assignee)

Comment 30

4 years ago
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.
Attachment #681531 - Attachment is obsolete: true
Attachment #681531 - Flags: review?(iann_bugzilla)
Attachment #681538 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 31

4 years ago
I think now the patch is alright :)

Comment 32

4 years ago
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-
(Assignee)

Comment 33

4 years ago
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.
(Reporter)

Comment 34

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

Comment 35

4 years ago
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.
Attachment #681979 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 36

4 years ago
By the way, I put changes to both files in the same patch.
(Reporter)

Comment 37

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

Comment 38

4 years ago
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+
(Assignee)

Comment 39

4 years ago
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.
(Reporter)

Updated

4 years ago
Keywords: checkin-needed
(Reporter)

Comment 40

4 years ago
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/73ac92981a1b
Status: ASSIGNED → RESOLVED
Last Resolved: 4 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.