Enable Greater Control over Reply All (Prompt or Disable) on BCC Messages

NEW
Unassigned

Status

Thunderbird
Mail Window Front End
7 years ago
2 years ago

People

(Reporter: Jerry Baker, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 17 obsolete attachments)

81.56 KB, application/x-xpinstall
Details
11.97 KB, patch
Details | Diff | Splinter Review
60.77 KB, application/x-xpinstall
bwinton
: ui-review-
Details
(Reporter)

Description

7 years ago
I have a patch that will disable the "Reply All" button if none of the recipients known email addresses appear in the To: or Cc: list.

The way I did it was to simply add the following code to mailWindowOverlay.js:

if (!imInAddresses) return false;

After much consternation I discovered that mail3PaneWindowCommands.js and messageWindow.js were enabling the ReplyAll button by checking IsReplyEnabled() instead of IsReplyAllEnabled(). After fixing those issues I noticed that the Reply All code doesn't enable the Reply All button unless there is more than one recipient of a message (not including myself). That's not how users are used to seeing Thunderbird behave due to the code for that button being hooked up to the IsReplyEnabled(), so I changed the behavior for IsReplyAllEnabled() to match.

Hopefully someone will let me know if this is an acceptable direction to go before I clean up the code and submit a clean patch (I have to make a build environment to diff against). If this is not an acceptable default behavior, what about if it's controlled by a preference?
(Reporter)

Updated

7 years ago
Version: unspecified → 3.1
(Reporter)

Comment 1

7 years ago
if (!imInAddresses) return false; is added to IsReplyAllEnabled() of course.
(Reporter)

Comment 2

7 years ago
I also have a version that uses a pref. For a placeholder I just tested with:

if (!imInAddresses) return gPrefBranch.getBoolPref("mail.test.ReplyAll");

The name of the pref doesn't matter.
Jerry could you provide the patch in the normal mozilla way see https://developer.mozilla.org/en/creating_a_patch and attach it here ?

Bryan would this behavior something we would want into the product ?
(Reporter)

Comment 4

