Last Comment Bug 451728 - Special folder selection in Account Manager does not show server
: Special folder selection in Account Manager does not show server
Status: RESOLVED FIXED
: regression
Product: MailNews Core
Classification: Components
Component: Account Manager (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 14.0
Assigned To: :aceman
:
Mentors:
: 592251 (view as bug list)
Depends on: 768779
Blocks: 436630
  Show dependency treegraph
 
Reported: 2008-08-22 10:09 PDT by Kent James (:rkent)
Modified: 2012-06-26 23:41 PDT (History)
7 users (show)
mkmelin+mozilla: wanted‑thunderbird3+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (15.80 KB, patch)
2012-02-08 15:12 PST, :aceman
bwinton: ui‑review+
Details | Diff | Review
patch v2 (16.14 KB, patch)
2012-02-15 13:19 PST, :aceman
mkmelin+mozilla: feedback-
Details | Diff | Review
patch v3 (15.90 KB, patch)
2012-02-19 05:06 PST, :aceman
iann_bugzilla: review+
Details | Diff | Review
patch v4 (15.92 KB, patch)
2012-02-19 10:58 PST, :aceman
no flags Details | Diff | Review
patch v5 (15.92 KB, patch)
2012-02-20 01:02 PST, :aceman
no flags Details | Diff | Review
patch v6 (16.54 KB, patch)
2012-02-20 13:55 PST, :aceman
iann_bugzilla: review+
standard8: review+
mkmelin+mozilla: feedback+
Details | Diff | Review

Description Kent James (:rkent) 2008-08-22 10:09:51 PDT
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 2008-08-25 17:35:44 PDT
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 2008-08-25 17:55:23 PDT
"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 2008-08-25 22:57:07 PDT
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 2008-08-26 03:31:12 PDT
(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 2008-08-27 11:21:04 PDT
I think it's best to keep the bugs as is.
Comment 6 :aceman 2012-02-08 08:47:04 PST
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 2012-02-08 15:12:15 PST
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.
Comment 8 :aceman 2012-02-11 16:51:38 PST
*** Bug 592251 has been marked as a duplicate of this bug. ***
Comment 9 Magnus Melin 2012-02-14 13:46:43 PST
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) (:☕️) 2012-02-14 14:44:23 PST
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) (:☕️) 2012-02-14 15:11:13 PST
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 2012-02-15 00:30:01 PST
(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 2012-02-15 13:19:09 PST
Created attachment 597537 [details] [diff] [review]
patch v2

Couldn't find out how to get rid of the last else.
Comment 14 Magnus Melin 2012-02-19 04:00:01 PST
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 2012-02-19 04:38:14 PST
(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 2012-02-19 04:45:03 PST
(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 2012-02-19 05:06:16 PST
Created attachment 598654 [details] [diff] [review]
patch v3
Comment 18 Ian Neal 2012-02-19 10:35:15 PST
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 2012-02-19 10:58:56 PST
Created attachment 598676 [details] [diff] [review]
patch v4

Probably yes.
Comment 20 Magnus Melin 2012-02-20 00:18:42 PST
(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 2012-02-20 01:02:31 PST
Created attachment 598792 [details] [diff] [review]
patch v5
Comment 22 Magnus Melin 2012-02-20 01:30:25 PST
(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 2012-02-20 12:52:36 PST
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 2012-02-20 13:35:05 PST
(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 2012-02-20 13:55:29 PST
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.
Comment 26 Ian Neal 2012-02-20 15:52:03 PST
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 2012-02-20 22:11:44 PST
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 2012-02-21 00:20:11 PST
(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 2012-02-21 08:19:00 PST
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 2012-02-21 08:35:52 PST
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 2012-02-22 11:54:58 PST
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] 2012-03-13 06:07:34 PDT
(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 2012-03-13 10:03:33 PDT
Surely, that was just a hint for standard8 :)
Comment 34 Mark Banner (:standard8) 2012-03-26 02:48:26 PDT
Checked in: http://hg.mozilla.org/comm-central/rev/6601c429c675

Note You need to log in before you can comment on or make changes to this bug.