437750 - New message still return preferred format window when we use "additional email" address

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Pascal Kreyer (Private)]

User-Agent:       Mozilla/5.0 (X11; U; SunOS i86pc; en-US; rv:1.9) Gecko/2008052012 Firefox/3.0
Build Identifier: Mozilla/5.0 (X11; U; SunOS i86pc; en-US; rv:1.9) Gecko/2008052012 Firefox/3.0

New message cannot determine preferred format in addressbook for "additional email"

Reproducible: Always

Steps to Reproduce:
1. add an additional email in a contact and select HTML format as preferred format
2. create an new email with "html" content
3. put this additional email in the "To:" field
Actual Results:  
"HTML Mail Question" panel appears

Expected Results:  
Should send directly the email

Comment 1

[Ye Guo]

I can reproduce it in thunderbird 3.0a2pre(2008071503) on Solaris_11.

Comment 2

[Boying Lu]

Attachment: patch

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 330542 [details] [diff] [review]
patch

I'm going to jump in here.

This is the wrong approach. As you're now effectively getting the card for "any" email address, the better solution would be to use nsIAbMDBDirectory::cardForEmailAddress.

Then, not needing to use nsIAddrDatabase for getting the card, you can change the later calls to abDatabase->EditCard and abDatabase->Close to use nsIAbMDBDirectory::modifyCard.

This will need a little extra work, but IMHO lead to a much better solution (and one step closer to removing mdb specific dependence).

Comment 4

[Boying Lu]

Attachment: new patch based on comment #3

Comment 5

[Ye Guo]

This bug isn't only on Solaris, but also on Windows and Linux platform.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 330709 [details] [diff] [review]
new patch based on comment #3

Excellent, this is much better. r=me with the comments below fixed.

>       abDirectory = addrbookDirArray[k];
> 
>-      // These will fail for non-MDB address books, therefore, just ignore
>-      // errors, we'll check to see if we have got a valid database before using
>-      // it.
>-      nsCOMPtr<nsIAddrDatabase> abDataBase;

Can you remove the nsIAddrDatabase include as well please; it is no longer necessary.

>@@ -4629,7 +4621,7 @@
> 
>             // If we haven't got a database, don't try to increase the
>             // popularity index.
>-            if (!abDataBase)
>+            if (!mdbDirectory)

Could you change the comment to, "if we haven't got a mdb directory, don't try to get the email format preference or increase the popularity index" (with appropriate wrapping)

>@@ -4638,10 +4630,9 @@
>             // Then if we have a card for this email address
>             // Please DO NOT change the 4th param of GetCardFromAttribute() call to
>             // PR_TRUE (ie, case insensitive) without reading bugs #128535 and #121478.
>-            rv = abDataBase->GetCardFromAttribute(abDirectory, kPriEmailColumn,
>-                                                  NS_ConvertUTF16toUTF8(recipient.mEmail),
>-                                                  PR_FALSE /* case insensitive */,
>-                                                  getter_AddRefs(existingCard));
>+	    rv = mdbDirectory->CardForEmailAddress(NS_ConvertUTF16toUTF8(recipient.mEmail),
>+                                              getter_AddRefs(existingCard));
>+

nit: There's a tab in here that needs fixing, also please ensure that the g of getter aligns with the start of NS_ once that is fixed.

Would you also consider updating the existing unit test to cover this case as well? This isn't required to submit this patch, but I think it'd be good to include to ensure we don't regress. Here are some links to point you in the right direction:

http://mxr.mozilla.org/seamonkey/source/mailnews/compose/test/unit/test_nsMsgCompose1.js
http://wiki.mozilla.org/MailNews:Automated_Testing

Comment 7

[Boying Lu]

Attachment: new patch based on comment #6

Comment 8

[Mark Banner (:standard8)]

