Last Comment Bug 718342 - Add "Reply to Sender" button to message header for NNTP posts
: Add "Reply to Sender" button to message header for NNTP posts
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Message Reader UI (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: Thunderbird 14.0
Assigned To: Jim Porter (:squib)
:
Mentors:
Depends on: 788304 789400 747298
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-15 15:20 PST by Jim Porter (:squib)
Modified: 2012-10-28 16:19 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Split "reply" and "followup" (12.62 KB, patch)
2012-01-15 15:20 PST, Jim Porter (:squib)
bwinton: review-
bwinton: ui‑review+
Details | Diff | Splinter Review
What it looks like (29.03 KB, image/png)
2012-01-15 15:24 PST, Jim Porter (:squib)
no flags Details
Address review comments (17.13 KB, patch)
2012-03-18 13:16 PDT, Jim Porter (:squib)
bwinton: review+
bwinton: ui‑review+
Details | Diff | Splinter Review

Description Jim Porter (:squib) 2012-01-15 15:20:15 PST
Created attachment 588781 [details] [diff] [review]
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) 2012-01-15 15:24:09 PST
Created attachment 588782 [details]
What it looks like

And this is what it looks like.
Comment 2 Blake Winton (:bwinton) (:☕️) 2012-01-17 07:47:43 PST
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) 2012-01-17 15:51:36 PST
(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) (:☕️) 2012-01-18 13:56:40 PST
(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) 2012-01-22 14:42:25 PST
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) 2012-03-18 13:16:34 PDT
Created attachment 607002 [details] [diff] [review]
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) (:☕️) 2012-04-05 12:27:55 PDT
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) 2012-04-08 19:03:37 PDT
Checked in: http://hg.mozilla.org/comm-central/rev/f5d3b855f290

Note You need to log in before you can comment on or make changes to this bug.