7 years ago
(In reply to comment #3)
> Jerry could you provide the patch in the normal mozilla way see
> https://developer.mozilla.org/en/creating_a_patch and attach it here ?

I am working on it. I'm not sure why, but context menus do not use the same code to see whether Reply or Reply All should be enabled, so I'm hunting down just how those work. I also plan to work on popping a dialog that warns the user but allows them to continue. I figure this can then be an intpref with 0 being to behave as Thunderbird does now, 1 to completely disable the ability to Reply All to messages where you are not in the To: or CC: fields, and 3 to pop a confirmation dialog if you Reply All to a message where you are not in the To: or Cc: field. Once I reach that stage I will make it a patch.

I do have the changes packaged up in an XPI for anyone who would like to try it as it is now. Just be aware that you can still Reply All from a context menu.
(Reporter)

Comment 5

7 years ago
Attaching XPI to demonstrate the proposed behavior. This XPI includes my fix to force the "Reply to All" context menu item to clone the state of the Reply All button and menu item.

It might be worth a separate bug to note the fact that a lot of the context menu items do not check whether they should be enabled or not, or at least not the same code as the corresponding toolbar buttons and command menu items.
(Reporter)

Comment 6

7 years ago
Created attachment 520199 [details]
An extension demonstrating the proposed changes.
Attachment #520199 - Flags: ui-review?(clarkbw)
(Reporter)

Comment 7

7 years ago
Ok. I've added in a confirm dialog. I'm stuck at how to properly localize the string literal contained in the confirm dialog as there is no XUL involved.
(Reporter)

Comment 8

7 years ago
Created attachment 520260 [details]
Now with confirm dialog option

Sorry for all the bugspam. Here is an extension that is configurable by preference. The default is to check whether a configured email address in Thunderbird is in the To: or Cc: field upon initializing a Reply All and popping a confirm dialog if not. The user can also choose to completely disable Reply All on messages where they're  or just have Thunderbird behave as if it did not have the extension installed.

I believe I have properly localized the JavaScript strings used in the confirm dialog and properly accounted for whether we're looking at a news message vs. email message. Please test and comment before I create a patch (I'm being lazy and don't want to create a build environment just to make a patch until I really have to).
(Reporter)

Comment 9

7 years ago
Trying to create a patch, but there seem to be multiple locations in the tree for the same files. Not sure which ones to update. I need to update the following files:

mail3PaneWindowCommands.js
mailCommands.js
mailWindowOverlay.js
messageWindow.js
nsContextMenu.js

But those files reside in /suite/mailnews/ and in /mail/base/content/

I patched against /mail/base/content/, but I'm still working on adding a .properties file and the proper place to add a default pref.
(Reporter)

Comment 10

7 years ago
Created attachment 520327 [details]
First stab at a patch

Probably a lot wrong with it, but it's the first stab. Prefer getting this merged into the tree as opposed to as an extension since there a lot of opportunity to collide with other extensions by overriding five different base files.

Comment 11

7 years ago
(In reply to comment #9)
> But those files reside in /suite/mailnews/ and in /mail/base/content/

suite/ is Seamonkey and mail/ is Thunderbird. You can probably just patch mail/, but I'm sure the Seamonkey folks wouldn't mind if you made a patch for their stuff too. :)
(Reporter)

Comment 12

7 years ago
Created attachment 520338 [details] [diff] [review]
Fixed patch

Fixed a couple small issues. Waiting to get feedback before I go patching Seamonkey.
Attachment #520327 - Attachment is obsolete: true
(Reporter)

Updated

7 years ago
Summary: Disable Reply All button on Messages I'm bcc'd on → Enable Greater Control over Reply All (Prompt or Disable) on BCC Messages
(Reporter)

Comment 13

7 years ago
Created attachment 520393 [details] [diff] [review]
Latest patch

OK. Work continues. Now the confirmation dialog uses nsIPromptService's confirmEx to present a dialog with YES/NO buttons and a checkbox for "Don't ask me again."
Attachment #520338 - Attachment is obsolete: true
(Reporter)

Comment 14

7 years ago
Created attachment 520396 [details] [diff] [review]
Latest patch
Attachment #520393 - Attachment is obsolete: true
(Reporter)

Comment 15

7 years ago
Created attachment 520446 [details] [diff] [review]
Patch

Fixed location of default pref and updated pref documentation. Also updated confirm prompt to be more descriptive.
Attachment #520396 - Attachment is obsolete: true
(Reporter)

Comment 16

7 years ago
Created attachment 520447 [details]
Extension format with latest changes

In extension format for those who would like to try it out without building Thunderbird from source.
Attachment #520199 - Attachment is obsolete: true
Attachment #520260 - Attachment is obsolete: true
Attachment #520199 - Flags: ui-review?(clarkbw)
(Reporter)

Comment 17

7 years ago
Created attachment 520451 [details] [diff] [review]
Latest patch

Previous versions did not iterate through known identities like I thought they did. It was only checking the identity associated with the current server.
Attachment #520446 - Attachment is obsolete: true
(Reporter)

Updated

7 years ago
Duplicate of this bug: 295503
(Reporter)

Comment 19

7 years ago
Created attachment 520473 [details]
This patch in extension format
Attachment #520447 - Attachment is obsolete: true
Comment on attachment 520473 [details]
This patch in extension format

Jerry try to carry the flags over when you obsolete an attachment. The ui-review is the best way to get feedback from our UI lead ;-)
Attachment #520473 - Flags: ui-review?(clarkbw)
(In reply to comment #17)
> Created attachment 520451 [details] [diff] [review]
> Latest patch
> 
> Previous versions did not iterate through known identities like I thought they
> did. It was only checking the identity associated with the current server.

Jerry I don't know if you are aware of our policy about patches - we do requires unit test for them to be accepted (or tests descriptions to add to our list of tests in litmus). We use mozmill for our unit tests and the documentation is at : https://developer.mozilla.org/en/Thunderbird/Thunderbird_MozMill_Testing .
(Reporter)

Comment 22

7 years ago
Wow. It's not easy to share a fix with the community.

Perhaps I'll just leave it as an extension. How does Thunderbird handle the case where a user has two extensions that override the same file?
(Reporter)

Comment 23

7 years ago
Well, I tried. It looks like you have to have a Microsoft build environment to run mozmill properly. I'll leave it to someone who has a Microsoft compiler installed. I tried hacking my way through it, but python was getting angry.
(Reporter)

Comment 24

7 years ago
Created attachment 520529 [details] [diff] [review]
Latest patch

Going back and forth between the patch and extension file versions takes a toll. I had to correct a chrome uri that was pointing to an extension's location.
Attachment #520451 - Attachment is obsolete: true
(Reporter)

Updated

7 years ago
Attachment #520529 - Flags: review?
Attachment #520529 - Flags: review? → review?(bwinton)
(Reporter)

Comment 25

7 years ago
It seems like a pretty high barrier to entry to require a that a contributor submitting a JavaScript patch go through the process of getting, installing, and configuring Microsoft Visual Studio, Python, MozBuild, and MozMill and then learn the ins and outs of building Thunderbird. That's already after having to download and configure a Mercurial client and the Thunderbird source just to generate a patch file. That's much more work than the actual research and programming required to address whatever it was that needed fixing.

I'm not clear where this bug stands. Since I do not have the capabilities to run unit testing, should I just abandon this bug?
Hi Jerry.

Yes, I agree the barrier is a little too high for comfort, and I believe we're looking at making it easier to run and write UI tests. However I think I can look at writing some tests for this.

We also need UI feedback for this. In particular, I'm worried about the case where an e-mail is sent to you via a mailing list.
(Reporter)

Comment 27

7 years ago
Here's what it does in a nutshell:

If your preference is set to the default, all this does when you use Reply All is look in the To:, From:, and Cc: header fields of the original message for an email address associated with an identity in Thunderbird. If none of your configured email addresses are present it presents you with a dialog that says:

It appears that this message is not addressed to any of your configured email addresses. This may be because the sender intended your receipt of this message to be a secret. Using 'Reply All' will reveal your receipt of this message to all recipients. Are you sure you wish to continue?

The dialog has a "Yes" and "No" button as well as a "Don't ask me again" checkbox. If you select don't ask me again Thunderbird will revert to behaving as it does now, which is to allow Reply All on everything.

The only other option is for people who want to completely disable Reply All when Thunderbird cannot find a preconfigured email address in the From:, To:, or Cc: fields.
(Reporter)

Comment 28

7 years ago
If a list you belong to blind-copies you, the default will be to show this dialog if you use Reply All. I've thought about it a lot, but there's really no way around it. The whole point is to make users pause for a second and contemplate whether they really want to Reply All to a BCC message. It only has to happen once like the warning on submitting forms.
(Reporter)

Comment 29

7 years ago
I was worried about any potential UI delays that might be caused by looping through identities each time a new messages was selected, but Venkman shows this:

  Function Name: IsReplyAllEnabled  (Lines 856 - 932)
  Total Calls: 241 (max recurse 0)
  Total Time: 238.27 (min/max/avg 0.85/1.63/0.99)
  Time (ex. calls): 61.32 (min/max/avg 0.23/0.38/0.25)

Without my code, the speed looks like this:

    Function Name: IsReplyAllEnabled  (Lines 852 - 904)
    Total Calls: 47 (max recurse 0)
    Total Time: 39.76 (min/max/avg 0.75/1.16/0.85)
    Time (ex. calls): 6.48 (min/max/avg 0.13/0.17/0.14)

So, it appears that this code adds a min/max/avg of 0.10/0.47/0.14 ms to the time spent in IsReplyAll() enabled. Not too shabby.
Just catching up with this now.  I've read through the comments and I'm going to try out the extension now.  Thanks!
(Reporter)

Comment 31

7 years ago
Beat it up and try to kill it. Don't forget to try out the different pref values.

// 0 - Do nothing (Reply All works normally).
// 1 - Reply All buttons and menu items disabled.
// 2 - User is presented with a confirmation dialog.

extensions.ReplyAllControl.ReplyAllBehavior
(Reporter)

Comment 32

7 years ago
Created attachment 520825 [details] [diff] [review]
Latest patch

Cleans up some tab/space issues, wrap the code in an IF statement to prevent execution when the preference is to allow Reply All to any message.
Attachment #520529 - Attachment is obsolete: true
Attachment #520529 - Flags: review?(bwinton)
Attachment #520825 - Flags: review?(bwinton)
(Reporter)

Comment 33

7 years ago
Created attachment 520828 [details]
Extension implementing the patch
Attachment #520473 - Attachment is obsolete: true
Attachment #520473 - Flags: ui-review?(clarkbw)
Attachment #520828 - Flags: review?(clarkbw)
(Reporter)

Comment 34

7 years ago
There's a ton of code in IsReplyAllEnabled() that exists for the sole purpose of determining whether there is more than one email address to reply to (aside from the owner's). All of this effort is wasted because the Reply All button has historically been hooked up to the IsReplyEnabled() function, which only returns false if the message is a feed. Historical functionality (i.e., transparent to the user) can be retained while removing some 30 lines of code and improving the execution speed of that function by 56% according to Venkman. 1) Is there some reason that the email address counting code should remain (it's not been used as far as I can tell), and 2) should this be a separate bug?
(In reply to comment #28)
> If a list you belong to blind-copies you, the default will be to show this
> dialog if you use Reply All. I've thought about it a lot, but there's really no
> way around it. The whole point is to make users pause for a second and
> contemplate whether they really want to Reply All to a BCC message. It only has
> to happen once like the warning on submitting forms.

My understanding is that lists you're on always have the effect of blind-copying you, so you'll always show the dialog…

I think I would like this patch more if you also did nothing if the ReplyList button was enabled.  (i.e. "IsReplyListEnabled() == true" had the same behaviour as "extensions.ReplyAllControl.ReplyAllBehavior == 0".)  How does that sound to you?

Thanks,
Blake.
(Reporter)

Comment 36

7 years ago
Created attachment 520928 [details]
Extension implementing the patch

Seems reasonable to me. See if this extension behaves as you're describing.

As an aside, we may want to look deeper into improving the list detection capabilities of IsReplyListEnabled() to include List-Id or other list headers in another bug.
Attachment #520828 - Attachment is obsolete: true
Attachment #520828 - Flags: review?(clarkbw)
Attachment #520928 - Flags: review?(clarkbw)
I was thinking of that, but if there isn't a List-Post header, then we have no way to post to the list, and so enabling the ReplyList button would be the wrong thing.  (Also, I don't think checking for other headers would really get us any benefit, since I haven't seen a list which has some of the headers, and not List-Post.)

Thanks,
Blake.
(Reporter)

Comment 38

7 years ago
Created attachment 520936 [details]
Extension implementing the patch

Stupid missing parenthesis :)
Attachment #520928 - Attachment is obsolete: true
Attachment #520928 - Flags: review?(clarkbw)
Attachment #520936 - Flags: review?(clarkbw)
(Reporter)

Comment 39

7 years ago
Yahoo Groups send messages with List-Id but no List-Post headers. Their List-Id is set to the List-Post address. RFC 2919 doesn't require that a List-Post address be used for List-Id, but it might be worth checking if List-Id == Reply-To.
(Reporter)

Comment 40

7 years ago
I'm having some trouble with IsReplyListEnabled(). It's returning true and false on the same message. Sounds impossible, but if you stick an alert(IsReplyListEnabled()) in the IsReplyAllEnabled() function, you get

true
true
true
true
false
true

on each message. That false is mucking up the code. I'm looking into it.
(Reporter)

Comment 41

7 years ago
Well, the problem is not in the IsReplyListEnabled() function. I changed it to this and it still returns false sometimes when called from IsReplyAllEnabled():

function IsReplyListEnabled() {
  return true;
}
(Reporter)

Comment 42

7 years ago
Sorry, that should read:

if ("list-post" in currentHeaderData)
  return true;
else
  return false;

I can't figure out why it returns true sometimes and false sometimes on the same message.

Comment 43

7 years ago
How are you replying when you get "false"? That is, what button/menu item do you select? Is the message you're replying to currently displayed?
(Reporter)

Comment 44

7 years ago
It's when the message is selected in the header view. That triggers some UI updates which call IsReplyAllEnabled(). If I stick an alert(IsReplyListEnabled()) in the IsReplyAllEnabled(), I get several alerts (each button and menu item triggers) and some are true and some are false.
I wonder if the alerts are stealing focus, and causing things to be unselected somehow.

What happens if you change the line to:
dump(IsReplyListEnabled()+"\n");
or:
Application.console.log(IsReplyListEnabled());
?
(Reporter)

Comment 46

7 years ago
Upon selecting a message that DOES have a List-Post header, the Error Console displays:

false
true
true

Upon selecting a message that does NOT have a List-Post header, the Error Console displays:

true
false
false

Something's backwards when that first call is made.
(Reporter)

Comment 47

7 years ago
I'm getting a suspicion that somehow currentHeaderData is holding on to data from the previously selected message. If I select two messages with a List-Post header in a row, the first reads like described before:

false
true
true

But, the second one reads:

true
true
true

The same holds true going the other way. The first message without a List-Post header returns:

true
false
false

But if I select another message without a List-Post header, it returns:

false
false
false
(Reporter)

Comment 48

7 years ago
The problem is originating in the call to IsReplyAllEnabled() from mail3PaneWindowCommands.js. If I remove the call there the problem goes away. I just can't figure out why. Unfortunately, removing the call there removes the ability to disable buttons and menu items.
(Reporter)

Comment 49

7 years ago
I have confirmed that the first incorrect IsReplyListEnabled() value is being caused by an incorrect message reference. I used 

Application.console.log(currentHeaderData["message-id"]["headerValue"] + ": " + IsReplyListEnabled());

And where the incorrect value was appearing, the message-id of the previously selected message was appearing. I'm going to see what I can find out by debugging it with Venkman.
(Reporter)

Comment 50

7 years ago
It appears it might be a timing issue. The error goes away depending on where I set the breakpoint. :(
(Reporter)

Comment 51

7 years ago
It's a timing issue for sure. Putting a simple alert() in mail3PaneWindowCommands.js just before the call to IsReplyAllEnabled() cures the problem.

I don't know where to go from here. There appears to be a problem with trying to read header values too early in the process. I think resolving this is beyond my expertise. Should I give up on checking for list-post headers until this is fixed?
(Reporter)

Comment 52

7 years ago
I'm trying to figure out how currentHeaderData works, but it's apparently a magical property. According to MXR, it only appears in two JS files across the entire Mozilla tree. It is only defined as an empty object. I cannot find when and where it is populated with data.
(Reporter)

Comment 53

7 years ago
Ok. currentHeaderData appears to be populated by processHeaders: function(headerNameEnumerator, headerValueEnumerator, dontCollectAddress) in msgHdrViewOverlay.js.

It looks like the problem is that mail3PaneWindowCommands.js has events firing before currentHeaderData has a chance to get populated. I put this as the first line in processHeaders:

Application.console.log("Starting processHeaders()");

Now, when I switch messages in the header pane to a message that has a List-Post header this is what appears in my error console:

false
Starting processHeaders()
true
true

The question now is how to wait for processHeaders().
Could you check that the id of the selected message is the same as currentHeaderData["message-id"]["headerValue"]?

I'm not sure of the implementation of processHeaders, but we might finish processing the headers before the next event fires…
(Reporter)

Comment 55

7 years ago
Here's how it looks with the message-id included in the log. Keep in mind these are all events fired after selecting a message with a list-post header:

<4d7bd735991a5_4f2f157fde14c29c3d8@patchwrkr-d02.ihost.aol.com.tmail>: false
Starting processHeaders()
<009f01cbe11a$f909a7f0$eb1cf7d0$@net>: true
<009f01cbe11a$f909a7f0$eb1cf7d0$@net>: true

The AOL message-id is the message-id of the message that was selected before selecting the message with the list-post header.
(Reporter)

Comment 56

7 years ago
Created attachment 521182 [details]
extension with console messages

Use this extension to test it out in your Thunderbird. It's probably easier to see what I'm describing first-hand.
(Reporter)

Comment 57

7 years ago
Created attachment 521193 [details]
Extension implementing the patch

Well, I don't want to look into the asynchronous nature of complicated things like header parsing so I just put a flag in the IsReplyListEnabled() function and it now seems to work perfectly. Please test it. I will submit a source patch shortly.
Attachment #520936 - Attachment is obsolete: true
Attachment #520936 - Flags: review?(clarkbw)
Attachment #521193 - Flags: review?(clarkbw)
(Reporter)

Comment 58

7 years ago
Created attachment 521205 [details] [diff] [review]
Latest patch

This should take care of it.
Attachment #520825 - Attachment is obsolete: true
Attachment #520825 - Flags: review?(bwinton)
Attachment #521205 - Flags: review?(bwinton)
(Reporter)

Comment 59

7 years ago
Created attachment 521214 [details] [diff] [review]
Latest patch

Sorry for the bugspam. Fixed an edge case where right-clicking on a message while another message was selected did not check the status of IsReplyAllEnabled().
Attachment #521205 - Attachment is obsolete: true
Attachment #521205 - Flags: review?(bwinton)
Attachment #521214 - Flags: review?(bwinton)
(Reporter)

Comment 60

7 years ago
Created attachment 521216 [details]
Extension implementing the patch
Attachment #521193 - Attachment is obsolete: true
Attachment #521193 - Flags: review?(clarkbw)
Attachment #521216 - Flags: review?(clarkbw)
Comment on attachment 521214 [details] [diff] [review]
Latest patch

(So that I don't get distracted by this while I can't review it, I'm going to clear out the r? flag, but please re-ask me for review once you've gotten a ui-r+ from Bryan.)
Attachment #521214 - Flags: review?(bwinton)
Comment on attachment 521216 [details]
Extension implementing the patch

Sorry for the delay.  I played with this a little while and things seemed ok to me.  Mostly I have concerns about the complexity.  Blake should make the call here.
Attachment #521216 - Flags: review?(clarkbw) → review?(bwinton)
Comment on attachment 521216 [details]
Extension implementing the patch

I dunno…  While I appreciate the idea behind this extension, I don't really like the interactions I'm seeing, particularly with mailing lists.

Setting mail.ReplyAllBehavior to 1 seems particularly bad, since it removes the ability to reply to mailing lists which don't set the List-Post header entirely for me, leaving no workarounds.

When I set mail.ReplyAllBehavior to 0 or 2, I then saw the ReplyAll button all the time, instead of only seeing it when the reply would have gone to more than one person.

I think this is because, while we want to enable the Reply All menu item if there are one or more email addresses, we only want to enable the button if there are two or more.  This is so that we can let the user always use Ctrl-Shift-R to reply to everyone, while also showing them the correct button for what will actually happen.  I think this might have been the source of your confusion as to what was calling IsReplyAllEnabled vs. IsReplyEnabled…  (If so, that could definitely use some documentation!)

And while I'm here, I've got a couple of comments.
1) The patch doesn't apply cleanly for me anymore.  :(

2) You should probably be using Services.strings instead of var strBundleService = Components.classes["@mozilla.org/intl/stringbundle;1"].getService().QueryInterface(Components.interfaces.nsIStringBundleService);
and Services.prompt instead of gPromptService.

