Closed Bug 953533 Opened 10 years ago Closed 10 years ago

No prompt for confirmation when deleting an account

Categories

(Instantbird Graveyard :: Account manager, enhancement)

0.1.2
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: romain, Assigned: romain)

References

Details

Attachments

(1 file, 1 obsolete file)

*** Original post on bio 82 at 2008-08-09 21:25:00 UTC ***

When we delete an account on account manager, it would be nice to have confirm prompt, with a checkbox in order to hide this prompt next time.
Attached patch Added a confirmCheck prompt (obsolete) — Splinter Review
*** Original post on bio 82 as attmnt 40 at 2008-08-09 21:27:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351784 - Flags: review?
Comment on attachment 8351784 [details] [diff] [review]
Added a confirmCheck prompt

*** Original change on bio 82 attmnt 40 at 2008-08-09 22:07:34 UTC ***

>Index: instantbird/mozilla/instantbird/locales/en-US/chrome/instantbird/accounts.properties

>+account.deletePrompt.title=Delete account ?"
>+account.deletePrompt.message=Do you really want to delete this account ?
No space before '?' in English.

I think I would prefer "Are you sure you want to delete this account?"

>+account.deletePrompt.checkbox=Don't show this message again
"Do not ask next time." (peer discussion on IRC)

>Index: instantbird/mozilla/instantbird/base/content/instantbird/account.xml
>===================================================================
>--- instantbird/mozilla/instantbird/base/content/instantbird/account.xml	(revision 259)
>+++ instantbird/mozilla/instantbird/base/content/instantbird/account.xml	(working copy)
>@@ -159,9 +159,35 @@
>      <method name="delete">
>       <body>
>       <![CDATA[
>+        var bundle = document.getElementById("accountsBundle");
>+
>         var pcs = Components.classes["@instantbird.org/purple/core;1"]
>                             .getService(Ci.purpleICoreService);
>-        pcs.deleteAccount(this._account.id);
>+
>+        var prefs = Components.classes["@mozilla.org/preferences-service;1"]
>+                            .getService(Components.interfaces.nsIPrefBranch);
Please fix the indentation here.


>+
>+        var showPrompt = prefs.getBoolPref("messenger.accounts.promptOnDelete");
>+
>+        if (showPrompt) {
>+          var prompts = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
>+                              .getService(Components.interfaces.nsIPromptService);
Ditto. The . should be aligned.

>+
>+          var checkbox = {};
>+
>+          var promptTitle    = bundle.getString("account.deletePrompt.title");
>+          var promptMessage  = bundle.getString("account.deletePrompt.message");
>+          var promptCheckbox = bundle.getString("account.deletePrompt.checkbox");
>+
>+          if (prompts.confirmCheck(window, promptTitle, promptMessage, promptCheckbox, checkbox)) {
Just return if it doesn't return true, and you don't need the else.

>+            pcs.deleteAccount(this._account.id);
Please move the "var pcs = ..." line so that it keeps being just before this line.

>+            if (checkbox.value)
>+              prefs.setBoolPref("messenger.accounts.promptOnDelete", false);
>+          }
>+        }
>+        else
>+          pcs.deleteAccount(this._account.id);

Looks good otherwise.

For the next diff, please cd instantbird/mozilla so that the mozilla folder is the current folder.
Attachment #8351784 - Flags: review? → review-
Assignee: florian → romain
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Other → All
Target Milestone: --- → 0.2
Version: trunk → 0.1.2
Attached patch patch V2Splinter Review
*** Original post on bio 82 as attmnt 41 at 2008-08-09 22:34:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Comment on attachment 8351784 [details] [diff] [review]
Added a confirmCheck prompt

*** Original change on bio 82 attmnt 40 at 2008-08-09 22:34:47 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8351784 - Attachment is obsolete: true
Comment on attachment 8351785 [details] [diff] [review]
patch V2

*** Original change on bio 82 attmnt 41 at 2008-08-09 22:48:10 UTC ***

>+        var bundle = document.getElementById("accountsBundle");

Should be placed just before it is used, so that it is not executed if the pref says we shouldn't prompt...

>         var pcs = Components.classes["@instantbird.org/purple/core;1"]
>                             .getService(Ci.purpleICoreService);
>+
>         pcs.deleteAccount(this._account.id);
We don't need this empty line. Just need picking anyway...

r=me (I'll fix the nits before landing it)

Thanks!
Attachment #8351785 - Flags: review+
Status: NEW → ASSIGNED
*** Original post on bio 82 at 2008-08-13 21:43:20 UTC ***

Sending        instantbird/app/profile/all-instantbird.js
Sending        instantbird/base/content/instantbird/account.xml
Sending        instantbird/locales/en-US/chrome/instantbird/accounts.properties
Transmitting file data ...
Committed revision 269.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Hardware: x86 → All
Resolution: --- → FIXED
*** Original post on bio 82 at 2008-09-11 17:49:32 UTC ***

Comment on attachment 8351785 [details] [diff] [review] (bio-attmnt 41)
patch V2

>Index: instantbird/locales/en-US/chrome/instantbird/accounts.properties
>===================================================================
>--- instantbird/locales/en-US/chrome/instantbird/accounts.properties	(revision 259)
>+++ instantbird/locales/en-US/chrome/instantbird/accounts.properties	(working copy)
>@@ -1,6 +1,9 @@
> account.connection.error=Error: %S
> account.connection.progress=Connecting: %S...
> account.connecting=Connecting...
>+account.deletePrompt.title=Delete account?"

Hmm, would be better without the trailing quote...
Depends on: 953566
Target Milestone: 0.2 → 0.1.3
You need to log in before you can comment on or make changes to this bug.