Comment 271 for bug 52667

Created an attachment (id=379240)
The latest patch, with the previous two merged

(In reply to comment #217)
> i still get the updating problem - even such that with the detailed headers
> showing things are ok, but when i switch to compact the wrong button gets
> shown

That was exactly the clue I needed to fix this. Thank you!

> >+ let addresses = {};
> >+ let names = {};
> >+ let fullNames = {};
> You can pass in {} without naming the fields here, for the stuff you don't
> need later, no?

Fixed.

> >+ if (aUrl.scheme == "news")
> >+ // If it's news, we should not show the ReplyAll button.
> >+ showReplyAll = false;
> Per previous comment, we should, only not as default.

Fixed. We still have three buttons.
Reply Reply All Reply To List
----- --------- -------------
Reply All Reply Reply All
                             Reply

But the "-----" and "Reply All" in the first button get shown for news/hidden otherwise.

(In reply to comment #218)
> >+ let buttonToShow = showReplyList ? "replyList" :
> >+ showReplyAll ? "replyAll" : "reply";
> While it might be correct, this sure is almost unreadable... use an if clause
> to improve readability?

Fixed.

> >+ <key id="key_replylist" key="&replyToListMsgCmd.key;" oncommand="goDoCommand('cmd_replylist')" modifiers="accel, shift"/>
> Nit: Alignment is off here, though it's a very long line so you can also wrap
> it and align oncommand with the id on the previous line.

Fixed. None of the elements around it are split between lines, so I'm leaving this one on one line.

> >+ <menuitem id="menu_replyToList" label="&replyToListMsgCmd.label;"
> >+ accesskey="&replyToListMsgCmd.accesskey;"
> >+ key="key_replylist"
> >+ command="cmd_replylist"/>
> Nit: Align accesskey with the id on the previous line here too although the
> surrounding stuff isn't well aligned.

Fixed, and I've cleaned up the stuff around it, cause I was there.

> BTW, please submit these too patches as one for the next round, as they will go
> in together anyway.

Done, and done.

> >+ let replyListCommand = document.getElementById("cmd_replylist");
> >+ replyListCommand.disabled = !showReplyList;
> I think you're supposed to return true/false for the list commands when they
> get checked in mail3PaneWindowCommands.js+messageWindow.js isCommandEnabled()

I'm going to look into this one, but I wanted to get the latest patch up, to see if there is anything else I can change while I'm fixing that. (Note for me: Check out mail/base/content/mail3PaneWindowCommands.js, line 332-ish.)

Many thanks to dmose who helped me out a lot on IRC this afternoon.

Later,
Blake.