[evolution-patches] Re: Various libebook patches

[Chris Toshok <toshok ximian com>]

On Thu, 2004-01-01 at 08:03, Ross Burton wrote:
> 2) Removal of old-style addressbook opening functions
>
> e_book_load_local_addressbook and e_book_get_default_addressbook have
> hard-coded paths to the local address books.  At the moment,
> get_default_addressbook() even loads the Evo1.4 book.
I agree that the default_addressbook call should be removed, but I think the local addressbook call should stay.  It should always load the addressbook that corresponds to On This Computer/Personal.  People shouldn't have to deal with ESource's when all they want is a tool that modifies their personal addressbook.

> I think these functions should be removed, and applications should read
> the GConf keys and/or use the source selector widgets.  Putting
> knowledge of the Evolution gconf keys into libebook is a possible
> alternative, but I don't think it's a good one.

Hm, there is already knowledge of the gconf keys in libebook. (e_book_get_addressbooks).  This is necessary because some tools want to operate on the set of all addressbooks (or just list them for the user, or something), and having one place with knowledge of the gconf key is better than having 20 million places with knowledge of it.  Of course, evolution should be using this call to retrieve the ESourceList as well (which it's not at the moment).

-----

on to the reviews:

the e-contact.[ch] changes for the video url look good.

from the e-book-async changes:
>     
>      _get_book_view_dtor (EBookMsg *msg)
>      {
>             GetBookViewMsg *view_msg = (GetBookViewMsg *)msg;
>     -       
>     +       /* TODO: handle requested_fields */
>             e_book_query_unref (view_msg->query);
>             g_free (view_msg);
>      }
>     
This should be:

> > >             g_list_foreach (view_msg->requested_fields, (GFunc)g_free, NULL);
> > >             g_list_free (view_msg->requested_fields);
> > >         
> >     

and:

>     @@ -1049,7 +1053,9 @@
>             e_book_msg_init ((EBookMsg*)msg, _get_book_view_handler, _get_book_view_dtor);
>      
>             msg->book = g_object_ref (book);
>     -       msg->query = e_book_query_from_string (query);
>     +       msg->query = e_book_query_ref (query);
>     +       msg->requested_fields = requested_fields; /* TODO: clone? ref? */
>     +       msg->max_results = max_results;
>             msg->cb = cb;
>             msg->closure = closure;
>     
the TODO line should be replaced with:

> > >             GList *l;
> > >             msg->requested_fields = g_list_copy (requested_fields);
> > >             for (l = msg->requested_fields; l; l = l->next)
> > >                 l->data = "" (l->data);
> > >             
> > >         
> >     
In the following:

>     @@ -1142,7 +1148,8 @@
>             e_book_msg_init ((EBookMsg*)msg, _get_contacts_handler, _get_contacts_dtor);
>      
>             msg->book = g_object_ref (book);
>     -       msg->query = e_book_query_from_string (query);
>     +       e_book_query_ref (query);
>     +       msg->query = query;
>             msg->cb = cb;
>             msg->closure = closure;

the change should be:

> > >             msg->query = e_book_query_ref (query);
> > >             
> > >         
> >     
otherwise things look ok.

Chris