After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 701260 - Add asynchronous method to connect to the addressbook in DRA mode
Add asynchronous method to connect to the addressbook in DRA mode
Status: RESOLVED FIXED
Product: evolution-data-server
Classification: Platform
Component: Contacts
3.10.x (obsolete)
Other Linux
: Normal normal
: ---
Assigned To: evolution-addressbook-maintainers
Evolution QA team
Depends on:
Blocks: 705178
 
 
Reported: 2013-05-30 10:35 UTC by Tristan Van Berkom
Modified: 2013-10-15 08:54 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add e_book_client_connect_direct() (10.10 KB, patch)
2013-05-30 10:35 UTC, Tristan Van Berkom
committed Details | Review
Test Utils: Added argument to test opening the book asynchronously (6.60 KB, patch)
2013-05-30 10:37 UTC, Tristan Van Berkom
committed Details | Review
EBookClient tests: Open the addressbook asynchronously for all async tests (23.07 KB, patch)
2013-05-30 10:40 UTC, Tristan Van Berkom
committed Details | Review
ECalClient tests: Open the calendar asynchronously for all async tests (14.25 KB, patch)
2013-05-30 10:40 UTC, Tristan Van Berkom
committed Details | Review

Description Tristan Van Berkom 2013-05-30 10:35:07 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).
Comment 1 Tristan Van Berkom 2013-05-30 10:37:36 UTC
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.
Comment 2 Tristan Van Berkom 2013-05-30 10:40:12 UTC
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.
Comment 3 Tristan Van Berkom 2013-05-30 10:40:54 UTC
Created attachment 245636 [details] [review]
ECalClient tests: Open the calendar asynchronously for all async tests

Same for calendar tests
Comment 4 André Klapper 2013-08-17 13:34:32 UTC
How to get a review for these patches?
Comment 5 Milan Crha 2013-09-13 08:22:19 UTC
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())
Comment 6 Milan Crha 2013-09-13 08:26:38 UTC
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 :)
Comment 7 Milan Crha 2013-09-13 08:28:39 UTC
Review of attachment 245635 [details] [review]:

Looks fine, please commit after branch for 3.10 is done.
Comment 8 Milan Crha 2013-09-13 08:31:06 UTC
Review of attachment 245636 [details] [review]:

Looks fine, please commit after branch for 3.10 is done.
Comment 9 Tristan Van Berkom 2013-10-14 18:58:50 UTC
(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()