274452 - When accounts are removed, files associated with that account are not deleted

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[Michael Kraft [:morac]]

When an account (mail, RSS or news) is created, a bunch of files are created and
associated with that account.  When the account is removed, the files associated
with that account remain on the disk with no way to access them or delete them.

Steps to recreate:
1. Add an RSS account (or mail or news account)
2. Add a bunch of RSS feeds (or download mail or download news articles)
3. Remove account

Actual results:
1. Data files remain in profile

Expected Results:
1. Data files should be deleted or moved into the Local Storage file area.

Comment 1

[Jo Hermans]

Seamonkey bug 128744

Comment 2

[Mike Cowperthwaite]

*** Bug 279133 has been marked as a duplicate of this bug. ***

Comment 3

[RenegadeX]

This is a great way for parents, bosses or the nosy-parker-types to find out what newsgroups other users on a computer have visited .. ever!

Comment 4

[Mike Cowperthwaite]

*** Bug 321293 has been marked as a duplicate of this bug. ***

Comment 5

[Matthias Versen [:Matti]]

*** Bug 323449 has been marked as a duplicate of this bug. ***

Comment 8

[Sean]

I have just created a bug 507352 and then I find that it has been marked at a duplicate of this bug 274452.

How can this be, this bug is 5 years old. Surely a bug this bad would of been fixed in 5 years or is it a bug that is never going to be fixed and we should all just ignore it. 5 years wow

Comment 9

[Sean]

I have since read elsewhere that "This is by Design" and will not be worked on.

In this case, could someone add the ability into Thunderbird to give you the option of also remove the files and directories of old accounts, as it is difficult to do manually by hand if you have lots of accounts as the naming convention the TB uses for the directories is poor. I have read that one person who wanted to delete some accounts had to do it manually however he deleted the wrong ones due to the naming convention and lost accounts that he actually used.

This should be done as...
1) Security and privicy tool
2) Help keep the profile compact
3) Save disk space

Comment 10

[Magnus Melin [:mkmelin]]

Patches welcome ;)

Comment 11

[Michael Kraft [:morac]]

Personally I think if it's not going to delete the files, that they should into the "Local Storage Area" so the user can at least see them and then delete them manually if desired.

I don't know what that would entail, but it's probably not that difficult.

Comment 13

[[:jberkus] Josh Berkus]

I am adding my vote that this is broken by design.  There is no way within Thunderbird to safely remove old account files.  Mine currently total over 1GB of disk space and hugely clutter up my backups.

Comment 14

[[:jberkus] Josh Berkus]

BTW, as long as this bug remains unresolved, publication of a workaround would be really, really appreciated.   Thanks!

Comment 15

[Magnus Melin [:mkmelin]]

Workaround: just go to your profile folder (http://kb.mozillazine.org/Profile_folder_-_Thunderbird) and delete the files for the account in question.

Comment 16

[Don Albertson]

I wouldn't buy a commercial product that made me manually delete files it had created.  That's sloppy and wouldn't get past any professional Q/A tester I have ever met.

Comment 18

[[:jberkus] Josh Berkus]

Magnus,

How do I identify which folders are no longer in use?  TB does not give me any way to distinguish between account folders which are in use and ones which aren't.  Given that many of my folders have names like "mail.01.com-11" it's not very indicative.

Comment 19

[[:jberkus] Josh Berkus]

Answering my own question:

1) You can find the location of active accounts by looking in Account Settings --> Server --> LocalDirectory (bottom of screen).  This can allow you to figure out which folders are active so you can delete the rest.

2) ThunderbirdPlunger AddOn will clean up a lot of deleted-stuff files.  At this time, it does NOT do IMAP folders, but if we butter up the developer maybe they'll add it.

Comment 20

[bugzilla_mozilla]

Thanks Josh for the tip. ThunderPlunger now also deleted IMAP folders.

But I can't believe we need an add-on for this. By design? I say "delete it" and Thunderbird just silently keeps it?
There should be at least a checkbox "delete all files of the account as well" or similar.
And if Thunderbird does not delete the files, there should be at least an option to recover the account.

Comment 22

[:aceman]

Bwinton, what about at least warning the user about this?

"The account <name> has been removed. Note that the files containing the messages in the <local directory path> will be preserved. Delete them if they are no longer needed." [OK]

Or, if somebody helps me how to delete whole directory from Account manager Javascript, I could implement the question and delete the files if the user chooses so. Maybe I should look into that extension.

Comment 23

[Sean]

I can not believe that this bug is still with us after 8 years. Quite a shoddy state of affairs

Comment 24

[Ronald Killmer]

Mailnews should not cause any data loss while patching this bug. Historically the remnants of a removed account have been retained even though users may have meant it when doing a delete. The retained data may have been seen as an archive method.

