Closed Bug 507871 Opened 14 years ago Closed 14 years ago

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

Categories

(SeaMonkey :: MailNews: Message Display, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.0b2

People

(Reporter: philip.chee, Assigned: philip.chee)

References

Details

Attachments

(1 file, 2 obsolete files)

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.)
Assignee: nobody → philip.chee
Attached patch Patch v1.0 (obsolete) — Splinter Review
> 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.
Attachment #393777 - Flags: review?(mnyromyr)
Interesting, SMILE opens a possibility to get rid of some global scope stuff we did so far, that's really nice!
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");
Attachment #393777 - Flags: review?(mnyromyr) → review-
(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.
(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.
Attached patch Patch v1.1 Review Nits Fixed. (obsolete) — Splinter Review
> 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.
Attachment #393777 - Attachment is obsolete: true
Attachment #395508 - Flags: superreview?(neil)
Attachment #395508 - Flags: review?(mnyromyr)
Status: NEW → ASSIGNED
Comment on attachment 395508 [details] [diff] [review]
Patch v1.1 Review Nits Fixed.

>+  const kIDs = {true: "button-newMsgHTML", false: "button-newMsgPlain"}
Nit: missing ;
Attachment #395508 - Flags: superreview?(neil) → superreview+
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.
Attachment #395508 - Flags: review?(mnyromyr) → review+
Carrying forward r=mnyromyr sr=neil

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

> Make id the first attribute.
Fixed.
Attachment #395508 - Attachment is obsolete: true
Attachment #396030 - Flags: superreview+
Attachment #396030 - Flags: review+
Keywords: checkin-needed
Comment on attachment 396030 [details] [diff] [review]
Patch v1.2
[Checkin: Comment 10]


http://hg.mozilla.org/comm-central/rev/23d7144c0e7c
Attachment #396030 - Attachment description: [for checkin] Patch v1.2 r=mnyromyr sr=neil → Patch v1.2 [Checkin: Comment 10]
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.0b2
You need to log in before you can comment on or make changes to this bug.