Last Comment Bug 39121 - Unspecialize Trash folder for IMAP "Mark as Deleted" & "Remove Immediately" modes
: Unspecialize Trash folder for IMAP "Mark as Deleted" & "Remove Immediately" m...
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: All All
: -- trivial with 1 vote (vote)
: Thunderbird 11.0
Assigned To: Magnus Melin
:
Mentors:
: 91952 (view as bug list)
Depends on:
Blocks: 444147 91958
  Show dependency treegraph
 
Reported: 2000-05-12 14:33 PDT by laurel
Modified: 2012-09-15 17:17 PDT (History)
10 users (show)
mkmelin+mozilla: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
proposed fix (18.04 KB, patch)
2011-10-17 11:10 PDT, Magnus Melin
no flags Details | Diff | Splinter Review
proposed fix v2, with tests (20.20 KB, patch)
2011-11-02 13:44 PDT, Magnus Melin
no flags Details | Diff | Splinter Review
proposed fix v3 (21.82 KB, patch)
2011-11-03 13:30 PDT, Magnus Melin
no flags Details | Diff | Splinter Review
fix namespace issue (15.91 KB, patch)
2011-11-07 18:05 PST, David :Bienvenu
no flags Details | Diff | Splinter Review
ignore xlist trash if the user is using imap delete (16.59 KB, patch)
2011-11-08 08:09 PST, David :Bienvenu
mozilla: review+
Details | Diff | Splinter Review
proposed fix, v5 (25.59 KB, patch)
2011-11-13 12:02 PST, Magnus Melin
mkmelin+mozilla: review+
neil: superreview+
Details | Diff | Splinter Review

Description laurel 2000-05-12 14:33:21 PDT
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 2000-05-12 14:49:17 PDT
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 2000-05-12 14:49:58 PDT
that bug number is bug #39124.
Comment 3 laurel 2000-05-12 14:58:52 PDT
This should also apply to Mark As Deleted. Let me know if you want that as a
separate bug.
Comment 4 scottputterman 2000-05-15 13:08:02 PDT
reassigning to jefft.
Comment 5 Karen Huang 2000-05-16 23:25:49 PDT
I believe this issue has been addressed from some comments of bug 33235....
Comment 6 Karen Huang 2000-05-16 23:30:39 PDT
I mean from bug 33235 2000-04-05 11:58 my comments: 
Comment 7 scottputterman 2000-06-11 13:54:26 PDT
moving to future milestone.
Comment 8 Karen Huang 2000-08-04 16:31:49 PDT
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 2000-11-17 17:43:26 PST
reassigning jefft's bugs to naving
Comment 10 Karen Huang 2001-07-23 12:54:39 PDT
*** Bug 91952 has been marked as a duplicate of this bug. ***
Comment 11 Karen Huang 2001-07-23 13:40:56 PDT
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 seth@sspitzer.org instead) 2003-05-08 10:28:51 PDT
mass re-assign.
Comment 13 Mike Fedyk 2004-03-31 12:25:48 PST
(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 2008-07-08 11:13:41 PDT
version 3.0a2pre (2008070703) - problem still there.
Comment 15 Nikolay Shopik 2009-09-20 05:55:32 PDT
I could delete trash if it never used(nothing trashed) and MAD mode is enabled from beginning.
Comment 16 Magnus Melin 2011-10-17 11:10:18 PDT
Created attachment 567502 [details] [diff] [review]
proposed fix

Not sure what's the easiest way to do an automated test for this...
Comment 17 David :Bienvenu 2011-10-18 18:00:29 PDT
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 2011-11-02 13:44:35 PDT
Created attachment 571441 [details] [diff] [review]
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 2011-11-02 14:00:08 PDT
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 2011-11-03 13:30:06 PDT
Created attachment 571757 [details] [diff] [review]
proposed fix v3

Clearing/setting trash flag when deletemodel changes.
Comment 21 David :Bienvenu 2011-11-03 13:47:18 PDT
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 2011-11-07 13:03:53 PST
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 2011-11-07 18:05:11 PST
Created attachment 572702 [details] [diff] [review]
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 2011-11-08 08:09:11 PST
Created attachment 572819 [details] [diff] [review]
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 2011-11-08 08:21:35 PST
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 2011-11-08 08:40:07 PST
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 neil@parkwaycc.co.uk 2011-11-08 09:53:44 PST
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 2011-11-09 06:40:23 PST
(In reply to neil@parkwaycc.co.uk 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 neil@parkwaycc.co.uk 2011-11-09 14:25:34 PST
(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 2011-11-13 12:02:57 PST
Created attachment 574171 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2011-11-13 13:53:07 PST
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 2011-11-26 10:50:34 PST
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.

Note You need to log in before you can comment on or make changes to this bug.