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

[Patrick Ohly <patrick ohly gmx de>]

On Mo, 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.

First of all, +1 for rethinking the API. I'd like to suggest that
besides modernizing the API we also take this opportunity to move more
of EBook/ECal into a common core.

Opening and listing databases don't have to be separate. Turn
ECalClientSourceType into EClientSourceType and all of the _new,
_new_from_uri, _new_system, _new_default, _get_sources functions can be
moved into e-client.h.

The advantage would be that clients (like SyncEvolution) which work with
both only need to implement the common operations once. Right now I have
a lot of duplicate code in the address book and calendar backends.

Matthew suggested to replace some of these with
e_source_registry_get_default_calendar/address_book:
        
        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);

The same logic would apply here: instead of having separate calls for
calendar and address book, have one call with an enum.

BTW, in this particular example, wouldn't
e_source_registry_get_default_calendar() need such an enum anyway to
distinguish between default events, tasks and memos?

Matthew already made a similar comment about error codes and
"get_capabilities". I agree that these should be common, too. I don't
see why get_capabilities needs to be in ecal resp. ebook, except for the
convenience wrapper functions which query specific capabilities that
only apply to one or the other.

Talking of capabilities, I found their usefulness a bit limited because
they a) cannot change during the life time of a client and b) they only
report "exists/doesn't exit".

Their static nature IMHO only has one benefit, which is that they can be
safely cached on the client side. I don't see that as very useful,
because for those capabilities which are known to be static, the client
is likely to only query them once at startup and then set some internal
state accordingly. Even for that the API must be asynchronous because of
the underlying D-Bus call, as Matthew said.

What I'd prefer is a generic key/value mechanism, with value being a
string. I prefer a string because it is easy to handle and more complex
types (if ever needed) can be mapped to it on a case by case basis.

Setting values should also be allowed. That gives us a way how a client
can configure the storage to behave in certain ways. This can be
per-database (tweak the actual on-disk storage) or per EClient (modify
behavior as part of current session).

Future use cases for such a key/value mechanism:
      * "change tag", read-only - a string which changes each time the
        database changes (same as the CTag in CalDAV), would be used to
        make change detection in SyncEvolution more efficient [1]
      * "32 bit IDs" - check whether (read) or ensure that (write) IDs
        are 32 bit integers instead of strings, simplifies
        QtContacts-EDS [2]; I have a patch which reduces the size of IDs
        from 64 bit to 32 in the file backend, more on that in a
        separate email

There can also be convenience functions for this, if they are considered
important enough. Only adding such functions and not the generic API
would have the downside that code cannot be compiled against an old API
implementation and still use the new features if they happen to be
available at runtime.

At the D-Bus level this could be mapped to properties.

[1] http://wiki.meego.com/Architecture/planning/evolution-data-server/eds-improvements#quick_check_for_.22data_changed.22
[2] http://lists.meego.com/pipermail/meego-dev/2011-April/482731.html

-- 
Bye, Patrick Ohly
--  
Patrick Ohly gmx de
http://www.estamos.de/