GNOME Bugzilla – Bug 729251
EBookSqlite: Allow record detailed changes made to contacts
Last modified: 2014-05-09 09:28:18 UTC
Created attachment 275475 [details] [review] Proposed changes for enabling additional modules in EBookSqlite (based on openismus-work-3.8 branch) EBookSqlite should support additional modules that will be activated upon modifications to contacts database and could record detailed history of changes made to contacts
(In reply to comment #0) > (based on openismus-work-3.8 branch) Hmm, right, I cannot test it locally due to this fact - many chunks just fail on git master. Would it make sense to develop this on master first, and then backport to the openismus-work-3.8 branch? I see the openism branch has some sever issues from backport already, like some important compiler warnings, then also it's not buildable due to missing LC_COLLATE, so it kind of makes sense that the work will be developed on master primarily, thus there will not be caused any issue by "up-port" (not backport, when porting the code to the newer version). I can do a read-review at least. By the way, the warnings and errors I get: e-data-book.c: In function 'e_data_book_set_locale': e-data-book.c:2559:3: warning: passing argument 1 of 'g_dbus_interface_skeleton_flush' from incompatible pointer type [enabled by default] g_dbus_interface_skeleton_flush (book->priv->dbus_interface); ^ In file included from /usr/include/glib-2.0/gio/gio.h:149:0, from e-data-book.c:26: /usr/include/glib-2.0/gio/gdbusinterfaceskeleton.h:104:6: note: expected 'struct GDBusInterfaceSkeleton *' but argument is of type 'struct EDBusAddressBook *' void g_dbus_interface_skeleton_flush (GDBusInterfaceSkeleton *interface_); ^ e-data-book-factory.c: In function 'data_book_factory_list_books': e-data-book-factory.c:132:10: warning: assignment from incompatible pointer type [enabled by default] for (l = books; l; l = l->next) { ^ e-data-book-factory.c:134:4: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement] EDataBook *insert_book = g_object_ref(book); ^ e-data-book-factory.c: In function 'data_book_factory_interpret_locale': e-data-book-factory.c:455:3: error: implicit declaration of function 'setlocale' [-Werror=implicit-function-declaration] const gchar *system_locale = setlocale (LC_COLLATE, NULL); ^ e-data-book-factory.c:455:3: warning: nested extern declaration of 'setlocale' [-Wnested-externs] e-data-book-factory.c:455:43: error: 'LC_COLLATE' undeclared (first use in this function) const gchar *system_locale = setlocale (LC_COLLATE, NULL); ^ e-data-book-factory.c:455:43: note: each undeclared identifier is reported only once for each function it appears in cc1: some warnings being treated as errors
> Would it make sense to develop this on master first, and then > backport to the openismus-work-3.8 branch? Sure, I can develop this on master and then I will backport
Review of attachment 275475 [details] [review]: Apart of the few below things, which are pretty minor, the change looks fine. If you provide a version for master, thus I'll be able to see it in action too, then I believe you'll be pretty much done. ::: addressbook/libedata-book/Makefile.am @@ +5,3 @@ lib_LTLIBRARIES = libedata-book-1.2.la + No need for the extra empty line here. @@ +27,2 @@ libedata_book_1_2_la_SOURCES = \ + $(MARSHAL_GENERATED) \ the same should go to BUILT_SOURCES (not there, to be added, search for it in other Makefile.am files). ::: addressbook/libedata-book/e-book-sqlite.c @@ +6069,3 @@ + if (!handler_result) + return FALSE; + return TRUE; why not return handler_result directly? @@ +6089,3 @@ + signals[BEFORE_INSERT_CONTACT] = g_signal_new ( + "before-insert-contact", + G_OBJECT_CLASS_TYPE (object_class), there might be used 'class' instead of 'object_class' here (and few lines lower) @@ +6599,3 @@ + &success); + + if(!success) coding style: missing space after 'if' @@ +6710,3 @@ + for (l = uids; success && l; l = l->next) { + contact_uid = (const gchar*) l->data; coding style: missing space before '*' @@ +6836,2 @@ /** + * e_book_sqlite_get_contact_unlocked: the name here doesn't match the actual name being used in the code; these should be also added to @@ +6845,3 @@ + * + * If @meta_contact is specified, then a shallow #EContact will be created + * holing only the %E_CONTACT_UID and %E_CONTACT_REV fields. typo: holing @@ +6849,3 @@ + * Returns: %TRUE on success, otherwise %FALSE is returned and @error is set appropriately. + * + * Since: 3.13 The next stable version is usually used, which is 3.14 right now; Some of these three apply to bottom comments as well @@ +6998,3 @@ + search_data->vcard = NULL; + + g_slist_free_full (vcards, (GDestroyNotify)e_book_sqlite_search_data_free); coding style: missing space before the '....free' function name
Created attachment 275879 [details] Proposed changes for enabling additional modules in EBookSqlite (based on master branch) I've made two small changes (except fixes from previous review) - I'm using g_cclosure_marshal_generic (it seems to be used everywhere on master) and also I removed additional check for succes in e_book_sqlite_remove_contacts, as next loop is checking it
Created attachment 275880 [details] [review] Proposed changes for enabling additional modules in EBookSqlite (based on master branch)
Review of attachment 275880 [details] [review]: Looks pretty good, I also noticed the usage of g_cclosure_marshal_generic(), when a NULL is used for that argument, but I didn't have a strong opinion on it, thus I didn't claim anything. The code is simpler as you made it right now, good job. There is one issue unfortunately (see the next comment). ::: addressbook/libedata-book/e-book-sqlite.c @@ +2810,3 @@ ebsql->priv->user_data = user_data; ebsql->priv->user_data_destroy = user_data_destroy; + ebsql->priv->source = source; I'm wondering whether it would make sense to reference the source, for thread safety, if not NULL. @@ +6412,3 @@ +*/ +ESource * +e_book_sqlite_get_source (EBookSqlite *ebsql) similar use e_book_sqlite_ref_source(), for thread safety? @@ +6511,3 @@ extra_data = (const gchar *) ll->data; + g_signal_emit_by_name (ebsql, "before-insert-contact", do not emit by name, it's better/quicker to use the signal ID and so on @@ +6635,3 @@ + contact_uid, + cancellable, error, + &success); similar here: a) not by-name b) indent parameters one-more level to the right ::: addressbook/libedata-book/e-book-sqlite.h @@ +293,1 @@ void e_book_sqlite_search_data_free (EbSqlSearchData *data); As you bumped soname version, you can safely add the new signals prototypes into the 'class' structure, thus it's more obvious what parameters are provided with the signal.
I compiled the patch and run evolution against it, and when I was removing a contact in a local book, evolution-addressbook-factory crashed: 1469 /* Remove from summary as well */ 1470 if (!e_book_sqlite_remove_contacts (bf->priv->sqlitedb, removed_ids, 1471 cancellable, &local_error)) { 1472 g_warning ("Failed to remove contacts: %s", local_error->message); 1473 g_propagate_error (error, local_error); 1474 }
+ Trace 233569
Created attachment 276078 [details] [review] Proposed changes for enabling additional modules in EBookSqlite (based on master branch) Added default signal handlers (returning always TRUE), so in case where no plugin is provided there will be no crash (like in previous patch)
Thanks for the update. I did some minor tiny changes in the patch and committed it to master for you. Created commit a0d905d in eds master (3.13.2+) [1] [1] https://git.gnome.org/browse/evolution-data-server/commit/?id=a0d905d