427311 - Choice of a subfolder at "When I delete a message/Move it to this folder" generate string of only lowest level name in mail.server.serverX.trash_folder_name

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, defect)

Description

[WADA:World Anti-bad-Duping Agency]

Tb trunk has improved UI to set mail.server.serverX.trash_folder_name. But it doesn't work, if mail folder user want to use as "trash bin" is subfolder of a parent folder(tested with Tb trunk 2008/4/02 build on MS Win-XP SP2).
If subfolder is selected as "trash bin" folder, only lowest level folder name is saved in mail.server.serverX.trash_folder_name, instead of full folder structure.

(1) Create a subfolder for "trash bin". 
    When Gmail IMAP, [Gmail]/Trash is standard one.
    I tested with My_Label/My_Trash_X (no other folder named My_Trash_X).
(2) Account Settings/Server Settings of a IMAP account
(3) Check "When I delete a message/Move it to this folder"
(4) Expand selection list of folder
(5) Choose My_Label/My_Trash_X (My_Trash_X folder under My_Label folder)
(6) Close Account Setting panel
(7) Config Editor : check mail.server.serverX.trash_folder_name
    ==> My_Trash_X is set, instead of My_Label/My_Trash_X
(8) Account Settings/Server Settings again
    ==> Folder choice in trash bin selection box becomes blank
(9) Close Account Settings
    ==> mail.server.serverX.trash_folder_name is not changed(still My_Trash_X)

Expected Result:
  mail.server.serverX.trash_folder_name=My_Label/My_Trash_X (or [Gmail]/Trash)

Note: If the UI improvement is done on Semonkey trunk too, I think same problem will occur, but test with Semonkey trunk is not done yet.

Comment 1

[WADA:World Anti-bad-Duping Agency]

Note: Impact of this bug is very low when Gmail IMAP, because recommendation of delete model is "Remove them immediately" when Gmail IMAP (use of "move to [Gmail]/Trash" model is rather dangerous when Gmail IMAP). See Bug 400931 Comment #8 & Bug 400931 Comment #9 for this. See also Bug 417167 & Bug 427007 for special situation when Gmail IMAP.

Comment 2

[WADA:World Anti-bad-Duping Agency]

Recently, situation becomes slightly worse than before... See Bug 450712. 

Comment 3

[Magnus Melin [:mkmelin]]

Attachment: proposed fix

Patch to apply on top of the fix for bug 450712.

Comment 4

[[e-mail]]

Comment on attachment 340784 [details] [diff] [review]
proposed fix

>+/** Generate the relative folder path from the root. */
>+function getFolderPathFromRoot(folder)
>+{
>+  var path = folder.name;
>+  var tmpParent = folder.parent;
>+  while (tmpParent && tmpParent.URI != folder.rootFolder.URI) {
>+    path = tmpParent.name + "/" + path;
>+    tmpParent = tmpParent.parent;
>+  }
>+  return path;
Can't you just slice the root URI off the folder's URI?

Comment 5

[Magnus Melin [:mkmelin]]

Unfortunately no. IMAP URIs are ugly beasts in case you have non-ascii in them.

E.g. test/åäö_trash would be imap://[e-mail]/test/%26AOUA5AD2-_trash

And there doesn't seem to exist a javascript way to decode those as normal url decoding doesn't apply.

Comment 6

[[e-mail]]

Comment on attachment 340784 [details] [diff] [review]
proposed fix

I'm going to have to pass on this one, as I can't convince myself that the backend supports trash subfolders (except children of Inbox).

>+  var tmpParent = folder.parent;
>+  while (tmpParent && tmpParent.URI != folder.rootFolder.URI) {
>+    path = tmpParent.name + "/" + path;
>+    tmpParent = tmpParent.parent;
>+  }
Nit: could make this a for loop.
Nit: could call tmpParent just parent. Or folder ;-)
Nit: IIRC you don't have to compare URIs, just == on the XPCOM objects.

Comment 7

[Magnus Melin [:mkmelin]]

(In reply to comment #6)
> (From update of attachment 340784 [details] [diff] [review])
> I'm going to have to pass on this one, as I can't convince myself that the
> backend supports trash subfolders (except children of Inbox).

It does work just fine. Without this patch it's very confusing trying to set up a gmail account (which uses [Gmail]/Trash), because you just end up creating a Trash tag instead.

Comment 8

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v2

Using direct " == " comparison works fine, thx!
I'd rather use the while loop here as it's easier to read. 
Also parent.parent looked odd, so i made it named parentFolder.

Comment 9

[Magnus Melin [:mkmelin]]

changeset:   527:70c3c9246dec
http://hg.mozilla.org/comm-central/rev/70c3c9246dec

->FIXED

Comment 11

[Jorg K (CEST = GMT+2)]

Just for the record, http://hg.mozilla.org/comm-central/rev/70c3c9246dec was pretty wrong. The right way to do it is here: https://hg.mozilla.org/comm-central/rev/35d2dc1476b40f7414ade6efffe758e3e10a2cc9#l1.13 (bug 1430168).

(In reply to [e-mail] from comment #4)
> Can't you just slice the root URI off the folder's URI?
Yes, that's the correct approach.

(In reply to Magnus Melin from comment #5)
> And there doesn't seem to exist a javascript way to decode those as normal
> url decoding doesn't apply.
The JS I'm using here is new, but back in the days, the scriptable unicode decoder would have handled MUTF-7 just fine. Now that it's build on Rust, M-C dropped the MUTF-7 "bolt on" support, so we're using our own.