Created attachment 563347 [details] Address Book Junk Folders Bug Tbird 7.0.0 Win 7 x64.png User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0) Gecko/20100101 Firefox/7.0 Build ID: 20110922153450 Steps to reproduce: Find multiple alternate entries of "personal address book" and "collected addresses" in junk mail property on profile with two email accounts. Originally 12 entries. Actual results: Unchecking all boxes reverts to normal display. Rechecking them gives number of entries for the accounts in that profile. ie if I create two dummy email accounts I will see 8 entries in the property sheet. This increased in sequence for each email account setting you view. ie, look at the first account there are the normal two entries, personal and collected, then second one you see four, third one six etc.. Expected results: Only personal and collected for the relevant email should be visible on its property sheet available for junk mail filtering
Created attachment 563350 [details] Junk Property Sheet duplicate address book entries
Ok, I see it too, on Linux.
It is needed to open Account settings->Junk Settings and then directly jumping to Junk settings of another account. The Addressbooks in the list will accumulate. Jumping to another tab of the preferences dialog (e.g. Security) will reset the list to normal (all addressbooks only once). Repeating the process will accumulate entries again. I'll look into the source, this could be trivial, just the array populating the list not being reset and ABs reenumerated when jumping to the tab with the list.
This is my first time diving into a Mozilla project's code, but it looks like this could be easily fixed in mailnews/base/prefs/content/am-junk.js From what I can tell, the Junk Settings page is initialized by building a list of all the applicable address books (line 95), then appending them as children of the listbox (line 124). The reference to the listbox itself is obtained on line 91: var wList = document.getElementById("whiteListAbURI"); For whatever reason, this element already has child nodes when switching directly between Junk Settings pages, but not when switching from other pages to Junk Settings. Iterating over wList and removing all children before populating it seems to resolve this issue for me: while (wList.firstChild) wList.removeChild(wList.firstChild); I'll attach a naive patch to this effect, but I'm not sure if it's an appropriate fix. After all, it seems like the children shouldn't be there at all when onInit() is invoked.
Created attachment 570523 [details] [diff] [review] Naive fix to clear out preexisting entries before populating list
Hm, I think I had a comment here, that was somehow lost... Anyway, I came to the same conclusion, that that element is not properly cleared. Please request a review of the patch by setting the necessary flags on the attachment. Try :bienvenu as a reviewer.
Comment on attachment 570523 [details] [diff] [review] Naive fix to clear out preexisting entries before populating list Thanks for the patch, I'll take a look at this in a bit.
Comment on attachment 570523 [details] [diff] [review] Naive fix to clear out preexisting entries before populating list Many thanks for the patch Dan, this is spot-on. Just a couple of minor review comments: >+ // Ensure the whitelist is empty (bug 690277) Normally we don't reference bug numbers in code, as if we need to get back to a bug for the reason why, we follow the blame via our repository history. >+ while (wList.firstChild) >+ wList.removeChild(wList.firstChild); In this situation we usually use .lastChild as it is more efficient in terms of less work in needing less updates of the list. Also, we generally use two-space indentation rather than four, although generally keeping it to the style of the code you are in is good. So this is r+ from me. Normally at this stage, I'd expect you to update the patch before we land it, and then add checkin-needed to the keywords which will flag someone to land it. However, I want to get it into the final 8.0 beta which we're just about to start building, so I'll do all of that for you.
Created attachment 570732 [details] [diff] [review] Updated patch Here's the updated patch. I'm also granting approval for our branches, as that's what is needed to get it landed for 9 and 8, as trunk is developing for TB 10 at the moment.
Checked in: http://hg.mozilla.org/comm-central/rev/2edbbb6c5f59 http://hg.mozilla.org/releases/comm-aurora/rev/17b5b7a4fee0 http://hg.mozilla.org/releases/comm-beta/rev/42a8fe4e9215 Thanks again Dan :-)
Noobie-Question: How do I apply the patch? Another question: Could this be somehow linked to bug 536768? (Sorry, don't know how to link here... again, noobie and everything...)
You would have to compile whole Thunderbird from source, which is quite hard (or maybe edit the appropriate .js file inside omni.jar). Otherwise, you can wait for Thunderbird 9, which should come out 20th Dec 2011.
You can pick up the beta of version 9 at http://www.mozilla.org/en-US/thunderbird/channel/