Closed Bug 467249 Opened 11 years ago Closed 10 years ago

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

Categories

(MailNews Core :: Backend, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b4

People

(Reporter: mcsmurf, Assigned: mcsmurf)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

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).
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → bugzilla
Status: NEW → ASSIGNED
Attachment #350658 - Flags: review?(jminta)
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.
Blocks: 444913
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.
Attachment #350658 - Flags: review?(jminta) → review+
Attachment #350658 - Flags: superreview?(bienvenu)
Attachment #350658 - Flags: superreview?(bienvenu) → superreview?(neil)
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.]
Attached patch Patch (obsolete) — Splinter Review
Forward r+ from Attachment 350658 [details] [diff]
Attachment #362878 - Flags: review+
Attachment #350658 - Attachment is obsolete: true
Attachment #350658 - Flags: superreview?(neil)
Attachment #362878 - Flags: superreview?(neil)
Comment on attachment 362878 [details] [diff] [review]
Patch

Neil: You were correct, empty mode works, too.
Attached patch Patch (obsolete) — Splinter Review
Ok, this is the one.
Attachment #362878 - Attachment is obsolete: true
Attachment #362879 - Flags: superreview?(neil)
Attachment #362879 - Flags: review+
Attachment #362878 - Flags: superreview?(neil)
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.
Attachment #362879 - Flags: superreview?(neil) → superreview+
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.
Attachment #362879 - Attachment is obsolete: true
Attached patch Patch (obsolete) — Splinter Review
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 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.
Attachment #366698 - Flags: review?(jminta)
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?
Attached patch PatchSplinter Review
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
Attachment #366698 - Attachment is obsolete: true
Attachment #367472 - Flags: review?(jminta)
Attachment #366698 - Flags: review?(jminta)
Attachment #367472 - Flags: review?(jminta) → review+
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
Pushed to comm-central, http://hg.mozilla.org/comm-central/rev/7036c57fcce4
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Component: MailNews: Backend → Backend
Product: SeaMonkey → MailNews Core
QA Contact: mailnews-backend → backend
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0b4
You need to log in before you can comment on or make changes to this bug.