Closed Bug 611738 Opened 10 years ago Closed 10 years ago

Expand single or last message ID in headers display by default

Categories

(SeaMonkey :: MailNews: Message Display, defect)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.1b2

People

(Reporter: rsx11m.pub, Assigned: rsx11m.pub)

Details

Attachments

(1 file, 2 obsolete files)

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: <tests@example.com>
> References: 1, 2, <replied-to@example.com>

Expanded:

> References: <first@example.com>, <follow-up@example.com>, <replied-to@example.com>
Attached patch Proposed patch (obsolete) — Splinter Review
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.
Assignee: nobody → rsx11m.pub
Status: NEW → ASSIGNED
Attachment #490147 - Flags: superreview?(neil)
Attachment #490147 - Flags: review?(iann_bugzilla)
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.]
Attachment #490147 - Flags: superreview?(neil) → superreview+
Attached patch Proposed patch (v2) (obsolete) — Splinter Review
Updated with Neil's comment addressed (yes, that's shorter = saves 3 lines).
Attachment #490147 - Attachment is obsolete: true
Attachment #490460 - Flags: superreview+
Attachment #490460 - Flags: review?(iann_bugzilla)
Attachment #490147 - Flags: review?(iann_bugzilla)
(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 on attachment 490460 [details] [diff] [review]
Proposed patch (v2)

r=me with comment 4 examined.
Attachment #490460 - Flags: review?(iann_bugzilla) → review+
(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!
(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)
Attachment #490460 - Attachment is obsolete: true
Attachment #490496 - Flags: superreview+
Attachment #490496 - Flags: review+
Keywords: checkin-needed
Whiteboard: [c-n: comm-central]
Comment on attachment 490496 [details] [diff] [review]
Final patch for checkin [Checkin: comment 8]

http://hg.mozilla.org/comm-central/rev/49d830521725
Attachment #490496 - Attachment description: Final patch for checkin → Final patch for checkin [Checkin: comment 8]
Leaving the status change decision to you.
Keywords: checkin-needed
Whiteboard: [c-n: comm-central]
Target Milestone: --- → seamonkey2.1b2
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).
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.