39121 - Unspecialize Trash folder for IMAP "Mark as Deleted" & "Remove Immediately" modes

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[laurel]

Using 2000-05-10m16 commercial builds

This may wind up being a duplicate of bug #30750, but not sure so will log
separately.  Not sure who this'll belong to...

When changing an IMAP account server settings for delete model from MoveToTrash
to Remove Immediately, the account's existing Trash folder should have it's
special folder status removed and sort to the "regular" folder hierarchy.  This
is not currently happening.  (I'm comparing to 4.x behavior.)

1.  Login to an IMAP account which has its server setting for delete as Move to
Trash.  In this state, the account should have a Trash folder sorted in with the
special folders at the top of the folder hierarchy.
2.  Edit the account settings to change the delete mode to Remove Immediately.
Confirm OK to the change and confirm out of account settings dialog.
3.  Note the trash folder is still present in the special folders portion of the
hierarchy and still carries its trashcan icon.
4.  Exit and relaunch, login to same account.  Note the trash is still in the
special folders section.

Actual result:  no visual indication that the trash is no longer a special
folder.

Expected result:  (again, I'm using 4.x behavior as my basis) Upon confirmation
of the pref change that account's trash folder should lose special folder
status; it should lose trash folder icon and be sorted in with the regular user
folder portion of the account's folder hierarchy.

Comment 1

[laurel]

Added info:  4.x immediately disables Empty Trash functionality upon confirming
the pref change to Remove Immediately.  Current seamonkey build allows empty
trash within the session of the pref change and subsequent session will not
perform the empty trash function.  I've logged a bug to have the menu item
disable... bug #3912

Comment 2

[laurel]

that bug number is bug #39124.

Comment 3

[laurel]

This should also apply to Mark As Deleted. Let me know if you want that as a
separate bug.

Comment 4

[scottputterman]

reassigning to jefft.

Comment 5

[Karen Huang]

I believe this issue has been addressed from some comments of bug 33235....

Comment 6

[Karen Huang]

I mean from bug 33235 2000-04-05 11:58 my comments: 

Comment 7

[scottputterman]

moving to future milestone.

Comment 8

[Karen Huang]

Since the trash folder will fall in to the "regular" folder hierarchy , so 
"delete folder" menu should be available for delete this trash folder. 
I am just updating the summary a little bit, so this way can remind me to 
include these scenarions to verify in the "future".

Comment 9

[scottputterman]

reassigning jefft's bugs to naving

Comment 10

[Karen Huang]

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

Comment 11

[Karen Huang]

bug 91958 already logged for "delete folder" menu problem for "Mark as Deleted" 
& "Remove Immediately" modes.

Let's keep this bug for only fixing Trash folder problem for "Mark as Deleted" & 
"Remove Immediately" modes.

Comment 12

[(not reading, please use [e-mail] instead)]

mass re-assign.

Comment 13

[Mike Fedyk]

(In reply to comment #11)
> bug 91958 already logged for "delete folder" menu problem for "Mark as Deleted" 
> & "Remove Immediately" modes.
> 
> Let's keep this bug for only fixing Trash folder problem for "Mark as Deleted" & 
> "Remove Immediately" modes.

Lets not.

There has been no activity in this bug except for a mass-reassign for over two
years, and the other bug is general enough.

Comment 14

[Nikolay Shopik]

version 3.0a2pre (2008070703) - problem still there.

Comment 15

[Nikolay Shopik]

I could delete trash if it never used(nothing trashed) and MAD mode is enabled from beginning.

Comment 16

[Magnus Melin [:mkmelin]]

Attachment: proposed fix

Not sure what's the easiest way to do an automated test for this...

Comment 17

[David :Bienvenu]

I thought we removed the Trash flag when we switch delete models to a non-trash delete model. We don't want the trash folder to show up with the trash icon in that case...if we were doing that correctly, then some of this patch wouldn't be needed, though in general, the patch seems to be an improvement...

For a test, you could simply write an xpcshell test that checks the deleteable property, cuz mozmill and imap don't play well together. Though you could put TB into offline mode in mozmill and then do stuff with imap folders...

Comment 18

[Magnus Melin [:mkmelin]]

Attachment: proposed fix v2, with tests

With tests.

I don't see the trash flag being cleared, and i don't know where to do that in a clean way.

Comment 19

[David :Bienvenu]

Magnus, see this code:

http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapIncomingServer.cpp#3191

We call this when the user picks a different trash folder. It clears the trash flag from the existing trash folder before setting it on the new trash folder.

We should be doing something similar when the user switches to the imap delete model. That way, the user could then delete the trash folder simply because it wouldn't have the trash flag set. But, when the user switches back to the trash model, we'd need to make sure we restored the trash flag to the folder specified by the pref. This approach seems a lot cleaner to me than checking the imap delete model in various places and gives the right user-feedback as well.

Comment 20

[Magnus Melin [:mkmelin]]

Attachment: proposed fix v3

Clearing/setting trash flag when deletemodel changes.

Comment 21

[David :Bienvenu]

Comment on attachment 571757 [details] [diff] [review]
proposed fix v3

thx, Magnus, that looks a lot better. I'll review it soon.

Comment 22

[David :Bienvenu]

OK, this doesn't work for me because of our screwed up trash folder name handling (nothing specifically about this patch). For example, my fastmail account has a trash folder of Inbox.Trash, but my trash folder name is set to Trash. nsImapIncomingServer::GetFolder is asked for the folder named "Trash", so it returns a non-existent top-level trash folder. The root issue is that the person who implemented the custom trash folder name stuff didn't want to use URI's, so it's all essentially broken and needs to be fixed. So I think I need to fix all that before this fix is really useful. I'll try to find a little time to do that.

Comment 23

[David :Bienvenu]

Attachment: fix namespace issue

turns out that the issue I was seeing with fastmail was specific to the way we try to find the trash folder. This version of the patch fixes it by trying the personal namespace if the folder doesn't exist.

The remaining issue I see is that with XLIST servers, we set the trash flag on the trash folder even in the non-trash delete model, which puts the trash flag back on the folder next time you run. I'll look at where that's done and see if I can fix it.

Comment 24

[David :Bienvenu]

Attachment: ignore xlist trash if the user is using imap delete

This fixes it so we ignore the xlist trash folder flag if the user isn't using the trash delete model.

Comment 25

[David :Bienvenu]

Comment on attachment 572819 [details] [diff] [review]
ignore xlist trash if the user is using imap delete

I don't know why the old code did this:

+  *deletable = !(isServer || mFlags & nsMsgFolderFlags::Inbox ||
+    mFlags & nsMsgFolderFlags::Drafts ||
+    mFlags & nsMsgFolderFlags::Templates ||
+    mFlags & nsMsgFolderFlags::SentMail ||
+    mFlags & nsMsgFolderFlags::Archive ||
+    mFlags & nsMsgFolderFlags::Junk ||
+    mFlags & nsMsgFolderFlags::Trash);


instead of just checking
mFlags & (nsMsgFolderFlags::Drafts|nsMsgFolderFlags::Templates|nsMsgFolderFlags::SentMail|nsMsgFolderFlags::Archive|nsMsgFolderFlags::Junk|nsMsgFolderFlags::Trash)

Otherwise, this looks OK - I'll just do some testing and perhaps you can do the same with my slight tweaks.

Comment 26

[David :Bienvenu]

Comment on attachment 572819 [details] [diff] [review]
ignore xlist trash if the user is using imap delete

r=me, I'll let Neil comment about the flag checking...

Comment 27

[[e-mail]]

Comment on attachment 572819 [details] [diff] [review]
ignore xlist trash if the user is using imap delete

>diff --git a/mail/base/content/mailContextMenus.js b/mail/base/content/mailContextMenus.js
Was expecting a suite port of this...

>+  bool deleteIsMoveToTrash = DeleteIsMoveToTrash();
Not used? (Can't comment on the multiple tests without this.)

>+            if (isNewsURI(selectedFolder.URI))
>             {
>                 var unsubscribe = ConfirmUnsubscribe(selectedFolder);
>                 if (unsubscribe)
>                     UnSubscribe(selectedFolder);
>+                continue;
>+            }
>+
>+            var canDelete = (specialFolder == "Junk") ?
>+                CanRenameDeleteJunkMail(folder.URI) : folder.deletable;
>+            if (!canDelete)
>+            {
>+                continue;
>             }
>             else
>             {
Eww. My preference would be
if (isNewsURI(selectedFolder.URI))
{
    var unsubscribe = ConfirmUnsubscribe(selectedFolder);
    if (unsubscribe)
        UnSubscribe(selectedFolder);
}
else if (specialFolder == "Junk" ?
         CanRenameDeleteJunkMail(folder.URI) : folder.deletable)
{
etc. but at the very least don't use if (!canDelete) continue; else

Comment 28

[Magnus Melin [:mkmelin]]

(In reply to [e-mail] from comment #27)
> >+  bool deleteIsMoveToTrash = DeleteIsMoveToTrash();
> Not used? (Can't comment on the multiple tests without this.)

Yes this is a leftover from an earlier version of the patch. Now that only the flags are checked it's not needed.

Comment 29

[[e-mail]]

(In reply to Magnus Melin from comment #28)
> (In reply to comment #27)
> > >+  bool deleteIsMoveToTrash = DeleteIsMoveToTrash();
> > Not used? (Can't comment on the multiple tests without this.)
> 
> Yes this is a leftover from an earlier version of the patch. Now that only
> the flags are checked it's not needed.

In that case I agree that the flag checking could be simplified.

Comment 30

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v5

Carrying fwd r=bienvenu

Simplifying the flags test and porting the context menu changes to sm - earlier i was just making it not breake.

Comment 31

[[e-mail]]

Comment on attachment 574171 [details] [diff] [review]
proposed fix, v5

>+    nsMsgFolderFlags::Drafts|nsMsgFolderFlags::Templates|
>+    nsMsgFolderFlags::SentMail|nsMsgFolderFlags::Archive|nsMsgFolderFlags::Junk|
>+    nsMsgFolderFlags::Trash)));
Please put spaces before and after the | characters. Also Junk should probably moved down with Trash to even up the lines a bit. sr=me with that fixed.

>+            if (isNewsURI(selectedFolder.URI))
>             {
>                 var unsubscribe = ConfirmUnsubscribe(selectedFolder);
>                 if (unsubscribe)
>                     UnSubscribe(selectedFolder);
>+                continue;
I don't think that this continue is needed any more.

>   var isMail = serverType != 'nntp';
>+  var canDelete = (specialFolder == "Junk") ?
>+                  CanRenameDeleteJunkMail(msgFolder.URI) : msgFolder.deletable;
>+
>+  var showRemove = (numSelected <=1) && isMail && canDelete;
> 
>   ShowMenuItem("folderPaneContext-remove", showRemove);
>   if (showRemove)
>     EnableMenuItem("folderPaneContext-remove", msgFolder.isCommandEnabled("cmd_delete"));
>-  if (isMail && !isSpecialFolder)
>+  if (isMail && canDelete)
canDelete is always false for nntp, isn't it? In which case you could remove all the isMail checks. (I don't know about canRename; separate bug I guess.)

Comment 32

[Magnus Melin [:mkmelin]]

changeset:   8899:16b5f81c65f5
http://hg.mozilla.org/comm-central/rev/16b5f81c65f5

->FIXED

For testing: if the flag is set on the trash folder "Delete" shouldn't show up. Altering the delete model will clear/set the flag as appropriate.