456596 - emails with lots of addresses in the To: field should only show first N by default, not just one address + "more" designation

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect, P3)

Description

[David Ascher (:davida)]

and then give people a way to show all the recipients.

Comment 1

[Bryan Clark (DevTools PM) [:clarkbw]]

We need to also indicate the number of recipients beyond N
ex-
   to: David, Dan, Magnus, Mark, Ronald, Karen, Kerry, ( 22 others v )

Comment 2

[David Ascher (:davida)]

Note that https://bugzilla.mozilla.org/attachment.cgi?id=340049 on bug 455801 will mitigate the current situation, but doesn't really do this.  

Bryan: do we care about N, or do we care about # of lines being shown, in particularly for people not in the address book, where "Dan" could be "Dan Mosedale <[e-mail]>".

Comment 3

[Bryan Clark (DevTools PM) [:clarkbw]]

> Bryan: do we care about N, or do we care about # of lines being shown

Our ultimate goal is to control is the height of the element from expanding further than necessary.  Number of lines would be a way of controlling the height, however I think we control via # of addresses by using the assumption that email addresses might take 2 lines at most.

I'm estimating that 7 addresses shown is enough for the to and cc lines each.  Of course since the goal is total control of height it would be nice to just use to + cc = 14 instead of individual stops of to = 7, cc = 7.

For example if we had a ceiling on each section at 7 and the separation worked out to be to = 2 and cc = 8 then we'd hide 1 email address in the CC line when we actually have planned for the header to be able to expand up to 14 lines when needed.  However this might be tricky programming to work out and so a reasonable first step forward would be to just limit each section to 7.

I'd just use some simple text for the expander ( ... and # more v )  That would probably look fine if it copied the other actions / hide details button look.  Then using a similar ( less addresses ^ ) button after expanding.  Need to work on the right text for this.

I'll upload an attachment of a mockup in a second.

Comment 4

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: mockup of collapsing email addresses after 7 are shown

Comment 5

[Mike Cowperthwaite]

The To and CC address lists already collapse by default if they exceed one line.   (Maybe that's new?  I'm looking at Gecko/20081005 Shredder/3.0b1pre.)

What's the additional benefit that's worth the cost of requiring the user to expand the list twice to see the whole thing?


As for displaying count, I'd be satisfied with a simple "(and more...)" or even just an ellipsis.  No matter what the actual number is -- 1, 2, 3, 7, or 185 -- it's not really germane to my mail-reading experience.  What's important to me is (a) how did I get this message? (am I on the To: or CC: list or, presumably, BCC'd?) and then (b) was this message received by X?  Sorting the list to put me first and my contacts second, then all the rest, might be sensible, but otherwise if I want to expand the list, I probably always want to see every item.

Comment 6

[Bryan Clark (DevTools PM) [:clarkbw]]

> The To and CC address lists already collapse by default if they exceed one
> line.   (Maybe that's new?  I'm looking at Gecko/20081005 Shredder/3.0b1pre.)

That expand / collapse has been there for a while.  We'd be removing that in favour this, a list that shows a number of addresses and truncates before too many instead of just one.

> What's the additional benefit that's worth the cost of requiring the user to
> expand the list twice to see the whole thing?

It wouldn't be 2 expanders, this is replacing the original.

> Sorting the list to put
> me first and my contacts second, then all the rest, might be sensible, but
> otherwise if I want to expand the list, I probably always want to see every
> item.

Probably for a different bug, but this appears to be a classic debatable item.  On one hand it's valuable to have yourself listed at the top so you can see via the to: or cc: how you were sent an email.  On the other hand many people claim to use the order of email addresses as a social cue.  Was the list completely alphabetical or some other priority and where is my address relative to others...

Comment 7

[rsx11m]

