Comment 291 for bug 52667

Revision history for this message
In , Mkmelin+mozilla (mkmelin+mozilla) wrote :

(From update of attachment 382141)
This seems to work well, however I have a few nits.

Please capitalize the js method names like it's elsewhere in the file/s.

>diff --git a/mail/base/content/mail3PaneWindowCommands.js b/mail/base/content/mail3PaneWindowCommands.js
>--- a/mail/base/content/mail3PaneWindowCommands.js
>+++ b/mail/base/content/mail3PaneWindowCommands.js
>@@ -323,15 +323,25 @@
> goSetMenuValue(command, whichText);
> goSetAccessKey(command, whichText + "AccessKey");
> }
>+ let retval = false;

Mozilla code almost always use rv for that.

> if (GetNumSelectedMessages() > 0)
> {
> if (gDBView)
> {
> gDBView.getCommandStatus(nsMsgViewCommandType.cmdRequiringMsgBody, enabled, checkStatus);
>- return enabled.value;
>+ retval = enabled.value;
> }
> }
>- return false;
>+ if (retval)
>+ {
>+ if (command == "cmd_reply" || command == "button_reply")
>+ retval = isReplyEnabled();
>+ else if (command == "cmd_replyall" || command == "button_replyall")
>+ retval = isReplyAllEnabled();
>+ else if (command == "cmd_replylist" || command == "button_replylist")
>+ retval = isReplyListEnabled();
>+ }

Why aren't these higher up directly after each "case foo:"?

>+ return retval;
> case "cmd_printpreview":
> if ( GetNumSelectedMessages() == 1 && gDBView)
> {
>diff --git a/mail/base/content/mailWindowOverlay.js b/mail/base/content/mailWindowOverlay.js
>--- a/mail/base/content/mailWindowOverlay.js
>+++ b/mail/base/content/mailWindowOverlay.js
>@@ -807,10 +807,45 @@
> junkButtonDeck.selectedIndex = SelectedMessagesAreJunk() ? 1 : 0;
> }
>
>-function UpdateReplyButtons()
>+function getServerType()

Maybe name the method GetCurrentMsgServerType or something more descriptive?

> {
> let msgHdr = messenger.msgHdrFromURI(GetLoadedMessage());
>
>+ let serverType = null;
>+ try
>+ {
>+ serverType = msgHdr.folder.server.type;
>+ }
>+ catch (ex)
>+ {
>+ // This empty catch block needs to be here because msgHdr.folder will
>+ // throw an exception when you try to access it on a .eml file.
>+ }
>+ return serverType
>+}
>+
>+function isReplyEnabled()
>+{
>+ if (getServerType() == "rss")
>+ // If we're in an rss item, we never want to Reply, because there's
>+ // usually no-one useful to reply to.
>+ return false;
>+ return true;
>+}

Could just be |return getServerType() != "rss";|