718342 - Add "Reply to Sender" button to message header for NNTP posts

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[Jim Porter (:squib)]

Attachment: Split "reply" and "followup"

Currently, the reply button in the message header "replies" to the newsgroup. It also lets you "reply all" to the newsgroup and the poster, but there's no button to reply just to the poster. There's a menu item to do this, but no button. It's especially strange when you swap between a mailing list and a newsgroup, which are conceptually very similar, but have a different button layout.

This has been bugging me for a while, so I fixed it locally. Here's the patch for that. Strings might need some work, though.

Comment 1

[Jim Porter (:squib)]

Attachment: What it looks like

And this is what it looks like.

Comment 2

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 588781 [details] [diff] [review]
Split "reply" and "followup"

It seems to work pretty well.  ui-r=me.

One thing I noticed is that the icon when the Smart Reply is in the customize dialog varies.  We should probably fix that in a followup bug.

So, I'm guessing you changed "reply" to "replyNews", and "replyOnly" to "reply"?

>+++ b/mail/base/content/mailWindowOverlay.js
>@@ -1028,7 +1028,7 @@
>   if (gFolderDisplay.selectedMessageIsNews)
>   {
>     // News messages always default to the "reply" dual-button.

I think this comment isn't correct anymore.

>-    buttonToShow = "reply";
>+    buttonToShow = "replyNews";

(Also, is "replyNews" actually "followup", and if so, could we rename it to that?)

>+++ b/mail/base/content/msgHdrViewOverlay.xul
>@@ -297,7 +277,33 @@
>+                <toolbarbutton id="hdrReplyNewsButton" label="&hdrReplyNewsButton1.label;"
>+                               tooltiptext="&hdrReplyNewsButton1.tooltip;"
>+                               oncommand="MsgReplyGroup(event);RestoreFocusAfterHdrButton();"
>+                               observes="button_reply"

That observes value seems wrong.  (I don't think there is a button_replynews, but I do think we want to create one for this.)

>+                               class="toolbarbutton-1 msgHeaderView-button hdrReplyNewsButton"
>+                               type="menu-button">
>+                  <menupopup id="hdrReplyNewsDropdown">
>+                    <menuitem id="hdrReplyNews_ReplySubButton"
>+                              label="&hdrReplyNewsButton1.label;"
>+                              tooltiptext="&hdrReplyNewsButton1.tooltip;"
>+                              observes="button_reply"/>

That observes value seems wrong.  (There may not be a 

>+                    <menuseparator id="hdrReplyNewsSubSeparator"/>
>+                    <menuitem id="hdrReplyNews_ReplyAllSubButton"
>+                              label="&hdrReplyAllButton1.label;"
>+                              tooltiptext="&hdrReplyAllButton1.tooltip;"
>+                              observes="button_replyall"
>+                              oncommand="MsgReplyToAllMessage(event); event.stopPropagation();
>+                                         RestoreFocusAfterHdrButton();"/>
>+                    <menuitem id="hdrReplyNews_ReplySubButton"
>+                              label="&hdrReplyButton1.label;"
>+                              tooltiptext="&hdrReplyButton1.tooltip;"
>+                              observes="button_replyall"

I don't think this one is correct, either.  ;)

>+++ b/mail/themes/qute/mail/primaryToolbar-aero.css
>@@ -254,7 +254,7 @@
>     #button-previous, #button-mark, #button-tag, #button-goback,
>     #button-goforward, #button-compact, #button-archive, #hdrArchiveButton,
>     #hdrReplyButton, #hdrReplyToSenderButton, #hdrReplyAllButton,
>-    #hdrReplyOnlyButton, #hdrReplyListButton, #hdrForwardButton,
>+    #hdrReplyListButton, #hdrReplyNewsButton, #hdrForwardButton,
>     #hdrTrashButton, #hdrJunkButton
>     ) .toolbarbutton-icon {
>   margin: 0 !important;

Why is this change only made for aero?  (I'm guessing it's because this rule only exists in aero, but I want to double-check that.)

And I'm going to say r-, so that I can take a look at the button_replynews code.

Thanks,
Blake.

Comment 3

[Jim Porter (:squib)]

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #2)
> One thing I noticed is that the icon when the Smart Reply is in the
> customize dialog varies.  We should probably fix that in a followup bug.

Filed as bug 718860, since it was about 5 minutes of actual work.

> So, I'm guessing you changed "reply" to "replyNews", and "replyOnly" to
> "reply"?

Right.

> (Also, is "replyNews" actually "followup", and if so, could we rename it to
> that?)

Do you want that change everywhere? I was calling it "replyNews" to keep it in line with the other smart reply buttons, but I'm not strongly opposed to calling it "followup" in the code too.

> Why is this change only made for aero?  (I'm guessing it's because this rule
> only exists in aero, but I want to double-check that.)

Yeah, it's an aero-only thing. It resizes the icons for the buttons, which are 18x18 on aero. It's not the best way of doing things, but we can file another bug to simplify that CSS if need be.

Comment 4

[Blake Winton (:bwinton) (:☕️)]

(In reply to Jim Porter (:squib) from comment #3)
> (In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #2)
> > One thing I noticed is that the icon when the Smart Reply is in the
> > customize dialog varies.  We should probably fix that in a followup bug.
> Filed as bug 718860, since it was about 5 minutes of actual work.

Cool.

> > So, I'm guessing you changed "reply" to "replyNews", and "replyOnly" to
> > "reply"?
> Right.

Cool.

> > (Also, is "replyNews" actually "followup", and if so, could we rename it to
> > that?)
> Do you want that change everywhere? I was calling it "replyNews" to keep it
> in line with the other smart reply buttons, but I'm not strongly opposed to
> calling it "followup" in the code too.

I think so, since it's only kinda a reply button.  (The other all have "Reply" in their name, iirc.)

> > Why is this change only made for aero?  (I'm guessing it's because this rule
> > only exists in aero, but I want to double-check that.)
> Yeah, it's an aero-only thing. It resizes the icons for the buttons, which
> are 18x18 on aero. It's not the best way of doing things, but we can file
> another bug to simplify that CSS if need be.

Cool.

Thanks,
Blake.

Comment 5

[Jim Porter (:squib)]

I think I'm going to put this on hold for a bit to focus on cloud file stuff. So I don't forget, here are some of the things that we should probably handle here or in another bug:

* Hide "Reply to List" in newsgroups
* Rename "Reply to Newsgroup" in the menus
* Fix bug 498448

Comment 6

[Jim Porter (:squib)]

Attachment: Address review comments

This should resolve all the issues with the previous review. I also updated some strings, notably that "Reply to Newsgroup" in the Message and context menus now reads "Followup to Newsgroup". Consistency is good. :)

Comment 7

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 607002 [details] [diff] [review]
Address review comments

I like it, both the ui and the code.

r=me, ui-r=me!

Thanks,
Blake.

Comment 8

[Jim Porter (:squib)]

Checked in: http://hg.mozilla.org/comm-central/rev/f5d3b855f290