Status

MailNews Core
Address Book
RESOLVED FIXED
16 years ago
5 years ago

People

(Reporter: Ken Guest, Assigned: Evan Stratford)

Tracking

Trunk
Thunderbird 3.3a1
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: ucosp)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

16 years ago
it would be very useful to be able to import (File| Import | Addressbook) vcard
entries.

Comment 1

16 years ago
updating product and component
Assignee: asa → chuang
Status: UNCONFIRMED → NEW
Component: Browser-General → Address Book
Ever confirmed: true
Product: Browser → MailNews
QA Contact: doronr → esther
(Reporter)

Comment 2

16 years ago
I don't know if this should be split up into a seperate bug report, it is also 
to do with importing vcard information, but triggered by downloading a file 
of text/x-vCard mime type:

on a similar note, it would be most useful if vcards could also be
imported/added to the address book when the user downloads/clicks on a file of
text/x-vCard mime type. If the user wants that file saved to the harddrive (i.e.
not import it) he can always do a right-click and choose "save file as" from the
popup menu.

Updated

16 years ago
QA Contact: esther → fenella

Comment 3

16 years ago
This is pretty similar to bug 38970 "Ability to Add vcard from browser"

Updated

16 years ago
QA Contact: fenella → nbaca

Comment 4

16 years ago
Note that the correct MIME type is 'text/directory', not 'text/x-vCard'. See
RFC 2425.

Comment 5

15 years ago
*** Bug 163108 has been marked as a duplicate of this bug. ***
taking all of chuang's bugs.  she doesn't work on mozilla anymore.
Assignee: chuang → sspitzer
scott, does BMS require this?
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: PC → All

Comment 8

14 years ago
I'm considering switching to Thunderbird and have a large personal addressbook
in Palm Desktop that can be exported as one large, multi-entry vcard.

By this, I mean a single vcard file with several people listed in it. It can be
imported into Evolution's addressbook without a problem. (Evolution recognizes
it as being in GnomeCard/vCard/VCF format) and the field names and data
associated with them are properly handled.

I would like to do the same with Thunderbird - that is, import this single
multi-entry vcard into my Thunderbird addressbook. I currently have no way to
migrate my addressbook into TB.

Ken Guest summarized the issue best in Comment #2.

Comment 9

13 years ago
(In reply to comment #8)
> I would like to do the same with Thunderbird - that is, import this single
> multi-entry vcard into my Thunderbird addressbook. I currently have no way to
> migrate my addressbook into TB.

I would like this too. However, since we don't have it yet, I'll point out that
there *is* a workaround, albeit a ReallyAnnoying(tm) one. Plus, you need Windows.

Basically, you just need to import your address book into Outlook Express (I
assume Outlook or Eudora would work as well), then import from *that* into
Thunderbird. In my case, I didn't even have to start OE - I just clicked on the
vcard file in Windows which brought up the default viewer, and I went through
and clicked "Add to Address Book" on each of them, then went to Thunderbird and
imported from Outlook Express.

Of course, this theory should work if you can get your vCard format into
anything else that TB recognizes.

HTH.
*** Bug 240332 has been marked as a duplicate of this bug. ***

Comment 11

13 years ago
Hi.

No promises, but I'm considering having a crack at this one as my first piece of Thunderbird coding and 
would appreciate some pointers.

I've downloaded the source to Thunderbird 0.7.1 and have looked at mozilla/mailnews/addrbook and 
mozilla/mailnews/import directories. Seems to me that if I add my handler inside the import, then 
create something that loops through a file and creates vCard objects as specified in the addrbook 
section then I should be able to complete this.

As far as I can tell, I should be opening up a stream, and looking at nsMsgVCardService to process that 
stream. Questions...


1. Is the above right?
2. Do I need to create the stream, or will some existing dialog pass it to me? What's the entry point I 
must define if the stream gets passed to me?
3. How do I then go about commiting the found objects to the address book?
4. Are there certain required fields that I absolutely -must- have filled in before the address book will 
allow commits?


Should mention my reason for doing this: I'm a Mail.app/Address Book user on OS X, and a 
Thunderbird user on Windows. Would like to sync my address books between the two machines.


Cheers,
Ian
Product: Browser → Seamonkey

Comment 12

13 years ago
I was looking for a way to import/export addresses between Thunderbird (1.0) and
my Sony Ericsson T630 mobile phone. The phone is capable of sending the whole
address book as a single vCard file (via bluetooth or infrared), which I thought
would help me a lot.

However, now it depends on Thunderbird beeing able to import this. I actually
considered this a must-have for Thunderbird.

Developers, note: Import options are vital in order to make an application more
popular. :) Just a user's $ 0.02.

