On Wed, 2004-01-07 at 01:42, Chris Toshok wrote:
> 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.
What if the user deletes/renames On This Computer/Personal? Just return
NULL? Or the first addressbook in On This Computer?
> > 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).
Yeah, fair enough.
> 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 = g_strdup (l->data);
Done. I was going to do roughly that, but I wasn't sure if there was an
accepted idiom I didn't know about.
> 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);
Done.
> otherwise things look ok.
Fab. OK to commit the patches?
Ross
--
Ross Burton mail: ross burtonini com
jabber: ross burtonini com
www: http://www.burtonini.com./
PGP Fingerprint: 1A21 F5B0 D8D0 CFE3 81D4 E25A 2D09 E447 D0B4 33DF
Attachment:
signature.asc
Description: This is a digitally signed message part