Comment 232 for bug 52667

Revision history for this message
In , Blake Winton (bwinton) wrote :

Created an attachment (id=366330)
A patch to change the default button, with fixes suggested by mkmelin.

> Yeah, there's some problem with updating the button when changing mails.

Fixed.

> I think it's better to just do it all in one patch, especially as if you
> change entity string you have to bump the names...

Perhaps, although the bit to add the Reply-To-List button is fairly big already, and I'm really enjoying using the functionality without changing the button names, so I think I'm going to keep it as 3 patches.

>>diff -r 7bfd2406b167 mail/base/content/mail3PaneWindowCommands.js
>>+ case "cmd_replyall":
>>+ case "button_replyall":
>>+ msgHdr = msgHdrForCurrentMessage();
> Needs to be declared? (Or maybe it is alreay earlier, either way, not so
> nice.)

The call has been moved into mail/base/content/mailWindowOverlay.js, where the function is defined. (I am still calling it earlier in the file than it's defined, but so is the UpdateJunkButton function, and I'm not sure whether both of those should be moved down below where msgHdrForCurrentMessage has been defined, or whether msgHdrForCurrentMessage should be moved up above those two functions, or if it doesn't matter, as long as they're in the same file. If you would like it to be changed, I'll be happy to change it.)

>>+ let showReplyAllButton = msgHdr &&
>> (msgHdr.recipients.split(",").length > 1 || msgHdr.ccList);
> Trailing space, but break the line after ||

Fixed.

>>+ UpdateReplyButtons( showReplyAllButton?"replyAll":"reply" );
> Here and else where, please skip the spaces after/before "("
> (Add spaces around "?" though.)

Fixed.

>>+ if ( command == "cmd_replyall" || command == "button_replyall" )
>>+ return showReplyAllButton; // else fall through
> Remove extra spaces.

Fixed. Well, deleted, but I'll remove them in the future if I add similar constructs.

>>+ if (replyAllButton)
>>+ replyAllButton.hidden = (buttonsToShow != "replyAll");
>>+ if (replyButton)
>>+ replyButton.hidden = (buttonsToShow != "reply");
> The buttons don't always exist?

Fixed.

> Also, please try to be consistent with " and '

Fixed.

Sorry it took me so long to update the patch, my weekend was busier than I thought it would be.

Thanks,
Blake.