Re: [Evolution-hackers] New 'eclient' branch in eds

[Matthew Barnes <mbarnes redhat com>]

On Mon, 2011-04-18 at 15:57 +0200, Milan Crha wrote: 
> I just created a new branch 'eclient' in eds [1] where is added my work
> on the new API which will deprecate EBook/ECal. It is following glib/gio
> async pattern and, I believe, makes things more coherent.

Thanks for posting this, Milan.  I want to emphasize how important these
new APIs are and ask everyone to look it over and provide comments.

I had a sneak peek at this over the weekend and jotted some notes.  So
far I've only reviewed in detail the client-side headers because that's
what I'm most concerned about getting right.  The rest of it can be
tweaked and changed as needed -- even the backend APIs are never truly
frozen.  But the client-side APIs *will* be frozen, since that's what
other projects will be migrating to and attempting to use.

The new client-side APIs are:

EClient (abstract base class) 
http://git.gnome.org/browse/evolution-data-server/tree/libedataserver/e-client.h?h=eclient

ECalClient (replaces ECal)
http://git.gnome.org/browse/evolution-data-server/tree/calendar/libecal/e-cal-client.h?h=eclient

EBookClient (replaces EBook)
http://git.gnome.org/browse/evolution-data-server/tree/addressbook/libebook/e-book-client.h?h=eclient

ECredentials (authenication)
http://git.gnome.org/browse/evolution-data-server/tree/libedataserver/e-credentials.h?h=eclient


I'll split my comments into two posts so this doesn't get too
overwhelming.  Simple, hopefully non-controversial stuff here and
meatier topics in a separate post.  Overall I'm pretty happy with
the APIs.


* There's some overlap between the new EClient API and the new
  ESource API that I'm working on.  Some functions will need to be
  dropped once the new ESource API is in place, so I don't know if
  you want to do this now or wait.

  ESourceList is being removed so obviously any functions involving
  ESourceList will be dropped:

    e_client_util_get_system_source()
    e_client_util_set_default()
    e_client_util_get_source_for_uri()
    e_cal_client_get_sources()
    e_book_client_get_sources()

  We will no longer refer ESources using URIs.  We will only refer
  to ESources by their unique ID string.  So the following functions
  will be dropped:

    e_client_get_uri()
    e_cal_client_new_from_uri()
    e_book_client_new_from_uri()

  Default sources will be tracked using the new ESourceRegistry API,
  so the following functions will be dropped:

    e_cal_client_set_default()
    e_cal_client_set_default_source()
    e_book_client_set_default()
    e_book_client_set_default_source()

  There's a few functions that I think are too trivial to keep in light
  of the lookup capabilities of ESourceRegistry.  I'd like to keep the
  EClient APIs as slim as possible initially and drop these functions:

    e_cal_client_new_system()

      Instead do:
    
      source = e_source_registry_lookup_by_uid (registry, "system");
      client = e_cal_client_new (source, source_type, error);

    e_cal_client_new_default()

      Instead do:

      source = e_source_registry_get_default_calendar (registry); 
      client = e_cal_client_new (source, E_CAL_SOURCE_TYPE_EVENT, error);

      (similar functions exist for tasks and memos)

    e_book_client_new_system_addressbook()

      Instead do:

      source = e_source_registry_lookup_by_uid (registry, "system");
      client = e_book_client_new (source, error);

    e_book_client_new_default_addressbook()

      Instead do:

      source = e_source_registry_get_default_address_book (registry);
      client = e_book_client_new (source, error);


* We should use GIO error codes whenever possible, and I see quite a bit
  of overlap here.  I think following error codes should be dropped:

    E_CAL_CLIENT_ERROR_SUCCESS
    E_BOOK_CLIENT_ERROR_SUCCESS

      There's no need for an error code for successful operations.

    E_CAL_CLIENT_ERROR_INVALID_ARG
    E_BOOK_CLIENT_ERROR_INVALID_ARG

      Use G_IO_ERROR_INVALID_ARGUMENT.

    E_CAL_CLIENT_ERROR_BUSY
    E_BOOK_CLIENT_ERROR_BUSY

      Use G_IO_ERROR_BUSY.

    E_CAL_CLIENT_ERROR_PERMISSION_DENIED
    E_BOOK_CLIENT_ERROR_PERMISSION_DENIED

      Use G_IO_ERROR_PERMISSION_DENIED.

    E_CAL_CLIENT_ERROR_NOT_SUPPORTED
    E_CAL_CLIENT_ERROR_PROTOCOL_NOT_SUPPORTED
    E_BOOK_CLIENT_ERROR_NOT_SUPPORTED
    E_BOOK_CLIENT_ERROR_PROTOCOL_NOT_SUPPORTED

      Use G_IO_ERROR_NOT_SUPPORTED.

    E_CAL_CLIENT_ERROR_CANCELLED
    E_BOOK_CLIENT_ERROR_CANCELLED

      Use G_IO_ERROR_CANCELLED.

    E_CAL_CLIENT_ERROR_INVALID_SERVER_VERSION
    E_BOOK_CLIENT_ERROR_INVALID_SERVER_VERSION

      I don't think these are necessary anymore since we started
      using versioned bus names for the addressbook and calendar
      services.  If there's a client/server version mismatch,
      the client will get a D-Bus error about it.

    E_CAL_CLIENT_ERROR_DBUS_ERROR
    E_BOOK_CLIENT_ERROR_DBUS_ERROR

      I expect GDBus will handle D-Bus errors and hand us back a
      G_IO_ERROR_DBUS_ERROR.

    E_BOOK_CLIENT_ERROR_NO_SPACE

      Use G_IO_ERROR_NO_SPACE.

    E_CAL_CLIENT_ERROR_OTHER_ERROR
    E_BOOK_CLIENT_ERROR_OTHER_ERROR

      Use G_IO_ERROR_FAILED.


* Of the remaining error codes, a number of them in ECalClient and
  EBookClient can be combined and moved to EClient.  Especially ones
  related to offline and authentication, since that's what EClient
  handles.


* I would really prefer we use GList instead of GSList throughout the
  API.  GSList is silly and really should never have been added to GLib,
  in my opinion.  Most modern GNOME APIs that I've seen prefer GList,
  and it's a pain in the butt having to convert between the two.


* I thought backends could send their own custom error messages now, so
  are e_cal_client_error_to_string() and e_book_client_error_to_string()
  still necessary?


* In EBookClient, drop the 'const' qualifier from EContact arguments.
  Trying to use 'const' with GObjects is misguided and pointless.  I've
  cursed at EIterator many times for this.


* Why do we have "get_capabilities" functions in EClient, ECalClient and
  EBookClient?  Are they different sets of capabilities?  And if getting
  capabilities involves a D-Bus call then doesn't it need to be async
  everywhere?


That's all I have for now.  I'll save the heavier stuff for a separate
post.