Closed Bug 496440 Opened 12 years ago Closed 11 years ago

reply-button defaults to reply-all even when there are no header recipients

Categories

(Thunderbird :: Message Reader UI, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b3

People

(Reporter: dmose, Assigned: bwinton)

References

Details

Attachments

(1 file, 3 obsolete files)

Sometimes messages get sent to a group of people and the sender doesn't want to publicize the list of people they're sending to, so everyone ends up BCCed.

I got a message today with the following header:

To: undisclosed-recipients:;

Thunderbird got confused and offered "reply all" as the default menu choice rather than "reply".
Flags: blocking-thunderbird3+
Yeah, I believe that my code is thinking that "undisclosed-recipients:" is a recipient, and so would try to reply-all to the sender and "undisclosed-recipients:".

Assuming there's a way to find out whether an address is real, this should be a fairly small fix.
Status: NEW → ASSIGNED
Seems like we could tweak nsIMsgDbHdr and/or nsIMsgHeaderParser to understand that syntax, if they don't already.
That seems reasonable, but it's not really a syntax, just a convention.  I've seen, in my inbox, the following values:
undisclosed-recipients:
undisclosed-recipients: ;
undisclosed-recipients:;

And from X-Mailer: Microsoft Outlook Express 6, I also saw:
To: "Undisclosed-Recipient:;"@outbox.allstream.net <mailto:>  
To: <"Undisclosed-Recipient:;"@latte.ca>
To: <"Undisclosed-Recipient:;"@pobox10.ipowerweb.com>

I think it would be reasonable, if not perfect, to punt on the last three.  For the first three, I'm calling nsIMsgHeaderParser.parseHeadersWithArray().  Would it be reasonable to return null or "" in the email address array for these cases, or would it be better to omit them entirely?

I've got a quick patch which I'll put up, which kind of hacks around the problem.  Let me know if you think it's okay for now, or if you'ld rather leave it the way it is until I (or someone else) can change the nsIMsgHeaderParser/nsIMsgDbHdr.  (I think the HeaderParser might be a better place to do it, just from the methods I'm calling.)
It's a quick hack, but it works for most of the email I have access to, and might let us push a more involved fix off to a later beta.

Let me know what you think.

Thanks,
Blake.
Attachment #382172 - Flags: review?(dmose)
If it's just one recipient, check that ':' and ';' aren't present in it?

It doesn't have to say "undisclosed-recipients", but can be any valid rfc2822 group syntax.
OS: Mac OS X → All
Hardware: x86 → All
(In reply to comment #5)
> If it's just one recipient, check that ':' and ';' aren't present in it?
> It doesn't have to say "undisclosed-recipients", but can be any valid rfc2822
> group syntax.

I've seen "undisclosed-recipients:" (without the ';') in the wild, so I'm going to just check for ':'.  This is reasonably safe, since from my tests, it looks like if there is an email address group, the ':' will become part of the first name, and not show up in the email address list.

After implementing that, I had something that seemed to work fairly well, and then I tried it on my sent messages, and it totally didn't do the right thing, so I've reworked the logic to include the sender's address when we're trying to figure out how many people we'll be replying to.

One of the things I'm not sure about is the indentation for:
+  let numAddresses = hdrParser.parseHeadersWithArray(uniqueAddresses,
+                                                     emailAddresses, {}, {});

I've seen a few different formats for lines like it, and picked the one I thought you would like the most.  ;)

The other thing I'm not sure about is:
+  if ("bcc" in currentHeaderData)
+    addresses += currentHeaderData.bcc.headerValue;
I sort of expected the data from the bcc header to show up in msgHdr.bccList, but it's not there, and is in currentHeaderData.bcc instead.  Since I can get to the data, I'm not sure whether it's worth filing a bug on.

Thanks,
Blake.
Attachment #382172 - Attachment is obsolete: true
Attachment #382910 - Flags: review?(mkmelin+mozilla)
Attachment #382172 - Flags: review?(dmose)
Comment on attachment 382910 [details] [diff] [review]
Re-work the logic to include the sending address in our calculations.

Sorry about the delay, unfortunately this bitrotted :(

Did this work when people have e.g. "Example [:foo] <example@example.com>" addresses?
Attachment #382910 - Attachment is obsolete: true
Attachment #382910 - Flags: review?(mkmelin+mozilla)
(In reply to comment #7)
> (From update of attachment 382910 [details] [diff] [review])
> Sorry about the delay, unfortunately this bitrotted :(

No worries.  I like the change that bitrotted it.  :)

> Did this work when people have e.g. "Example [:foo] <example@example.com>"
> addresses?

Yes, because in that case the ":" will appear in their name, not in their address, so the check on line 863 will never see it.

It will fail for people with a ":" in their email address, but from my reading of the spec, I believe that's illegal.  Also, the only address I've seen with a ":" in it was '"Undisclosed-Recipient:;"@latte.ca', which is fairly clearly not an actual address.

Thanks,
Blake.
Attachment #383298 - Flags: review?(mkmelin+mozilla)
Attachment #383298 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 383298 [details] [diff] [review]
The un-bit-rotted version of the previous patch.


>+  for (i in emailAddresses.value)
>+    if (/:/.exec(emailAddresses.value[i]))
>+      numAddresses--;

Three nits:
 o var i
 o use test() instead of exec()
 o use {'s for the for loop (easier to read as its body is multiline

r=mkmelin with that
Depends on: 498480
(In reply to comment #9)
> Three nits:
>  o var i

Fixed.

>  o use test() instead of exec()

Fixed.

>  o use {'s for the for loop (easier to read as its body is multiline

Fixed.

> r=mkmelin with that

Thanks.
Attachment #383298 - Attachment is obsolete: true
Keywords: checkin-needed
Checked in: http://hg.mozilla.org/comm-central/rev/9f8df696c12b
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: Thunderbird 3.0rc1 → Thunderbird 3.0b3
You need to log in before you can comment on or make changes to this bug.