563612 - Star in header doesn't update status when contact is added or removed from

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[Ludovic Hirlimann [:Usul]]

Attachment: email with which I can recreate the issue.

On the attached test case, if you try to add the second email address listed, it will get added but the star won't change (it will stay empty), reloading the message will  show the star full. Same applies when you remove the contact. The first address works properly.

I think it's a recent regression - (less than 2 week), Gary can you hunt it ?

Comment 1

[Ludovic Hirlimann [:Usul]]

In the threepane window.

Comment 2

[Mark Banner (:standard8)]

At the moment, this blocks - the star doesn't get updated and the address can get added to the address book multiple times.

I see it with the test email in a standalone message window.

I've not reproduced it on all email combinations, but I have reproduced it on some.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Jim, any chance this is a regression from one of your recent changes?

Comment 4

[Jim Porter (:squib)]

I don't think I've had any recent check-ins. I can confirm this on the latest Windows nightly, though.

Perhaps more interesting: I tried this with TB 3.0.4 and I get a similar, but slightly different error. Adding the various people in the To: header works, but adding Ludovic (who's CC'ed) exhibits this same problem, provided you haven't clicked "more" on the To: list.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

 Created an attachment (id=443315) [details]
> email with which I can recreate the issue.
> 
> On the attached test case, if you try to add the second email address listed,
> it will get added but the star won't change (it will stay empty), reloading the
> message will  show the star full. Same applies when you remove the contact. The
> first address works properly.

If by "second email address", you mean Phil Lacy, I can't reproduce that.

If, however, by "second email address" you mean yourself, I can reproduce that.  In that case, I see this in the error console:

Error: hideEmailAddressPopup is not defined
Source File: chrome://messenger/content/messageWindow.xul
Line: 1

which suggests that this is actually a DUP of bug 563615.

> I think it's a recent regression - (less than 2 week), Gary can you hunt it ?

Which is interesting, since Jim's comment in bug 563615 suggests that this was at least theoretically introduced in 2009.  Perhaps the bug was introduced then, but something else hid it until now?

Comment 6

[Jim Porter (:squib)]

(In reply to comment #5)
>  Created an attachment (id=443315) [details] [details]
> > email with which I can recreate the issue.
> > 
> > On the attached test case, if you try to add the second email address listed,
> > it will get added but the star won't change (it will stay empty), reloading the
> > message will  show the star full. Same applies when you remove the contact. The
> > first address works properly.
> 
> If by "second email address", you mean Phil Lacy, I can't reproduce that.
 
I only saw this when I loaded it up in the three-pane.

> If, however, by "second email address" you mean yourself, I can reproduce that.
>  In that case, I see this in the error console:
> 
> Error: hideEmailAddressPopup is not defined
> Source File: chrome://messenger/content/messageWindow.xul
> Line: 1
> 
> which suggests that this is actually a DUP of bug 563615.
> 
> > I think it's a recent regression - (less than 2 week), Gary can you hunt it ?
> 
> Which is interesting, since Jim's comment in bug 563615 suggests that this was
> at least theoretically introduced in 2009.  Perhaps the bug was introduced
> then, but something else hid it until now?

I'm pretty sure this is a different error. Bug 563615 *should* only be causing an issue where, once you mouse over an address in the standalone window, that address never gets un-highlighted (until you close the window).

Comment 7

[Ludovic Hirlimann [:Usul]]

I meant second email as being phil-lacy's.

Comment 8

[Jim Porter (:squib)]

I'm looking at this now, and it appears that the DOM contains several instances of the email-address node for Phil Lacy (ditto for Nikolay). The first of Phil's nodes is getting updated properly, but that one is hidden, so it has no practical effect.

There also appears to be a problem with Nikolay's node since it doesn't have a cardDetails attribute.

Comment 9

[Jim Porter (:squib)]

Attachment: Minimalist patch

This patch is pretty minimalist; it fixes the problem (as far as I can tell) in the easiest way I could manage. There might be some performance regressions but I think "right and slow" is better than "wrong and fast". :)

Basically here's what got fixed:
* Counting the number of cached address nodes in the header field didn't account for trailing commas (present when "more" is there).
* Only one cached address node was used. The others were hidden and then more were appended at the end, causing chaos.

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

