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

[Milan Crha <mcrha redhat com>]

On Mon, 2011-04-18 at 12:48 -0400, Matthew Barnes wrote:
> * 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.
>
> ...

	Hi,
I do not think dropping all of them is any of gain, just the opposite.
Why to write same code, even 4-lined, all around instead of call one
simple function? Notice that all these functions are hiding where the
sources' configuration is stored. (4-lined is because you may get
somewhere the "registry" pointer and free it afterwards, instead of your
simplified 2-lined sample). There are also simplified functions for
freeing GSList-s and their content, which is mostly two-lined code too.

> * 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:

Well, from my point of view, the error also tells you the place of
origin, and the origin for these is not a GIO at all.

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

Ah, right, since returning GError instead of error status/code only it
doesn't make sense to be there anymore.

> ...
>     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.

It's used by Groupwise exclusively.

> * 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.

Good idea, I didn't think of it in this way, also because the codes are
different when passed through D-Bus, but it can be done on the client
side for sure.

> * 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.

Yeah, my other goal was to harmonize internal API to consistently use
one of these types, and I chose GSList, because it's simpler and smaller
than GList, and because the usual case is to traverse the list in one
direction and not both, thus the GSList is sufficient. If there will be
more voices for this, then I can make it GList.

> * 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?

Hmm, maybe it isn't. Consider it as debugging function :)

> * 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.

Yet another thing I wanted to address was a clear memory management
recongnizable from function prototype. Thus if the function doesn't
change object's/variable's content, then I made it 'const' to indicate
it's used for reading only. Of course, inconsistencies all around the
code in this makes it hard to do right, so the function type-casts it
back to non-const pointer internally, because EContact API is not
written in that way. (Though it's not only about EContact.)

> * 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?

They are same capabilities. Same as ECal/EBook the EClient caches
capabilities once it's asked for them, and reuses them, same as is
responsible for its memory. The cached values are also used for
capability checking. These are fetched on demand, and are always
synchronous. For cases where this is unusable, and where the caller will
take care of the memory, I added also get_capabilities functions to
ECalClient/EBookClient, which has both sync and async versions.

	Bye,
	Milan