Closed Bug 567062 Opened 14 years ago Closed 14 years ago

Wrong number in "x more" recipients button for singleline case

Categories

(Thunderbird :: Message Reader UI, defect)

defect
Not set
normal

Tracking

(thunderbird3.1 .3-fixed)

VERIFIED FIXED
Thunderbird 3.3a1
Tracking Status
thunderbird3.1 --- .3-fixed

People

(Reporter: allblue, Assigned: rsx11m.pub)

References

Details

Attachments

(1 file, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; de; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3 ( .NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; de; rv:1.9.2.5pre) Gecko/20100430 Thunderbird/3.1b2

TB 3.1beta2 now shows more recipients than TB 3.0.x. Depending on window size there is a button "x more" in the header, which tells how many recipients cannot be displayed. The number is too less

Reproducible: Always

Steps to Reproduce:
1. Compose/Send an mail e.g. 16 recipients
2. Look at the mail in the Drafts or Sent folder
Actual Results:  
Depending on windows size you see some recipients listed in the header part of the mail, e.g. 4 recipients. At the right you'll find a button to show also the other recipients, labelled with "x more". The number is always to less, e.g. ("11 more").

Expected Results:  
The correct number
This should be a side effect of bug 565209. The width used by the individual items in the header list is underestimated by the current algorithm, thus it is assumed that more addresses are shown than actually present, consequently the number shown with the "more" too small.

I'd expect this to resolve once the patch there is checked in.
Thx for working on it, hope the patch will get into RC1
It obviously didn't make it into 3.1 RC1, but I'm hoping to get it in for RC2. The issue described here is independent from what bug 565209 was originally opened for, so your bug is technically not a duplicate (hence confirming). Having run RC1 with the patch there for a while, it resolves this bug as well. Once a patched nightly build is available, I'll ask you to test it with your setup to confirm that it's fixed, then we can close it.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Thx, I will be waiting.
Depends on: 565209
Hardware: x86 → All
Whiteboard: [likely fixed by bug 565209]
Version: unspecified → Trunk
This didn't make it for RC2, and in fact needs a few days of "baking" on trunk first before it will be considered for the 3.1 branch. Thus, you can either wait until a regular 3.1 nightly for testing, or try a trunk tinderbox build which has the bug 565209 fix in it already. It works fine for me, but better make a backup of your 3.1 profile before trying a 3.2a1pre build.

The link for download is http://ftp.mozilla.org/pub/mozilla.org/thunderbird/tinderbox-builds/comm-central-trunk-win32/1276071456/ (for Windows).

http://kb.mozillazine.org/Testing_pre-release_versions
Thanks. Unfortunately I can't see any success. Same behavior as before.

In this screenshot you can see a mail with 17 recipients, but the number "x more" is wrong (compare source code):
http://www.imagebanana.com/img/r4nbtxaa/Zwischenablage1.png

On more detail: Opening the mail at first time, there is written "11 more". When I change the folder and com back, there is written "12 more".
Thanks for the screen shot. I think I see two issues:

 - There is an encoding error between "Tobi Example 16" and "Tobi Example 17"
   where the comma comes after #17 rather than #16, and the same happens again
   between #10 and #11, thus you'll have to swap the commas here to get the
   correct count. Here, you would have n=15 now, two pairs count once only.

 - The second issue is more subtle; the "Tobi Example 03" address appears to
   be the last one in the line, then comes the comma. Based on the original
   implementation of bug 550487, the "Tobi Example 04" address may be just
   hiding underneath the "more" text. So, this is a borderline case where
   bug 565209 comment #26 actually would have made a difference.

I'm wondering a bit why you get "11 more" (i.e., the 4th address is counted and sliding underneath the "more" button) one time and "12 more" (i.e., the third address is the sliding one and the fourth is now counted as a hidden address) when coming back. There are apparently a few pixels difference between building the list from scratch and retrieving it from the cached addresses, but I don't see how this could be happening in the code (the commaNodeWidth is counted in either case and should be a constant).

If you vary the window width a little bit to get out of this borderline case, do you get consistent display and "more" counts then?
I apologize about this mistake. In order to make the screenshot anonymous, I replaced subject, body and the original addresses with the "example.com" ones. I made a Copy&Paste mistake, so the commas were missing.

In TB 3.1beta2 and rc1 I dealed with the original mail (with all commas needed) and the problem was existing there.

In fact, now I added the missing commas, and with this new version (incl. your patch) it is working as it should do. As long as the last shown address is displayed fractionally, this address is not counted in "x more":
http://www.imagebanana.com/img/g10ys4ml/Zwischenablage1.png

Thank you again and sorry for inconvenience.
No problem, that's why it was a good idea to show the testcase you've used! :-)