Ooooh, good catch, Jim!  I bet this is actually my regression.  We do already have some performance problems in that code that block 3.1, so I suspect this needs to be combined with the work I'll be doing in bug 560695.

Comment 11

[Jim Porter (:squib)]

The "right" way to fix this is probably to change the logic from:

    for each cached node:
        update node
    for each remaining address:
        create new node

to

    for each address:
        if cached node exists:
            update node
        else:
            create new node

Comment 12

[Jim Porter (:squib)]

I'm looking at implementing the method I described in comment 11, and one thing that's especially bothersome with the current code is that sometimes there's a trailing comma node and sometimes there isn't. I see two ways around this:

1) make the comma part of the "more" indicator
2) always add a trailing comma, but hide it when there is no "more"

I'm going to go with #2 for now.

Comment 13

[Jim Porter (:squib)]

Attachment: Better patch

Since it sounds like this is going to get incorporated into a larger patch (bug 560695), I'm going to try and avoid writing tests of my own for this, but this lightly-tested version appears to work and is pretty close to what the original meant to do, I think. It also has a net reduction in code, which is usually a good thing.

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

The main reason it was looking like it would make sense to merge those two patches together was because I was fearing having to make non-trivial surgery on the performance patch that have weird interactions with this patch.

Sorry to disappoint, but the performance fix turned out to be relatively straightforward, and it is compatible with the most recent patch here, so I think it probably makes more sense to continue to drive them separately.

Note that after applying this patch, the other one needs to be manually rebased before it will apply (and vice versa), but that's purely syntactic patch-level interface; there's no semantic-level interference between the two patches.

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

s/interface/interference/

Comment 16

[Jim Porter (:squib)]

Attachment: Patch + MozMill tests

Ok, here's a patch with tests. The updated test-message-header.js fails without the patch in mailWidgets.xml and passes with it. dmose, I flagged you for review since you've already looked at this area recently, but no worries if you forward it onto someone else.

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 444447 [details] [diff] [review]
Patch + MozMill tests

Nicely done!  I like the state this leaves the code in, and a fine choice to get rid of some old unused code.  r+ conditional on two changes:

* Depending on there already being an address that doesn't have a card in the address book seems fragile, and I'm concerned it could lead to random tinderbox oranges if some other unrelated part of this file changes.  Instead of traversing the list looking for an address without a card, maybe just take the first address and delete any existing card?

* test_more_widget is already a bit too long, so I'd rather see the new code pulled out into another function in the interest of readability rather than just inserted into the existing function.  Maybe subtest_more_widget_star_click() or something.

I'd like to say that I really appreciate that you worked through the fix and test for this bug on such short notice, and since I'm reviewing at the last minute, I'm happy to go ahead and make those changes myself if you don't have time.  I can't do it tonight, but I'll happily do it tomorrow and land before code freeze.

Comment 18

[Jim Porter (:squib)]

Attachment: Refactor test_more_widget

Ok, I pulled the test_more_widget code out into three subtests:

* subtest_more_widget_display
* subtest_more_widget_click
* subtest_more_widget_star_click

I also added a helper function to ensure that no card exists. I'll eventually be pulling that out of this file (bug 474721 wants a function like that for testing too), but I'm just putting it in here for now for the sake of simplicity.

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v.next (checked in on branch)

Looks great; thanks, Jim!  Merged to the tip of the 1.9.2 branch and updated a comment.

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch v.next, trunk version

Comment 21

[Dan Mosedale (:dmosedale, :dmose)]

Landed:

http://hg.mozilla.org/comm-central/rev/95cbf01b138b
http://hg.mozilla.org/releases/comm-1.9.2/rev/91b530f8d342

Comment 23

[Dan Mosedale (:dmosedale, :dmose)]

I made a mistake and only checked the test in on branch, not trunk.  Adding in-testsuite? so this fact doesn't get lost.  I've added it to my calendar to land tomorrow.

Comment 24

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 445484 [details] [diff] [review]
patch v.next, trunk version

Turns out I _actually_ landed the right patch, with tests, on both trunk and branch, but uploaded the wrong thing here which then confused me after the fact.  Good times!