451728 - Special folder selection in Account Manager does not show server

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[Kent James (:rkent)]

Previously, special folder selections in Account Manager (for example, Copies and Folders, Drafts, Other) showed long folder names including the server. After bug 436630, these were removed (but readded for junk settings only in bug 444220). These special folders can often have the same name on multiple servers, so simply saying "Drafts" without giving the server can lead to misunderstanding about the actual folder selected.

The original "long folder names" should be restored to these selections.

Comment 1

[WADA:World Anti-bad-Duping Agency]

Same problem(short name only) exists in folder selection at "Server Settings/When I delete a message/Move it to this folder" of IMAP account(Trash selection). Problem of Bug 450712(INBOX is always shown by UI) also exists when "Trash".

Comment 2

[WADA:World Anti-bad-Duping Agency]

"Trash selection" has another issue, Bug 427311. Only lowest level name is saved in mail.server.serverX.trash_folder_name. This phenomenon is not seen on other special folders.

To Kent James:
Please DUP them to this bug, if those "Trash selection" related problems are also covered by this bug.

Comment 3

[Magnus Melin [:mkmelin]]

I think those are a bit different different, at least bug 450712. (trash_folder can only be on the same account, it's not a problem with the dropdown)

Comment 4

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #3)
> (trash_folder can only be on the same account, it's not a problem with the dropdown)

Oh, you are absolutely right. I was confused with UI for "Junk selection". 
  - Other than Trash(Selection of Junk, Drafts, Sent, Templates) : 
    - Standard name folder on any account
    - Any folder of any account(Other:) 
  - Trash :
    - Still "any folder of the account" only
     (to set folder name for Trash in mail.server.serverX.trash_folder_name)
Sorry for my comments about different problems from this bug.

To Magnus Melin:  
By the way, we'd better to request UI like "Junk/Drafts/Sent/Templates selection" for Trash, instead of requesting to resolve Bug 427311 or Bug 450712? 

Comment 5

[Magnus Melin [:mkmelin]]

I think it's best to keep the bugs as is.

Comment 6

[:aceman]

I think I can do this. Make all the fields look like "<folder> on <server>" as in the Junk settings. Would it be accepted from the UI part? It looks like yes, as it have wanted-tb3+.

Comment 7

[:aceman]

Attachment: patch

I moved the folder name formatting to amUtils.js and use it from Junk settings and Copies&Folders. I wanted to merge the formatting even with code in msgFolderPickerOverlay.js but it seems Account manager is not using that. I couldn't access code there.

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 595551 [details] [diff] [review]
patch

I'm not a reviewer for mailnews... but anyways.

>+function prettyFolderName(folder, isServer)

Doxygen comment for the function please, it helps people know what the method is about even if it seems trivial while implementing. And shouldn't that isServer be more like showServer?

>+{
>+  var folderName;
>+  var serverName;

lets? But you don't need any variables form the, really.

>+  if (isServer) {
>+    folderName = folder.prettyName;

Early return

>+  } else {

Else would have bee n on a new line. 

>+    if (folder.server)
>+      serverName = folder.server.prettyName;
>+    else {
>+      dump("Can't find server for " + uri + "\n");
>+      serverName = "???";
>+    }

The else shouldn't be necessary. (but if it were, braces for all if/elses if one of them has it.

>+
>+    folderName = document.getElementById("bundle_messenger")
>+                         .getFormattedString("verboseFolderFormat",
>+                         [folder.prettyName, serverName]);
>+  }
>+
>+  return folderName;
>+}

Comment 10

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 595551 [details] [diff] [review]
patch

I'm having a tough time seeing what's changed.  (I think my setup might be broken somehow, since I had a similar problem yesterday.)

To help me ui-review it, could you post screenshots of the affected areas before and after the patch?

Thanks,
Blake.

Comment 11

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 595551 [details] [diff] [review]
patch

Okay, I rebuilt all of Thunderbird and I'm seeing the change now.

Yeah, I like it.  ui-r=me.

Thanks,
Blake.

Comment 12

[:aceman]

(In reply to Magnus Melin from comment #9)
> I'm not a reviewer for mailnews... but anyways.
> 
> >+function prettyFolderName(folder, isServer)
> 
> Doxygen comment for the function please, it helps people know what the
> method is about even if it seems trivial while implementing.

Doxygen comment is like this?
 /**
  * Capture any menulist changes and update the folder property.
  *
  * @param aGroup the prefix for the menulist we're handling (e.g. "drafts")
  * @param aType "Account" for the account picker or "Folder" for the folder
  **/

> And shouldn't
> that isServer be more like showServer?

It should indicate the passed in folder is to be displayed only as a server name, not "folder on server". So yeah, it could be showAsServer.

Comment 13

[:aceman]

Attachment: patch v2

Couldn't find out how to get rid of the last else.

Comment 14

[Magnus Melin [:mkmelin]]

Comment on attachment 597537 [details] [diff] [review]
patch v2


>+/**
>+ * Return folder name formatted with server name if needed.
>+ *
>+ * @param folder       result of GetMsgFolderFromUri(Uri)

@param folder nsIMsgFolder to format name for

>+ * @param showAsServer if TRUE, the folder object is really a server object and only
>+ *                     it's name is shown.

@param showAsServer if true and the folder is a server, only its name is returned. 
                    Otherwise return the name as <foldername> on <servername>

>+ *                     Otherwise show the name as <foldername> on <servername>
>+ **/
>+function prettyFolderName(folder, showAsServer)
>+{
>+  let serverName;
>+
>+  if (!folder)
>+    return "???";

Just let it blow up if no folder was given.

>+  if (showAsServer)
>+    return folder.prettyName;

was this supposed to be something with folder.isServer?

>+  if (folder.server) {
>+    serverName = folder.server.prettyName;
>+  }
>+  else
>+  {
>+    dump("Can't find server for " + folder + "\n");
>+    serverName = "???";
>+  }

Every folder has a server so you don't need this else if at all. Just do

  return document.getElementById("bundle_messenger")
                 .getFormattedString("verboseFolderFormat",
                                     [folder.prettyName, folder.server.prettyName]);

Comment 15

[:aceman]

(In reply to Magnus Melin from comment #14)
Thanks, I took some of the checks from msgFolderPickerOverlay.js so that the functions is very robust. Maybe they are outdated.

Comment 16

[:aceman]

(In reply to Magnus Melin from comment #14)
> >+  if (showAsServer)
> >+    return folder.prettyName;
> 
> was this supposed to be something with folder.isServer?

The caller determines if it wants to format it as a server or a "folder on server". Sometimes it is via .isServer, sometimes it knows it via the position of the field (this was the current state and I didn't touch that).
Therefore I do not check for .isServer here.

Comment 17

[:aceman]

Attachment: patch v3

Comment 18

[Ian Neal]

Comment on attachment 598654 [details] [diff] [review]
patch v3

>+function InitFolderDisplay(folder, folderPicker, rootFolder) {
Would isRootFolder be a better variable name or showAsServer?

r=me with that answered/addressed.

Comment 19

[:aceman]

Attachment: patch v4

Probably yes.

Comment 20

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #16)
> (In reply to Magnus Melin from comment #14)
> > >+  if (showAsServer)
> > >+    return folder.prettyName;
> > 
> > was this supposed to be something with folder.isServer?
> 
> The caller determines if it wants to format it as a server or a "folder on
> server". Sometimes it is via .isServer, sometimes it knows it via the

In that case, at least the function documentation is incorrect.

Comment 21

[:aceman]

Attachment: patch v5

Comment 22

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #21)
> Created attachment 598792 [details] [diff] [review]
> patch v5

Shouldn't that first return be folder.server.prettyName then?

Comment 23

[:aceman]

Well I didn't want to touch the existing logic much. It seems the callers pass an object into folder that is a server and the name can be retrieved directly via .prettyName. So the 'isServer' parameter name had its sense.

But I can try to rework it so that prettyFolderName can check .isServer itself and format it automatically.

Comment 24

[Ian Neal]

(In reply to Magnus Melin from comment #22)
> (In reply to :aceman from comment #21)
> > Created attachment 598792 [details] [diff] [review]
> > patch v5
> 
> Shouldn't that first return be folder.server.prettyName then?

Looking at the existing logic, the patch appears to be correct using foler.prettyName

Comment 25

[:aceman]

Attachment: patch v6

I wanted to say in comment 23 that caller pass a server and set showAsServer=true and other time they pass folder and set showAsServer=false.

But I have redone it to find out what has been passed.

Comment 26

[Ian Neal]

Comment on attachment 598954 [details] [diff] [review]
patch v6

>+  let server = GetMsgFolderFromUri(spamActionTargetAccount);
>+  document.getElementById("actionTargetAccount")
>+          .setAttribute("label", prettyFolderName(server));
Is this extra compared to previous patches?

Comment 27

[Magnus Melin [:mkmelin]]

Comment on attachment 598954 [details] [diff] [review]
patch v6

Ah, i see now, it was assumed the caller had already checked it's a server.

I think this is better though! (untested)

Comment 28

[:aceman]

(In reply to Ian Neal from comment #26)
> Comment on attachment 598954 [details] [diff] [review]
> patch v6
> 
> >+  let server = GetMsgFolderFromUri(spamActionTargetAccount);
> >+  document.getElementById("actionTargetAccount")
> >+          .setAttribute("label", prettyFolderName(server));
> Is this extra compared to previous patches?

Yes. I noticed this one folderpicker in am-junk was not setting the label. Maybe the label was set automatically via .setFolder() (as the label was shown properly regardless). But I added it for consistency as all the other pickers (in am-copies) set the label in all cases.

Comment 29

[Ian Neal]

Comment on attachment 598954 [details] [diff] [review]
patch v6


>   try
>   {
>+    let folder = GetMsgFolderFromUri(spamActionTargetFolder);
>     document.getElementById("actionTargetFolder")
>+            .setAttribute("label", prettyFolderName(folder));
>   }
>   catch (e) { /* OK for folder to not exist */ }
Nit: Can't we just check that the folder is not null rather than try/catch?

r=me either way.

Comment 30

[:aceman]

I am not sure. The spamActionTargetFolder variable must not be null at that spot.
However, the folder may not really exist (on disk) yet. I get "couldn't find folder to select" (something like that) in the Error console (if the try catch is removed). And we do not want to create the folder on disk beforehand, because the user may not even choose to use it (select it as target). So either the GetMsgFolderFromUri is throwing, or something in prettyFolderName is throwing, but I think then the error would be different (if folder were null). Well if GetMsgFolderFromUri throws, the label should not have been set, but it is. So I am not yet sure what happens here so I just left it as it was. I just tested that the try can't be just removed.
Maybe this call should pass 'false' as the second argument?

Comment 31

[:aceman]

I will change this spot again in bug 536768 so I can play with that try {} again. Please land this as is.

Comment 32

[Ludovic Hirlimann [:Usul]]

(In reply to :aceman from comment #31)
> I will change this spot again in bug 536768 so I can play with that try {}
> again. Please land this as is.

doesn't it needs review + before it lands ?

Comment 33

[:aceman]

Surely, that was just a hint for standard8 :)

Comment 34

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/6601c429c675