Re: [evolution-patches] [addressbook] CSV and Tab Importers for addressbook

[Tor Lillqvist <tml novell com>]

Some comments:
> 
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif

I think the ifdef HAVE_CONFIG_H can be dropped around #include
<config.h>. This code is not going to be built anywhere except inside
Evolution's source code, so the ifdef is mostly pointless. IMHO.

> +#include <gtk/gtkvbox.h>

In new code, please just #include <gtk/gtk.h>. I don't think there is
any rational reason to try to hand-pick what gtk headers to include.


> +gint importer;
> +char delimiter;

Make these static. And do these really need to be global variables,
can't they be fields in some struct?

> +                               g_string_append_unichar(value, *ptr);

What about the character set? Files that have been exported from Outlook
are in the system codepage of the exporting machine, presumably. Not
necessarily CP1252 (ISO-8859-1) which using g_string_append_unichar()
implies (as the bytes of CP1252 are identical to the first 256 Unicode
code points). This is actually a rather complicated issue if one also
wants to be able to import CSV files exported in double-byte codepages
(in CJK locales). And why not support CJK locales? 

What you probably want to to is to have a GUI to select what the charset
the input file is in, convert each line as it has been read to UTF-8,
and then scan and parse the line using GLib's UTF-8 functionality. This
probably is a more generic issue for Evo's import stuff, and should be
taken care of on a higher level, though. At least the GUI to select the
charset of the file to be imported?
that
As the imported file might originate from Windows, you should look for
potential CRLF line terminators, too.

> +       file = fopen(s->uri_src+7, "r");

For portability, use g_fopen (g_filename_from_uri (s->uri_src, NULL,
NULL), "r").

> +               g_message(G_STRLOC ":Couldn't Create EBook");

Is it really useful to have G_STRLOC in all these messages, if they are
intended for the end-user?

The functions outlook_csv_import(), mozilla_csv_import() and
evolution_csv_import() are essentially identical. Refactor please into
one function with a parameter instead.

--tml