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

RESOLVED FIXED

Status

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Usul, Assigned: squib)

Tracking

({regression})

x86
Mac OS X
regression
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-thunderbird3.1 rc1+, thunderbird3.1 rc1-fixed)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

8 years ago
Created attachment 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.

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

Comment 1

8 years ago
In the threepane window.
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.
blocking-thunderbird3.1: --- → rc1+
Jim, any chance this is a regression from one of your recent changes?
(Assignee)

Comment 4

8 years ago
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.
 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?
(Assignee)

Comment 6

8 years ago
(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).
(Reporter)

Comment 7

8 years ago
I meant second email as being phil-lacy's.
(Assignee)

Comment 8

8 years ago
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.
(Assignee)

Comment 9

8 years ago
Created attachment 443976 [details] [diff] [review]
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.
Assignee: nobody → squibblyflabbetydoo
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.
(Assignee)

Comment 11

8 years ago
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
(Assignee)

Comment 12

8 years ago
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.
(Assignee)

Comment 13

8 years ago
Created attachment 444178 [details] [diff] [review]
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.
Attachment #443976 - Attachment is obsolete: true
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.
s/interface/interference/
Keywords: regressionwindow-wanted
(Assignee)

Comment 16

8 years ago
Created attachment 444447 [details] [diff] [review]
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.
Attachment #444178 - Attachment is obsolete: true
Attachment #444447 - Flags: review?(dmose)

Updated

8 years ago
Whiteboard: [has patch; needs review 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.
Attachment #444447 - Flags: review?(dmose) → review+

Updated

8 years ago
Whiteboard: [has patch; needs review dmose] → [has reviewed patch; needs update dmose or jim, landing]
(Assignee)

Comment 18

8 years ago
Created attachment 445424 [details] [diff] [review]
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.
Attachment #444447 - Attachment is obsolete: true
Created attachment 445478 [details] [diff] [review]
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.
Attachment #445424 - Attachment is obsolete: true
Attachment #445478 - Flags: review+

Updated

8 years ago
Whiteboard: [has reviewed patch; needs update dmose or jim, landing] → [has reviewed patch; needs checkin dmose]

Updated

8 years ago
Attachment #445478 - Attachment description: patch, v.next → patch, v.next (checked in on branch)
Created attachment 445484 [details] [diff] [review]
patch v.next, trunk version
Attachment #445484 - Flags: review+
Landed:

http://hg.mozilla.org/comm-central/rev/95cbf01b138b
http://hg.mozilla.org/releases/comm-1.9.2/rev/91b530f8d342
Status: NEW → RESOLVED
Last Resolved: 8 years ago
status-thunderbird3.1: --- → rc1-fixed
Resolution: --- → FIXED
Whiteboard: [has reviewed patch; needs checkin dmose]

Updated

8 years ago
Duplicate of this bug: 565641

Updated

8 years ago
Depends on: 456596
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.
Flags: in-testsuite?

Updated

8 years ago
Attachment #445484 - Attachment is obsolete: true
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!

Updated

8 years ago
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.