(In reply to comment #7)
> Created an attachment (id=330743) [details]
> new patch based on comment #6

Boying, this has bitrotted a little, any chance you could update it, and I'll add David to the cc list which means he's more likely to see it.

Comment 9

[Boying Lu]

(In reply to comment #8)
> (In reply to comment #7)
> > Created an attachment (id=330743) [details] [details]
> > new patch based on comment #6
> 
> Boying, this has bitrotted a little, any chance you could update it, and I'll
> add David to the cc list which means he's more likely to see it.

I can't re-produce the bug on the latest TB trunk code.  So I think the bug has been fixed

Comment 10

[Pascal Kreyer (Private)]

Is not fixed on 2008081902 build. I will check again this with next version (3.0b1).

Comment 11

[Mark Banner (:standard8)]

(In reply to comment #10)
> Is not fixed on 2008081902 build. I will check again this with next version
> (3.0b1).

I can also reproduce it on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b1pre) Gecko/20080904025916 Shredder/3.0b1pre

Comment 12

[Boying Lu]

Attachment: new patch

Mark, sorry.  I made a mistake. I can re-produce the bug now.

I updated the patch which is based on the patch of the bug 449618

Comment 13

[Mark Banner (:standard8)]

So I'm starting to think I've already seen a patch in our system like this, especially as I'm sure I'm making the same comment again. cc'ing Joshua as he may know where it is.

Anyway the comment is:

               // bump the popularity index for this card since we are about to send e-mail to it
               PRUint32 popularityIndex = 0;
               if (NS_SUCCEEDED(existingCard->GetPropertyAsUint32(
                       kPopularityIndexProperty, &popularityIndex)))
               {
                 existingCard->SetPropertyAsUint32(kPopularityIndexProperty,
                                                   ++popularityIndex);
-                // Since we are not notifying anyway, send null
-                abDataBase->EditCard(existingCard, PR_FALSE, nsnull);
-
-                // commit the database changes if we updated the popularity
-                // count.
-                abDataBase->Close(PR_TRUE);
+                abDirectory->ModifyCard(existingCard);

You should be first checking the operations attribute on abDirectory to see if it is writeable. If not, then you shouldn't bother increasing the popularity index / trying to modify the card.

Comment 14

[Boying Lu]

Attachment: patch with checking writable permission

Comment 15

[David :Bienvenu]

Comment on attachment 337376 [details] [diff] [review]
patch with checking writable permission

can you remove the space after ( and before )?

+                if ( isWriteable && nsIAbDirectory::opWrite ) 
+                {

Otherwise, this looks ok, but asking Standard8 for a quick r.

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 337376 [details] [diff] [review]
patch with checking writable permission

Now I look at it again...

>               recipient.mPreferFormat = nsIAbPreferMailFormat::unknown;
>               rv = existingCard->GetPropertyAsUint32(kPreferMailFormatProperty,
>                                                      &recipient.mPreferFormat);
>               if (NS_SUCCEEDED(rv))
>                 recipient.mProcessed = PR_TRUE;
> 
>               // bump the popularity index for this card since we are about to send e-mail to it
>               PRUint32 popularityIndex = 0;
>               if (NS_SUCCEEDED(existingCard->GetPropertyAsUint32(
>                       kPopularityIndexProperty, &popularityIndex)))
>               {
>-                existingCard->SetPropertyAsUint32(kPopularityIndexProperty,
>-                                                  ++popularityIndex);
>-                // Since we are not notifying anyway, send null
>-                abDataBase->EditCard(existingCard, PR_FALSE, nsnull);
>-
>-                // commit the database changes if we updated the popularity
>-                // count.
>-                abDataBase->Close(PR_TRUE);
>+                PRInt32 isWriteable;
>+                rv = abDirectory->GetOperations(&isWriteable);
>+                NS_ENSURE_SUCCESS(rv,rv);
>+
>+                if ( isWriteable && nsIAbDirectory::opWrite ) 
>+                {
>+                   existingCard->SetPropertyAsUint32(kPopularityIndexProperty,
>+                                                     ++popularityIndex);
>+                   abDirectory->ModifyCard(existingCard);
>+                }

Can you move the isWriteable get and check to before the if for popularity index (but after the recipient.mProcessed statement) please? Then we won't be getting the property if we don't need to.

r=me with that fixed (and the spacing around the brackets as per David's comment).

Comment 17

[Boying Lu]

Attachment: new patch

patch for quick review.

Comment 18

[Boying Lu]

Attachment: new patch

for quick review

Comment 19

[Mark Banner (:standard8)]

Comment on attachment 339003 [details] [diff] [review]
new patch

>+	      isWriteable &= nsIAbDirectory::opWrite;
>+	      
>               // bump the popularity index for this card since we are about to send e-mail to it
>               PRUint32 popularityIndex = 0;
>-              if (NS_SUCCEEDED(existingCard->GetPropertyAsUint32(
>+              if (isWriteable && NS_SUCCEEDED(existingCard->GetPropertyAsUint32(
>                       kPopularityIndexProperty, &popularityIndex)))

I think this bit would be simpler as (hence get rid of the &= statement):

if ((isWriteable & nsIAbDirectory::opWrite) &&
    NS_SUCCEEDED(existingCard->GetPropertyAsUint32(kPopularityIndexProperty,
                 &popularityIndex)))

r=me with that change.

Comment 20

[Boying Lu]

Attachment: patch for checkin

Comment 21

[Mark Banner (:standard8)]

Checked in, changeset id: 368:f8ca45bb7b11