Special folder selection in Account Manager does not show server

RESOLVED FIXED in Thunderbird 14.0

Status

MailNews Core
Account Manager
RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: rkent, Assigned: aceman)

Tracking

({regression})

Trunk
Thunderbird 14.0
regression
Dependency tree / graph
Bug Flags:
wanted-thunderbird3 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

16.54 KB, patch
Ian Neal
: review+
standard8
: review+
Magnus Melin
: feedback+
Details | Diff | Splinter Review
(Reporter)

Description

9 years ago
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.
Flags: wanted-thunderbird3?

Updated

9 years ago
Flags: wanted-thunderbird3? → wanted-thunderbird3+
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".
Blocks: 450712
"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

9 years ago
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)
No longer blocks: 450712
(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

9 years ago
I think it's best to keep the bugs as is.
(Assignee)

Comment 6

6 years ago
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+.
Keywords: uiwanted
(Assignee)

Updated

6 years ago
Assignee: nobody → acelists
Component: Account Manager → Account Manager
Product: Thunderbird → MailNews Core
QA Contact: account-manager → account-manager
(Assignee)

Comment 7

6 years ago
Created attachment 595551 [details] [diff] [review]
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.
Attachment #595551 - Flags: ui-review?(bwinton)
Attachment #595551 - Flags: review?(mkmelin+mozilla)
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

6 years ago
Duplicate of this bug: 592251

Comment 9

6 years ago
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;
>+}
Attachment #595551 - Flags: review?(mkmelin+mozilla)
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 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.
Attachment #595551 - Flags: ui-review?(bwinton) → ui-review+
(Assignee)

Comment 12

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

Comment 13

6 years ago
Created attachment 597537 [details] [diff] [review]
patch v2

Couldn't find out how to get rid of the last else.
Attachment #595551 - Attachment is obsolete: true
Attachment #597537 - Flags: feedback?(mkmelin+mozilla)

Comment 14

6 years ago
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]);
Attachment #597537 - Flags: feedback?(mkmelin+mozilla) → feedback-
(Assignee)

Comment 15

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

Comment 16

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

Comment 17

6 years ago
Created attachment 598654 [details] [diff] [review]
patch v3
Attachment #597537 - Attachment is obsolete: true
Attachment #598654 - Flags: review?(iann_bugzilla)

Comment 18

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

Comment 19

6 years ago
Created attachment 598676 [details] [diff] [review]
patch v4

Probably yes.
Attachment #598654 - Attachment is obsolete: true
Attachment #598676 - Flags: review?(mbanner)

Comment 20

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

Comment 21

6 years ago
Created attachment 598792 [details] [diff] [review]
patch v5
Attachment #598676 - Attachment is obsolete: true
Attachment #598792 - Flags: review?(mbanner)
Attachment #598676 - Flags: review?(mbanner)

Comment 22

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

Comment 23

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

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

Comment 25

6 years ago
Created attachment 598954 [details] [diff] [review]
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.
Attachment #598792 - Attachment is obsolete: true
Attachment #598954 - Flags: review?(iann_bugzilla)
Attachment #598954 - Flags: feedback?(mkmelin+mozilla)
Attachment #598792 - Flags: review?(mbanner)

Comment 26

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

6 years ago
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)
Attachment #598954 - Flags: feedback?(mkmelin+mozilla) → feedback+
(Assignee)

Comment 28

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

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

Comment 30

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

Updated

6 years ago
Attachment #598954 - Flags: review?(mbanner)
(Assignee)

Comment 31

6 years ago
I will change this spot again in bug 536768 so I can play with that try {} again. Please land this as is.
(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 ?
(Assignee)

Comment 33

5 years ago
Surely, that was just a hint for standard8 :)
Attachment #598954 - Flags: review?(mbanner) → review+
(Assignee)

Updated

5 years ago
Keywords: uiwanted → checkin-needed
Checked in: http://hg.mozilla.org/comm-central/rev/6601c429c675
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 14.0
(Assignee)

Updated

5 years ago
Depends on: 768779
You need to log in before you can comment on or make changes to this bug.