Closed Bug 690277 Opened 11 years ago Closed 11 years ago
Multiple (duplicate) Address Book Entries / checkboxes in "do not mark mail as junk if the sender is in" (Account settings -> Junk settings)
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
Ok, I see it too, on Linux.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86_64 → x86
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.
Summary: Multiple Address Book Entry In Junk Mail Property → Multiple (duplicate) Address Book Entries In "do not mark mail as junk if the sender is in" (Account settings -> Junk settings)
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.
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.
Attachment #570523 - Flags: review?(mbanner)
Assignee: nobody → dan.callahan
Hardware: x86 → All
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.
Attachment #570523 - Flags: review?(mbanner) → review+
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.
Summary: Multiple (duplicate) Address Book Entries In "do not mark mail as junk if the sender is in" (Account settings -> Junk settings) → Multiple (duplicate) Address Book Entries / checkboxes in "do not mark mail as junk if the sender is in" (Account settings -> Junk settings)
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/
You need to log in before you can comment on or make changes to this bug.