Last Comment Bug 611738 - Expand single or last message ID in headers display by default
: Expand single or last message ID in headers display by default
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: MailNews: Message Display (show other bugs)
: Trunk
: All All
: -- minor (vote)
: seamonkey2.1b2
Assigned To: rsx11m
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-11-12 10:24 PST by rsx11m
Modified: 2010-11-17 10:13 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Proposed patch (2.84 KB, patch)
2010-11-12 10:32 PST, rsx11m
neil: superreview+
Details | Diff | Splinter Review
Proposed patch (v2) (2.72 KB, patch)
2010-11-14 09:46 PST, rsx11m
iann_bugzilla: review+
rsx11m.pub: superreview+
Details | Diff | Splinter Review
Final patch for checkin [Checkin: comment 8] (2.75 KB, patch)
2010-11-14 16:23 PST, rsx11m
rsx11m.pub: review+
rsx11m.pub: superreview+
Details | Diff | Splinter Review

Description rsx11m 2010-11-12 10:24:15 PST
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>
Comment 1 rsx11m 2010-11-12 10:32:05 PST
Created attachment 490147 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2010-11-14 08:22:47 PST
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 2010-11-14 09:46:03 PST
Created attachment 490460 [details] [diff] [review]
Proposed patch (v2)

Updated with Neil's comment addressed (yes, that's shorter = saves 3 lines).
Comment 4 Ian Neal 2010-11-14 14:08:15 PST
(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 2010-11-14 14:09:18 PST
Comment on attachment 490460 [details] [diff] [review]
Proposed patch (v2)

r=me with comment 4 examined.
Comment 6 neil@parkwaycc.co.uk 2010-11-14 16:03:18 PST
(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 2010-11-14 16:23:21 PST
Created attachment 490496 [details] [diff] [review]
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) 2010-11-17 10:04:52 PST
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) 2010-11-17 10:05:16 PST
Leaving the status change decision to you.
Comment 10 rsx11m 2010-11-17 10:13:17 PST
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).

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