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

RESOLVED FIXED in seamonkey2.0b2

Status

SeaMonkey
MailNews: Message Display
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Philip Chee, Assigned: Philip Chee)

Tracking

Trunk
seamonkey2.0b2

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

3.14 KB, patch
Philip Chee
: review+
Philip Chee
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

8 years ago
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)

Updated

8 years ago
Assignee: nobody → philip.chee
(Assignee)

Comment 1

8 years ago
Created attachment 393777 [details] [diff] [review]
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.
Attachment #393777 - Flags: review?(mnyromyr)

Comment 2

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

Comment 3

8 years ago
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-

Comment 4

8 years ago
(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

8 years ago
(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.
(Assignee)

Comment 6

8 years ago
Created attachment 395508 [details] [diff] [review]
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.
Attachment #393777 - Attachment is obsolete: true
Attachment #395508 - Flags: superreview?(neil)
Attachment #395508 - Flags: review?(mnyromyr)
(Assignee)

Updated

8 years ago
Status: NEW → ASSIGNED

Comment 7

8 years ago
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 8

8 years ago
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+
(Assignee)

Comment 9

8 years ago
Created attachment 396030 [details] [diff] [review]
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.
Attachment #395508 - Attachment is obsolete: true
Attachment #396030 - Flags: superreview+
Attachment #396030 - Flags: review+
(Assignee)

Updated

8 years ago
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
Last Resolved: 8 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.