507871 - "New Msg" button: Highlight default action in the pull down menu.

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Philip Chee]

From Bug 16908 Comment 53:

> Nit: Followup needed to "highlight" the default action. (All the other
> menubuttons do; Get Messages seems to be the the oddity, but then it applies to
> the current folder while the menu actually lists accounts.)

Comment 1

[Philip Chee]

Attachment: Patch v1.0

> Application.prefs.getValue("mail.html_compose", true);

mailWindowOverlay.js uses both prefs.getBoolPref() and gPrefBranch.getBoolPref() rather haphazardly. I'm using the SMILE API here since after the "clean up global scope" bug neither prefs nor gPrefBranch may be available.

Comment 2

[Robert Kaiser]

Interesting, SMILE opens a possibility to get rid of some global scope stuff we did so far, that's really nice!

Comment 3

[Karsten Düsterloh]

Comment on attachment 393777 [details] [diff] [review]
Patch v1.0

Basically okay, just some nits:

>+++ b/suite/mailnews/mailWindowOverlay.js
>+function InitNewMsgMenu(aPopup)
>+{
>+  var identity;
>+  var folder = GetFirstSelectedMsgFolder();
>+
>+  if (folder)
>+    identity = getIdentityForServer(folder.server);
>+
>+  if (!identity)
>+    identity = Components.classes["@mozilla.org/messenger/account-manager;1"]
>+                         .getService(Components.interfaces.nsIMsgAccountManager)
>+                         .defaultAccount.defaultIdentity;

Initialize identity. Drop the two empty lines above.

>+  // If the identity is not found, use the mail.html_compose pref to
>+  // determine the message compose type (HTML or PlainText).
>+  var composeHTML = identity ? identity.composeHtml :
>+                               Application.prefs.getValue("mail.html_compose", true);
>+

Align the : below the ? (yes, that's an exception from the general wrapping rules).
Drop the empty line.

>+  if (composeHTML) {

Please use brace-on-its-own-line bracing style.

>+    // Compose HTML is the first menuitem
>+    aPopup.firstChild.setAttribute("default", "true");
>+    aPopup.lastChild.removeAttribute("default");
>+  }
>+  else {
>+    // Compose PlainText is the last menuitem
>+    aPopup.lastChild.setAttribute("default", "true");
>+    aPopup.firstChild.removeAttribute("default");
>+  }

This will be problematic when extensions like Lightning add their own menuitems. Please put ids on the menuitems and use them here.
Also, you could fold the two branches, like

const kIDs = {true: idhtml, false: idplain};
getByID(kIDs[composeHTML]).set("default", true);
getByID(kIDs[!composeHTML]).remove("default");

Comment 4

[Robert Kaiser]

(In reply to comment #3)
> >+  if (composeHTML) {
> 
> Please use brace-on-its-own-line bracing style.

Have you read the updated coding style guide at https://developer.mozilla.org/En/Developer_Guide/Coding_Style (and all the discussion leading to it on m.d.platform)? The official Mozilla style is now brace on the same line for control structures (like that if), brace on new line for class and function definitions.

Comment 5

[Karsten Düsterloh]

(In reply to comment #4)
> Have you read the updated coding style guide

Yes. But:
1. MailNews is different and always was. Most of the code does not use that particular style and having a lot of different files with different styles sucks.
2. Frankly, that rule is crap.
MailNews code is hard enough to read, there's no use in making it even harder. The only valid pro argument for "{ at the end of line" is file size, and that's a pretty stupid one. (I hope you don't even claim "consistency with && etc.", that'd even weaker. Or "extremely small screens" - come on...)

> The official Mozilla style is now brace on the same line for control
> structures (like that if), brace on new line for class and function
> definitions.

That's just dumb, to be honest. 
The main reason for formatting source code is to keep it readable, 
and that guide rule clearly violates this principle.

But this bug really isn't the place for that discussion.

Comment 6

[Philip Chee]

Attachment: Patch v1.1 Review Nits Fixed.

> Initialize identity. Drop the two empty lines above.
Fixed.

>>+  var composeHTML = identity ? identity.composeHtml :
>>+                               Application.prefs.getValue("mail.html_compose", true);
>>+
> 
> Align the : below the ? (yes, that's an exception from the general wrapping
> rules). Drop the empty line.
Fixed. Fixed.
 
>>+  if (composeHTML) {
> Please use brace-on-its-own-line bracing style.
Fixed.
 
> This will be problematic when extensions like Lightning add their own
> menuitems. Please put ids on the menuitems and use them here.
Fixed.

> Also, you could fold the two branches, like
> 
> const kIDs = {true: idhtml, false: idplain};
> getByID(kIDs[composeHTML]).set("default", true);
> getByID(kIDs[!composeHTML]).remove("default");
Fixed.

Comment 7

[[e-mail]]

Comment on attachment 395508 [details] [diff] [review]
Patch v1.1 Review Nits Fixed.

>+  const kIDs = {true: "button-newMsgHTML", false: "button-newMsgPlain"}
Nit: missing ;

Comment 8

[Karsten Düsterloh]

Comment on attachment 395508 [details] [diff] [review]
Patch v1.1 Review Nits Fixed.

>         <menuitem label="&newHTMLMessageCmd.label;"
>+                  id="button-newMsgHTML"
>                   accesskey="&newHTMLMessageCmd.accesskey;"
>                   mode="HTML"/>
>         <menuitem label="&newPlainTextMessageCmd.label;"
>+                  id="button-newMsgPlain"
>                   accesskey="&newPlainTextMessageCmd.accesskey;"
>                   mode="PlainText"/>

Make id the first attribute.

r=me with that.

Comment 9

[Philip Chee]

Attachment: Patch v1.2 [Checkin: Comment 10]

Carrying forward r=mnyromyr sr=neil

>>+  const kIDs = {true: "button-newMsgHTML", false: "button-newMsgPlain"}
> Nit: missing ;
Fixed.

> Make id the first attribute.
Fixed.

Comment 10

[Serge Gautherie (:sgautherie)]

Comment on attachment 396030 [details] [diff] [review]
Patch v1.2
[Checkin: Comment 10]


http://hg.mozilla.org/comm-central/rev/23d7144c0e7c