>  - The second issue is more subtle; the "Tobi Example 03" address appears to
>    be the last one in the line, then comes the comma. Based on the original
>    implementation of bug 550487, the "Tobi Example 04" address may be just
>    hiding underneath the "more" text. So, this is a borderline case where
>    bug 565209 comment #26 actually would have made a difference.

I could reproduce this effect though; varying the right border of the 3-pane window in a borderline case where (i) addresses are fully visible and the (i+1)th address can just occupy a tiny space counts as (i+1) addresses shown even if the first pixel only is actually visible (in the single-line case).

Bryan, how big of a deal do you think this is? I could require a minimum
number of pixels of the last address being showable in order to consider it displayed and counting towards the "more" number, but it appears to me that this is a fairly negligible borderline issue of the overall approach.
Attached patch Follow-up to bug 565209 (obsolete) — Splinter Review
This is an incremental patch to attachment 447859 [details] [diff] [review] introducing a
threshold to the last address in single-line mode. The following cases are distinguished:

- if at least 30px remain  visible of the last address, it will slide under
  the "more" button and not counted as hidden;

- if less than 30px would be visible of the last address, it is hidden and
  counted in the "more" button.

The UX motivation for not showing the address <30px at all is to provide an unambiguous feedback to the user whether or not the address is included in the "more" button count (i.e., even if just partially visible, it is not). This also reduces code complexity. The 30px value is arbitrary but looks ok. Basing it on percentage width of the last address would cause variable appearance.

This works fine and introduces only a bit of additional code. If you think it's not worth it, just minus the patch and I'll close the bug given that everything else is resolved; if you like that modification, I'll ask Blake to review the patch with target 3.1.x.
Attachment #450362 - Flags: ui-review?(clarkbw)
Steps for testing the patch, mailnews.headers.show_n_lines_before_more=1:

1. Find an e-mail with more recipients than fit into a single header line;
2. drag the right window border so that less than 30 pixel between a comma
   and the "more" button remain (below-threshold case);
3. go to a different message and come back (due to bug 560698);
4. now you should see a space between the comma and the "x more";
5. drag the right border 30 pixels to the right (>threshold);
6. go to a different message and come back again;
7. now you should see part of the address and "x-1 more".
Attachment #450362 - Flags: ui-review?(clarkbw) → ui-review?(nisses.mail)
Comment on attachment 450362 [details] [diff] [review]
Follow-up to bug 565209

Bryan wanted me to take over this ui-review
Thanks, any ETA for the review? It seems to be the only one in your queue.
Comment on attachment 450362 [details] [diff] [review]
Follow-up to bug 565209

Sorry for the delay, I had problems reproducing the bug properly up until now.

Even though we could probably do even better here (it still lies to me if I resize the window without switching between messages, but that can be another bug). Anyway, this seems to work better than previously, and I can't spot any drawbacks so ui-r+ from me.
Attachment #450362 - Flags: ui-review?(nisses.mail) → ui-review+
Assignee: nobody → rsx11m.pub
Status: NEW → ASSIGNED
Summary: TB 3.1beta2: Wrong number in "x more" recipients button → Wrong number in "x more" recipients button for singleline case
Whiteboard: [likely fixed by bug 565209]
Comment on attachment 450362 [details] [diff] [review]
Follow-up to bug 565209