(In reply to comment #5)
> The To and CC address lists already collapse by default if they exceed one
> line.   (Maybe that's new?  I'm looking at Gecko/20081005 Shredder/3.0b1pre.)

Up to 1.8.0.x, there used to be a mailnews.max_header_display_length pref to determine the number of e-mail addresses shown before the list was collapsed. This was depreciated in 1.8.1.x in favor of the one-line-only mechanism.

> What's the additional benefit that's worth the cost of requiring the user to
> expand the list twice to see the whole thing?

IMO, there should only be one level of expansion: A configurable number of addresses (or lines of addresses) before the list is collapsed, and then a single expand-all to open the full list.

(In reply to comment #6)
> That expand / collapse has been there for a while.  We'd be removing that in
> favour this, a list that shows a number of addresses and truncates before too
> many instead of just one.

That's in part what is asked for in the MailNews Core bug 381491, which I filed last year, thus would be good to have.

Comment 8

[David :Bienvenu]

is this really needed for b1, or should we move it b2?

Comment 9

[David :Bienvenu]

Bryan and I talked about this - it doesn't seem like a blocker, though it would be great to have it improved. marking wanted.

Comment 10

[rsx11m]

Attachment: Draft for hidden pref

That's more than likely too late for 3.0, but I'm posting it anyway while I'm still having it in my head, so that this can get done for 3.1.

This partial patch introduces a new preference "mailnews.show_more_after" which is then used in buildViews to override the default of maxAddressesBeforeMore:

* if mailnews.show_more_after > maxAddressesBeforeMore, pass that limit to
  fillAddressesNode() and show up to that number of addresses before the "more"
  button becomes visible;

* for a counter larger than the default, allow wrapping of the address line
  (why is "singleline" present in the first place?), which works now that
  bug 489609 has removed the scrollbar introduced by bug 223132 unless the
  headers are expanded (which may likely need to be changed after bug 492645
  is fixed and bug 525225 can land).

What the patch does not take care of:

* introduce a default pref definition, e.g., in all-thunderbird.js;

* modifying the binding for "mail-multi-more-indicator" so that the number of
  hidden addresses can be shown as "<n> more" in the button (it's easy to
  calculate that number, but how it is passed to that binding for display is
  beyond my knowledge).

If someone wants this for the TB 3.0 builds already, just apply this patch to content/messenger/mailWidgets.xml in messenger.jar for the time being.

Comment 11

[rsx11m]

According to a comment in the patch for bug 499989, implementing the "more" button as a "mail-multi-more-indicator" binding was necessary due to the 3.0 string freeze with more.label defined in a DTD file. This is supposed to be modified to a property definition, then building the node within "buildViews", thus allowing to add the number of hidden addresses to the label.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Thanks for the patch-in-progress.  This is indeed too late for 3.0, but we could look at something like it for 3.1.  In the ideal case, we'd actually end up taking a patch that does something good for the default case (eg clarkbw's suggestion in comment 4).  That way we also help users unable/unwilling to dabble with hidden prefs.

Comment 13

[rsx11m]

Attachment: Draft for extended label

This patch is incremental to attachment 413249 [details] [diff] [review] and allows to show "<n> more" with the number of hidden addresses hidden included in the label.
I figured out how to make the value of the "mail-multi-more-indicator" label available by inheritance while retaining its binding character, thus not having to mess with those nested parentNode constructs. The draft also satisfies the part in comment #11 on replacing the entity with a property.

(In reply to comment #12)
> In the ideal case, we'd actually end up taking a patch that does something
> good for the default case (...) That way we also help users unable/unwilling to 
> dabble with hidden prefs.

The combination of these two patches roughly resemble what Bryan has suggested in comment #4, thus I'm not exactly sure what you suggest. If you are talking about having a default set for the preference to present it visibly in the Config Editor, that's easy. Or, if you are thinking of providing a UI for that preference (Advanced > Reading & Display?), should be straightforward as well.

Comment 14

[rsx11m]

Attachment: Mockup for preference dialog

Dan, Bryan: Is this what you have in mind to make that default better visible?

Comment 15

[rsx11m]

Attachment: Proposed patch (w/o pref dialog)

This patch combines the two drafts and also adds the new preference to all-thunderbird.js so that it can be easier found. Also accounting for
some bitrotting from bug 530239, changing the default from 2 to 1 now.

Note that attachment 413822 [details] is not yet included, I'm still waiting
for feedback whether or not it is wanted in the preferences UI.

Comment 16

[Ben Bucksch (:BenB)]

Nice!

Comment 18

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 413822 [details]
Mockup for preference dialog

This could work for me.  Though it does seem that giving this kind of options means we should really be doing something better by default.

Comment 19

[rsx11m]

Attachment: Possible fix, v2

This patch adds the preferences UI of attachment 340205 [details] to the
initial patch of attachment 414926 [details] [diff] [review], other than that no changes.

Some additional housekeeping in advanced.{xul,dtd} performed by this patch:

 - renamed "reading.caption" and "display.caption" to "*Desc.label" for
   consistency and made them more specific;

 - moved "showCondensedAddresses" preference and entity definitions down
   to match their actual position within the preference pane.

Comment 20

[rsx11m]

(In reply to comment #19)
> This patch adds the preferences UI of attachment 340205 [details]

That's of course attachment 413822 [details].

(In reply to comment #18)
> This could work for me.  Though it does seem that giving this kind of options
> means we should really be doing something better by default.

It will always be a trade-off: Some users just want a single line per header there to save vertical space, others need to see as much as possible and don't care about spanning multiple lines. Thus, a configuration option is better than trying to find a static on-fits-all default solution.

Comment 21

[Wayne Mery (:wsmwk)]

sorry for coming late to the conversation. A solution is wanted for us to deploy v3 campus wide which doesn't match the current direction.

I understand the desire to simplify the logic behind this, but I think a solution based on #contacts may not be best in creating a solid user experience. I don't have an official consensus for our site, but I suspect it will be a preference based on max #lines. #contacts shown then just ends at what fits in #lines.

The following immediately come to mind if pref is #contacts instead of #Lines:
* height of header will frequently jump, which to me is bad UE and distracting
* a default setting of 1 contact is, for lack of a better word, absurd in the context of corporate users, as is making the user click "more" when there is room on a line to display more contacts
* the last (sometimes only) line will probably be ~ half blank - which isn't a polished look, plus the annoying experience mentioned above. (again, to paraphrase a coworker's reaction, absurd)

Appreciate the efforts here. Please rethink.

Comment 22

[rsx11m]

Anything is better than the current display of a single contact per header, and the solution to introduce a number of addresses to show is the best I can offer. The 2.0 behavior was indeed to display a single line (rather than a single contact) per header, but this was hard-wired too. Since I don't fully understand that old code, the prior bug 381491 covering this remains unsolved.

As for TB 3.1, I'd like to point out that the intended feature freeze is in about three weeks form now, if the schedule for 3.1b1 holds, thus I'd say that a compromise is better than nothing...

Comment 23

[Ben Bucksch (:BenB)]

Bug 520249 is for a proper solution for this whole problem, without pref, possibly by rewriting the header pane.

Comment 24

[rsx11m]

Wrong, the issue here has nothing to do with the headers/buttons conflict described in bug 520249; the pref is needed to allow the user configuration of the address headers. As I stated in comment #22, the main issue addressed here is the desire of some users to commit only a single line to a header, whereas others prefer to see more or all of the recipients at once.

Comment 25

[rsx11m]

Bryan, Dan: Ping for the reviews, the clock keeps ticking for 3.1 ...

Comment 26

[rsx11m]

To add some statistics, since TB 3.0.1 was released with bug 530239 reducing the number of addresses before the "more" to one, I have advised 5 users on the MZ support forum on how to hack the code to get more addresses by default. Also, a quick search on GS gave me 4 threads during that duration:

> http://www.getsatisfaction.com/mozilla_messaging/topics/how_do_i_show_more_email_addresses_automactically
> http://www.getsatisfaction.com/mozilla_messaging/topics/show_all_recipients_automatically_in_header_pane 
> http://www.getsatisfaction.com/mozilla_messaging/topics/why_cant_i_see_more_people_cc_ed_on_an_incoming_email
> http://www.getsatisfaction.com/mozilla_messaging/topics/how_to_disable_more_button_in_distribution_list

Comment 28

[Martin Fleurke]

PLEASE fix this! It is the most annoying thing in TB3. There is usually plenty of room to show more email addresses on a single line. To have to click 'more' for each email (to see just 1 or 2 extra addresses that could have been displayed already without any difference in space occupation) is terrible!

Comment 29

[Ben Bucksch (:BenB)]

> to see just 1 or 2 extra addresses that could have been
> displayed already without any difference in space occupation
[because there is free horizontal space]

That is something which may be fixable without pref. That's exactly what bug 520249 is about.

Comment 30

[rsx11m]

...but only for the single-line scenario. I know I'm repeating myself (so do you), but unless you are more specific what bug 520249 would do in terms of making the "more" truly customizable, I consider these two different issues.

Comment 31

[rsx11m]

I've added the 2nd link from comment #26 as URL to this bug as it describes most comprehensively the issues within the GS feedback:

Item #1 of that GS post is one extending the number of addresses to utilize the single "To:" and "Cc:" lines (bug 520249, if I understand Ben correctly); the items #2 and #3 are covered here for configurability (the "All" case #3 can be achieved by setting the preference to 99, no need to cover that separately).

As stated in the companion bug 381491, filling up the line while allowing the user to configure a larger number of addresses to be opened by default with as many lines as needed are complementary and not conflicting approaches.

Hope this helps to stay on topic, patch in attachment 421270 [details] [diff] [review] applies as posted.

Comment 32

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #25)
> Bryan, Dan: Ping for the reviews, the clock keeps ticking for 3.1 ...

Indeed; apologies for not getting back to this sooner, and thanks so much for picking up this bug and driving it forward.  You're quite right that we need to do something about this for 3.1; I'm going to mark it as blocking-beta1+.  Note that our schedule is not yet set in stone, Bryan and I are in the midst of working through feature planning and blocker driving, so we'll know more about the schedule soonish.  We'll figure out some way to have the schedule accommodate this bug.

(In reply to comment #30)
> ...but only for the single-line scenario. I know I'm repeating myself (so do
> you), but unless you are more specific what bug 520249 would do in terms of
> making the "more" truly customizable, I consider these two different issues.

This matches my understanding of bug 520249 as well.

I've talked to Bryan about the UX some yesterday, and he's still not comfortable that the patch as written is what we want for 3.1.  He sounded willing to be pretty flexible as far as approaches are concerned.  I know that in the past, he filed https://bugzilla.mozilla.org/show_bug.cgi?id=517611>, but I'm not sure about his thoughts on that currently.

I think that trying to work through this via Bugzilla comment ping-pong is likely to be very slow going.  Would you be able to join Bryan and me sometime this coming week for a chat to work through this, either via IRC, phone, or Skype?  Pacific time work hours would be ideal, but I suspect we can be somewhat flexible there...

Comment 33

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 421270 [details] [diff] [review]
Possible fix, v2

Removing r?, as I think we need to sort through the UX questions before we'll be ready for review.

Comment 34

[rsx11m]

Dan, thanks for responding. Let me sort out my schedule for next week and
I'll get back to you and Bryan on comment #32 soon.

Comment 35

[Dan Mosedale (:dmosedale, :dmose)]

We had a good chat yesterday and have a theory about how to move forward by hoisting (more) up one level so that the recipients get clipped by it.  I need to do some bugzilla archeology and figure out why I abandoned this the last time I tried it.  I'll be contacting RSX11 with that info later this week.

Comment 36

[rsx11m]

First of all, a big thanks to Dan, Bryan, and Blake for devoting more than an hour of their time on IRC yesterday to discuss this bug. I'll try to briefly summarize the possible options and their dependencies below (in decreasing order of preference), please comment/correct me if I've got something wrong.

1. As suggested in comment #21, allow the display of one completely filled line per header by default, then show the "more" button; per preference, a minimum number of lines can be configured up to which the header box is allowed to wrap before the "more" appears. This seems to be the most intuitive way to steer the vertical height while making best use of the available space.

2. Let the single line fill up with as many addresses as possible, then show the "more"; if a specifiable minimum number of addresses hasn't been reached yet, wrap and allow the box to expand until that number is filled, and show the "more" then if still addresses remain.

3. By default, disallow wrapping and show a single line until it is filled and the "more" appears; if the user specifies an explicit limit, show that number regardless of how much space is used and allow the header box to wrap as necessary, then show "more" if not all addresses could be shown.

While option #1 would be the nicest solution, it depends on XUL being able to precalculate the line wrap correctly. As discussed in bug 492645, width and height are currently mangled when combining overflow and wrapping, thus it may be tricky to accurately calculate the needs for, let's say, exactly 3 lines. The fallback in option #2 restricts the problem to a single line, would still require though an accurate detection of an overflow situation in order to allow the headers to wrap and to add more addresses. Option #3 simplifies the issue further by switching between a one-line and a multi-line scenario; this still relies on filling one line correctly being done, but not needing the addition of further lines depending on whether or not the box capacity will be exceeded (it will be known upfront if the user wants to see one line or n addresses).

There are also options 4a. and 4b.: (a) reimplementation of the single-line default similar to what was initially designed by bug 335973 for Thunderbird 2.0 (Dan's preferred fallback, but it wouldn't satisfy those users needing a different default for their way of work); like #1 to #3, this requires moving the "more" button itself outside of the header box, this is what Dan is looking into (in addition to the XUL issues). Option #4b would be the current patch (which may still leave horizontal space unused), but with a more useful default (n=3 was suggested, which corresponds to the Thunderbird 1.5 setting); this would also be the only variant which doesn't require any XUL-based overflow counting at all or moving the "more" button, thus it's definitely feasible as a mitigating solution even if the other options don't work out as expected.

Comment 37

[rsx11m]

Attachment: Draft patch for line-counting version

This is a barely functioning patch but should be sufficient to illustrate a possible configuration option based on the number of lines displayed rather than the number of addresses. It is re-using the idea in bug 335973 to fill
all addresses into the header value box but deny it to overflow.

The patch allows the box to wrap again even with the singleline="true" attribute set, while retaining the "overflow: hidden;" definition. After filling /all/ of emailAddresses, the scrollHeight parameter is used to get
the total height if allowed to fully expand. Also, the height of the first e-mail address is taken as a reference, then multiplied with the number of allowed lines given by the pref value. Whatever the smaller of those values determines the height of the longEmailAddresses box. There is some padding around the complete block but not between adjacent address lines, so that calculation works out. When the header is reset or expanded using the "more" button, the "height" attribute is removed to allow recalculation or expansion, respectively.

So much for a proof of concept, here the problems with this implementation:

(1) The timing is unclear to me, initially the first header usually works but the following headers produce a zero (0) scrollHeight, resulting in a collapsed header line. Going to a different message and then revisiting the same one usually resolves the issue.

(2) The sizes would have to be recalculated when the window is resized, the current patch retains the height calculated once. Setting maxHeight instead likely wouldn't work due to bug 492645 not allowing the box to fully expand until maxHeight is actually reached.

(3) Events need to be wired. I tried to find something for "toggleWrap()" in the "more" definition (now no longer in its own binding but part of the
multi-address header binding) but failed with any parentNode combination.
Also, overflow and underflow conditions would need to hide or unhide the "more" button as needed.

(4) Counting lines now instead of numbers, a version of "x more" becomes ambiguous. We'd need to figure out how many addresses are actually displayed
to determine the number of remaining addresses. Also, it will change when resizing the window, thus numbers need to be recalculated here as well.
Showing something like "+x lines" instead may be easier to calculate, if the scrollHeight can be reliably calculated.

(5) In terms of layout, the "more" button is now pushed to the right and looks detached from the address list it belongs to. I don't know how this could be resolved, as the address list and the more button are now distinct XUL elements in an hbox.

Comment 38

[rsx11m]

Attachment: Screen shot of v3, with n=3 on Windows XP

This shows a list of 9 "to" and 4 "cc" addresses, filling about two addresses per line in this average window width. While the "from" and "cc" headers are allowed to fully expand, the "to" header is truncated to 3 lines, leaving 3 addresses unshown.

Note that instead of the "more" label, the buttons show for debug information the full height of all headers / height of the first address = selected box height. For illustration, one address box is highlighted.

Comment 39

[rsx11m]

Attachment: Possible fix, v3 (other approach, broken)

After looking at bug 499989 (which introduced the current "more" mechanism), I've noticed that I'm actually rolling back quite a few changes which were made in that patch, thus giving me a clue for the event handling (that's item 3 in comment #37).

This patch now enables and disables the "more" as needed, based on overflow and underflow events. By moving the "more" button out of the hbox, toggleWrap() suddenly works again (no clue why). Since the calculated height may be a little smaller than the initial box height, I'm now comparing line numbers rather than heights, and pick a box height plus one to avoid the overflow to be triggered if the "more" button is not to be shown.

All other problems listed in comment #37 still exist. Also, clicking "more" actually reduces now the header-pane height due to bug 492645 kicking in as soon as "overflow: auto;" is switched on.

Comment 40

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: old, half-finished patch for reference

After grovelling around for a while, I found the old patch where I started to work on hoisting out the (more) node.  You'll notice that there's a lot of undone stuff here.  The reason I abandoned it was because it became clear that there were still a bunch of issues to be sorted out there were going to require a bunch of unraveling.  I'm not sure whether this will actually offer any interesting insight, but it might.

One possibility that occurs to me while reading your recent comments it that it's conceivable that some of the apparent non-determinism you're seeing might be caused by the caching code in there.  You could try adding something to effectively disable the cache in order to rule that out.

So the main reason I was removing code where I could before was for maintainability.  One of the biggest problems with this code is that was (and still is, to a lesser extent) pretty hard to hold all the necessary state in one's head to work with it.  So it's definitely worth keeping in mind going forward.

It's not clear to me if there's more specific input that you would find helpful in order to move forward.  Is there?

Comment 41

[rsx11m]

Thanks for looking up your old patch. I'll have to understand what it's doing first before I can comment if it helps though... ;-)

Yes, caching may be a candidate for causing those weird height calculations.

Comment 42

[rsx11m]

Well, I've tried to get rid of the address caching by avoiding the call to this.fillCachedAddresses() in fillAddressesNode() and setting index = 0; to
force the address list to be rebuilt, removing possible prior addresses by a this.clearChildNodes(aAddressesNode). This didn't change anything in the height calculation though, it appears that the scrollHeight and clientHeight values are delayed and don't reflect the current filling status of the box. Unless there is a way to flush/sync the box with its content (or maybe I'm missing that), this will remain unreliable.

Some other possible issues with attachment 427230 [details] [diff] [review]:

- Catching the mail window being resized isn't simply a matter of overflow or underflow detection; e.g., for n=2 lines, if 1.5 lines are filled (no overflow) and the window size is increased so that a single line would suffice, the box will still be 2 lines high and no event is generated.

- Height calculations are based on the first address under the assumption that all address boxes have the same height. Is this always the case? What happens, for example, if Western and Asian encodings are mixed, would they have the same height? If not, the concept of determining the box height as a multiple of an address box height wouldn't hold, a more complex way of identifying the maximum height for each line separately would be needed to get it right.

Some remarks on Dan's patch in attachment 427247 [details] [diff] [review]:

- I like the idea of keeping the current implementation of making the "more" button a node of the address list, this avoids the somewhat detached appearance before bug 499989 which would be reintroduced by my n-line patch.

- The problem I see is that the timing/update is even more critical than with my height-restricting patch. It relies on an overflow being triggered when one address is added which is one too many, then removing it and placing the "more" node instead (which in turn might cause another overflow if it doesn't fit into the line either, then another address node needs to be removed).

- Resizing the box would require some iteration of removing the "more" node, adding or removing addresses until a new overflow/underflow condition is triggered, then "more" added back and again a possible overflow situation may need to be resolved if it won't fit into the last line. This sounds like a lot of going forth and back, keeping track of states and events.

So, neither of the two variants seem to be without challenges and issues...

Comment 43

[Ben Bucksch (:BenB)]

> This didn't change anything in the height calculation though, it appears
> that the scrollHeight and clientHeight values are delayed and don't
> reflect the current filling status of the box.

That's correct. Gecko delays calculation and rendering and indeed does it delayed. That's why, when you do appendElement() and then *Height, you don't notice a change.

> Unless there is a way to flush/sync the box with its content
> (or maybe I'm missing that), this will remain unreliable.

There is a way to do that. There are certain functions which per spec require changes to take effect, and Gecko will then force the rendering and block your call until this is done. I don't remember which ones those are, but you can ask bz or some other guys on #developers.

Comment 44

[rsx11m]

I had tried document.defaultView.getComputedStyle() per Dan's patch, which he added before taking the height and width values, and applied it to various elements (longEmailAddresses, emailAddresses, and childNodes[0]), but it didn't make any difference. All returned 0 for the second header (but not for "from") when opening the message first. Directly using getPropertyValue as suggested in https://developer.mozilla.org/Talk:en/Gecko_DOM_Reference/Examples gave me no useful results either.  :-\

Comment 45

[Dan Mosedale (:dmosedale, :dmose)]

After email discussion with rsx11m and voice discussion with clarkbw, it's pretty clear that we're not going to have something ready here for 3.1b1, and rsx11m has other commitments for the next few weeks.  Moving out to block beta2; bwinton, clarkbw, and I will be work out a plan here later this week.

Comment 46

[Ben Bucksch (:BenB)]

That "ninja" idea (bug 517611) would fix at least part of it and be easier to implement.

That said, I like rsx11m's patches (from code perspective, didn't try them).

Comment 47

[Ben Bucksch (:BenB)]

rsx said he doesn't have time for this at the moment, so I'll take over from here.

Comment 48

[rsx11m]

That's fine, thanks. I'll keep an eye (or maybe half) on my bugmail so that I can provide feedback as necessary.

> (comment #46) That "ninja" idea (bug 517611) would fix at least part

Combining attachment 421270 [details] [diff] [review] with such an approach would certainly be
possible as a quick(er) fix. It's a bit diluting though what the preference represents and what the "n more" display is good for, due to the then fuzzy calculation of how much an address is "worth" for the count.

Comment 49

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 421270 [details] [diff] [review]
Possible fix, v2

just removing my review from this as we seem to have something else in the works.

Comment 50

[Ben Bucksch (:BenB)]

Actually, Bryan, rsx's patches are our best bet, as said in the conference call.

Comment 51

[Ben Bucksch (:BenB)]

I can't see anything wrong with patch v2. It's just implementing the preference, basically. Also seems to work OK for me in my limited test. Just some minor code changes, but otherwise, if Bryan thinks the pref is a good idea in general, why not take patch v2?

Comment 52

[Ben Bucksch (:BenB)]

Attachment: Fix, v4

This is fix v2, but the code fixed up quite a bit.

Changes (some):
- Technically the most interesting change of the patch overall is probably that I removed the singleline= . It's been a code/combinatoral headache and think it now outlived its usefulness (I can no longer enforce it when more than 1-2 addresses are shown, so why bother?).
- I changed the pref name to mailnews.headers.show_n_addresses_before_more, because I was looking for a branch specific to header display, and we already have a number of prefs like mailnews.headers.showMessageId, so I use that branch.
(In the light of merges, I don't think it makes sense to differentiate between Thunderbird and TB+Seamonkey, and that's also not where "mail"/"mailnews" comes from originally. It's an obsolete separation and we should not stick to it.)
- No need to force a minimum in the code
- Removed the temporary binding, as the TODO/XXX comment asks to
  (and solved the .parentNode. problem rsx ran into when doing so)

Comment 53

[Ben Bucksch (:BenB)]

Attachment: Fix, v5

Gack, attached the wrong patch.

Note: all this patch does is to implement the pref and remove the singleline. It doesn't really fix any problem in the default case.

Also note that this patch will inherently conflict with any solution for "ninja" bug 517611, because the pref here says "number of addresses", and the ninja likes count some addresses only as half, so we need to rename the pref and UI text that this patch here introduces, from "n addresses" to "about n lines".

Comment 54

[rsx11m]

Comment on attachment 430222 [details] [diff] [review]
Fix, v5

> +++ b/mail/app/profile/all-thunderbird.js
> +pref("mailnews.headers.show_n_addresses_before_more", 1);

> +++ b/mail/components/preferences/advanced.xul
> @@ -266,18 +271,31 @@
> +                       type="number" default="1" min="1" max="99" size="2"

Per discussion on IRC the other day, the idea was to change the default to n=3 if we go with the v2 patch. And yes, this is currently the only version known to work correctly.

Comment 55

[rsx11m]

Comment on attachment 427045 [details]
Screen shot of v3, with n=3 on Windows XP

That's actually a screenshot of the patch in attachment 427230 [details] [diff] [review] (v3).
The n-line version with the problems stated in comment #42.

Comment 56

[Dan Mosedale (:dmosedale, :dmose)]

What's the next step here?  A screen shot of the latest patch for ui-review?

Comment 57

[Ben Bucksch (:BenB)]

dmose, Bryan told me on IRC he considers bug 517611 to be more important than this bug here, and bug 517611 inherently needs to use a different pref based on lines instead of addresses. However, I found that bug 517611 doesn't work terribly well, so the next step is to try to show N *lines* of addresses instead of showing N *addresses*, and calculate the lines, somehow. (I don't know which bug that would be.) I'll try to do that next, today.

Comment 58

[Ben Bucksch (:BenB)]

I filed bug 550487 about the latter. Will attach patches there, to not confuse them with the patches here. My patch is along the same line as rsx' patch v3, and I run into similar problems, like mentioned in comment 43. I haven't found the solution yet.

Comment 59

[Ben Bucksch (:BenB)]

Our best bet is either bug 550487 comment 2 or this bug, Fix v5. (I don't think bug 517611 works well enough.)

Comment 61

[Dan Mosedale (:dmosedale, :dmose)]

Current belief is that the patch in bug 550487 is going to be what lands for 3.1, rather than one of the patches for this bug.  Removing the blocking flag here.

Comment 62

[rsx11m]

Agreed that the n-line preference would be nicer than the n-address setting proposed here, but let's nevertheless keep the v5 patch on the radar as the 3.1 fallback, if the efforts in bug 550487 don't make it for the Tuesday deadline.

Comment 63

[rsx11m]

I'm currently revisiting header-pane related bugs to see what's left to do.
Since Ben and Dan have done a great job in bug 550487 implementing the n-line variant, and only bug 560698 and bug 567062 follow-ups are still open for
window-size borderline cases, it appears that most issues here are resolved.

Bryan, Dan: There is still a preferences UI component which Bryan has already ui-review+'ed (attachment 413822 [details]) and which apparently got lost in
bug 550487. The semantics has now changed from "n addresses" to "n lines", but
it still should be useful to expose that feature in the settings UI (on trunk).
I'll be happy to revisit that part of the patch for the new pref if desired.

Comment 64

[Dan Mosedale (:dmosedale, :dmose)]

I think we ultimately decided that the cost of having that text add visual complexity and possible confusion for a larger number of people made it a less-than-ideal trade for having the functionality be more accessible to a smaller number of people, so my suggestion would be to leave things as they are now.

Since those other two cases have their own bugs, I think it'd be reasonable to mark this one as RESOLVED.

Comment 65

[rsx11m]

Closing per comment #64, given that no more remarks came in and everything is accounted for. Open follow-up bugs to bug 550487 are:

> Bug 560698: resizing message window doesn't adjust number of recipients on To/CC appropriately
> Bug 576611: Allow address headers to efficiently expand if no "more" is desired

->FIXED