Closed Bug 461660 Opened 16 years ago Closed 16 years ago

"Allow remote images in HTML mail" state not saved across restarts

Categories

(MailNews Core :: Address Book, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b3

People

(Reporter: bugzilla.i.sekler, Assigned: standard8)

References

Details

(Keywords: regression, Whiteboard: [has patch])

Attachments

(1 file)

User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b2pre) Gecko/20081025 Firefox/3.1b2pre Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b2pre) Gecko/20081025 Shredder/3.0b1pre Unchecking the checkbox to allow remote images in HTML mails in an address book card persists only until the address book window remains open. After closing and reopening the address book the checkbox is checked again. Reproducible: Always Steps to Reproduce: 1. Open Adress Book 2. Edit a contact, uncheck "Allow remote images in HTML mail" (click "OK") 3. Close the Adress Book window and open it again 4. Open the contact you disallowed to load remote images for editing. Actual Results: "Allow remote images in HTML mail" is checked. Expected Results: "Allow remote images in HTML mail" remains unchecked.
Version: unspecified → Trunk
The same applies to Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081101 SeaMonkey/2.0a2pre
OS -> All, based on Comment #1.
OS: Linux → All
I think the bug is somewhere in nsAbCardProperty.cpp which is backend code shared between TB and SM (if I'm right Product should be changed to MailNews Core). In abCardOverlay.js (TB and SM version), kNonVcardFields contains "allowRemoteContent" which suggests that this property should not be saved to a VCard. Accordingly nsAbCardProperty::ConvertToEscapedVCard() in nsAbCardProperty.cpp doesn't query and write the value of kAllowRemoteContentProperty. Furthermore this property isn't queried or written anywhere else in the file, only set to false during initialization. If the property is not to be written to a VCard, where should its value be stored (permanently) then?
Flags: blocking-thunderbird3+
Keywords: regression
Product: Thunderbird → MailNews Core
QA Contact: address-book → addressbook
Hardware: PC → All
Target Milestone: --- → Thunderbird 3.0b2
Joshua, any chance you could take a look at this as I think its a regression from the AB card refactoring? (In reply to comment #3) > In abCardOverlay.js (TB and SM version), kNonVcardFields contains > "allowRemoteContent" which suggests that this property should not be saved to a > VCard. Accordingly nsAbCardProperty::ConvertToEscapedVCard() in > nsAbCardProperty.cpp doesn't query and write the value of > kAllowRemoteContentProperty. Furthermore this property isn't queried or written > anywhere else in the file, only set to false during initialization. If the > property is not to be written to a VCard, where should its value be stored > (permanently) then? The convert to escaped vcard is only used for saving as a vcard for attaching to outgoing emails (dig around in top-level account preferences), hence you don't need to save it there.
This works for me on today's build. Can anyone else confirm?
(In reply to comment #5) > This works for me on today's build. Can anyone else confirm? No, still broken. Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b2pre) Gecko/20081118 Thunderbird/3.0b1pre hg identify be77b2aa036b tip
Ilja, please can you check the error console for any messages whilst you're editing/saving a contact. Also, is this in your local address book? Not an LDAP one? (not that it should be enabled in an LDAP book). Do other changes to your address book work?
(In reply to comment #7) > Ilja, please can you check the error console for any messages whilst you're > editing/saving a contact. No messages. The warning Warning: Timed textboxes are deprecated. Consider using type="search" instead. Source File: chrome://messenger/content/addressbook/addressbook.xul Line: 0 just opening the address book is unrelated, I think. > Also, is this in your local address book? Yes. > Not an LDAP one? [...]. No. > Do other changes to your address book work? Yes, I haven't found any other that would not get saved, except of this one. BTW: Is there a special reason to keep this bug UNCONFIRMED?
Also note that "separate address list for addresses allowed to load remote images for email" (bug 457296) is set for the b2 time frame. I'm not sure of the blocker status of this assuming the other bug gets some traction.
Bumping off the b2 blocker list, we're going to have to fix this soon.
Target Milestone: Thunderbird 3.0b2 → Thunderbird 3.0rc1
Assignee: nobody → bugzilla
Still see this on linux.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Target Milestone: Thunderbird 3.0rc1 → Thunderbird 3.0b3
I now see what is happening here: 1) AllowRemoteContent is intended as a boolean field (see nsIAbCard.idl). 2) When we edit a card, the AllowRemoteContent property is set to true or false as appropriate. 3) When we reload the card in the same session, the true/false is assigned to the .checked attribute on the checkbox and we correctly display the state. 4) When the card is saved to the local address book, it is converted to a string and saved as '0' or '1'. 5) TB is then restarted, and reads the value from the local AB -> '0' or '1', the .checked attribute doesn't know about this and always gets set to checked. Hence the issue. We may go away from storing this value within the address book via bug 457296, however this does point to the fact that our current local database implementation can't cope with boolean values correctly. Assigning to Joshua for comment as he knows what most of that revised code does.
Assignee: bugzilla → Pidgeot18
Summary: "Allow remote images in HTML mail" state not saved → "Allow remote images in HTML mail" state not saved across restarts
Whiteboard: [diagnosed][needs comment Joshua]
Attached patch Proposed fix (deleted) — Splinter Review
Proposed fix. I don't think we're going to get a sane way to deal with boolean values without a lot of special casing. So I think its best just to ensure the values are boolean when we need them to be.
Assignee: Pidgeot18 → bugzilla
Status: NEW → ASSIGNED
Attachment #370199 - Flags: superreview?(neil)
Attachment #370199 - Flags: review?(Pidgeot18)
Whiteboard: [diagnosed][needs comment Joshua] → [has patch][needs review Joshua,Neil]
Attachment #370199 - Flags: superreview?(neil) → superreview+
Comment on attachment 370199 [details] [diff] [review] Proposed fix Personally I'd lean to != false
Comment on attachment 370199 [details] [diff] [review] Proposed fix If we were already using SQLite address books, we could differentiate between strings and integers in storage; I don't see an efficient, backwards-compatible way to do that in mork, so we're stuck as is for right now. And this textbox is way too small...
Attachment #370199 - Flags: review?(Pidgeot18) → review+
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][needs review Joshua,Neil] → [has patch]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: