517611 - ninja like algorithm for the header address more expander

Status: RESOLVED WONTFIX

(Thunderbird :: Message Reader UI, defect)

Description

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

Our new more expander is incredibly simpler.  It's time to complicate it! :)

Because contacts in our address book are smaller by at least some amount we should only count them as a 1/2 object in our, is it time for a more, calculation.

Similarly because we use the "You" address for yourself we should only count that for 1/4 or so points as it will never take up much space.

The goal of these ideas are an effort to prevent the more from showing up when it's really not necessary, i.e. at the boundary of necessity.

Comment 1

[Jim Porter (:squib)]

The algorithm chosen for this is probably (partly) dependent on the results of bug 474721, which suggests showing two display names if there's a conflict between the provided name from the mail header and the address book name (e.g. "Alice B. (Alice Bee)").

I suppose the ideal way to calculate the "is it time for a more button" value is to use the content widths of each item, but I'm not sure what the performance implications of that would be.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

You're quite right that the way to do the Right Thing here would be to do our own text measuring.  I suspect we could cope with the performance implications without too much pain.  I'm more worried about the code complexity implications here, given how complex the code already is (this is even more true this late in the Tb3 cycle).

The intent of this bug is to use some basic heuristics to do a pretty good job here with a small amount of work, rather than trying to do the Right Thing.

Comment 3

[Wayne Mery (:wsmwk)]

I'm not 100% clear of the precise aim of this bug:
a) is more being presented when it shouldn't be? or is the aim to correctly calculate space so addresses fit on the line?
b) what's the psuedo code of the present algorithm?
c) will this help with Bug 456596 comment 21 emails with lots of addresses in the To: field should only show first N by default (which I am rather passionate about)

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

a) the idea with this bug was to calculate space as correctly as is reasonably straightforward, given code constraints, and show as many addresses as possible most of the time, plus a (more).
b) if n=1 (n=number of combined To + Cc), show address
else show first address plus more button
c) I _think_ this bug is effectively a DUP of bug 456596 these days.  Bryan, do you agree that that's true? If so, please mark it as such.

Comment 5

[Ben Bucksch (:BenB)]

> You're quite right that the way to do the Right Thing here would be to do our
> own text measuring.

If we manage to go back to limiting the number of lines instead of addresses, we don't need to measure text, Gecko will do it. That would probably depend on bug 543954, though.

If you're dead-set on making it complicated, add addresses until the width (in px) of the line reaches the width of the box/line in the header pane.
But I would recommend against it. We've had enough problems in the header pane, no need for more hacks.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Heh, the only reason I came to that conclusion was because after significant debugging, test-case simplification, and work with Neil R, he convinced me that the XUL box model in fact cannot and will not do that calculation on our behalf.  I don't recall all the details.  I was, however, told by someone else that some of the Firefox bookmarks code also does text-measuring by hand for exactly this reason.

Comment 7

[Ben Bucksch (:BenB)]

> the XUL box model in fact cannot  do

Right, that's why I said "That would probably depend on bug 543954, though".

Comment 8

[Ben Bucksch (:BenB)]

Please ignore my last comment, I got confused. You're right nevertheless.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

I think we need this for 3.1 as part of softening the landing pad for Tb2 users.

Comment 10

[Thomas D. (:thomas8)]

In line with Dan's comment 9, IMO this bug will be a small yet highly-visible and very painful regression for TB 2 users who switch to TB 3, as you'll see this with virtually every mail you get.

Does "blocking-thunderbird 3.1: needed" mean that it's blocking+ or is "needed" some lesser variant of blocking?

What's the right target milestone for this? -> please adjust

Comment 11

[Thomas D. (:thomas8)]

Exactly this problem seems to be worked on in bug 456596.
-> This bug should be closed as a duplicate of bug 456596, as suggested by Dan's comment 4. I'll set a closeme date for that.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Actually, I'm no longer convinced this is really a DUP, so I'd suggest holding off on that.

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

I believe that Ben agreed to take a run at this this week, so giving it to him.  Please let me know if I misunderstood.

Comment 14

[Ben Bucksch (:BenB)]

Attachment: Fix, v1

This implements the suggested algo.
Includes pref (bug 456596).

However, this doesn't work too well in practice.
1. Letting the user specify the approximate number of lines determined by magic algo (which is what we calculate here) doesn't make much sense to users.
2., and more importantly, the length of email addresses and even of names is widely different in practice. Some of my contacts are called "Dan Veditz" and some "Katheria Stehli-Kommradenbauer". If the pref says 2 lines = 6 names without email addresses, it Dan may fill 1-2 lines, while Katherina fills 6 lines. That's rather random and not understandable anymore and doesn't adhere the user pref at all.