Thanks,
Blake.
Attachment #521216 - Flags: review?(bwinton) → ui-review-
(Reporter)

Comment 64

7 years ago
(In reply to comment #63)
> Setting mail.ReplyAllBehavior to 1 seems particularly bad, since it removes the
> ability to reply to mailing lists which don't set the List-Post header entirely
> for me, leaving no workarounds.

Doesn't just "Reply" work? I never knew there was a such thing as "Reply List" until I wrote this despite using Netscape and Mozilla since 1994. I didn't add any code to disable "Reply List" under any circumstances. Whatever code is doing that was there before this patch. The only thing I changed was to add code to enable "Reply All" when there was a List-Post header.


> When I set mail.ReplyAllBehavior to 0 or 2, I then saw the ReplyAll button all
> the time, instead of only seeing it when the reply would have gone to more than
> one person.

That's how it's always behaved. The code in mail3PaneWindowCommands.js hooks the "Reply All" button to IsReplyEnabled(). The "Reply All" and "Reply" button states will always be identical the way the current code is written.

> I think this is because, while we want to enable the Reply All menu item if
> there are one or more email addresses, we only want to enable the button if
> there are two or more.  This is so that we can let the user always use
> Ctrl-Shift-R to reply to everyone, while also showing them the correct button
> for what will actually happen.  I think this might have been the source of your
> confusion as to what was calling IsReplyAllEnabled vs. IsReplyEnabled…  (If so,
> that could definitely use some documentation!)

There was no confusion. Reply All is never disabled in the original code. That's because the "Reply All" button is checking IsReplyEnabled() not IsReplyAllEnabled().
 
> And while I'm here, I've got a couple of comments.
> 1) The patch doesn't apply cleanly for me anymore.  :(

