525718 - From header misaligned when adding the first tag/removing the last tag

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[Simon Bünzli]

This should be easily fixed by calling syncGridColumnWidths from OnTagsChange.

Comment 1

[Simon Bünzli]

Attachment: screenshot

Comment 2

[Simon Bünzli]

BTW: This bug only affects locales where the word for "tag" is longer than any of the other displayed header bits (such as with the German build I've tested).

Comment 3

[Ludovic Hirlimann [:Usul]]

(In reply to comment #0)
> This should be easily fixed by calling syncGridColumnWidths from OnTagsChange.

Could you provide  a patch ?

Comment 4

[Simon Bünzli]

Attachment: fix per comment #0

Haven't got a full tree, but this should do it. Who'd be an appropriate reviewer?

Comment 5

[Ludovic Hirlimann [:Usul]]

(In reply to comment #4)
> Created an attachment (id=409659) [details]
> fix per comment #0
> 
> Haven't got a full tree, but this should do it. Who'd be an appropriate
> reviewer?

dmose or philor

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

I'm on the fence about whether this should block; I'd like clarkbw to make that call.

Somewhat but not entirely separately, I'd like to start requiring mozmill tests for all message header changes, so in some ideal world, I'd r- this patch because it doesn't have one.  bwinton, if you get done with the reply / reply all changes, would you be up for picking this up and getting some testage up and going?

If not, I'll be back with full attention on Thursday, so I could potentially pick this up then.

Comment 7

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

I think it would be good to get this in though I wouldn't block the release on it because I don't think it's bad enough that blocking is necessary.

However the code change here is simple enough and the called function is also pretty simple and idempotent IIRC so I would hope we could get this in anyway.  Perhaps blocking P3 is a better status for this, I'll let blake decide since I assume it will be up to him to get in or not.

Unit tests are up to blake as well but it feels like at this point in the release it would be really hard to require that.  I would think the only possible unit test would be that we check that the from label column is always the same width as the other grid label column when any header functions are called.

Comment 8

[David Ascher (:davida)]

Comment on attachment 409659 [details] [diff] [review]
fix per comment #0

I think dmose is effectively assigning review to bwinton =)

Comment 9

[Blake Winton (:bwinton) (:☕️)]

Attachment: The previous patch, with some tests.

I like the change itself.  It seems simple, and I would r+ it, if someone added tests.  So I added tests.  :)

I would kind of like to slide this in to TB3, if only because it gives us a place to hang more message header tests off of, if we need to change it further.

(As for tests, I tested that the headers were the same length, then changed the length of the tags label, added a tag, and checked that they were still the same length, and that they were a different length than before.)

Comment 10

[Phil Ringnalda (:philor)]

Comment on attachment 410298 [details] [diff] [review]
The previous patch, with some tests.

>+/*
>+ * Test that we can open and close a standalone message display window from the
>+ *  folder pane.
>+ */

ORLY?

>+  // create three messages in the folder to display
>+  let msg1 = create_thread(1);
>+  let msg2 = create_thread(1);
>+  let thread1 = create_thread(2);
>+  let thread2 = create_thread(2);
>+  add_sets_to_folders([folder], [msg1, msg2, thread1, thread2]);

I'm confused enough by create_message() being create_thread(1), without creating six messages, or four threads, and calling them three messages.

Also, why? Not that I really understand our tests like I should, but it sure seems like you're selecting the first message, opening the first message, adding a tag to the first message, and only creating the second one and the two threads for fun.

Comment 11

[Blake Winton (:bwinton) (:☕️)]

Attachment: The previous patch, with my actual code.

(In reply to comment #10)
> (From update of attachment 410298 [details] [diff] [review])
> >+ * Test that we can open and close a standalone message display window from
> >+ * the folder pane.
> ORLY?

No, not really.  Fixed.

> >+  // create three messages in the folder to display
> I'm confused enough by create_message() being create_thread(1), without
> creating six messages, or four threads, and calling them three messages.

Fixed.

> Also, why? Not that I really understand our tests like I should, but it sure
> seems like you're selecting the first message, opening the first message,
> adding a tag to the first message, and only creating the second one and the
> two threads for fun.

It's the copy-paste design pattern at work.  Fixed.

Also, I had apparently not checked in the actual code I was using, so here is a test case that even passes.

Sorry about that,
Blake.

Comment 12

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 410309 [details] [diff] [review]
The previous patch, with my actual code.

So, we've got a really small patch, and we've got tests for it, and the extra call is idempotent, and does fix a layout bug.

So could we add this fix to TB-3.0?

Thanks,
Blake.

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 410309 [details] [diff] [review]
The previous patch, with my actual code.

Tests as well, excellent.

Comment 14

[Blake Winton (:bwinton) (:☕️)]

Checked in, as:
http://hg.mozilla.org/releases/comm-1.9.1/rev/195a69f1cd25
http://hg.mozilla.org/comm-central/rev/c6624ad72ba7

Thanks,
Blake.