This is another minor follow-up to bug 550487 and related to bug 560695 on performance when opening the full address list. While in general the response on bug 550487 and bug 565209 on the new preference is positive, performance in rendering the addresses is decreasing with large numbers n of lines to be displayed. The case in question had n=30 with usually large address lists, seeking for a fast equivalent of opening all addresses by default. The solution would be to restore parity to the behavior that the old TB 1.5 preference had, considering a value of 0 to equal "all" without regard of the length of the address list. While it's a borderline case, the solution is simple enough to not increase code complexity. This would also resolve the currently undefined behavior for n<1, showing a single line but omitting the "more" button even if it was needed.

After the landing of bug 536542, this is a rather trivial extension of the if() clause also checking for n<1, which in this case is treated the same as View > Headers > All, but only for the e-mail headings. The implementation in bug 560695 handles this efficiently, thus minimizing any delays. As a drive-by fix for bug 536542, this patch changes the default for a missing mail.show_headers preference to NormalHeaders rather than AllHeaders, per the mailnews.js default specification.

I would prefer 'all' to be the default with some easy way to change it which is obvious to non-technical users - perhaps an entry in Preferences. Better still if there is a short warning indicating the speed consequences of high, non-all, n. However, the speedy introduction of 'all' is far too important for some users to have it delayed by my above preference.

Neville, based on the discussions in the other bugs I've referred to in my description, the 1-line header is considered the default use case and not likely to change. However, for those users who want to see 2 or 3 lines, or all addresses like yourself, the hidden preference provides that option.

Thanks for the information. All users I have talked to are so unhappy with the default short header that, in my opinion, it is harming the reputation of TB. I suspect we have arrived here because of the known speed consequences of high n. However it has now been established that 'all' is very fast. Perhaps it is not too late to review this.

Yes, using n=0 as special case for "all" has the advantage that there is no need to calculate explicit widths for each address with this patch. With n=99, one could achieve the same display result, but at a much higher computational cost due to the unnecessary overhead of width calculations and line counting.

Comment on attachment 455758 [details] [diff] [review] Simple patch Seems reasonable.

Comment on attachment 455758 [details] [diff] [review] Simple patch Thanks Bryan, asking Blake for the technical review as the submodule owner.

This is attachment 455758 [details] [diff] [review] + mozmill test for the n=0 case. The test follows the examples established by bug 550487 and bug 536542, testing the n=0 case directly. Since it is established from the previous test_more_widget_with_maxlines_of_3() that n=3 is fulfilled, n>3 is asserted for mailnews.headers.show_n_lines_before_more = 0. As another minor follow-up on bug 536542, I have declared moreNode in subtest_change_to_all_header_mode() properly to ensure its correct scope.

Sorry, v2 was the wrong patch...

> (comment #8)... minor follow-up on bug 536542, I have declared moreNode in > subtest_change_to_all_header_mode() properly to ensure its correct scope. Note that test_show_all_header_mode() currently fails due to bug 579305, with or without the correct scope. This is unrelated to this patch.

Am I correct in thinking that 579305 relates to a quirk of the test procedure. If so can this patch proceed without the quirk being fixed?

It's an independent issue, so I don't see any impact on the patch here. As said, I saw the missing declaration and added it as a drive-by fix.

Comment on attachment 457223 [details] [diff] [review] Patch (v3) with updated test [comment #23] I think I like this patch, but I would like to get some feedback on it from dmose before we check it in, since he's done more work in this area than I have. Thanks, Blake.

Dan, any concerns with this patch from your end or can this be checked in? (and thanks to Blake for the review.)

Sorry for not getting to this sooner; at this point, I'm not expecting to have time to look at it until Friday.

I have just had an email with 442 addresses in the 'To' field. Even with the normally fast 'always show all' method (http://forums.mozillazine.org/viewtopic.php?p=9570923#p9570923) my TB 3.1.1 on OS X took a long time to 'open' the email. Unfortunately it did not open so that I could read it - all I got was a screen full of header with no vertical scroll bar. This was unexpected - I had expected TB to insert a scroll bar in these circumstances. I saved as file and removed most of the To field to opened it - I have not investigated if I had many more options. I only expect to get about two emails like this a year so am not too concerned but I thought others should consider this. I intend to always 'show all' and I hope this slight negative point does not slow down this worthy bug.

Neville, you cannot have it both ways. What you see is to be expected and a trade-off to be made by those who select this option. The scrollbar doesn't work along with wrapping, that's bug 525225 and bug 492645. You can limit the height of the pane with a userChrome.css entry as I explained in the forum.

Thanks It is disappointing to discover that the "XUL layout and HTML layout interaction is fundamentally broken".

Comment on attachment 457223 [details] [diff] [review] Patch (v3) with updated test [comment #23] Looks reasonable to me; thanks for the patch!

This is the corresponding branch patch to complete the series of follow-up patches handling the mailnews.headers.show_n_lines_before_more preference. It is slightly different in the main part as bug 536542 hasn't been checked in for the 1.9.2 branch, the added test is identical to the trunk version. Unless dmose or standard8 state that this fix is unlikely to be accepted for comm-1.9.2, I'll flag this for review once the trunk patch has landed and no issues showed up.

Does n have to be 0 for the 'all' option or <1 - what happens for 0.5, -1 etc?

No, the comparison is for n<1 on purpose to ensure that any given value has a defined meaning (which wasn't the case before this patch). Also, the Config Editor won't allow you to enter fractions for an integer.

Comment on attachment 457223 [details] [diff] [review] Patch (v3) with updated test [comment #23] Checked in by standard8, http://hg.mozilla.org/comm-central/rev/3c6e74437e01

Mark, thanks for finding a slot between trunk bustages to get this in. I assume that you forgot to clear the checkin-needed, thus I'm removing it. The new test run fine a few times on Linux and Windows, Mac OSX didn't run it before bug 586620 closed the tree again. I'll leave this bug open as the branch patch still needs to be reviewed.

Were there significant changes needed in the 1.9.2 patch? In any case, you might want to actually request review on that...

Comment on attachment 463924 [details] [diff] [review] Patch (v3) for comm-1.9.2 Changes for comm-1.9.2 vs. comm-central are fairly minimal, just the two lines in the main part of the patch; bug 536542 modified the handling of the second argument in fillAddressesNode and the removeAttribute("singleline") condition.

(In reply to comment #24) > The new test run fine a few times on Linux and Windows, Mac OSX didn't run it > before bug 586620 closed the tree again. Tests look good on all platforms after a couple of iterations.

Comment on attachment 463924 [details] [diff] [review] Patch (v3) for comm-1.9.2 Okay, I'm convinced this will work. Later, Blake.

Comment on attachment 463924 [details] [diff] [review] Patch (v3) for comm-1.9.2 Nominating for branch approval. This is a low-risk patch to (1) ensure that all values of mailnews.headers.show_n_lines_before_more have a defined behavior; and (2) to avoid that users have to set n to a high value as a workaround to see all addresses when opening a message, thus may run into an unresponsive script situation with the current implementation. Tests are running fine on trunk.

Checked into 1.9.2: http://hg.mozilla.org/releases/comm-1.9.2/rev/0dd73f9f9e69

Will this impact the use of or be impacted by the use of the Compact Header extension?

I don't think so but didn't verify. Nevertheless, it would be the extension's author's responsibility to update his add-on to work with a new version.

I build Thunderbird from the comm-1.9.2 branch and tried CompactHeader. I didn't see any problems.