GNOME Bugzilla – Bug 701260
Add asynchronous method to connect to the addressbook in DRA mode
Last modified: 2013-10-15 08:54:31 UTC
Created attachment 245633 [details] [review] Add e_book_client_connect_direct() Currently only e_book_client_connect_direct_sync() exists, this patch adds the asynchronous variant which was omitted in the 3.8 cycle (it was omitted because we were very close to code freeze when landing the Direct Read Access patches).
Created attachment 245634 [details] [review] Test Utils: Added argument to test opening the book asynchronously Previously all tests opened the book/calendar synchronously, even if there are many asynchronous API variants. This patch adds an argument to the ETestServerClosure structure to declare whether a given test should open the book / calendar asynchronously.
Created attachment 245635 [details] [review] EBookClient tests: Open the addressbook asynchronously for all async tests This patch makes all asynchronous addressbook tests connect/open the addressbook asynchronously. Consequently this thoroughly tests the new asynchronous variant to connect to the address book in Direct Read Access mode.
Created attachment 245636 [details] [review] ECalClient tests: Open the calendar asynchronously for all async tests Same for calendar tests
How to get a review for these patches?
Review of attachment 245633 [details] [review]: I'm wondering whether those runtime warnings do any sense when opening in DRA. It's legal to fail, right? Thus the warnings will be useless. An approach with added test function which will return in which mode respective client is opened would do better, even one loses the error information (which you do anyway). Feel free to fix the three nitpicks and commit to master as soon as it's branched for 3.10 (do not forget to update 3.10 in API docs to 3.12). ::: addressbook/libebook/e-book-client.c @@ +1324,3 @@ + g_object_ref (registry); + else { + registry = e_source_registry_new_sync (NULL, &local_error); you have cancellable here, then use it @@ +1333,3 @@ + local_error->message); + + g_error_free (local_error); it can sometimes fail on runtime checks, then count with local_error being NULL (both use g_clear_error() and in the message itself) @@ +1392,2 @@ g_clear_object (&priv->direct_backend); + g_error_free (local_error); similar here, count with local_error being NULL @@ +1515,3 @@ + client = g_object_new ( + E_TYPE_BOOK_CLIENT, + "source", source, NULL); err, this is when the new API makes things worse (not your, but the "connect" API with deprecated e_book_client_new())
Review of attachment 245634 [details] [review]: Looks fine, please commit after branch for 3.10 is done. ::: tests/test-server-utils/e-test-server-utils.c @@ +227,3 @@ case E_TEST_SERVER_DEPRECATED_ADDRESS_BOOK: + /* Dont bother testing the Async apis for deprecated APIs */ Don't :) @@ +264,3 @@ case E_TEST_SERVER_DEPRECATED_CALENDAR: + /* Dont bother testing the Async apis for deprecated APIs */ Don't :)
Review of attachment 245635 [details] [review]: Looks fine, please commit after branch for 3.10 is done.
Review of attachment 245636 [details] [review]: Looks fine, please commit after branch for 3.10 is done.
(In reply to comment #5) > Review of attachment 245633 [details] [review]: > > I'm wondering whether those runtime warnings do any sense when opening in DRA. > It's legal to fail, right? Thus the warnings will be useless. An approach with > added test function which will return in which mode respective client is opened > would do better, even one loses the error information (which you do anyway). > Feel free to fix the three nitpicks and commit to master as soon as it's > branched for 3.10 (do not forget to update 3.10 in API docs to 3.12). Good points, for now I just removed all of those warnings and the local_error variable in the `connect_direct()' function. If there is a need for a gboolean e_book_client_dra_enabled() function in the future then we could consider adding one then. Committed the attached patches as: commit b7746654ff611642e2542eed5431bc4a15ea766f Author: Tristan Van Berkom <tristanvb@openismus.com> Date: Wed Apr 24 17:48:42 2013 +0900 ECalClient tests: Use async apis to open the client for every async test. commit 727d83ca1a4b48274ec18bca59b19f149a2af4ef Author: Tristan Van Berkom <tristanvb@openismus.com> Date: Wed Apr 24 17:18:06 2013 +0900 EBookClient tests: Use async apis to open the client for every async test. commit 2c68c19c52d1fc9f8aec148fb2dffa6671b39ffc Author: Tristan Van Berkom <tristanvb@openismus.com> Date: Wed Apr 24 16:56:02 2013 +0900 test-utils: Added argumement to the fixture closure data to open async If a closure specifies async then the EBookClient or ECalClient will be opened using async APIs instead of sync APIs. commit e6d4a34c5b312aa35f04c18686d76bd91ff352fa Author: Tristan Van Berkom <tristanvb@openismus.com> Date: Wed Apr 24 16:16:57 2013 +0900 EBookClient: (bug 701260) Added async apis to connect to a DRA book. Added e_book_client_connect_direct() and e_book_client_connect_direct_finish()