520108 - Drop-down menu of Forward button shows "Attachment" as default, disregards preference setting

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Composition, defect)

Description

[rsx11m]

In the mail/news window, bug 17796 added a drop-down context menu to the Forward button, allowing to directly select whether to forward the selected message inline or as attachment. Judging from the patch there, it was intended to show the default setting (given by mail.forward_message_mode) in bold.

This somehow got broken, even with mail.forward_message_mode=2 for inline the "Attachment" entry is bold, thus the user may be confused about the current default selected from the preference.

Comment 1

[rsx11m]

Attachment: Proposed fix

This replaces gPrefs.getIntPref() with gPrefBranch.getIntPref(), correctly getting the value of mail.forward_message_mode to mark the default. The second change is trivial, but an explicit (forwardType > 0) should make it clearer.

Requesting r?/sr? as UI-changes are involved, no string changes.

Comment 2

[[e-mail]]

Comment on attachment 404180 [details] [diff] [review]
Proposed fix

>-  if (forwardType) {
>+  if (forwardType > 0) {
MsgForwardMessage (in the same file, so no excuse) compares equality, so I'd accept != 0 here, but not > 0. sr=me with this reverted or fixed.

Comment 3

[rsx11m]

Attachment: Proposed patch (v2)

(In reply to comment #2)
> MsgForwardMessage (in the same file, so no excuse) compares equality, so I'd
> accept != 0 here, but not > 0. sr=me with this reverted or fixed.

Certainly correct, no excuses attempted. :-) Updated patch using != now, it's better readable to have an explicit comparison for the integer here.

Comment 4

[Karsten Düsterloh]

Comment on attachment 404232 [details] [diff] [review]
Proposed patch (v2)

> function InitMessageForward(aPopup)
> {
>   var forwardType = 0;

If you need special "values with meaning", define a constant to make the code more readable. Since mailWindowOverlay.js often could use it (at least for the 0 value, e.g. MsgForwardMessage), make it global:
  const FORWARD_AS_ATTACHMENT = 0;

>-    forwardType = gPrefs.getIntPref("mail.forward_message_mode");
>+    forwardType = gPrefBranch.getIntPref("mail.forward_message_mode");

Actually, no need to bother with different global variables, we have Application.prefs.getValue(prefname, default value) by now, which doesn't throw, afaik.

>+  if (forwardType != 0) {

And adhere to the styling guide, please.
<https://wiki.mozilla.org/SeaMonkey:MailNews:CodingStyle>

Comment 5

[rsx11m]

Attachment: Proposed patch (v3) [Checkin: comment #9]

Ok, I wasn't aware that I'm supposed to fix coding-style issues introduced by others in parts which I don't touch, and didn't bother much to read myself into the Application.prefs as I was under the impression that those changes were to be done in a batch (there was a bug somewhere, but maybe just mentioning it).

Anyway, here the new patch with changes as requested:

> If you need special "values with meaning", define a constant to make the code
> more readable. Since mailWindowOverlay.js often could use it (at least for the
> 0 value, e.g. MsgForwardMessage), make it global:

I have defined consts kMsgForwardAsAttachment and kMsgForwardInline now, the names matching the pattern of the other declarations.

> Actually, no need to bother with different global variables, we have
> Application.prefs.getValue(prefname, default value) by now, which doesn't
> throw, afaik.

The only other occurrence of Application.prefs is in InitNewMsgWindow(), coincidentally in the same file just above. I used that as a template, and no try/catch is done there either. This indeed simplifies things tremendously.

I've also replaced the gPrefBranch usage in MsgForwardMessage() respectively.
> >+  if (forwardType != 0) {
> And adhere to the styling guide, please.

Not my code, I had just changed the operator, but I've put the two '{' on their own lines for compliance. I didn't see any other style violations.

Neil, I'm carrying forward your sr+ from attachment 404232 [details] [diff] [review] and assume
that you will comment if you see anything in the updated patch.

Comment 6

[rsx11m]

> <https://wiki.mozilla.org/SeaMonkey:MailNews:CodingStyle>

Hmm, I've noticed that this style guide links to MDC, in which a section https://developer.mozilla.org/En/Developer_Guide/Coding_Style#Line_Length specifies a maximum line length of 80 characters. That's for C++ rather than JavaScript though, thus I'm not sure what actually applies. There are lots of lines beyond 80ch in mailWindowOverlay.js, so I'm wondering if I should break 

> +  var forwardType = Application.prefs.getValue("mail.forward_message_mode", kMsgForwardAsAttachment);

up into something like this:

> +  var forwardType = Application.prefs.getValue("mail.forward_message_mode",
> +                                               kMsgForwardAsAttachment);

Please let me know if you want me to split the two new statements like that, otherwise I'll leave those with the current slight overlength.

Comment 7

[rsx11m]

Ping @mnyromyr for the review. The freeze for RC1 is just a few hours away and this should be ready to go now, will you be able to take care of it today?

Comment 8

[Karsten Düsterloh]

Will look tonight (UTC+2).

Comment 9

[Karsten Düsterloh]

Comment on attachment 404430 [details] [diff] [review]
Proposed patch (v3) [Checkin: comment #9]

r/a=me, pushed as <http://hg.mozilla.org/comm-central/rev/06ae9dd4bf85>.

(In reply to comment #6)
> Hmm, I've noticed that this style guide links to MDC, in which a section
> https://developer.mozilla.org/En/Developer_Guide/Coding_Style#Line_Length
> specifies a maximum line length of 80 characters. That's for C++ rather than
> JavaScript though, thus I'm not sure what actually applies.

It does, if feasible. Readability always wins over dogmatism, though.

> There are lots of lines beyond 80ch in mailWindowOverlay.js,

... which usually are just caused by lazyness. ;-)

> so I'm wondering if I should break 

I did that on checkin. :)


Thanks for patching this!

Comment 10

[rsx11m]

Great, thanks!