Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in Thunderbird 11.0

Status

MailNews Core
Backend
--
trivial
RESOLVED FIXED
17 years ago
5 years ago

People

(Reporter: laurel, Assigned: Magnus Melin)

Tracking

(Blocks: 1 bug)

Trunk
Thunderbird 11.0
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

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

Updated

17 years ago
QA Contact: lchiang → huang
(Reporter)

Comment 1

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

Comment 2

17 years ago
that bug number is bug #39124.
(Reporter)

Comment 3

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

Comment 4

17 years ago
reassigning to jefft.
Assignee: putterman → jefft

Updated

17 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → M17

Comment 5

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

Comment 6

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

Comment 7

17 years ago
moving to future milestone.
Target Milestone: M17 → Future

Comment 8

17 years ago
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".
Summary: IMAP: change to Remove Immediately should unspecialize Trash → IMAP: change to Remove Immediately should unspecialize Trash folder and also allow to be deleted

Comment 9

17 years ago
reassigning jefft's bugs to naving
Assignee: jefft → naving
Status: ASSIGNED → NEW

Comment 10

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

Updated

16 years ago
Blocks: 91958

Comment 11

16 years ago
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.

Summary: IMAP: change to Remove Immediately should unspecialize Trash folder and also allow to be deleted → Unspecialize Trash folder for IMAP "Mark as Deleted" & "Remove Immediately" modes
mass re-assign.
Assignee: naving → sspitzer

Comment 13

14 years ago
(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.
Product: MailNews → Core
(Assignee)

Updated

11 years ago
Assignee: sspitzer → nobody
QA Contact: huang → backend

Comment 14

9 years ago
version 3.0a2pre (2008070703) - problem still there.
Priority: P3 → --

Updated

9 years ago
Blocks: 444147
Product: Core → MailNews Core

Comment 15

8 years ago
I could delete trash if it never used(nothing trashed) and MAD mode is enabled from beginning.
Severity: normal → trivial
Target Milestone: Future → ---
(Assignee)

Comment 16

6 years ago
Created attachment 567502 [details] [diff] [review]
proposed fix

Not sure what's the easiest way to do an automated test for this...
Assignee: nobody → mkmelin+mozilla
Status: NEW → ASSIGNED
Attachment #567502 - Flags: superreview?(neil)
Attachment #567502 - Flags: review?(dbienvenu)

Comment 17

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

Comment 18

6 years ago
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.
Attachment #567502 - Attachment is obsolete: true
Attachment #571441 - Flags: superreview?(neil)
Attachment #571441 - Flags: review?(dbienvenu)
Attachment #567502 - Flags: superreview?(neil)
Attachment #567502 - Flags: review?(dbienvenu)

Comment 19

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

Comment 20

6 years ago
Created attachment 571757 [details] [diff] [review]
proposed fix v3

Clearing/setting trash flag when deletemodel changes.
Attachment #571441 - Attachment is obsolete: true
Attachment #571757 - Flags: superreview?(neil)
Attachment #571757 - Flags: review?
Attachment #571441 - Flags: superreview?(neil)
Attachment #571441 - Flags: review?(dbienvenu)

Comment 21

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

thx, Magnus, that looks a lot better. I'll review it soon.
Attachment #571757 - Flags: review? → review?(dbienvenu)

Comment 22

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

6 years ago
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.
Attachment #571757 - Attachment is obsolete: true
Attachment #571757 - Flags: superreview?(neil)
Attachment #571757 - Flags: review?(dbienvenu)

Comment 24

6 years ago
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.
Attachment #572702 - Attachment is obsolete: true

Comment 25

6 years ago
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.
Attachment #572819 - Flags: superreview?(neil)
Attachment #572819 - Flags: review?(dbienvenu)

Comment 26

6 years ago
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...
Attachment #572819 - Flags: review?(dbienvenu) → review+
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
(Assignee)

Comment 28

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

Comment 30

6 years ago
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.
Attachment #572819 - Attachment is obsolete: true
Attachment #574171 - Flags: superreview?(neil)
Attachment #574171 - Flags: review+
Attachment #572819 - Flags: superreview?(neil)
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.)
Attachment #574171 - Flags: superreview?(neil) → superreview+
(Assignee)

Comment 32

6 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 11.0
You need to log in before you can comment on or make changes to this bug.