1427507 - Selecting a different trash folder under INBOX/ does not take effect easily

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[gene smith]

User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20171130114032

Steps to reproduce:

Assume "Trash" is configured as a sub-folder of INBOX, and delete method configured as "Move it to this folder: Trash on <my-acct>" and all is working fine and the expected trash folder icon is present on INBOX/Trash. Then find (or create) another folder under INBOX called, for example, "alt-trash".  Now set "Move it to this folder: alt-trash on <my-acct> and click OK.


Actual results:

Only normal icons are present on both INBOX/Trash and INBOX/alt-trash. Collapsing and expanding the folder tree for the account and even restarting thunderbird does not cause the special trash icon to appear on alt-trash. Also, any email deleted from INBOX or other folders while in this state (no trash icon on any folder) does not appear in alt-trash or Trash and seems to be lost. 


Expected results:

Ideally the trash icon should immediately appear on the INBOX/alt-trash folder and deleted items should be moved to it. Less ideal, but maybe acceptable, INBOX/alt-trash icon should appear and become the working 
deletion destination  after collapsing and expanding the folder tree for the account or after a thunderbird restart. 

This can be resolved by setting the delete method to either "Just mark it as deleted" or "Remove it immediately" in the server settings and click OK. Then in server settings again select "Move it to this folers: alt-trash on <my-acct>" and click OK. Now the trash icon is present on INBOX/alt-trash and deletions are moved there and not lost. Having to do this to obtain the expected result (and avoid possible date loss!) is not intuitive. 

Note: This bug was found while working on bug 1335982. The reporter found that an additional folder named "Trash" with the trash icon being created by TB at the same levels as INBOX while keeping the original INBOX/Trash folder, also with trash icon. So two seemingly valid trash folder were visible.

Comment 1

[gene smith]

This bug only occurs if the personal namespace delimiter on the folder path is dot ('.'). When the personal namespace delimiter is the more typical slash ('/'), contracting and expanding the account folder tree marks the newly designated trash folder with the trash icon and deleted items are moved to it. The dovecot imap server uses '.' as the delimiter, e.g., trash path is INBOX.Trash instead of INBOX/Trash.

Comment 2

[gene smith]

(In reply to gene smith from comment #1)
> This bug only occurs if the personal namespace delimiter on the folder path
> is dot ('.'). ...

However the other aspect of this bug, where selecting a new trash folder causes no folder to be the trash destination (no trash icon) and for any email deleted to be seemingly lost, occurs for all imap servers and accounts regardless of namespace delimiter. 

The reason why a dot as the personal namespace delimiter causes failure to detect the trash folder is because in nsImapProtocol.cpp in function DiscoverMailboxSpec() the call to AllocateCanonicalPath() assumes the trash folder path, e.g., "INBOX/Trash" is not already "canonical" which, I think, means slash separated. But the paths passed to AllocateCanonicalPath() at this line are *all* already slash separated, not the non-canonical "INBOX.Trash" that is expects. So when this function is passed the path "INBOX/Trash" and is also passed the delimiter '.' (dovecot server's namespace delimiter), it sees the / as a character that must be escaped by substituting a '^", so it returns the string "INBOX^Trash". This causes the bool variable trashExists to remain false and the kImapTrash bit to never be set in the mBoxFlags. This, in turn, causes the trash folder to not be marked as trash on folder tree contract/expand or on TB restart so the folder remains without an icon and no imap move/copy occurs when the user deletes a message; the email just has the \deleted flag set so it appears to be gone.

This is fixed by removing the call to AllocateCanonicalPath() and the variable serverTrashMatch and just using the already canonical trashMatch string (e.g., "INBOX/Trash") in place of serverTrashMatch. With this change, contract/expand of folder tree or tb restart causes the newly designated trash folder to be correctly marked and enabled.

Of course, this change still requires that a restart or account folder tree collapse/expand occur for the newly selected trash folder to be actually enabled. This can be fixed so the folder is instantly recognized, with the trash icon set, by adding code to SetTrashFolderName() in nsIMapIncomingServer.cpp. This function is only called by UI javascript code when the trash destination folder is set or changed in server settings. I have added setting the "Trash" flag for the newly selected trash folder. Originally SetTrashFolderName() only cleared the trash flag for the previous trash folder and stored the new folder path in pref "trash_folder_name".

I will attach the patch next.

Comment 3

[gene smith]

Attachment: 1427507-review-changes0.patch

See Comment 2 for detailed description of this patch.

Note: The bug seems to be completely fixed by the patch even if the changes in nsImapProtocol.cpp are not included. Without nsImapProtocol.cpp changes (removing call to AllocateCanonicalPath()), variable trashExists remains false for servers with personal namespace delimiter dot. I don't see ill effects without these changes in my testing, provide changes in nsImapIncomingServer.cpp remain in place.

However, since it appears that all trash paths that are processed by this change in nsImapProtocol.cpp are already "canonical" (or slash separated), I recommend keeping this in the fix.

Comment 4

[gene smith]

Found bug while doing further testing on this bug. Probably should be a new bug or maybe not a bug, not sure yet.

If a folder has Russian characters in the name and it is assigned to trash, the trash icon appears and you can delete to it OK and it has the icon and works just like folder names with ascii chars.  But if you go back and look at the setting again (in server setting screen) the trash destination now says "Choose folder..." instead of the expected Russian characters and the account name.

When you originally assign the Russian folder name to trash, it shows up with the expected characters in the "picker" box on the server settings screen. 

Or maybe this doesn't work because I am not localized to Russian? (Jorg, this is with that Russian test account from last summer that still seems to work!)

Comment 5

[gene smith]

(In reply to gene smith from comment #4)
> Found bug while doing further testing on this bug. Probably should be a new
> bug or maybe not a bug, not sure yet.

I have enter new Bug 1427940 for this.

Comment 6

[Jorg K (CEST = GMT+2)]

Comment on attachment 8939464 [details] [diff] [review]
1427507-review-changes0.patch

I'm cancelling the review for now to focus on bug 1428666. The changes in nsImapProtocol.cpp are slightly different, for example you're not using a prefix any more. Let's revisit this once the other bug is done.

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: 1427507-review-changes0.patch

Refreshed patch after bug 1335982 and bug 1428666.

Without the patch I can select a different trash folder in the UI and the trash icon is removed from the previous trash folder. It takes a restart for the trash icon to appear on the new trash folder.

With the patch, the icon is moved instantly. I took the liberty to adjust the string processing a little.

That should conclude trash handling for English trashes. We know that setting the trash in the account manager in localised versions stores localised names instead of server path. We'll follow up in another bug.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/96bc2a18af75
Ensure newly selected trash folder is enabled and has trash icon. r=jorgk

Comment 9

[Jorg K (CEST = GMT+2)]

After just landing three "handle trash better" bugs, bug 1335982 and bug 1428666 and this one here, let's head over to bug 1427940.

Comment 10

[Jorg K (CEST = GMT+2)]

Comment on attachment 8942211 [details] [diff] [review]
1427507-review-changes0.patch

This would need to go with four more bugs and it's not worth it at TB 52.7 when TB 60 ESR is coming out six weeks later.