611738 - Expand single or last message ID in headers display by default

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[rsx11m]

This is the SeaMonkey equivalent of Thunderbird bug 610603, which was initially filed for the Message-ID header only. Apparently as a consequence of bug 62033, not just the References but also the Message-ID header are opened by default "closed" with only the index numbers:

> Message-ID: 1
> References: 1, 2, 3

with mailnews.headers.showMessageId and mailnews.headers.showReferences set. The Message-ID header isn't of much use in this way (that's the motivation for bug 610603), while it makes sense for the References header, especially in long threads with many replies. Since those are effectively links, it is possible to trace back the replies within a thread. However, it may not be obvious which one is the  message replied to (i.e., the last one in the referenced chain), and it's hard to hit the tiny number to follow up to the replied-to message.

The proposal is to always expand a single message ID (there is no ambiguity here and sufficient space in a single line), and to expand the last (i.e., the most recent) message ID in a referenced reply chain:

> Message-ID: <[e-mail]>
> References: 1, 2, <[e-mail]>

Expanded:

> References: <[e-mail]>, <[e-mail]>, <[e-mail]>

Comment 1

[rsx11m]

Attachment: Proposed patch

This patch expands a single or the last message ID when opening a message,
thus resolving the issues mentioned in the bug description. In addition, the open/close twisty is hidden when just a single message ID is present.

Note that "toggleWrap" remains unpatched. Thus, upon closing after opening,
only the index numbers (1, 2, 3) are shown, assuming that this reflects the intention of the user by this action.

Comment 2

[[e-mail]]

Comment on attachment 490147 [details] [diff] [review]
Proposed patch

>+            if (numMessageIds > 1)
>+              this.toggleIcon.removeAttribute("hidden");
>+            else
>+              this.toggleIcon.setAttribute("hidden", "true");
Nit: this.toggleIcon.hidden = numMessageIds <= 1;

>+                this.updateMessageIdNode(messageIdNodes[index * 2], index + 1,
>+                  this.mMessageIds[index], index == numMessageIds - 1);
...
>+                this.updateMessageIdNode(itemInDocument, index + 1,
>+                  this.mMessageIds[index], index == numMessageIds - 1);
[Annoying, but not easily fixed without a rewrite.]

Comment 3

[rsx11m]

Attachment: Proposed patch (v2)

Updated with Neil's comment addressed (yes, that's shorter = saves 3 lines).

Comment 4

[Ian Neal]

(In reply to comment #2)
> Comment on attachment 490147 [details] [diff] [review]
> Proposed patch
> 
> >+                this.updateMessageIdNode(messageIdNodes[index * 2], index + 1,
> >+                  this.mMessageIds[index], index == numMessageIds - 1);
> ...
> >+                this.updateMessageIdNode(itemInDocument, index + 1,
> >+                  this.mMessageIds[index], index == numMessageIds - 1);
> [Annoying, but not easily fixed without a rewrite.]
One way round is to pass in numMessageIds instead and then do (aIndex == aNumIds) instead of aIsLastId in the if statement in the private method.

Comment 5

[Ian Neal]

Comment on attachment 490460 [details] [diff] [review]
Proposed patch (v2)

r=me with comment 4 examined.

Comment 6

[[e-mail]]

(In reply to comment #4)
>(In reply to comment #2)
>> (From update of attachment 490147 [details] [diff] [review])
>>>+                this.updateMessageIdNode(messageIdNodes[index * 2], index + 1,
>>>+                  this.mMessageIds[index], index == numMessageIds - 1);
>>...
>>>+                this.updateMessageIdNode(itemInDocument, index + 1,
>>>+                  this.mMessageIds[index], index == numMessageIds - 1);
>>[Annoying, but not easily fixed without a rewrite.]
>One way round is to pass in numMessageIds instead and then do (aIndex ==
>aNumIds) instead of aIsLastId in the if statement in the private method.
Good point, but I was actually thinking about the first parameter!

Comment 7

[rsx11m]

Attachment: Final patch for checkin [Checkin: comment 8]

(In reply to comment #4)
> One way round is to pass in numMessageIds instead and then do (aIndex ==
> aNumIds) instead of aIsLastId in the if statement in the private method.

So done, I renamed the parameter "aLastId" though, thus it's more obvious that any index can be passed as such.

Push on trunk, please (r=IanN, sr=Neil)

Comment 8

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 490496 [details] [diff] [review]
Final patch for checkin [Checkin: comment 8]

http://hg.mozilla.org/comm-central/rev/49d830521725

Comment 9

[Jens Hatlak (:InvisibleSmiley)]

Leaving the status change decision to you.

Comment 10

[rsx11m]

Thanks to all, I'm closing this bug as "Fixed" (if someone wants to address Neil's comment #6, this should be better handled in a follow-up bug).