Comment 13

12 years ago
Still missing the ability to import vCards.

This is a *MUST* if users should switch from other browsers/mail clients to use
Mozilla/TB

Cheers, Jörg

Comment 14

12 years ago
Exactly. All, not only claiming to be modern, mail clients have better 
handling of vCards than Tb. Also it is often the only format for exchanging 
data with many mobiles, e.g. my own Siemens and is accepted by all modern 
ones. I wonder why after more than three years after this bug had been filed, 
this really basic and uncomplicated feature hasn't been yet implemented. 
Microsoft, which by Mozilla Found. is said not to comply with open standards, 
has included this factually for years industial standard in its products. Is 
anything at all going to be done about it?

Comment 15

12 years ago
Robert, you must have written code that basically does this, right?
I've done export, as it applies to the iPod (iPod doesn't support multiple
vcards in 1 file, as it's not 'to spec' according to Apple's Engineers), and bug
221991, among a few little things.

Import should already exist.... just no UI for it.

when I receive an email with a vcard attached, I can import it by clicking on it.

IMHO that's the right place to work from.  Check with mscott, as I recall he
implemented that during Tbird, somewhat early as I recall.

Comment 17

12 years ago
IMHO, just adding vCards from received e-mails is not sufficient.
It should be also possible to import vCard directly from a file into the
addressbook, just in the same way as this can be done with LDIF or TXT files.

Cheers, Jörg

Comment 18

12 years ago
My wife just switched *from* Mozilla *to* Outlook because this feature is
missing in Mozilla/Thunderbird.
To summarize a quick conversation with bienvenu:

1.  We don't support vcard 3.0... we should.
2.  We have a addressboook: method to import a card.  We don't have a UI, or a
way to do them in bulk.

As part of 1, we need to decide if we will support multiple vcards in 1 file
(inline with 2 returns separating them).  Apple told me they don't support it
because they don't believe it's part of the Spec.  I've confirmed that PalmOne
(formerly Palm Inc.) does support this in Palm Desktop.

That of course is another bug, and regards both import and export.

vCard is slightly a mess... but the groundwork is there.  It's a matter of
catching up and finishing what was started.

Creating import could be done using addbook:add?action=add?vcard=<<vcard
urlencoded()>>

So it's a matter of looping through and doing that.  It should be feasable with
just some JS and some XUL changes.

Not sure if I'll have free time to do this in the near future.  David and Scott
would be much more likely.  Though I'll keep it in mind if I get free cycles.


Please refrain from unnecessary comments.  We know this is a desired feature. 
Add your vote if you wish, but don't keep the "me too" comments.  It doesn't
help anyone.

Comment 20

12 years ago
Thanks, Robert.

But most of the people (as do I) had the feeling that they need some feedback
whether you are still working on this.

Actually some tags as Target Milestone etc. would indicate this easily in the
bugtracking system and in this way stopping to people to ask when there haven't
been any comments for a while.

Cheers, Jörg
There is no target set... since nobody is working on this.

Comment 22

12 years ago
I offered to work on it a few comments up - would be my first bit of Thunderbird coding though. 
Anyone care to see if I was on the right track and perhaps give a few pointers?


Ian
I had a word with Seth and he isn't working on this at the moment. Therefore
returning status to new.
Status: ASSIGNED → NEW

Updated

12 years ago
Assignee: sspitzer → mail

Comment 24

12 years ago
Another workaround is to attach a vCard you want to import to a new (outgoing)
message and then click it and it will be added to your address book.

Comment 25