I'll try another patch where I simply check the width or height of the addresses nodes in pixels.

Comment 15

[Ben Bucksch (:BenB)]

I filed bug 550487 about the latter.

Comment 16

[rsx11m]

Reading comment #14, it seems to make more sense IMO to look into bug 550487 and keep bug 456596 as the fallback solution for 3.1; meaning, if the patch here only makes sense if the actual space requirements can be determined, thus being subject to the same XUL/DOM restrictions as bug 550487, and in case this issue is resolved, going directly with a "show n lines" approach should be preferable and easier to understand for the user. I find a "show at least n addresses, but maybe more if we think that those fit" logic rather confusing.

Comment 17

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

Comment on attachment 430229 [details] [diff] [review]
Fix, v1

Just to note, this patch doesn't seem to apply, it fails on the all-thunderbird.js

Comment 18

[Ben Bucksch (:BenB)]

Of course it's bound to rot and some point. Conflicts in all-thunderbird.js are trivial to resolve manually - just copy that one line manually using an editor.

Comment 19

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

Comment on attachment 430229 [details] [diff] [review]
Fix, v1

Agreed that the pref here seems wrong.  Though I'm not sure if it's that the calculation is taking into account both the to and cc lines.

One of my test messages has a single To value with many CC values.  I see the to value and 1 cc value with the more action.  I would have expected that each line would use as much space as possible and not the combination of space for both lines as it seems to be doing.

Lets try this without the pref until we have that part simpler and more understandable.

Comment 20

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

Comment on attachment 430229 [details] [diff] [review]
Fix, v1

sorry

Comment 21

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

Attachment: screenshot of single to line with multiple cc

here's a screenshot to help describe the situation better.  I would assume that cc line would show more addresses than just the one.

Comment 22

[Ben Bucksch (:BenB)]

Comment on attachment 432917 [details]
screenshot of single to line with multiple cc

Bryan,

what you see has nothing to do with the pref. The pref merely says how many lines we try to fill, default 1. If I was to hardcode it to one line, the algo would have the same outcode you see.

+            // Per bug 517611 (We're ninjas, hu-cha!), use a silly algo to
+            // approximate the number of lines the addresses will take.
+            // name + email address counts as 4, only the name counts as 2,
+            // and "You" counts as 1 (in English). We think that with a
+            // normal window width, a counter of 4-6 would usually fit on
+            // one line.

Your screenshot shows that it's each name + email address. Per bug 530239, not 2 of those would fit on line with, so per above algo, 2x 4 (name+email) > 6 (max per line), we show only one of them.

If you think that more fit on your line, just increase the pref to 2 or 3.

If they had been in your address book and only the name shows, it would have shown 3 names, even with the pref default of 1. With pref = 3, we'd show 9 names (without email address).

That said, I agree this is confusing. That's why I dropped this.

Comment 23

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

Attachment: a ninja has no honor and uses setTimeout to strike his opponent down

Here's my new take on the ninja like algorithm.

This does a boxObject.width compare of the children versus the parent.  If the children are too large it sets collapse=false to the more indicator.

There is a setTimeout feature built into this to not only give a nice effect of the more appearing (unintentional) but to also get out of the JS execution and allow the reflow event to occur so we can get real width values.   Without this you won't reliably get a width value which is correct.

Try this out and see how you like it. I didn't add the "# more" like BenB has, even though I really like it I just didn't get to it.  I think for this it would require figuring out which of the children is the overflow (very possible) and then doing that count minus the total.

Comment 24

[rsx11m]

Ben, Dan: Would the timeout hack help for the size issues seen in bug 550487?

Comment 25

[Ben Bucksch (:BenB)]

rsx, the last patch indeed belongs more into bug 550487 than here. I thought this bug was defined by the algo in the initial description.
However, we should try to find a solution without setTimeout() for bug 550487.

Bryan, shall we close this WONTFIX, given that the algo in the initial description doesn't seem to work well?

Comment 26

[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 27

[rsx11m]

It appears that this bug can now definitely be closed after bug 550487 along with its follow-ups is doing a good job solving the issue.

Comment 28

[Ben Bucksch (:BenB)]

Agreed, this was a workaround and we have a proper solution now. As owner, closing WONTFIX.