Bit rot sucks.
(Reporter)

Comment 65

7 years ago
I should clarify: In mail3PaneWindowCommands.js both the "Reply All" menu item AND button are enabled for all messages that "Reply" is by the following (line 354):

if (command == "cmd_replyall" ||command == "button_replyall")
  return IsReplyEnabled();

It's easy enough to change the behavior, but I was just trying to avoid "fixing" a lot of things at once.
(In reply to comment #64)
> (In reply to comment #63)
> > Setting mail.ReplyAllBehavior to 1 seems particularly bad, since it removes the
> > ability to reply to mailing lists which don't set the List-Post header entirely
> > for me, leaving no workarounds.
> Doesn't just "Reply" work?

So, I get email from Bob@example.com addressed to EveryoneInToronto@example.com, which is some sort of group, but apparently not an official mailing list, because it has no List-Post or List-ID header.  "Reply" will reply just to Bob.  There is no "Reply to List" because there's no List-Post header.  If I want to reply to the EveryoneInToronto@example.com address, I need to use "Reply All", but it no longer exists…  :(

> I never knew there was a such thing as "Reply List"
> until I wrote this despite using Netscape and Mozilla since 1994.

There wasn't for a while, but we added it in 3.0, I believe.  Maybe in 3.1.

> I didn't add
> any code to disable "Reply List" under any circumstances. Whatever code is
> doing that was there before this patch. The only thing I changed was to add
> code to enable "Reply All" when there was a List-Post header.

No, I wrote the code to disable "Reply to List" if there's no List-Post header.  :)
And I believe that code is correct, since these strange things aren't really mailing lists per se, and don't contain any sort of header that would let us know that they were.

> > When I set mail.ReplyAllBehavior to 0 or 2, I then saw the ReplyAll button all
> > the time, instead of only seeing it when the reply would have gone to more than
> > one person.
> That's how it's always behaved. The code in mail3PaneWindowCommands.js hooks
> the "Reply All" button to IsReplyEnabled(). The "Reply All" and "Reply" button
> states will always be identical the way the current code is written.

You may think so, but I promise you, I can see the "Reply All" button sometimes, and just the "Reply" button other times, so I think there's a piece of code you're missing somewhere.  Perhaps it's http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#982 ?

> > I think this is because, while we want to enable the Reply All menu item if
> > there are one or more email addresses, we only want to enable the button if
> > there are two or more.  This is so that we can let the user always use
> > Ctrl-Shift-R to reply to everyone, while also showing them the correct button
> > for what will actually happen.  I think this might have been the source of your
> > confusion as to what was calling IsReplyAllEnabled vs. IsReplyEnabled…  (If so,
> > that could definitely use some documentation!)
> There was no confusion. Reply All is never disabled in the original code.

The Reply All menu item is never disabled, yes.
But the Reply All button sometimes isn't shown, and the Reply button is.

> > And while I'm here, I've got a couple of comments.
> > 1) The patch doesn't apply cleanly for me anymore.  :(
> Bit rot sucks.

Yeah, I agree.  But, now that I'm (mostly) on top of my reviews, you should see less bit rot, at least less due to me.  :)

Thanks,
Blake.
(Reporter)

Comment 67

7 years ago
Perhaps I have something set differently, but I see the "Reply All" button all the time no matter what the nature of the message I select. I just tried it on several dozen messages with only myself as the recipient, mailing lists, and messages with multiple recipients. I have never seen Thunderbird behave differently across a half-dozen machines. Perhaps it's due to the fact that I've customized all of my toolbars to add the "Next" and "Previous" buttons?
(Reporter)

Comment 68

7 years ago
I tried restoring the default toolbar set, but then there's not a "Reply" or "Reply All" button at all. Color me confused.
Ah, I get it.  I was talking about the one in the message header in the preview pane, not the one on the main toolbar…

Thanks,
Blake.
(Reporter)

Comment 70

7 years ago
I don't have any buttons on the message headers. Is that a pref?
I mean the ones highlighted in pink in http://dl.dropbox.com/u/2301433/Buttons.png  I don't think it's a pref.  Are you running with any extensions enabled?  (Also in that screenshot, you can see the Reply button, and no Reply All button… :)
(Reporter)

Comment 72

7 years ago
Weird. My Thunderbird does not look like that.

http://i.imgur.com/we4K5.png
(In reply to comment #72)
> Weird. My Thunderbird does not look like that.
> http://i.imgur.com/we4K5.png

Hmm.  Try right-clicking on the area above the date/time, and choose "Restore Default Set", and see what that does…

Thanks,
Blake.
(Reporter)

Comment 74

7 years ago
It was a userchrome.css change I made to get rid of them (I hate them). I just had forgotten.
You need to log in before you can comment on or make changes to this bug.