Merge lp:~vanvugt/mir/super-simple-connect into lp:~mir-team/mir/trunk

[Daniel van Vugt]

Prototype a super-simple synchronous wrapper for mir_connect().

Other client API functions will be converted pending agreement on the general approach.

But for the record, I don't like this proposal. I prefer:
  lp:~vanvugt/mir/optional-callbacks
but have been told this approach here is more likely to get approved.

[Daniel van Vugt (vanvugt)]

I suspect someone will complain about reinterpret_cast. But consider the alternative with N callbacks (one for each API function) and each having to contain its own:
   *(static_cast<MirSomething**>(context)) = something;

I prefer the reinterpret_cast.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:500
http://jenkins.qa.ubuntu.com/job/mir-ci/80/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/81/console
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/81/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/mir-ci/80/rebuild

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

This is pretty much what I was thinking of doing - although I was was going to prefix with "s" not suffix with "_sync", and wasn't going to use reinterpret_cast.

[Alan Griffiths (alan-griffiths)]

> This is pretty much what I was thinking of doing - although I was was going to
> prefix with "s" not suffix with "_sync", and wasn't going to use
> reinterpret_cast.

Oh, and I was going to use a separate header.

[Kevin DuBois (kdub)]

i like it. slight preference for suffixed names (as it is), no reinterpret, and a separate header

[Daniel van Vugt (vanvugt)]

It's true that making up new words like "sconnect" is one way to avoid ambiguity. But it steepens the learning curve. Another possible convention is "mir_connect_now", which is a clear message to the reader that "mir_connect" might not connect /right now/.

Separate headers? I disagree. This function relates directly to the existing contents of mir_client_library.h. Separate headers would also steepen the learning curve and it would be generally bad design to have a "connect" function in a different header to its requisite disconnect function.

[Alan Griffiths (alan-griffiths)]

> It's true that making up new words like "sconnect" is one way to avoid
> ambiguity. But it steepens the learning curve. Another possible convention is
> "mir_connect_now", which is a clear message to the reader that "mir_connect"
> might not connect /right now/.
>
> Separate headers? I disagree. This function relates directly to the existing
> contents of mir_client_library.h. Separate headers would also steepen the
> learning curve and it would be generally bad design to have a "connect"
> function in a different header to its requisite disconnect function.

To clarify: I'm not blocking your approach. Just that I would have defined smir_connect(..) and smir_disconnect(..) and the rest of a synchronous API in a separate and independent header.

[Robert Ancell (robert-ancell)]

Looks good to me. I think *_sync is better than s* (it's more clear what "sync" means over "s") and don't see a need to use another header file.

review: Approve

[Chris Halse Rogers (raof)]

Another +1 for *_sync vs s*.

I'm not averse to having a separate, synchronous, header, but I think that as long as we maintain the convention of having _sync stuck on the end of anything that can block it's not particularly necessary.

If you call mir_connect_sync, it's pretty clear you don't want asynchronous behaviour, so I don't think we need to segregate the sync calls out for client's safety.

review: Approve

=== modified file 'include/client/mir_toolkit/mir_client_library.h'
--- include/client/mir_toolkit/mir_client_library.h	2013-03-07 08:04:05 +0000
+++ include/client/mir_toolkit/mir_client_library.h	2013-03-14 09:36:34 +0000
@@ -161,6 +161,14 @@
     void *client_context);
 
 /**
+ * Perform a mir_connect() but also wait for and return the result.
+ *   \param [in] server    A name identifying the server
+ *   \param [in] app_name  A name referring to the application
+ *   \return               The resulting MirConnection.
+ */
+MirConnection *mir_connect_sync(char const *server, char const *app_name);
+
+/**
  * Test for a valid connection
  * \param [in] connection  the connection
  * \return                 a non-zero value if the supplied connection is

=== modified file 'src/client/mir_client_library.cpp'
--- src/client/mir_client_library.cpp	2013-03-07 08:04:05 +0000
+++ src/client/mir_client_library.cpp	2013-03-14 09:36:34 +0000
@@ -42,6 +42,13 @@
 namespace
 {
 mir_toolkit::MirConnection error_connection;
+
+// assign_result is compatible with all 2-parameter callbacks
+void assign_result(void *result, void **context)
+{
+    *context = result;
+}
+
 }
 
 mir_toolkit::MirWaitHandle* mir_toolkit::mir_connect(char const* socket_file, char const* name, mir_connected_callback callback, void * context)
@@ -67,6 +74,17 @@
     }
 }
 
+MirConnection *mir_toolkit::mir_connect_sync(char const *server,
+                                             char const *app_name)
+{
+    MirConnection *conn = nullptr;
+    mir_wait_for(mir_connect(server, app_name,
+                             reinterpret_cast<mir_connected_callback>
+                                             (assign_result),
+                             &conn));
+    return conn;
+}
+
 int mir_toolkit::mir_connection_is_valid(MirConnection * connection)
 {
     return MirConnection::is_valid(connection);

=== modified file 'tests/acceptance-tests/test_client_library.cpp'
--- tests/acceptance-tests/test_client_library.cpp	2013-03-07 08:04:05 +0000
+++ tests/acceptance-tests/test_client_library.cpp	2013-03-14 09:36:34 +0000
@@ -122,6 +122,27 @@
     launch_client_process(client_config);
 }
 
+TEST_F(DefaultDisplayServerTestFixture, synchronous_connection)
+{
+    struct ClientConfig : ClientConfigCommon
+    {
+        void exec()
+        {
+            connection = NULL;
+            connection = mir_connect_sync(mir_test_socket,
+                                          __PRETTY_FUNCTION__);
+
+            ASSERT_TRUE(connection);
+            EXPECT_TRUE(mir_connection_is_valid(connection));
+            EXPECT_STREQ(mir_connection_get_error_message(connection), "");
+
+            mir_connection_release(connection);
+        }
+    } client_config;
+
+    launch_client_process(client_config);
+}
+
 TEST_F(DefaultDisplayServerTestFixture, client_library_creates_surface)
 {
     struct ClientConfig : ClientConfigCommon