12 years ago
But will that work for a vCard with multiple entries?
Assignee: mail → nobody
Component: Address Book → MailNews: Address Book
Product: Mozilla Application Suite → Core
QA Contact: nbaca → addressbook
*** Bug 271634 has been marked as a duplicate of this bug. ***
(In reply to comment #19)
> As part of 1, we need to decide if we will support multiple vcards in 1 file
> (inline with 2 returns separating them).  Apple told me they don't support it
> because they don't believe it's part of the Spec.

Just in case this isn't already known, Apple Address Book will export multiple VCards into a single file if you select multiple records and Export.  I don't see two returns between records, just:

END:VCARD
BEGIN:VCARD

Comment 28

11 years ago
*** Bug 354207 has been marked as a duplicate of this bug. ***

Comment 29

10 years ago
Isn't this the same as bug 176803?

Updated

10 years ago
Duplicate of this bug: 176803

Comment 31

10 years ago
This would be an important feature to get synching working with Thunderbird. As OpenSync gets better more and more Linux-Users will want to import their VCards.

Comment 32

9 years ago
isnt this blocked by bug 266891 ? Right now you just cant import/export vcards correctly, because of the inflexibility of the current adress book (ie. you cant save the vcard uid right now, which is a requirement, otherwise you will have tons of duplicates)
(In reply to comment #32)
> isnt this blocked by bug 266891 ? Right now you just cant import/export vcards
> correctly, because of the inflexibility of the current adress book (ie. you
> cant save the vcard uid right now, which is a requirement, otherwise you will
> have tons of duplicates)
> 
No. A vcard import could be done with the current address book structure. It would be possible to save the uid if done right as well (though only for local address books).
Product: Core → MailNews Core

Comment 34

9 years ago
According to the comments on this bug, Thunderbird has had some kind of vCard import ability for years, but just doesn't have it in the import UI?  Can someone look at adding this? Or find out what the difficulties might be in the current codebase? Or give pointers on how to apporoach it for someone completely new to working on Thunderbird?  Thanks

Comment 35

9 years ago
I found a perl script that converts a vcard file (with multiple vcards possible) to LDIF: 

http://stud4.tuwien.ac.at/~e0325716/vcard2ldif.html

However at least with my vcard file it resulted in blank "display" names, until I edited the perl script and added back in a commented out section labelled "# CREATE A DISPLAY NAME # BY KLOKIE" and changed the definition of $change_names to assign it 1 instead of 0.

Comment 36

9 years ago
(In reply to comment #33)
> (In reply to comment #32)
> > isnt this blocked by bug 266891 ? Right now you just cant import/export vcards
> > correctly, because of the inflexibility of the current adress book (ie. you
> > cant save the vcard uid right now, which is a requirement, otherwise you will
> > have tons of duplicates)
> > 
> No. A vcard import could be done with the current address book structure. It
> would be possible to save the uid if done right as well (though only for local
> address books).

I wrote an extension (synckolab) that imports vcards (and kolab xml) using imap - and I tried out quite a few things - but storing an uid was impossible for me - thats the reason why I had to use the 3rd custom field for the uid.

if anyone knows how I could write in a (hidden) uid field I could change this behavior

Comment 37

9 years ago
Importing and exporting a vCard would be useful.  This way you can, import one and assign it to a present account, or export one for backup purposes.

Comment 38

8 years ago
Isn't this duplicate/blocking of bug 29106?
(Assignee)

Comment 39

8 years ago
Looks like it's been a while since last post here :)

As noted in https://bugzilla.mozilla.org/show_bug.cgi?id=29106, I'm taking a look at adding support for vCard import/export in Address Book.

@Matej: As I understand it, this would logically be part of that bug.
Assignee: nobody → evan.stratford
Status: NEW → ASSIGNED
Whiteboard: ucosp
(Assignee)

Comment 40

7 years ago
I've essentially got something working here - just have to clean it up and add tests, then I'll post a patch for review :)
Yaaay!!! :)
(Assignee)

Comment 42

7 years ago
Created attachment 428865 [details] [diff] [review]
Adds vCard import to Address Book importer.

First patch submit; if there's a problem, please let me know. To test, make xpcshell-tests in mailnews/import/test.

Updated

7 years ago
Attachment #428865 - Flags: review?(bugzilla)
Comment on attachment 428865 [details] [diff] [review]
Adds vCard import to Address Book importer.

Thanks for the patch, it looks like a very good start. I may not be able to get to review it until sometime late next week, but here's some comments I noticed from a quick look.


>+# Short name of import module
>+## @name VCARDIMPORT_NAME
>+## @loc None
>+2000=vCard file (.vcf)

I know this is what the existing code does, however we are really trying to avoid using numeric ids for strings, as it makes it harder for localisers to do their job. Can you change these into names? e.g. vcardImportName

>+    dump("\n\n\n\n*******\nsupportsMultiple: " +
>+        addInterface.GetStatus( "supportsMultiple"));

This looks like extra debug code that we don't need.
>+
>+#ifdef NS_DEBUG
>+#define IMPORT_DEBUG  1
>+#endif
>+
>+// Use PR_LOG for logging.
>+#include "prlog.h"
>+extern PRLogModuleInfo *VCARDLOGMODULE;  // Logging module
>+
>+#define IMPORT_LOG0(x)          PR_LOG(VCARDLOGMODULE, PR_LOG_DEBUG, (x))
>+#define IMPORT_LOG1(x, y)       PR_LOG(VCARDLOGMODULE, PR_LOG_DEBUG, (x, y))
>+#define IMPORT_LOG2(x, y, z)    PR_LOG(VCARDLOGMODULE, PR_LOG_DEBUG, (x, y, z))
>+#define IMPORT_LOG3(a, b, c, d) PR_LOG(VCARDLOGMODULE, PR_LOG_DEBUG, (a, b, c, d))

Can these just be defined in nsVCardAddress.h?

>+  /* PRBool GetAutoFind (out wstring description); */
>+  NS_IMETHOD GetAutoFind(PRUnichar **description, PRBool *_retval);


I don't think we need to have the comments about what's in the idl. Most devs can look it up or work it out.

>diff -r cbdc1439e07d -r 29bc2cdd8b27 mailnews/import/vcard/src/nsVCardStringBundle.cpp
>--- /dev/null
>+++ b/mailnews/import/vcard/src/nsVCardStringBundle.cpp
>diff -r cbdc1439e07d -r 29bc2cdd8b27 mailnews/import/vcard/src/nsVCardStringBundle.h
>--- /dev/null
>+++ b/mailnews/import/vcard/src/nsVCardStringBundle.h

In bug 131040 we've been trying to remove the duplicate string bundles, and re-use nsImportStringBundle. Can you do that here as well please.

Attachment 260454 [details] [diff] shows an example where I did this for the text import.

Note you may need to add a couple of GetStringByName/GetStringFromName functions, to support the switch from numeric ids to names that I mentioned above, but I'd much rather that was done in the existing class, rather than duplicating the functions again.


As I've said at the start this has been a quick look set of comments, and hopefully I'll get back to it somewhere around the middle of next week, but if you have time to update the patch in-between, that would be great.
(Assignee)

Comment 44

7 years ago
Created attachment 428990 [details] [diff] [review]
Modified patch, taking into account the comments from Standard8.

Removed nsVCardStringBundle and moved nsVCardDebugLog macros into nsVCardAddress.h, as per comments from Standard8.
Attachment #428865 - Attachment is obsolete: true
Attachment #428865 - Flags: review?(bugzilla)
(Assignee)

Comment 45

7 years ago
Created attachment 430871 [details] [diff] [review]
switching to string-based indices for message bundle

Switching to string-based indices for vCard message properties; adding nsImportStringBundle::GetStringByName() to support this.
Attachment #428990 - Attachment is obsolete: true

Comment 46

7 years ago
Don't forget mark it as patch and put review? for Mark (bugzilla@standard8.plus.com)
(Assignee)

Updated

7 years ago
Attachment #430871 - Flags: review?(bugzilla)
Comment on attachment 430871 [details] [diff] [review]
switching to string-based indices for message bundle

Sorry for the big delay in reviewing this.

Unfortunately, the import code on which you based your patch isn't the best style, and for new code we prefer to use the more consistent newer style.

Therefore I've broken up the comments into two parts. The first are significant comments that affect functionality, performance etc, the second are comments about the style.

If you can at least fix the first set of comments that would be good. If you've got time to fix some of all of the second set, that would be very useful.

Here are the comments about functionality/performance/correctness etc:

>+++ b/mailnews/import/test/unit/test_csv_import.js
>+++ b/mailnews/import/test/unit/test_vcard_import.js

In debug mode, these tests are failing with:

###!!! ASSERTION: nsVariant not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file /Users/moztest/comm/main/src/mozilla/xpcom/ds/nsVariant.cpp, line 1694

I'm pretty sure this is due to the import running on one thread, but something in the address book saving not being proxied to the main thread - the mork database code has to run on the main thread. I haven't had chance to look at what it is that is causing this yet, but getting that in a debugger should help. I'll try and do that later if I get time.

>diff -r cbdc1439e07d -r bde920ee53b7 mail/locales/en-US/chrome/messenger/addressbook/addressBook.properties
>diff -r cbdc1439e07d -r bde920ee53b7 mail/locales/en-US/chrome/messenger/importMsgs.properties

You're missing the suite/ versions of changes to these files.

>+++ b/mail/locales/en-US/chrome/messenger/vCardImportMsgs.properties

Based on conversations I've had, drop the license header in this file (and the suite/ equivalent) for now.

>+# Short name of import module
>+## @name VCARDIMPORT_NAME
>+## @loc None
>+vCardImportName=vCard file (.vcf)

I think for all of these we can drop the old-style comments. The string names give a good idea to l10n as to what they represent.

>+vCardImportAddressSuccess=Imported address book %S
>+vCardImportAddressBadSourceFile=Error accessing file for address book %S.
>+vCardImportAddressConvertError=Error importing address book %S, all addresses may not have been imported.

These strings should all have a localisation note similar to:

# LOCALIZATION NOTE (vCardImportAddressSuccess): %S is replaced by the name of the address book being imported.

>diff -r cbdc1439e07d -r bde920ee53b7 mailnews/import/build/Makefile.in
>+		../vcard/src/$(LIB_PREFIX)impVCard_s.$(LIB_SUFFIX) \

The general standard is that we have all our lib names as lower case, hence...

>diff -r cbdc1439e07d -r bde920ee53b7 mailnews/import/vcard/src/Makefile.in
>+MODULE		= impVcard
>+LIBRARY_NAME   = impVCard_s

...please make the impVCard lower case.

>+# The Initial Developer of the Original Code is
>+# Netscape Communications Corporation.
>+# Portions created by the Initial Developer are Copyright (C) 2001
>+# the Initial Developer. All Rights Reserved.
>+#
>+# Contributor(s):

This and the other license headers need updating, I suspect you're not working for Netscape and the date needs updating.

>+nsresult nsVCardAddress::ReadRecord(
>+    nsILineInputStream *aLineStream, nsCString &aRecord, PRBool *aMore)
>+{

>+  // read BEGIN:VCARD
>+  rv = aLineStream->ReadLine(line, &more);
>+  if (!(more && line.Equals("BEGIN:VCARD"))) {
>+    rv = NS_ERROR_FAILURE;
>+    *aMore = more;
>+    return rv;
>+  }

If I'm reading this right, then it won't allow there to be one or more blank lines between END:VCARD and BEGIN:VCARD. Is that necessary? I'm just thinking about people copy and pasting cards into a file and not cleaning up.

It would also be good to add a log output here, so we know why this is failing.

>+  // read until END:VCARD
>+  while (true) {
>+    if (!more) {
>+      rv = NS_ERROR_FAILURE;
>+      break;
>+    }
>+    rv = aLineStream->ReadLine(line, &more);
>+    if (!aRecord.IsEmpty())
>+      aRecord.AppendLiteral(MSG_LINEBREAK);
>+    aRecord.Append(line);
>+    if (line.Equals("END:VCARD"))
>+      break;
>+  }

I'd suggest making this a do...while loop with the exit condition being the (line.Equals("END:VCARD")).

>diff -r cbdc1439e07d -r bde920ee53b7 mailnews/import/vcard/src/nsVCardAddress.h

>+  PRInt32 m_LFCount;
>+  PRInt32 m_CRCount;

m_LFCount and m_CRCount are initialised only. So these can be dropped.

>+static NS_DEFINE_IID(kISupportsIID, NS_ISUPPORTS_IID);

Drop this, I'll explain why below.

>+
>+  *name = nsImportStringBundle::GetStringByName(
>+      VCARDIMPORT_NAME, m_stringBundle);

Let's skip the #define for VCARDIMPORT_NAME and just put the string in here. Having the define doesn't actually gain us anything apart from a bit more complexity.

(ditto in the GetDescription function).

>+NS_IMETHODIMP nsVCardImport::GetImportInterface(
>+    const char *pImportType, nsISupports **ppInterface)

>+  nsresult rv;
>+  *ppInterface = nsnull;
>+  if (!strcmp( pImportType, "addressbook")) {

rv can be declared after within the if statement as its only used in that block.

>+          rv = pGeneric->QueryInterface(kISupportsIID, (void **)ppInterface);

This would be much better as:

ppInterface = do_QueryInterface(pGeneric, &rv);

(it also enforces type safety).

>+NS_IMETHODIMP ImportVCardAddressImpl::FindAddressBooks(
>+    nsIFile *pLoc, nsISupportsArray **ppArray)

>+    rv = desc->QueryInterface( kISupportsIID, (void **) &pInterface);

This should use the do_QueryInterface form.

>+    rv = array->QueryInterface(
>+        NS_GET_IID(nsISupportsArray), (void **) ppArray);

As 'array' is already an nsISupportsArray, we can very simply make this:

array.swap(*ppArray);

Which also avoids an additional reference count.
Attachment #430871 - Flags: review?(bugzilla) → review-
Comment on attachment 430871 [details] [diff] [review]
switching to string-based indices for message bundle

Here are the second set of comments about the style nits/issues.

>diff -r cbdc1439e07d -r bde920ee53b7 mailnews/import/build/nsImportModule.cpp

>+  if (NS_SUCCEEDED( rv)) {

nit: no space before rv (please apply generally in other places as well).

>+    NS_Free( theCID);

nit: no space before "theCID".

>+  return( rv);

nit: just drop the brackets here.

>diff -r cbdc1439e07d -r bde920ee53b7 mailnews/import/vcard/src/Makefile.in

>+CPPSRCS		= \
>+    nsVCardAddress.cpp \
>+    nsVCardImport.cpp \
>+		$(NULL)

I think we should format this like:

CPPSRCS = \
  nsVCardAddress.cpp \
  nsVCardImport.cpp \
  $(NULL)

>diff -r cbdc1439e07d -r bde920ee53b7 mailnews/import/vcard/src/nsVCardAddress.cpp

>+nsresult nsVCardAddress::ImportAddresses(

>+nsresult nsVCardAddress::ProcessRecord(nsCString& aRecord, nsString& errors)
...
>+  return( rv);

nit: no brackets here.

>diff -r cbdc1439e07d -r bde920ee53b7 mailnews/import/vcard/src/nsVCardAddress.h

>+/////////////////////////////////////////////////////////////////////////////////////
>+/////////////////////////////////////////////////////////////////////////////////////
>+/////////////////////////////////////////////////////////////////////////////////////

Drop these comments, they aren't necessary.

>diff -r cbdc1439e07d -r bde920ee53b7 mailnews/import/vcard/src/nsVCardImport.cpp

>+  NS_IMETHOD GetSupportsMultiple(PRBool *_retval)
>+  { *_retval = PR_FALSE; return( NS_OK);}

>+  NS_IMETHOD GetNeedsFieldMap(nsIFile *location, PRBool *_retval)
>+  { *_retval = PR_FALSE; return( NS_OK);}

>+  NS_IMETHOD InitFieldMap(nsIImportFieldMap *fieldMap)
>+  { return( NS_ERROR_FAILURE);}

>+  NS_IMETHOD GetSampleData( PRInt32 index, PRBool *pFound, PRUnichar **pStr)
>+  { return( NS_ERROR_FAILURE);}
>+
>+  NS_IMETHOD SetSampleLocation( nsIFile *)
>+  { return( NS_ERROR_FAILURE); } 

nit: no brackets around the return values please. Please apply this to all other places than just the ones I've picked out here.

>+  IMPORT_LOG0( "nsVCardImport Module Created\n");

nit: no space after (. Please fix this in multiple places.

>+NS_IMETHODIMP nsVCardImport::GetName( PRUnichar **name)

Ditto here, but for function calls.

>+{
>+  NS_PRECONDITION(name != nsnull, "null ptr");
>+  if (! name)
>+    return NS_ERROR_NULL_POINTER;

You can replace this by:

NS_ENSURE_ARG_POINTER(name);

Please do this with similar patters in the rest of the patch.

>+NS_IMETHODIMP nsVCardImport::GetDescription( PRUnichar **name)
>+NS_IMETHODIMP nsVCardImport::GetSupports( char **supports)
>+NS_IMETHODIMP nsVCardImport::GetSupportsUpgrade( PRBool *pUpgrade)

Please apply similar comments to these functions as per the ones I've given on the GetName function.

>+NS_IMETHODIMP nsVCardImport::GetImportInterface(
>+    const char *pImportType, nsISupports **ppInterface)

I should have mentioned this earlier, but can you make all of these function definitions formatted like this:

NS_IMETHODIMP
nsVCardImport::GetImportInterface(const char *pImportType,
                                  nsISupports **ppInterface)

>+  NS_PRECONDITION(pImportType != nsnull, "null ptr");
>+  if (! pImportType)
>+    return NS_ERROR_NULL_POINTER;
>+  NS_PRECONDITION(ppInterface != nsnull, "null ptr");
>+  if (! ppInterface)
>+    return NS_ERROR_NULL_POINTER;

Again, these need replacing.

>+    NS_IF_RELEASE( pAddress);
>+    NS_IF_RELEASE( pGeneric);
>+    return( rv);

no space after ( and no brackets around the return value.

>+////////////////////////////////////////////////////////////////////////

We can drop this comment.

>+  *aImport = new ImportVCardAddressImpl(aStringBundle);
>+  if (! *aImport)
>+    return NS_ERROR_OUT_OF_MEMORY;

nit: no space after !

>+NS_IMETHODIMP ImportVCardAddressImpl::FindAddressBooks(
>+    nsIFile *pLoc, nsISupportsArray **ppArray)

>+  nsCOMPtr<nsIImportABDescriptor>  desc;
>+  nsISupports * pInterface;

nit: no space after *.

>diff -r cbdc1439e07d -r bde920ee53b7 mailnews/import/vcard/src/nsVCardImport.h

>+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

This should be tab-width: 2. Please make sure its the same in other places.
(Assignee)

Comment 49

7 years ago
Created attachment 436596 [details] [diff] [review]
Addressing comments from standard8

Addressing most of the comments above, along with a bit of general cleanup. After some discussion on IRC, it seems these thread-safety issues are pretty well entrenched in the importing framework...
Attachment #430871 - Attachment is obsolete: true
Attachment #436596 - Flags: review?(bugzilla)
Comment on attachment 436596 [details] [diff] [review]
Addressing comments from standard8

>+ *
>+ * The Original Code is mozilla.org Code.
>+ *
>+ * Contributor(s):
>+ *   Evan Stratford <evan.stratford@gmail.com>
>

You're missing this section (which is required):

 * The Initial Developer of the Original Code is
 * ____________________________________________.
 * Portions created by the Initial Developer are Copyright (C) 2___
 * the Initial Developer. All Rights Reserved.
 *
 * Contributor(s):

You can put the initial developer as yourself (and fill in the year)

Also the vcard test is failing with:

TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | TypeError: importService.GetModule(aModuleName, i).GetImportInterface(aModuleName) is null - See following stack:

This patch is basically r+, but I'm still thinking about the threadsafe issues and the tests. I suggest you update it for the above fixes and request sr from :bienvenu. In the meantime, I'll try out something for fixing the threadsafe issues.
Created attachment 475533 [details] [diff] [review]
Fixed up

Sorry Evan, I really should have got to this ages ago. What I've just realised is that we said if unit tests were too hard/complex we could push them out to other things and still land the patch.

So I'm proposing that we land this with some litmus testing, and push out the issues with the threadsafey in the tests to another bug.

Therefore I've updated this to resolve the bitrot and the couple of remaining issues and asking David to take a quick look over.
Attachment #436596 - Attachment is obsolete: true
Attachment #436596 - Flags: review?(bugzilla)
Attachment #475533 - Flags: superreview?(bienvenu)
Attachment #475533 - Flags: review+

Updated

7 years ago
Attachment #475533 - Flags: superreview?(bienvenu) → superreview+
Blocks: 600798
I've moved the test cases out to bug 600798. Ludovic: we should get something in litmus in the meantime.

This bug is now landed: http://hg.mozilla.org/comm-central/rev/9021466c11d8
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-litmus?(ludovic)
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.3a1
Evan what version of vcard are we supporting with this patch ?
(In reply to comment #52)
> I've moved the test cases out to bug 600798. Ludovic: we should get something
> in litmus in the meantime.
> 
> This bug is now landed: http://hg.mozilla.org/comm-central/rev/9021466c11d8

Looks like a change to suite/locales/jar.mn is missing. Otherwise thanks for taking care of SeaMonkey! :-)
(In reply to comment #54)
> (In reply to comment #52)
> > I've moved the test cases out to bug 600798. Ludovic: we should get something
> > in litmus in the meantime.
> > 
> > This bug is now landed: http://hg.mozilla.org/comm-central/rev/9021466c11d8
> 
> Looks like a change to suite/locales/jar.mn is missing. Otherwise thanks for
> taking care of SeaMonkey! :-)

Fixed: http://hg.mozilla.org/comm-central/rev/24dafd19672c
(In reply to comment #55)
> (In reply to comment #54)
> > (In reply to comment #52)
> > > I've moved the test cases out to bug 600798. Ludovic: we should get something
> > > in litmus in the meantime.
> > > 
> > > This bug is now landed: http://hg.mozilla.org/comm-central/rev/9021466c11d8
> > 
> > Looks like a change to suite/locales/jar.mn is missing. Otherwise thanks for
> > taking care of SeaMonkey! :-)
> 
> Fixed: http://hg.mozilla.org/comm-central/rev/24dafd19672c

Nit: Missing ) ;-)

Comment 57

7 years ago
I can only see vCardImportAddressBadParam in the language files in the patch. Is that string unused?
Comment on attachment 475533 [details] [diff] [review]
Fixed up

[The commit date for this patch was 1 Apr, is this some kind of joke?]

>+  if (!line.EqualsIgnoreCase("BEGIN:VCARD")) {
[EqualsIgnoreCase doesn't exist in the external API; I can't remember but maybe
 we could use Equals("BEGIN:VCARD", CaseInsensitiveCompare) instead.]

Comment 59

7 years ago
could anyone answer my question from comment #57?
(In reply to comment #57)
> I can only see vCardImportAddressBadParam in the language files in the patch.
> Is that string unused?

Sorry for the delay, now fixed with rs from Neil over irc:

http://hg.mozilla.org/comm-central/rev/36440349f946

(In reply to comment #58)
> Comment on attachment 475533 [details] [diff] [review]
> Fixed up
> 
> [The commit date for this patch was 1 Apr, is this some kind of joke?]

That was probably something in the patch file to do with when it was last generated. Not sure why it got pushed through.

> >+  if (!line.EqualsIgnoreCase("BEGIN:VCARD")) {
> [EqualsIgnoreCase doesn't exist in the external API; I can't remember but maybe
>  we could use Equals("BEGIN:VCARD", CaseInsensitiveCompare) instead.]

Also fixed in the above changeset.

Comment 61

7 years ago
(In reply to comment #60)
> (In reply to comment #57)
> > I can only see vCardImportAddressBadParam in the language files in the patch.
> > Is that string unused?
> 
> Sorry for the delay, now fixed with rs from Neil over irc:
> 
> http://hg.mozilla.org/comm-central/rev/36440349f946

Thanks. Though SeaMonkey is in string freeze ATM, so you'll want to post to m.d.l10n.
(In reply to comment #61)
> (In reply to comment #60)
> > (In reply to comment #57)
> > > I can only see vCardImportAddressBadParam in the language files in the patch.
> > > Is that string unused?
> > 
> > Sorry for the delay, now fixed with rs from Neil over irc:
> > 
> > http://hg.mozilla.org/comm-central/rev/36440349f946
> 
> Thanks. Though SeaMonkey is in string freeze ATM, so you'll want to post to
> m.d.l10n.

I've just backed out the change to the SeaMonkey file as its simpler for me. Someone else can re-land it later.

Comment 63

7 years ago
OK, I filed bug 602188 about that.
Depends on: 602188
Tests 13624 and 13625 added to litmus.
Flags: in-litmus?(ludovic) → in-litmus+
Blocks: 603265

Updated

6 years ago
Duplicate of this bug: 630596
Depends on: 717736
With bug 600798 landing the tests, this is now in-testsuite, thanks Evan.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.