No problem. Resizing the window is bug 560698; I've noticed that too, but it behaves fine as long as you keep the window size intact.

Blake, I don't think there is any way to test this automatically as it depends on too many variables (including addresses, fonts, and window sizes), but the list in comment #11 should serve for a litmus test.
Attachment #450362 - Flags: review?(bwinton)
Blocks: 550487
Attachment #450362 - Flags: review?(bwinton) → review+
Comment on attachment 450362 [details] [diff] [review]
Follow-up to bug 565209

>+++ b/mail/base/content/mailWidgets.xml	Thu Jun 10 10:42:21 2010 -0500
>@@ -380,27 +380,31 @@
>                 // hide the last node spanning into the additional line (n>1)
>-                if (curLine >= this.maxLinesBeforeMore && this.maxLinesBeforeMore > 1) {
>+                // also hide it if <30px left after sliding the address (n=1)
>+                if (curLine >= this.maxLinesBeforeMore &&
>+                    (this.maxLinesBeforeMore > 1 || newLineWidth - overLineWidth < 30)) {

I think you could break this after the || and keep it under 80 characters.

Apart from that, I think I like it.

Later,
Blake.
Line-wrapping/white-space changes only; ui-r=andreasn, r=bwinton.
Attachment #450362 - Attachment is obsolete: true
Attachment #452826 - Flags: ui-review+
Attachment #452826 - Flags: review+
Thanks for the reviews, push on comm-central please. I'll request branch approval once this landed on trunk without any problems.
Keywords: checkin-needed
Whiteboard: [c-n: comm-central]
Checked in: http://hg.mozilla.org/comm-central/rev/83468aaa8c87
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [c-n: comm-central]
Target Milestone: --- → Thunderbird 3.2a1
Whiteboard: [needs bug 565209 landing on comm-1.9.2]
Comment on attachment 452826 [details] [diff] [review]
Patch for checkin

This works fine on today's trunk nightly, no test is throwing any errors.

Nominating for 3.1.1, it's a minor follow-up to bug 550487 and bug 565209 with minimum risk. Note that attachment 447859 [details] [diff] [review] needs to land first.
Attachment #452826 - Flags: approval-thunderbird3.1.1?
Comment on attachment 452826 [details] [diff] [review]
Patch for checkin

As bug 565209 has been delayed to 3.1.2, I'm delaying this one as well.
Attachment #452826 - Flags: approval-thunderbird3.1.1? → approval-thunderbird3.1.2?
Comment on attachment 452826 [details] [diff] [review]
Patch for checkin

a=Standard8 for 3.1.3. Please land on comm-1.9.2 default only.
Attachment #452826 - Flags: approval-thunderbird3.1.2? → approval-thunderbird3.1.3+
Keywords: checkin-needed
Whiteboard: [needs bug 565209 landing on comm-1.9.2] → [cn to c192 default after/at same time as bug 565209]
Checked in to 1.9.2: http://hg.mozilla.org/releases/comm-1.9.2/rev/dfa775337b79
Keywords: checkin-needed
Whiteboard: [cn to c192 default after/at same time as bug 565209]
Flags: in-litmus?
Whiteboard: [see comment #11 for litmus-test steps]
Thanks Ludo, maybe add a remark that this applies for 3.1.3 and later only.
The "3.0 branch" category is potentially confusing (no "3.1 branch" there?).
Whiteboard: [see comment #11 for litmus-test steps]
(In reply to comment #25)
> Thanks Ludo, maybe add a remark that this applies for 3.1.3 and later only.
> The "3.0 branch" category is potentially confusing (no "3.1 branch" there?).

Well formal testing the big next round will be for 3.2.next, so I'm not worried about that.

Good question , I don't know if it's worth the effort yet to clone and create a new "branch" for test results.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.9) Gecko/20100825 Thunderbird/3.1.3
Status: RESOLVED → VERIFIED
Blocks: 601206
You need to log in before you can comment on or make changes to this bug.