Ideally we will give a user the option to archive messages. Now that there is an archive function, perhaps moving the message data to an archive folder within Local Files would be a sensible method.

Comment 26

[[:jberkus] Josh Berkus]

Ronald, 

That would be an acceptable temporary solution, as it would at least make it easy to find the files which are no longer in use.  As a real solution, though, TB would need an interface to delete archive files.

Comment 27

[:aceman]

I'll look at this soon, I just need to land other improvements in the Account manager.

Comment 28

[Bryce Mozilla Nesbitt]

I think the brokenness here goes beyond what's been discussed so far.  It is definitely bad to leave a 2GB IMAP SBD file after the corresponding account is deleted.  But thunderbird mixes useless and precious files in the same directory:

* Crash Reports (useless after the moment)
* IMAP SBD files, Cache Files (can be recreated on demand)
* Application Settings (kind of in the middle)
* Local Sent Mail (might be priceless)

I'd prefer my true irreplaceable personal files be in an obviously separate directory that I'd be sure my backup software picks up.

Comment 29

[:aceman]

Attachment: patch

OK, I've prepared a simple version of this feature. I've intentionally made the message verbose and the deleting non-default (as was the behaviour till now).

Comment 30

[:aceman]

(In reply to Bryce Mozilla Nesbitt from comment #28)
> I think the brokenness here goes beyond what's been discussed so far.  It is
> definitely bad to leave a 2GB IMAP SBD file after the corresponding account
> is deleted.  But thunderbird mixes useless and precious files in the same
> directory:
> * Crash Reports (useless after the moment)
> * IMAP SBD files, Cache Files (can be recreated on demand)
> * Application Settings (kind of in the middle)
> * Local Sent Mail (might be priceless)
> 
> I'd prefer my true irreplaceable personal files be in an obviously separate
> directory that I'd be sure my backup software picks up.

This is not true, the data you listed is not in the same directory (at least if you don't mean the whole TB profile).

But please keep this bug for the account specific files. TB already removes Account settings. The bug here attempts to remove the account data (messages, filters, etc).

Please use different bugs for application-wide files you mentioned (e.g. crash reports, application settings).

Comment 31

[:aceman]

(In reply to :aceman from comment #29)
> Created attachment 742835 [details] [diff] [review]
> patch
> 
> OK, I've prepared a simple version of this feature. I've intentionally made
> the message verbose and the deleting non-default (as was the behaviour till
> now).

It would probably be good to mention this only affects data stored locally and not any that may be still on the server.

Comment 32

[:aceman]

OK, the UI review can still be done, but I found nsMsgAccountManager::RemoveIncomingServer can remove the files if passed the proper parameter so we do not need to make the deletion from account manager JS. So I rework the code a bit.

Comment 33

[:aceman]

Attachment: patch v2

Comment 34

[Ian Neal]

Comment on attachment 753832 [details] [diff] [review]
patch v2

>+++ b/mailnews/base/prefs/content/AccountManager.js
>+  // Ask the user about deletion of all account data (messages, filters, etc).
>+  let accountDataPath = server.localPath;
>+  let brandName = document.getElementById("bundle_brand").getString("brandShortName");
>+  confirmRemoveAccount = bundle.getFormattedString("confirmRemoveAccountData",
>+                                                   [accountDataPath.path, brandName]);
>+  let removeAccountData = bundle.getString("replyRemoveAccountData");
>+  let keepAccountData = bundle.getString("replyKeepAccountData");
>+
>+  let removeDataDecision = Services.prompt.confirmEx(
>+    window, confirmTitle, confirmRemoveAccount,
>+    (Services.prompt.BUTTON_POS_0 * Services.prompt.BUTTON_TITLE_IS_STRING) +
>+    (Services.prompt.BUTTON_POS_1 * Services.prompt.BUTTON_TITLE_CANCEL) +
>+    (Services.prompt.BUTTON_POS_2 * Services.prompt.BUTTON_TITLE_IS_STRING) +
>+    Services.prompt.BUTTON_DELAY_ENABLE, keepAccountData, null, removeAccountData, null, {});
>+  if (removeDataDecision == 1)
>+    return;
>+
I don't think we should be asking the user two questions about removing an account, I think it would be better to combine into a single question.

>+++ b/mailnews/base/public/nsIMsgAccountManager.idl
>-[scriptable, uuid(d5ab0eea-49c5-42f2-b2e6-8ad306606d8b)]
>+[scriptable, uuid(d5ab0eea-49c5-42f2-b2e6-8ad306606d8c)]
I think usually the uuid changes by more than a single character.

r- for the moment.

Comment 35

[:aceman]

Attachment: patch v3

OK, try this.

Comment 36

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 760121 [details] [diff] [review]
patch v3

ui-r-, based on https://dl.dropboxusercontent.com/u/2301433/Screenshots/DeleteAccount.png

The problems I see with this iteration (at least on Mac) are:
* The dialog is _way_ too wide.
* The text doesn't wrap at the window, but instead halfway down.
* The text is also too long, and includes details most users won't care about (i.e. the folder location of the files we're about to delete).
* The two buttons to remove the account shouldn't be around the cancel button.
* We actually shouldn't have two buttons there at all, but should instead use a checkbox to determine whether to delete the files or not.

Thanks,
Blake.

Comment 37

[:aceman]

(In reply to Blake Winton (:bwinton) from comment #36)
> The problems I see with this iteration (at least on Mac) are:
> * The dialog is _way_ too wide.
> * The text doesn't wrap at the window, but instead halfway down.
This seems to only happen on OS X.

> * The text is also too long, and includes details most users won't care
> about (i.e. the folder location of the files we're about to delete).
I think users seriously thinking whether to delete files may want to first look at them.

> * The two buttons to remove the account shouldn't be around the cancel
> button.
> * We actually shouldn't have two buttons there at all, but should instead
> use a checkbox to determine whether to delete the files or not.
OK, I can try this.

Comment 38

[Blake Winton (:bwinton) (:☕️)]

(In reply to :aceman from comment #37)
> > * The text is also too long, and includes details most users won't care
> > about (i.e. the folder location of the files we're about to delete).
> I think users seriously thinking whether to delete files may want to first
> look at them.

Perhaps, but then a "Show Files" button/link would be more helpful than a path that people will try to type somewhere…  ;)

Thanks,
Blake.

Comment 39

[:aceman]

OK, how do I add a checkbox to the dialog? Do I need to create a new xul document for it? That would then allow to have the "Show files" button there too.

Is it worth it? :)

Comment 40

[Magnus Melin [:mkmelin]]

To add a checkbox you just need to set the checkbox text param
https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIPromptService#confirmEx%28%29

Comment 41

[Blake Winton (:bwinton) (:☕️)]

Yes, it would be worth it.  ;)

Comment 42

[David E. Ross]

The problem is not limited to files.  I deleted an E-mail account more than a year ago, but I still find preference variables for that account when I select [Tools > Options > Advanced > General > Config Editor].  

Since some users might want to delete and the re-create an account, the cleanup -- both files and preferences -- should be per a user-interface option.

Comment 43

[Ludovic Hirlimann [:Usul]]

Removing myslef on all the bugs I'm cced on. Please NI me if you need something on MailNews Core bugs from me.

Comment 44

[Wayne Mery (:wsmwk)]

(In reply to Blake Winton (:bwinton) (:☕️) from comment #41)
> Yes, it would be worth it.  ;)

++

This is a frequent need of users - to free space, to redefine the account, etc. (just had a person on IRC with this issue)  They do not understand that when an account is deleted that a backing store is not removed. Nor should they need to know how to do it manually - and it would be a scarey thing for the average user to do, or be asked to do.

Can we shoehorn this into 45?

Comment 45

[Wayne Mery (:wsmwk)]

(In reply to David E. Ross from comment #42)
> The problem is not limited to files.  I deleted an E-mail account more than
> a year ago, but I still find preference variables for that account when I
> select [Tools > Options > Advanced > General > Config Editor].  
> 
> Since some users might want to delete and the re-create an account, the
> cleanup -- both files and preferences -- should be per a user-interface
> option.

Clearly a different issue, and different goal. Please file a separate bug report

Comment 46

[Marco A.G.Pinto [:marcoagpinto]]

Sorry... I haven't had the chance to read all posts.

What I am scared if by deleting an account, the e-mail messages are deleted as well which is dangerous if one deletes an account by accident.

All this should be taken into consideration.

Comment 47

[bugzilla_mozilla]

(In reply to Marco A.G.Pinto from comment #46)
> Sorry... I haven't had the chance to read all posts.
> 
> What I am scared if by deleting an account, the e-mail messages are deleted
> as well which is dangerous if one deletes an account by accident.
> 
> All this should be taken into consideration.

There is no way for a normal user to restore the e-mails anyway. Deleting stuff by accident is always possible in any application and always the users own fault. When I delete a Thunderbird profile in the profile manager, it is also permanently removed. This also removes all your mails and can be done by accident.

The solution to prevent the loss of important data is not making the deletion impossible (for inexperienced users). The solution is called backups.

Comment 48

[Marco A.G.Pinto [:marcoagpinto]]

[15:29] <marcoagpinto> aceman: I guess that it should open a window with two checkboxes: a) Delete Account b) Delete messages as well
[15:30] <marcoagpinto> it is important the "AS WELL"
[15:30] <marcoagpinto> so that no stupid person like me deletes the e-mails
[15:30] <marcoagpinto> :)
[15:30] <marcoagpinto> it must be "idiot proof" is what I meant
[15:32] <aceman> like "this will delete old messages permanently, including originals on the server"
[15:32] <marcoagpinto> "WARNING!" + chr(10) + "Are you really sure you want to delete all mail messages associated with the account?"

Comment 49

[bugzilla_mozilla]

(In reply to Marco A.G.Pinto from comment #48)
> [15:32] <aceman> like "this will delete old messages permanently, including
> originals on the server"

In my opinion, it should not delete mails from the server. If this is an IMAP account, than of course deleting the account in Thunderbird should not delete the mails on the server. It should only delete the local copies. At least this is what I would expect and how all other IMAP clients I know behave.
If this is a POP3 account, it should delete the local mails, which might or might not be already deleted on the server, depending on what the user selected. Also, in this case it should not delete mails that remain on the server.

Comment 50

[:aceman]

(In reply to bugzilla_mozilla from comment #49)
> (In reply to Marco A.G.Pinto from comment #48)
> > [15:32] <aceman> like "this will delete old messages permanently, including
> > originals on the server"
> 
> In my opinion, it should not delete mails from the server. If this is an
> IMAP account, than of course deleting the account in Thunderbird should not
> delete the mails on the server. It should only delete the local copies. At
> least this is what I would expect and how all other IMAP clients I know
> behave.
> If this is a POP3 account, it should delete the local mails, which might or
> might not be already deleted on the server, depending on what the user
> selected. Also, in this case it should not delete mails that remain on the
> server.

That quote of me does not belong into this bug. So don't worry, no messages from server are to be deleted when just removing and account from TB.

Comment 51

[:aceman]

(In reply to Marco A.G.Pinto from comment #48)
> [15:29] <marcoagpinto> aceman: I guess that it should open a window with two
> checkboxes: a) Delete Account b) Delete messages as well

It struck me now that there could be a use case for the user to actually uncheck "delete account" and only check "delete messages". In that case we could just wipe the data folder (local directory) and at next restart (or msg fetch) TB will recreate it and download messages anew. This could be useful for debugging (no need to recreate accounts) and e.g. for IMAP accounts accumulating junk (e.g. folders that grow and can't be purged/compacted).

Comment 52

[:aceman]

Attachment: WIP patch v4

So this version makes the question a full-featured xul dialog. It's just a a quick hack with strings inlined and does not really delete anything. It is just to get the feeling for the UI. How do you like this one?

Comment 53

[Blake Winton (:bwinton) (:☕️)]

(You're going to post a picture of the new dialog soon, right?  ;)

Comment 54

[Wayne Mery (:wsmwk)]

(In reply to :aceman from comment #51)
> (In reply to Marco A.G.Pinto from comment #48)
> > [15:29] <marcoagpinto> aceman: I guess that it should open a window with two
> > checkboxes: a) Delete Account b) Delete messages as well
> 
> It struck me now that there could be a use case for the user to actually
> uncheck "delete account" and only check "delete messages". In that case we
> could just wipe the data folder (local directory) and at next restart (or
> msg fetch) TB will recreate it and download messages anew. This could be
> useful for debugging (no need to recreate accounts) and e.g. for IMAP
> accounts accumulating junk (e.g. folders that grow and can't be
> purged/compacted).

I like the idea. And some people have been asking for it.

I like the expanded explanations. I wonder if we can do better still for "Removes only the information about this account."  I don't have wording that I like, but something like "Removes only Thunderbird's knowledge of this account."  (I not keen on the word "knowledge", but I'm also not keen on "information about ...")

Comment 55

[Richard Marti (:Paenglab)]

Comment on attachment 8717201 [details] [diff] [review]
WIP patch v4

This looks good.

The account name is now hard coded but this will be changed to dynamic name, true?
You will add access keys for the checkmarks and the data location button, true?

I'll let the wording decide from the native speaking.

Comment 56

[Richard Marti (:Paenglab)]

Attachment: Dialog.png

Dialog on opening and after clicking on "More Info".

Maybe a change of the button to "Less Info" could revert to initial state. Or disabling the button because after expanding the dialog the button does nothing and could make the user think it will be more infos again.

Comment 57

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8717201 [details] [diff] [review]
WIP patch v4

(Looks like Paenglab has this covered.  :)

Comment 58

[Wayne Mery (:wsmwk)]

Comment on attachment 8717201 [details] [diff] [review]
WIP patch v4

my feedback in comment 54 about improving wording

Comment 59

[:aceman]

Attachment: WIP patch v5

This version incorporates the comments so far.

Comment 60

[Richard Marti (:Paenglab)]

Comment on attachment 8721831 [details] [diff] [review]
WIP patch v5

Looks already good. But I found two issues which should be fixed.

Comment 61

[Richard Marti (:Paenglab)]

Attachment: dialogSize.png

At least on Windows the buttons touch the bottom border. The screenshot shows how it looks on XP and Win10.

Comment 62

[Richard Marti (:Paenglab)]

Attachment: lineGap.png

On the last line with the button the gap between the lines is bigger than on the first two lines. Maybe it's better when the button would be alone on a new line.

Comment 63

[bugzilla_mozilla]

"This does not affect messages and data still kept on the server. Do not ..."
I think this should be more clearly, depending on the account type:
In case of IMAP I would propose:
"This does not affect data on the server, all your messages will remain there."
In case of POP3 I would propose:
"Attention: This might delete messages that are not kept on the server anymore, so these messages will be deleted permanently. Do not ..."

>Maybe it's better when the button would be alone on a new line.
Yes, I agree.

Comment 64

[bugzilla_mozilla]

Is it possible to delete the message data and keep the account? I think it does not really make sense.
So I think radio buttons are better than check boxes. One "Remove account" and one "Remove account and message data".

With checkboxes, you could also uncheck both boxes and click "remove", which then should or should not remove something!?

-> Radio buttons would be better I think

Comment 65

[:aceman]

(In reply to Richard Marti (:Paenglab) from comment #61)
> At least on Windows the buttons touch the bottom border. The screenshot
> shows how it looks on XP and Win10.

I do not see this on linux, but I see the dialog does not resize properly even at the initial opening. On clicking the More info button, I need to call window.sizeToContent(); so it expands for the new uncollapsed texts.

Comment 66

[:aceman]

(In reply to bugzilla_mozilla from comment #64)
> Is it possible to delete the message data and keep the account? I think it
> does not really make sense.
We think it is useful, see the older comments.
Of course, I need to check if it is actually doable.

> With checkboxes, you could also uncheck both boxes and click "remove", which
> then should or should not remove something!?
Yes, we can disable the Remove button in this case.

Comment 67

[:aceman]

(In reply to bugzilla_mozilla from comment #63)
> "This does not affect messages and data still kept on the server. Do not ..."
> I think this should be more clearly, depending on the account type:
> In case of IMAP I would propose:
> "This does not affect data on the server, all your messages will remain
> there."
> In case of POP3 I would propose:
> "Attention: This might delete messages that are not kept on the server
> anymore, so these messages will be deleted permanently. Do not ..."

Yes, this is an interesting idea. On server-based accounts we could show a less scary message. We could consider IMAP and NNTP to be server based for now. But even in that case, you loose e.g. filter definitions.

Comment 68

[:aceman]

Attachment: WIP patch v6

I have implemented all the current proposals, even the different wording for IMAP+NNTP accounts vs. the rest.

It occurred to me that we actually do not report successful account removal to the user (only failure). Also now, when we can remove the local data, it could take a while to delete GBs of folders if they are on a network share. So I've added a message saying TB is actually doing something instead of appearing hung.

The patch already does contain the backend changes needed to see if removing data without removing folder is actually possible (so it makes sense to provide the UI option).

But the UI does not actually call the removal backend code yet. So please play with the UI a bit.

Comment 69

[Wayne Mery (:wsmwk)]

Comment on attachment 8723550 [details] [diff] [review]
WIP patch v6

SGTM. But I didn't play with the code

Comment 70

[Richard Marti (:Paenglab)]

Comment on attachment 8723550 [details] [diff] [review]
WIP patch v6

Looks good now with the space below the buttons.

Only one thing, the 'Show data location' button is now to prominent with the whole dialog width. Please make it only the width of the button text.

Comment 71

[:aceman]

Attachment: patch v7

Cleaned up the nits and added proper string entities.
This version has the remove account functionality enabled, use with caution!

Try build here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=310742e4c71a7be1f9bd90065caca8ae8ed98deb

Comment 72

[Richard Marti (:Paenglab)]

Comment on attachment 8747466 [details] [diff] [review]
patch v7

Looks good!

I have only one question, the Cancel button has as the only button no accesskey. Is it possible to add one for it?

With this question answered ui-r+

Comment 73

[:aceman]

I expected the default buttons would have default access keys.
It seems Cancel intenationally does not have a key:
http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/locales/en-US/chrome/global/dialog.properties

Should I add it in this dialog?

Comment 74

[Richard Marti (:Paenglab)]

I checked now other dialogs. They have also no accesskey for Cancel and OK. No need to add the accesskey.

Comment 75

[Magnus Melin [:mkmelin]]

I think that's because there's ESC.

Comment 77

[:aceman]

I found out implementing removing of files but not removing the account itself does not work in all account types yet. Some accounts need checks to recreate their files if they are missing.

So I propose to finish this patch as requested by the bug to only toggle removing files but remove the account unconditionally.

I'll file a new bug for the possible enhancement of only removing the files. Adding the needed checks and functions where missing may be risky at this time for TB52.

Comment 78

[:aceman]

Attachment: patch v8

Simplified version. Account removal is pre-selected and cannot be unchecked.

Comment 79

[Richard Marti (:Paenglab)]

Comment on attachment 8790889 [details] [diff] [review]
patch v8

I'm okay with the disabled checkmark with the hope you get it fixed in a followup bug.

Comment 80

[Magnus Melin [:mkmelin]]

Comment on attachment 8790889 [details] [diff] [review]
patch v8

Review of attachment 8790889 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/locales/en-US/chrome/messenger/removeAccount.dtd
@@ +1,5 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +<!ENTITY dialogTitle                  "Remove Account and Data">

I wouldn't bother with trying to align the values

::: mailnews/base/prefs/content/removeAccount.js
@@ +57,5 @@
> +}
> +
> +function removeAccount()
> +{
> +  gDialog.getButton("accept").label = "OK";

huh?

::: mailnews/base/prefs/content/removeAccount.xul
@@ +9,5 @@
> +<!DOCTYPE dialog SYSTEM "chrome://messenger/locale/removeAccount.dtd">
> +
> +<dialog title="&dialogTitle;"
> +        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> +        id="removeAccountDialog"

please put id as first attribute

::: mailnews/base/public/nsIMsgAccountManager.idl
@@ +12,5 @@
>  
>  interface nsIMsgFolderCache;
>  interface nsIFolderListener;
>  
> +[scriptable, uuid(467b38b4-6627-4818-9de9-621d8f91656a)]

wasn't the requirement to update uuids dropped?

::: mailnews/base/util/nsMsgIncomingServer.cpp
@@ +1028,5 @@
>  NS_IMETHODIMP
>  nsMsgIncomingServer::RemoveFiles()
>  {
>    // IMPORTANT, see bug #77652
> +  // TODO: Decide what to do for deferred accounts.

I think we need to handle that before enabling removal

::: suite/locales/en-US/chrome/mailnews/pref/removeAccount.dtd
@@ +7,5 @@
> +<!ENTITY removeButton.accesskey       "R">
> +<!ENTITY removeAccount.label          "Remove account">
> +<!ENTITY removeAccount.accesskey      "a">
> +<!ENTITY removeAccount.desc           "Removes only Thunderbird's knowledge of this account. Does not affect the account itself on the server.">
> +<!ENTITY removeData.label             "Remove message data">

account data?

@@ +10,5 @@
> +<!ENTITY removeAccount.desc           "Removes only Thunderbird's knowledge of this account. Does not affect the account itself on the server.">
> +<!ENTITY removeData.label             "Remove message data">
> +<!ENTITY removeData.accesskey         "d">
> +<!ENTITY removeDataLocalAccount.desc  "Removes all messages, folders and filters associated with this account from your local disk. This does not affect some messages which may still be kept on the server. Do not choose this if you plan to archive the local data or re-create this account later.">
> +<!ENTITY removeDataServerAccount.desc "Removes all messages, folders and filters associated with this account from your local disk. Your messages and folders are still kept on the server.">

These messages are wrong/misleading for POP users.

Comment 81

[Magnus Melin [:mkmelin]]

Another thought: instead of adding all this UI, how about always moving the files to system trash, and maybe just telling the user about that after they removed account? Why would someone really want to leave the files behind, except for testing purposes?

Comment 82

[:aceman]

I don't think any app behaves like that. Maybe for reasons like:
the data may be so big it will not fit into the trash so it will partly be purged (with everything that was in the trash before). Even without loosing any of the files, most users may not be able to put it back (remember they must put back (recover) all files as a set for it to recover all their previous data/settings). And for the testing purposes, it will be an unnecessary hassle for our testers.

Comment 83

[:aceman]

(In reply to Magnus Melin from comment #80)
> > +<!ENTITY dialogTitle                  "Remove Account and Data">
> 
> I wouldn't bother with trying to align the values

I like it because then it is easier to see differences in the similar strings like we have here.

> ::: mailnews/base/prefs/content/removeAccount.js
> > +  gDialog.getButton("accept").label = "OK";
> 
> huh?

Prototype shortcuts :) Thanks, fixed.

> ::: mailnews/base/prefs/content/removeAccount.xul
> please put id as first attribute

OK.

> ::: mailnews/base/public/nsIMsgAccountManager.idl

> > +[scriptable, uuid(467b38b4-6627-4818-9de9-621d8f91656a)]
> 
> wasn't the requirement to update uuids dropped?

Yes.

> ::: mailnews/base/util/nsMsgIncomingServer.cpp
> @@ +1028,5 @@
> >  NS_IMETHODIMP
> >  nsMsgIncomingServer::RemoveFiles()
> >  {
> >    // IMPORTANT, see bug #77652
> > +  // TODO: Decide what to do for deferred accounts.
> 
> I think we need to handle that before enabling removal
Yes, we can decide what to do in bug 77652.
So I can disable removing data in this bug if the account is deferred.


> ::: suite/locales/en-US/chrome/mailnews/pref/removeAccount.dtd
> @@ +7,5 @@
> > +<!ENTITY removeButton.accesskey       "R">
> > +<!ENTITY removeAccount.label          "Remove account">
> > +<!ENTITY removeAccount.accesskey      "a">
> > +<!ENTITY removeAccount.desc           "Removes only Thunderbird's knowledge of this account. Does not affect the account itself on the server.">
> > +<!ENTITY removeData.label             "Remove message data">
> 
> account data?

We explain account data as just the account information (server name, login, settings). So message data is the real meat of the data inside the account (messages, filters, popstate.dat).

> @@ +10,5 @@
> > +<!ENTITY removeAccount.desc           "Removes only Thunderbird's knowledge of this account. Does not affect the account itself on the server.">
> > +<!ENTITY removeData.label             "Remove message data">
> > +<!ENTITY removeData.accesskey         "d">
> > +<!ENTITY removeDataLocalAccount.desc  "Removes all messages, folders and filters associated with this account from your local disk. This does not affect some messages which may still be kept on the server. Do not choose this if you plan to archive the local data or re-create this account later.">
> > +<!ENTITY removeDataServerAccount.desc "Removes all messages, folders and filters associated with this account from your local disk. Your messages and folders are still kept on the server.">
> 
> These messages are wrong/misleading for POP users.

Why, which one? Only the removeDataLocalAccount is shown to POP users. Have I reversed the logic somewhere?

Comment 84

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #83)
> > > +<!ENTITY removeAccount.label          "Remove account">
> > > +<!ENTITY removeAccount.accesskey      "a">
> > > +<!ENTITY removeAccount.desc           "Removes only Thunderbird's knowledge of this account. Does not affect the account itself on the server.">
> > > +<!ENTITY removeData.label             "Remove message data">
> > 
> > account data?
> 
> We explain account data as just the account information (server name, login,
> settings). So message data is the real meat of the data inside the account
> (messages, filters, popstate.dat).

But it says "Remove account". Maybe "Remove account information" is better then, or "Forget...."

> 
> > @@ +10,5 @@
> > > +<!ENTITY removeAccount.desc           "Removes only Thunderbird's knowledge of this account. Does not affect the account itself on the server.">

Noticed this now, but this should use brandShortName

> > > +<!ENTITY removeData.label             "Remove message data">
> > > +<!ENTITY removeData.accesskey         "d">
> > > +<!ENTITY removeDataLocalAccount.desc  "Removes all messages, folders and filters associated with this account from your local disk. This does not affect some messages which may still be kept on the server. Do not choose this if you plan to archive the local data or re-create this account later.">

Didn't try, but what would be the benefit of having the data when re-creating the account? 

> > > +<!ENTITY removeDataServerAccount.desc "Removes all messages, folders and filters associated with this account from your local disk. Your messages and folders are still kept on the server.">

> > These messages are wrong/misleading for POP users.
> 
> Why, which one? Only the removeDataLocalAccount is shown to POP users. Have
> I reversed the logic somewhere?

Sorry, I was mistaken.

Comment 85

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #82)
> the data may be so big it will not fit into the trash 

I don't think that's how trash works. It's just moving files, so there is always room.

Comment 86

[:aceman]

(In reply to Magnus Melin from comment #84)
> But it says "Remove account". Maybe "Remove account information" is better
> then, or "Forget...."

Good idea. Should I also make it "forget message data" for the accounts like IMAP where data stays on server?

> 
> > 
> > > @@ +10,5 @@
> > > > +<!ENTITY removeAccount.desc           "Removes only Thunderbird's knowledge of this account. Does not affect the account itself on the server.">
> 
> Noticed this now, but this should use brandShortName

Yes, thanks. Hopefully that does not involve moving it to .properties ...

> > > > +<!ENTITY removeData.label             "Remove message data">
> > > > +<!ENTITY removeData.accesskey         "d">
> > > > +<!ENTITY removeDataLocalAccount.desc  "Removes all messages, folders and filters associated with this account from your local disk. This does not affect some messages which may still be kept on the server. Do not choose this if you plan to archive the local data or re-create this account later.">
> 
> Didn't try, but what would be the benefit of having the data when
> re-creating the account? 

Testing purposes? Or wiping some bad settings but wanting to keep the msgs. Or creating another account (another email) but wanting it to append to the existing msgs data.

(In reply to Magnus Melin from comment #85)
> (In reply to :aceman from comment #82)
> > the data may be so big it will not fit into the trash 
> 
> I don't think that's how trash works. It's just moving files, so there is
> always room.

At least on Windows there is a quota on Trash (in percentage of whole disk size). If you exceed it (put more files inside), trash will be purged even if you have ton of free space. At least that is what I remember in Win XP.

Comment 87

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #86)
> Good idea. Should I also make it "forget message data" for the accounts like
> IMAP where data stays on server?

I think remove is clearer at least for the data part.

> > > > > +<!ENTITY removeDataLocalAccount.desc  "Removes all messages, folders and filters associated with this account from your local disk. This does not affect some messages which may still be kept on the server. Do not choose this if you plan to archive the local data or re-create this account later.">
> > 
> > Didn't try, but what would be the benefit of having the data when
> > re-creating the account? 
> 
> Testing purposes? Or wiping some bad settings but wanting to keep the msgs.
> Or creating another account (another email) but wanting it to append to the
> existing msgs data.

But when creating a new account it doesn't pick up the old data anyway, right? Point was that let's not add UI text that's targeted at testing only.

Comment 88

[:aceman]

(In reply to Magnus Melin from comment #87)
> I think remove is clearer at least for the data part.

OK

> > Testing purposes? Or wiping some bad settings but wanting to keep the msgs.
> > Or creating another account (another email) but wanting it to append to the
> > existing msgs data.
> 
> But when creating a new account it doesn't pick up the old data anyway,
> right? Point was that let's not add UI text that's targeted at testing only.

You just need to point the Local Directory path to the old folder. But OK, we can reword it a bit.

Comment 89

[:aceman]

Attachment: patch v8.1

I tried to fix the problems.

Comment 90

[Magnus Melin [:mkmelin]]

Comment on attachment 8794397 [details] [diff] [review]
patch v8.1

Review of attachment 8794397 [details] [diff] [review]:
-----------------------------------------------------------------

I guess I'm pretty happy with the. r=mkmelin

::: mailnews/base/prefs/content/removeAccount.js
@@ +9,5 @@
> +var gServer;
> +var gDialog;
> +
> +function onLoad(event)
> +{

since this is new code, please use the standard braces style with { on the line before
(there's a bunch of these)

@@ +80,5 @@
> +}
> +
> +function removeAccount()
> +{
> +

nit: odd extra space here

::: mailnews/base/public/nsIMsgIncomingServer.idl
@@ +26,5 @@
>   * this is the base interface for all mail server types (imap, pop, nntp, etc)
>   * often you will want to add extra interfaces that give you server-specific
>   * attributes and methods.
>   */
> +[scriptable, uuid(67b7975c-2c40-47af-b605-4a327bc7ec36)]

doesn't need to change anymore

Comment 91

[Magnus Melin [:mkmelin]]

One more thought though: when Firefox asks you to do a refresh of a profile and you let it, it moves the old profile data to a folder on the desktop (can't recall the exact name now). That would also be an option for our deleted account data.

Comment 92

[:aceman]

Attachment: patch v8.2

Thanks. This also includes a fix to failing test due to clash of function names I introduced here (seen in https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=653bd147fce165e08e4f4ad2f7fd6dd2e0bbb02b).

Ian, please check the SM strings.

Comment 93

[Ian Neal]

Comment on attachment 8803668 [details] [diff] [review]
patch v8.2

>+<!ENTITY dialogTitle                  "Remove Account and Data">
>+<!ENTITY removeButton.label           "Remove">
>+<!ENTITY removeButton.accesskey       "R">
>+<!ENTITY removeAccount.label          "Remove account information">
>+<!ENTITY removeAccount.accesskey      "a">
>+<!ENTITY removeAccount.desc           "Removes only &brandShortName;'s knowledge of this account. Does not affect the account itself on the server.">
>+<!ENTITY removeData.label             "Remove message data">
>+<!ENTITY removeData.accesskey         "d">
Was just wondering why "d" rather than "m" was selected, is probably unimportant in the scheme of things.

r/a=me

Comment 94

[:aceman]

I probably wanted to emphasize the "Data", but maybe m for "Message" would work too.

Comment 95

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/d2345e14a58802e703ee9338e9890a494119bf5b