Re: [Evolution-hackers] Contacts database modifications history

[Milan Crha <mcrha redhat com>]

On Fri, 2014-04-25 at 12:00 +0000, Potrola, MateuszX wrote:
> What do you think about this proposal ?

        Hi,
thanks for it, but please open a bug report (and send a link to it
here), where we'll do all the review work. It will take couple
iterations. From a brief look into the patch:
a) license blocks are not correct in the new files, please use those
   from the current git master (the cleanup took me several hours)
b) the new "..._non_locking" functions, following GAsyncQueue API
   pattern, the suffix should better be "..._unlocked"
c) add developer comments to the new "private" functions and define them
   in the header - it'll save you many headaches in the future
d) there are some coding style issues, like (there are more similar)
   at the top of e-book-sqlite.h:
   - removed empty lines, which should stay there
   - the added #include "e-book-sqlite-changelog.h" should be
     done similar to the #include above it, thus it'll work
     even outside of eds, but:
e) is the EBookSqliteChangelog needed at all? What I had on mind were
   added signals in the EBookSqlite, which would be called on places
   where you currently engage the EBookSqliteChangelog functions. That
   way, with the signals, you do not need to know anything about
   the caller, neither specifically list the changelog from the
   extensions list - it is currently odd too, because if there are two
   changelog extensions then the way you have it right now will engage
   only one, hard to tell which of them, though
f) I would add the ESource argument also to e_book_sqlite_new() - as it
   will change API, you need to bump soname version for libedatabook.
   You need to bump it anyway, for the change of e_book_sqlite_new_full().
g) I do not like the changes in e_book_sqlite_class_init(), because:
   - there is a possible memory leak of modules_directory
   - load of extensions should be done transparently, supposing you
     save your extension to the proper modules' directory, which is
     $PREFIX/lib/evolution-data-server/addressbook-backends
     Hrm, pity it's named 'backends', but it belongs to the addressbook
     factory, thus it's it. What you should do in the EBookSqlite is to
     simply call e_extensible_load_extensions() in GObject::constructed
     override in EBookSqlite, after the parent struct 'constructed' is
     called.
h) I did not get from the changes how you'll access the sqlite3 database
   for writing. All the added "private" functions are for reading - they
   seem to be to me at least.

In any case, as I said above, let's continue in the bug report.
        Bye,
        Milan