467249 - Modify folderWidgets.xml to be able to XBLify the Go menu in MailNews

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Frank Wein [:mcsmurf]]

mailnews/base/resources/content/folderWidgets.xml needs to be modified to fix Bug 444913 for SeaMonkey. Currently it's not possible to list all accounts and folders and have an account name as label for the selection of the account itself (code is needed to replace showFileHereLabel with the account name).

Comment 1

[Frank Wein [:mcsmurf]]

Attachment: Patch

Comment 2

[Frank Wein [:mcsmurf]]

Comment on attachment 350658 [details] [diff] [review]
Patch

>+            } else {
>+              menuitem.setAttribute("label", this.getAttribute("fileHereLabel"));
>+              menuitem.setAttribute("accesskey", this.getAttribute("fileHereAccessKey"));
>
>+}

The indentation of the } is of course wrong, fixed locally.

Comment 3

[Joey Minta]

Comment on attachment 350658 [details] [diff] [review]
Patch

+        select: function filter_select(aFolder) {
+          // Generic filter, used to display all accounts/folders in the "Go"
+          // menu (SeaMonkey)
+          return true;

How about making this mode "navigation", since really ever mode is about selecting folders?

-            menuitem.setAttribute("accesskey", this.getAttribute("fileHereAccessKey"));
+            if (mode == "select") {
+              menuitem.setAttribute("label", this._parentFolder.prettyName);
+              menuitem.setAttribute("class", "folderMenuItem menuitem-iconic");
+              this._setCssSelectors(this._parentFolder, menuitem);
+            } else {
+
Can you put the common case, != first? 

This is ugly, but you're right, I can't think of a better way.

Comment 4

[[e-mail]]

Comment on attachment 350658 [details] [diff] [review]
Patch

>+        select: function filter_select(aFolder) {
>+          // Generic filter, used to display all accounts/folders in the "Go"
>+          // menu (SeaMonkey)
>+          return true;
What's the difference between mode="select" and the empty mode?

>-          if (this.getAttribute("showFileHereLabel") == "true" &&
>-              this._parentFolder && 
>-              ((this._parentFolder.noSelect || this._parentFolder.canFileMessages) ||
>-               mode == "newFolder")) {
>+          if (this._parentFolder &&
>+              (this.getAttribute("showFileHereLabel") == "true" &&
>+               (this._parentFolder.noSelect ||
>+                 this._parentFolder.canFileMessages || mode == "newFolder") ||
>+               mode == "select")) {
I'd put the mode == "select" as the first || half of the outer second && half.
if (this._parentFolder &&
    (mode == "select" ||
     (this.getAttribute("showFileHereLabel") == "true" &&
      (this._parentFolder.noSelect ||
       this._parentFolder.canFileMessages ||
       mode == "newFolder")))) {
[IMHO noSelect looks bogus here, but then again I don't have an IMAP server that requires it to test. Mind you, all sorts of code in that file looks bogus.]

Comment 5

[Frank Wein [:mcsmurf]]

Attachment: Patch

Forward r+ from Attachment 350658 [details] [diff]

Comment 6

[Frank Wein [:mcsmurf]]

Comment on attachment 362878 [details] [diff] [review]
Patch

Neil: You were correct, empty mode works, too.

Comment 7

[Frank Wein [:mcsmurf]]

Attachment: Patch

Ok, this is the one.

Comment 8

[[e-mail]]

Comment on attachment 362879 [details] [diff] [review]
Patch

>+          if (this._parentFolder && (mode == "" ||
I'd prefer it if you used !mode [or (mode)] as the check.

Comment 9

[Frank Wein [:mcsmurf]]

Comment on attachment 362879 [details] [diff] [review]
Patch

This does not work as Thunderbird does not define the mode attribute in one menupopup, see http://hg.mozilla.org/comm-central/annotate/b5562be0758b/mail/base/content/FilterListDialog.xul#l138 and relies on the old behavior. We don't want to break this of course.

Comment 10

[Frank Wein [:mcsmurf]]

Attachment: Patch

Ok, this should do it. If the fileHereLabel is present, set it as label, if not set the generic folder name as label instead.

Comment 11

[Frank Wein [:mcsmurf]]

Comment on attachment 366698 [details] [diff] [review]
Patch

Ok, this should finally work now. No extra mode required, we can check if the fileHereLabel attribute is present and decide what label to set based on that.

Comment 12

[Joey Minta]

Comment on attachment 366698 [details] [diff] [review]
Patch

I think this if block is getting complicated enough that we need a nice, clear comment explaining what's going on.  Something like...

We will include a menuitem referring back to the parent-folder when either (1) There's a showFileHereLabel attribute or (2) there's no mode attribute. However, we won't do this, even with the above factors when it doesn't make sense. That is, the following conditions override the above requests for a parent-folder reference: (1) The parent folder is no-select or (2) there's no _parentFolder reference for this copy of the widget.

When creating that parent-reference, we will label it with the value of the folderHereLabel attribute, or if that does not exist, with the name of the parent folder.

Can you attach another version with a similarly worded comment and that fixes the no-select problems discussed on irc?

Comment 13

[Frank Wein [:mcsmurf]]

Attachment: Patch

The old code regarding noSelect is actually ok as far as I see this. We provide visual feedback that way that this folder cannot be selected (the "file here" menuitem gets disabled if noSelect is true). Note that this patch changes the behavior for the code at http://hg.mozilla.org/comm-central/annotate/b5562be0758b/mail/base/content/FilterListDialog.xul#l138: Currently if the folder can be selected and no messages can be filed into that folder it does not add the fileHereLabel menu item. This is not correct though as the user can run mail filters on such a folder, only the filter actions of the mail filter matter in this case. An example would be an IMAP folder with the "i" (insert) right not set via ACL and subfolders below that folder. The patch also fixes this issue via !mode

Comment 14

[Joey Minta]

Comment on attachment 367472 [details] [diff] [review]
Patch

Not wild about overloading the lack of mode, but let's cross that bridge when/if it becomes a problem.

r=jminta, with may apologies for the delay

Comment 15

[Frank Wein [:mcsmurf]]

Pushed to comm-central, http://hg.mozilla.org/comm-central/rev/7036c57fcce4