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)
MailNews Core
Address Book
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)
(deleted),
patch
|
jcranmer
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•16 years ago
|
Version: unspecified → Trunk
Comment 1•16 years ago
|
||
The same applies to Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081101 SeaMonkey/2.0a2pre
Comment 3•16 years ago
|
||
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?
Updated•16 years ago
|
Flags: blocking-thunderbird3+
Keywords: regression
Product: Thunderbird → MailNews Core
QA Contact: address-book → addressbook
Hardware: PC → All
Target Milestone: --- → Thunderbird 3.0b2
Assignee | ||
Comment 4•16 years ago
|
||
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.
Assignee | ||
Comment 5•16 years ago
|
||
This works for me on today's build. Can anyone else confirm?
Reporter | ||
Comment 6•16 years ago
|
||
(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
Assignee | ||
Comment 7•16 years ago
|
||
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?
Reporter | ||
Comment 8•16 years ago
|
||
(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?
Comment 9•16 years ago
|
||
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.
Comment 10•16 years ago
|
||
Bumping off the b2 blocker list, we're going to have to fix this soon.
Target Milestone: Thunderbird 3.0b2 → Thunderbird 3.0rc1
Assignee | ||
Updated•16 years ago
|
Assignee: nobody → bugzilla
Comment 11•16 years ago
|
||
Also see bug 476275.
Comment 12•16 years ago
|
||
Still see this on linux.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Target Milestone: Thunderbird 3.0rc1 → Thunderbird 3.0b3
Assignee | ||
Comment 13•16 years ago
|
||
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]
Assignee | ||
Comment 14•16 years ago
|
||
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)
Assignee | ||
Updated•16 years ago
|
Whiteboard: [diagnosed][needs comment Joshua] → [has patch][needs review Joshua,Neil]
Updated•16 years ago
|
Attachment #370199 -
Flags: superreview?(neil) → superreview+
Comment 15•16 years ago
|
||
Comment on attachment 370199 [details] [diff] [review]
Proposed fix
Personally I'd lean to != false
Comment 16•16 years ago
|
||
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+
Assignee | ||
Updated•16 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][needs review Joshua,Neil] → [has patch]
Assignee | ||
Comment 17•16 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•