Mozilla/5.0 (X11; Linux x86_64; rv:8.0a1) Gecko/20110810 Firefox/8.0a1 SeaMonkey/2.5a1 ID:20110810003133 SeaMonkey-specific followup to bug 564423 MailNews Core::Database bug 564423 (fixed 2011-08-09 before 09:53:37 PDT, see bug 564423 comment #32) assumes a menuitem somewhere (probably somewhere in the mailnews View menu), "Show all body parts", to set mailnews.display.html_as to the new value 4. AFAICT such a menuitem does not exist in SeaMonkey (though the present nightly was built on the next day, more than 14 hours later).

That's a workaround for the changes bug 351224 introduced, no longer showing certain MIME parts in the attachment list that users would like to access, and also for recovering malformed messages which put attachments into a "related" context. The additional menu item has been introduced in Thunderbird bug 602718 and needs to be explicitly activated by the user with a hidden preference.

Looks like the following changes made in bug 602718 need to be ported to suite/ * mail/base/content/mailWindowOverlay.js * mail/base/content/mailWindowOverlay.xul * mail/locales/en-US/chrome/messenger/messenger.dtd

Indeed (see bug 564423 comment #37) the hidden preference mailnews.display.show_all_body_parts_menu adds the additional menuitem under "View → Message Body As" in Shredder but not in SeaMonkey. I'm repeating here for reference the user-agent strings and build IDs of the versions I tested: Mozilla/5.0 (X11; Linux x86_64; rv:8.0a1) Gecko/20110810 Firefox/8.0a1 SeaMonkey/2.5a1 ID:20110810003133 Mozilla/5.0 (X11; Linux x86_64; rv:8.0a1) Gecko/20110810 Thunderbird/8.0a1 ID:20110810030009

(In reply to Tony Mechelynck [:tonymec] from bug 564423 comment #36) > (In reply to rsx11m from bug 564423 comment #35) > > Tony, please see bug 602718. It's in View > Message Body As, but you have to > > toggle mailnews.display.show_all_body_parts_menu first before it's available. > > Hm, thanks for telling me. Not very «discoverable» IMHO. If I remember correctly the reason for preffing it off by default in Thunderbird was not to overwhelm the user with an option that's difficult to understand due to it's technical nature (and results in a display with mostly broken image boxes in HTML messages as all images are grouped as attachments with that setting). Of course, SeaMonkey could ignore the hidden pref and always show the option, or set it "true" by default to make it more visible.

plain copy of the changes mentioned in comment #2 Only differences: - vim-style modeline added - conforming with existing coding style (very different from Thunderbird in windowOverlay.xul) Reviewers, if this patch makes it, it will be my first. Don't hesitate to nitpick, that's the way for me to learn.

Does this show that menuitem for a feed, or am I overlooking something?

oops! typo! missing closing quote in v1.0

Comment on attachment 552261 [details] [diff] [review] patch v1.01 can't I copy two simple flags? I really need some sleep.

and when I said that... Let's hope I fixed the typo in the correction of the typo this time :-/

Comment on attachment 552263 [details] [diff] [review] patch v1.02 >+ var AllBodyParts_menuitem = document.getElementById(menuIDs[3]); This function is also called from the Feed body menu, in which case menuIDs[3] doesn't exist... >+ document.getElementById("bodyAllParts").hidden = >+ ! pref.getBoolPref("mailnews.display.show_all_body_parts_menu"); ... but maybe you could write this as if (AllBodyParts_menuitem) AllBodyParts_menuitem.hidden = !Services.prefs.getBoolPref("mailnews.display.show_all_body_parts_menu"); (yes, we have too many prefs variables, we're trying to eliminate them...) >+ else if (!prefer_plaintext && html_as == 4 && !disallow_classes && >+ AllowHTML_menuitem) Shouldn't this be AllBodyParts_menuitem?

(In reply to neil@parkwaycc.co.uk from comment #10) > Comment on attachment 552263 [details] [diff] [review] > patch v1.02 > > >+ var AllBodyParts_menuitem = document.getElementById(menuIDs[3]); > This function is also called from the Feed body menu, in which case > menuIDs[3] doesn't exist... Hm, sorry, what do you mean? Must be something I never used — not the News & Blogs menubar, which includes a View menu. Something not present in Thunderbird then, and I should somehow guard this, or set it up dynamically (testing for the menu existence) rather than statically? > > >+ document.getElementById("bodyAllParts").hidden = > >+ ! pref.getBoolPref("mailnews.display.show_all_body_parts_menu"); > ... but maybe you could write this as > if (AllBodyParts_menuitem) > AllBodyParts_menuitem.hidden = > !Services.prefs.getBoolPref("mailnews.display.show_all_body_parts_menu"); > (yes, we have too many prefs variables, we're trying to eliminate them...) would that take care of that feed-body menu problem? > > >+ else if (!prefer_plaintext && html_as == 4 && !disallow_classes && > >+ AllowHTML_menuitem) > Shouldn't this be AllBodyParts_menuitem? You mean there is an error in mail/base/content/mailWindowOverlay.js line 498 as patched by bug 602718 attachment 546626 [details] [diff] [review] ? Then that other bug should be REOPENED? It does look strange the way it is written.

D'oh! Yes, that's definitely a bug in my patch to mailWindowOverlay.js. In the last chunk shown above, AllowHTML_menuitem should be AllBodyParts_menuitem as you noted. My fault, sorry. I don't think the bug I submitted that patch in should be reopened. This is a new bug. :-) We should just fix it. I can open a separate bug for it if you want and submit a patch myself (it's my bug, after all :-), or you can just fix it in this bug. Concerning the comment about calling getMenuById(menuIDs[3]) when menuIDs[3] is null, I don't think that hurts anything; it'll just return null, which is functionally correct.

(In reply to Tony Mechelynck from comment #11) > (In reply to neil@parkwaycc.co.uk from comment #10) > > (From update of attachment 552263 [details] [diff] [review]) > > >+ var AllBodyParts_menuitem = document.getElementById(menuIDs[3]); > > This function is also called from the Feed body menu, in which case > > menuIDs[3] doesn't exist... > Hm, sorry, what do you mean? Must be something I never used — not the News & > Blogs menubar, which includes a View menu. Something not present in > Thunderbird then, and I should somehow guard this, or set it up dynamically > (testing for the menu existence) rather than statically? View - Feed Body As, but it only appears when you're reading a feed. > > >+ document.getElementById("bodyAllParts").hidden = > > >+ ! pref.getBoolPref("mailnews.display.show_all_body_parts_menu"); > > ... but maybe you could write this as > > if (AllBodyParts_menuitem) > > AllBodyParts_menuitem.hidden = > > !Services.prefs.getBoolPref("mailnews.display.show_all_body_parts_menu"); > > (yes, we have too many prefs variables, we're trying to eliminate them...) > would that take care of that feed-body menu problem? Not directly, because you would still get the Error Console warning. I just wanted to avoid calling document.getElementById again, especially for the Feed body menu, which doesn't even have that item. (In reply to Jonathan Kamens from comment #12) > Concerning the comment about calling getMenuById(menuIDs[3]) when menuIDs[3] > is null, I don't think that hurts anything; it'll just return null, which is > functionally correct. It logs a warning to the error console though.

(In reply to neil@parkwaycc.co.uk from comment #13) > (In reply to Jonathan Kamens from comment #12) > > Concerning the comment about calling getMenuById(menuIDs[3]) when menuIDs[3] > > is null, I don't think that hurts anything; it'll just return null, which is > > functionally correct. > It logs a warning to the error console though. Does it? I don't see that. Maybe only when strict is enabled? I suppose if we're worried about those the fix is obvious enough... just precede that line of code with if (menuIDs[3]).

(In reply to Jonathan Kamens from comment #14) > (In reply to neil@parkwaycc.co.uk from comment #13) > > (In reply to Jonathan Kamens from comment #12) > > > Concerning the comment about calling getMenuById(menuIDs[3]) when menuIDs[3] > > > is null, I don't think that hurts anything; it'll just return null, which is > > > functionally correct. > > It logs a warning to the error console though. > Does it? I don't see that. Maybe only when strict is enabled? It should do. See http://mxr.mozilla.org/comm-central/ident?i=ReportEmptyGetElementByIdArg

(In reply to Jonathan Kamens from comment #12) > I don't think the bug I submitted that patch in should be reopened. This is > a new bug. :-) We should just fix it. I can open a separate bug for it if > you want and submit a patch myself (it's my bug, after all :-), or you can > just fix it in this bug. Given that the menus have to be fixed separately in mail/ and suite/ you should probably clone your original bug and mirror anything done here for Thunderbird.

updated mailWindowOverlay.js after re-reading Neil's comments (carrying over ui-r+) I've tried to guard against the isFeed possibility, I hope I did it correctly. the Thunderbird "error?" line (currently commented-out) will have to disappear in the final version, possibly after fixing the Thunderbird mailWindowOverlay.js

In reply to comment #12: considering that Thunderbird & SeaMonkey are different products with different owners & peers, I think it would be more "according to the rules" if I didn't fix a Thunderbird error in a SeaMonkey bug. Go ahead, it's your bug after all. In reply to comment #13: sorry, I didn't see you had written this comment while I was amending the patch according to Neil's remarks. The way I did it, menuIDs[3] should not get accessed at all in the isFeed case.

oops, wrong patch. Learning how to use hg diff, hg qpush, hg qpop, etc. carrying over sr+=Neil

er, I mean uir+

Comment on attachment 552423 [details] [diff] [review] patch v2.01 >+++ b/suite/mailnews/mailWindowOverlay.js >@@ -1,9 +1,10 @@ > /* -*- Mode: javascript; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ >+/* vim: set expandtab shiftwidth=2 :*/ Nit: this change not needed. >+ if (! isFeed) { >+ AllBodyParts_menuitem = document.getElementById(menuIDs[3]); >+ AllBodyParts_menuitem.hidden = >+ ! pref.getBoolPref("mailnews.display.show_all_body_parts_menu"); >+ } Nit: No space after ! please so !isFeed and !pref >+ else if (!prefer_plaintext && html_as == 4 && !disallow_classes && >+ /* AllowHTML_menuitem) */ /* this line (error?) is what Thunderbird has */ Nit: This comment should be removed. >+function MsgBodyAllParts() >+{ >+ gPrefBranch.setBoolPref("mailnews.display.prefer_plaintext", false); >+ gPrefBranch.setIntPref("mailnews.display.html_as", 4); >+ gPrefBranch.setIntPref("mailnews.display.disallow_mime_handlers", 0); >+ ReloadMessage(); >+} Other MsgBody functions return true, so perhaps we should do the same. >+++ b/suite/mailnews/mailWindowOverlay.xul >@@ -1,10 +1,10 @@ > <?xml version="1.0"?> >- >+<!-- vim: set et :--> Nit: this change not needed. >+ <menuitem id="bodyAllParts" >+ type="radio" >+ name="bodyPlaintextVsHTMLPref" >+ label="&bodyAllParts.label;" >+ accesskey="&bodyAllParts.accesskey;" >+ oncommand="MsgBodyAllParts()"/> Nit: semi-colon after () please. r=me with those issues addressed.

(In reply to Ian Neal from comment #21) > Comment on attachment 552423 [details] [diff] [review] > patch v2.01 > > >+++ b/suite/mailnews/mailWindowOverlay.js > >@@ -1,9 +1,10 @@ > > /* -*- Mode: javascript; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ > >+/* vim: set expandtab shiftwidth=2 :*/ > Nit: this change not needed. The previous line (an Emacs modeline, maybe?) was added by some other assignee in a recent patch, so why not a Vim modeline? From the blogs syndicated on Planet Mozilla, I know I'm not the only Vim user hereabouts. > > >+ if (! isFeed) { > >+ AllBodyParts_menuitem = document.getElementById(menuIDs[3]); > >+ AllBodyParts_menuitem.hidden = > >+ ! pref.getBoolPref("mailnews.display.show_all_body_parts_menu"); > >+ } > Nit: No space after ! please so !isFeed and !pref OK; but note that !<space>pref was in the Thunderbird patch. > > >+ else if (!prefer_plaintext && html_as == 4 && !disallow_classes && > >+ /* AllowHTML_menuitem) */ /* this line (error?) is what Thunderbird has */ > Nit: This comment should be removed. that was foreseen, see comment #17 > > >+function MsgBodyAllParts() > >+{ > >+ gPrefBranch.setBoolPref("mailnews.display.prefer_plaintext", false); > >+ gPrefBranch.setIntPref("mailnews.display.html_as", 4); > >+ gPrefBranch.setIntPref("mailnews.display.disallow_mime_handlers", 0); > >+ ReloadMessage(); > >+} > Other MsgBody functions return true, so perhaps we should do the same. OK. > > >+++ b/suite/mailnews/mailWindowOverlay.xul > >@@ -1,10 +1,10 @@ > > <?xml version="1.0"?> > >- > >+<!-- vim: set et :--> > Nit: this change not needed. That's to avoid adding hard tabs when editing with Vim (with this setting, anyone hitting <Tab> in Insert mode will insert one or more spaces instead). See also the first change at top of mailWindowOverlay.js > > >+ <menuitem id="bodyAllParts" > >+ type="radio" > >+ name="bodyPlaintextVsHTMLPref" > >+ label="&bodyAllParts.label;" > >+ accesskey="&bodyAllParts.accesskey;" > >+ oncommand="MsgBodyAllParts()"/> > Nit: semi-colon after () please. Well, I'll put one, since you insist; but not only there wasn't one in the Tb patch, the two menuitems in the "context" lines above this change also lack a final semicolon after the () in the oncommand= attribute. > > r=me with those issues addressed. modified patch is coming.

Carrying over ui-review=Neil I suppose an additional review round is needed for the following: - I kept the Vim modelines (see my arguments in the previous comment) - I added two spaces of indent in order to align the new js function with the related functions above it rather than on the less related function below it. This is a mailnews patch. Therefore according to http://www.seamonkey-project.org/dev/review-and-flags it requires superreview by the mailnews owner or peer (Mnyromyr?) unless a mailnews owner or peer (Neil?) says otherwise. Shall I request it?

Comment on attachment 552835 [details] [diff] [review] patch v2.10 (uir=Neil sr=Mnyromyr) adding sr+=Mnyromyr given (or waived) over IRC after seeing who is doing uir= and r=

(In reply to Tony Mechelynck [:tonymec] from comment #23) > - I kept the Vim modelines (see my arguments in the previous comment) FWIW, I'm using Vim too... :-) > This is a mailnews patch. You've checked with Mnyromyr already, but strictly speaking it isn't; you only modify suite/ components. That provision is for patches modifying the mailnews/ module (different from suite/mailnews/), thus the code shared with Thunderbird.

(In reply to rsx11m from comment #25) > (In reply to Tony Mechelynck [:tonymec] from comment #23) > > - I kept the Vim modelines (see my arguments in the previous comment) > > FWIW, I'm using Vim too... :-) > > > This is a mailnews patch. > > You've checked with Mnyromyr already, but strictly speaking it isn't; you > only modify suite/ components. That provision is for patches modifying the > mailnews/ module (different from suite/mailnews/), thus the code shared with > Thunderbird. Ah. I thought those rules in http://www.seamonkey-project.org/dev/review-and-flags were for bugs in the SeaMonkey product, not in the MailNews Core product. The way I understand it, "SeaMonkey" mailnews patches are patches for any bugs filed in the SeaMonkey (not MailNews Core) product and affecting the MailNews component of SeaMonkey (i.e., anything in the 3-pane, mail display, text compose, HTML compose and address-book windows plus the associated menus and popups as well as the "MailNews" part of Edit → Preferences and the mailnews account settings). Code shared (not forked) with Thunderbird would be MailNews Core code, I suppose, not SeaMonkey code, and as such subject to the MailNews Core module review requirements, unless shared also with Firefox in which case it wouldn't even be "MailNews" code. I would characterize this bug as a "SeaMonkey-specific MailNews UI" bug.

Now I'm confused myself, it only says "required for mailnews patches unless a mailnews owner or peer says otherwise", thus where common components like Address Book, Account Settings, or the backend are affected. For sr in general, "if any of the patch's reviewers request it or the patch is a major API change or a major addition/removal of code". Either way, looks like you are on the safe side.

In reply to comment #27: This being my first patch, I certainly wanted (in case of doubt) to be on the safe side. Belt 'n suspenders, y'know?

Comment on attachment 552835 [details] [diff] [review] patch v2.10 (uir=Neil sr=Mnyromyr) >+++ b/suite/mailnews/mailWindowOverlay.js >@@ -1,9 +1,10 @@ > /* -*- Mode: javascript; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ >+/* vim: set expandtab shiftwidth=2 :*/ The only place that suite has something similar to this is in tests copied from toolkit (and then it is "vim:set ts=2 sw=2 sts=2 et:", and occasionally including ft=javascript too), so I rather not have it but, if you do, then have one of those two options. >+function MsgBodyAllParts() >+{ >+ gPrefBranch.setBoolPref("mailnews.display.prefer_plaintext", false); >+ gPrefBranch.setIntPref("mailnews.display.html_as", 4); >+ gPrefBranch.setIntPref("mailnews.display.disallow_mime_handlers", 0); >+ ReloadMessage(); >+ return true; >+} You were correct the first time, the indentation should only be two spaces (it is wrong in the function above but don't tend to make whitespace only fixes). >+++ b/suite/mailnews/mailWindowOverlay.xul >@@ -1,10 +1,10 @@ > <?xml version="1.0"?> >- >+<!-- vim: set et :--> Suite does not have anything similar to this and even m-c only has it in 4 places (all in tests), so I rather not have it. r=me with those issues addressed.

(In reply to Ian Neal from comment #29) > Comment on attachment 552835 [details] [diff] [review] > patch v2.10 (uir=Neil sr=Mnyromyr) > > >+++ b/suite/mailnews/mailWindowOverlay.js > >@@ -1,9 +1,10 @@ > > /* -*- Mode: javascript; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ > >+/* vim: set expandtab shiftwidth=2 :*/ > The only place that suite has something similar to this is in tests copied > from toolkit (and then it is "vim:set ts=2 sw=2 sts=2 et:", and occasionally > including ft=javascript too), so I rather not have it but, if you do, then > have one of those two options. ft=javascript is implicit for *.js files. OK about the rest. > > >+function MsgBodyAllParts() > >+{ > >+ gPrefBranch.setBoolPref("mailnews.display.prefer_plaintext", false); > >+ gPrefBranch.setIntPref("mailnews.display.html_as", 4); > >+ gPrefBranch.setIntPref("mailnews.display.disallow_mime_handlers", 0); > >+ ReloadMessage(); > >+ return true; > >+} > You were correct the first time, the indentation should only be two spaces > (it is wrong in the function above but don't tend to make whitespace only > fixes). Ah. Well I'll unindent it again. > > >+++ b/suite/mailnews/mailWindowOverlay.xul > >@@ -1,10 +1,10 @@ > > <?xml version="1.0"?> > >- > >+<!-- vim: set et :--> > Suite does not have anything similar to this and even m-c only has it in 4 > places (all in tests), so I rather not have it. Since you insist, I'll remove it, but beware that it might lead to insertion of hard tabs by later not-too-attentive patchers using Vim. > > r=me with those issues addressed. new patch is coming, and it will have the checkin-needed keyword.

carrying forward uir=Neil r=IanN sr=Mnyromyr final version of the patch

I don't know what to do about setting up tests, or if they are necessary.

Comment on attachment 552980 [details] [diff] [review] patch v2.11 final (uir=Neil r=IanN sr=Mnyromyr) [Checkin: comment 33] http://hg.mozilla.org/comm-central/rev/498b0870ae9f

(In reply to Tony Mechelynck [:tonymec] from comment #32) > I don't know what to do about setting up tests, or if they are necessary. Leaving open; feel free to close once you know.

AFAICT the backend changes and the parallel Thunderbird menu (which, like this change, has l10n impact) won't be backported to aurora or earlier, hence 2.4-wontfix.

Definitely, string changes can't land on comm-aurora.

(In reply to rsx11m from comment #1) > The additional menu item has been introduced in > Thunderbird bug 602718 and needs to be explicitly activated by the user with > a hidden preference. Wrong. The pref introduced by bug 602718 is not hidden: it has been added to mailnews.js, so it shows up in about:config even if not set by the user. It just defaults to false, so it's only the menu item that is hidden by default. Adding relnote keyword following bug 688953 comment 14.

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #37) > Wrong. The pref introduced by bug 602718 is not hidden: it has been added to > mailnews.js, so it shows up in about:config even if not set by the user. It > just defaults to false, so it's only the menu item that is hidden by default. Perhaps your interpretation of "hidden preference" is reasonable, but it is not helpful. What rsx11m and others mean by "hidden preference" is one that is not accessible through the user interface.

Yes, that was the intended meaning. I don't know if there is any "standardized" way to distinguish between prefs which are accessible by some UI element, those which are present by default but not "visible" in that sense, and those which have to be set explicitly by the user and don't exist by default.