Merge lp:~vanvugt/mir/wait-result into lp:mir

[Daniel van Vugt]

Add a new client function: mir_wait_for_result() which returns a bool indicating success or failure of a request.

This is needed for use with client functions that change server state but can't read back results (or simply don't care for anything more than a bool). The need for this is limited right now, but it's coming in the near future, I promise.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2388
http://jenkins.qa.ubuntu.com/job/mir-ci/3176/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1579
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1578
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1533
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1173
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1173/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1533
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1533/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4549
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/18691

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/3176/rebuild

review: Approve (continuous-integration)

[Chris Halse Rogers (raof)]

On first inspection this looks unnecessary and ugly to me; maybe once I see the use you intend to put it to it'll appear more reasonable?

review: Needs Information

[Daniel van Vugt (vanvugt)]

It's necessary for the reasons outlined in the description. And it avoids the need for adding new callback types, which would be very ugly.

[Chris Halse Rogers (raof)]

Yes, but could you give an example of such a function?

The closest I can think of is along the lines of mir_surface_set_state, but you can't reasonably just wait on the boolean result - the shell could respond by changing your state but not to the exact state you asked for. Indeed, the shell can change your state at any point without you calling mir_surface_set_state, so if you need to react to state changes you already need to be listening to surface events.

[Daniel van Vugt (vanvugt)]

Oh good point. You can find the answer buried deep in the diff but I will explain:

   mir_surface_set_state(surface, valid_supported_state) == true
   mir_surface_set_state(surface, invalid_state) == false
   mir_surface_set_state(surface, valid_unsupported_state) == false

The last case is the curious one. So I'm saying if result == true then the shell has successfully set exactly what you asked for.

If result == false, then the shell hasn't done what you asked for, but it might have done something different (e.g. chosen a similar state it supports instead of what you asked for).

So maybe the attribute/state result needs a rethink. But mir_surface_set_state isn't the important case here. The important case for this proposal is multi-attribute morphing in future:

   mir_surface_morph(surface, new_spec) == result

only returns true if ALL attributes of new_spec could be applied and supported together. Else result==false and nothing is changed. Totally clear and unambiguous in that case. And the behaviour of such functions should be documented clearly in the client headers.

[Chris Halse Rogers (raof)]

Ah, right.

My feeling is that this use in particular could be better done on the callback-side by providing an aggregate surface event with all the relevant new state at once. Morph then becomes a set_state-like void call.

This also cleans up server-driven changes somewhat - for example, we can send the client can an event containing both state_fullscreen and the new surface size.

Presumably the server will be sending the updated state anyway? The client can't actually rely on the values matching new_spec even after mir_wait_for_result(mir_surface_morph(surface, new_spec)) returns true, because the server may have already sent - and the client event thread already processed - new attributes.

My feeling is that any client code that responds to attributes that isn't being directly driven from a surface event callback is fundamentally incorrect, and we shouldn't make it appear as though you can safely do anything else.

[Chris Halse Rogers (raof)]

Ah, right.

My feeling is that this use in particular could be better done on the callback-side by providing an aggregate surface event with all the relevant new state at once. Morph then becomes a set_state-like void call.

This also cleans up server-driven changes somewhat - for example, we can send the client can an event containing both state_fullscreen and the new surface size.

Presumably the server will be sending the updated state anyway? The client can't actually rely on the values matching new_spec even after mir_wait_for_result(mir_surface_morph(surface, new_spec)) returns true, because the server may have already sent - and the client event thread already processed - new attributes.

My feeling is that any client code that responds to attributes that isn't being directly driven from a surface event callback is fundamentally incorrect, and we shouldn't make it appear as though you can safely do anything else.

[Chris Halse Rogers (raof)]

Ah, right.

My feeling is that this use in particular could be better done on the callback-side by providing an aggregate surface event with all the relevant new state at once. Morph then becomes a set_state-like void call.

This also cleans up server-driven changes somewhat - for example, we can send the client can an event containing both state_fullscreen and the new surface size.

Presumably the server will be sending the updated state anyway? The client can't actually rely on the values matching new_spec even after mir_wait_for_result(mir_surface_morph(surface, new_spec)) returns true, because the server may have already sent - and the client event thread already processed - new attributes.

My feeling is that any client code that responds to attributes that isn't being directly driven from a surface event callback is fundamentally incorrect, and we shouldn't make it appear as though you can safely do anything else.

[Alan Griffiths (alan-griffiths)]

> Ah, right.
>
> My feeling is that this use in particular could be better done on the
> callback-side by providing an aggregate surface event with all the relevant
> new state at once. Morph then becomes a set_state-like void call.
>
> This also cleans up server-driven changes somewhat - for example, we can send
> the client can an event containing both state_fullscreen and the new surface
> size.
>
> Presumably the server will be sending the updated state anyway? The client
> can't actually rely on the values matching new_spec even after
> mir_wait_for_result(mir_surface_morph(surface, new_spec)) returns true,
> because the server may have already sent - and the client event thread already
> processed - new attributes.
>
> My feeling is that any client code that responds to attributes that isn't
> being directly driven from a surface event callback is fundamentally
> incorrect, and we shouldn't make it appear as though you can safely do
> anything else.

+1

review: Disapprove

[Daniel van Vugt (vanvugt)]

Well, I don't need this right now.

But in the near future when you can't tell if client functions succeeded or not, we'll need something. Preferably not callbacks, because they're so awkward to use. Maybe events, but still they're much more awkward for simple clients to use than what's presented here.

I really like this approach. In theory it allows us to completely abolish callbacks in future as you could do something like:

   wait = mir_create_surface();
   ...
   ... other things ...
   ...
   surf = mir_wait_for_a_surface(wait);

[Chris Halse Rogers (raof)]

You can't completely abolish callbacks without making asynchronously waiting on a result painful, but, yes, I do like the continuation/AsyncContext wait API I proposed (and that Windows¹ and GLib² use, among others) for our asynchronous API some years ago :).

I don't see how this is a step in that direction, though, and I think the specific uses in this MP make the API harder to use - in the case of waiting for a construction it's a duplication of foo_is_valid() without the ability to query the error condition, and in the case of waiting for set_foo() it's promoting an incorrect interaction with the client API.

I'm fully supportive of reworking our MirWaitHandle API to be less cumbersome, but that probably merits a ML discussion.

¹: Via ctx = FooBegin/result = FooEnd(ctx) pairs
²: Via GAsyncResult

review: Disapprove

[Daniel van Vugt (vanvugt)]

1. The assertion at the start of your comment is incorrect. As demonstrated by this branch. A single function call is obviously less "painful" than writing callbacks. Admittedly this counts as opinion, but I think you'd probably find 99% of developers agree that a single function call that avoids the need for callbacks is preferable.

2. "in the case of waiting for a construction it's a duplication of foo_is_valid()". Yep foo_is_valid would become unnecessary, which is a good thing. That's one less function that clients need to bother calling.

3. "without the ability to query the error condition". Incorrect. I already documented future plans for that:
376 + // Suggested future uses:
377 + // const char* error;

For example (bug 1431190):
   int err = mir_wait_for_error(wait_handle);
   fprintf(stderr, "Thingy failed due to: %s\n", strerror(err));
or (without fixing bug 1431190):
   const char *err = mir_wait_for_error(wait_handle);
   fprintf(stderr, "Thingy failed due to: %s\n", err);

Another value in the approach here is that it solves the thread safety problem of querying errors we have today (mir_X_get_error_message). If you have multiple threads operating on a single Mir object then our current API will leak incorrect error values between threads. The approach proposed here will not.

[Chris Halse Rogers (raof)]

I'm confused. How does your branch allow asynchronously waiting on a result? mir_wait_for_result blocks until the result is available; doing something with that is either synchronous or requires spinning up a thread to block.

Look, I'm all for replacing MirWaitHandle with MirAsyncResult and all the relevant things. I think that warrants a mailing list discussion, though, and I don't think that this branch gets us closer to MirAsyncResult.

[Daniel van Vugt (vanvugt)]

I don't understand what's unclear. It allows asynchronously waiting as the client can choose to do other things before it gets around to calling any mir_wait_for* function. Just as the existing mir_wait_* functions allow clients to be asynchronous.

I totally agree some WaitForMultipleObjects (win32, as desrt has played with recently) would be better. But that's a potentially a whole new API and I'm not sure if or when Mir will get around to growing such functionality. It's also not mutually exclusive... such an API could live side-by-side with our existing mir_wait_for_ functions. For example:
   MirWaitHandle handles[5];
   mir_wait_for_any(handles, 5);
   mir_wait_for_all(handles, 5);

So there's no reason why such future functionality should invalidate the work proposed here.

[Alan Griffiths (alan-griffiths)]

I don't see anything in the recent discussion that relates to the pointlessness of being able to wait for a boolean result that tells the client nothing useful. In fact, I see little point in being able to wait on these requests to the server.

> The last case is the curious one. So I'm saying if result == true then the shell has successfully
> set exactly what you asked for.

But even if we were to make a guarantee that the corresponding state change event has been processed by the client before the wait completes with true there can be no guarantee that the state hasn't changed again.

So whether the result is true or false the boolean tells nothing about the current state.

review: Disapprove

[Chris Halse Rogers (raof)]

On Mon, Mar 16, 2015 at 6:34 PM, Daniel van Vugt
<email address hidden> wrote:
> I don't understand what's unclear. It allows asynchronously waiting
> as the client can choose to do other things before it gets around to
> calling any mir_wait_for* function. Just as the existing mir_wait_*
> functions allow clients to be asynchronous.

Aaah! This is where my misunderstanding is; we have different
definitions of “asynchronously waiting”.

When I say “asynchronously waiting” I mean some mechanism by which
the client can be *notified* when the thing has happened, and can do
arbitrary processing in the meantime. This basically requires
callbacks, or equivalent language features (async/await for C# & Vala,
for example).

I don't think that “the client can choose to do other things before
blocking its thread” is valuable. It's insufficient for toolkits and
not much better than *_sync for the few simple clients using the client
API directly.

Additionally, I'm with Alan - having mir_surface_set_* return a wait
handle is a mistake that we should correct by removing the wait handle
(and possibly providing a “the server is not going to honour that
request, sorry” mechanism), not by providing a boolean return value
that is impossible to use correctly.

[Daniel van Vugt (vanvugt)]

The value of this branch is more clear in the surface morphing prototype:
https://code.launchpad.net/~vanvugt/mir/rename-api/+merge/252864

It's not "pointless". It is /probably/ required that the client be told if their multi-attribute modification has happened or not. In the latter "not" case there is no event to wait for. So you need this wait-for-result, or something similar like wait-for-error-code-or-success.

In the mean time, it hurts nobody to not land anything in this area. Just that we'll soon have a situation where some features of the client API can't return errors to the client. Which might turn out to be acceptable...

[Chris Halse Rogers (raof)]

On Tue, Mar 17, 2015 at 3:07 PM, Daniel van Vugt
<email address hidden> wrote:
> The value of this branch is more clear in the surface morphing
> prototype:
> https://code.launchpad.net/~vanvugt/mir/rename-api/+merge/252864
>
> It's not "pointless". It is /probably/ required that the client be
> told if their multi-attribute modification has happened or not. In
> the latter "not" case there is no event to wait for. So you need this
> wait-for-result, or something similar like
> wait-for-error-code-or-success.

I agree that we need something; I think that the something has to be an
event, because it has to be useful to toolkits for which blocking their
thread is unacceptable.

GTK isn't going to use wait_for_error_code_or_success unless we can
provide a guarantee that it doesn't block, so we'll either need an
event or to turn our wait-handles into something more continuationy.

[Daniel van Vugt (vanvugt)]

Come to think of it, needs more opinions still.

I get what you're saying but you're making the assumption that some generic "event" (in win32 known as a HANDLE) can't be implemented under the name "MirWaitHandle". I think it could be, transparently. I know this because I've done the cross-platform wait handle API exercise before. In fact it's quite easy with the internal structure of MirWaitHandle right now.

On that theme, it would be easy to implement the non-blocking "peek" functionality you're talking about too.

Given our client types are all opaque, it sounds like you're just blocking because you don't like the name "MirWaitHandle". And I kind of agree, but that's a pre-existing issue and it's not a major penalty to live with that name.

[Chris Halse Rogers (raof)]

Oh, indeed. MirWaitHandle *could* grow a mir_wait_handle_set_callback()
that would set the callback to be triggered when the wait handle
becomes ready.

I think that if we were to do that we'd want to significantly rework
the rest of the client API, though - we'd have two separate ways to do
essentially the same thing. Not that I'm opposed to that, mind.

What I'm specifically blocking on here is mir_surface_set_* returning a
wait handle that returns a value. You could resolve this by making all
the setters return a null wait handle, which we already handle as
immediately available. And later, when we feel like breaking client
API, we remove the wait handle return from them completely.

That would make me abstain, pending a ML discussion of what we actually
want WaitHandles to be.

[Daniel van Vugt (vanvugt)]

"What I'm specifically blocking on here is mir_surface_set_* returning a wait handle that returns a value."

You'll notice I haven't directly modified any of those functions here. It would be inconsistent to treat those MirWaitHandles differently to any other MirWaitHandle. It should also be remembered that this proposal solves the thread safety problem without resorting to callbacks. Nobody has proposed any other solution that achieves this.

"You could resolve this by making all the setters return a null wait handle"

You're asking me to break existing behaviour that has been in place for two years already. Admittedly the wait handle on setters is low value and will be ignored by most users, but we get it for free. And any corresponding _get call needs to be synchronized with the wait handle from the _set, because all our _get functions are non-blocking (they just return the local cache contents). So we presently have non-blocking _get/_set functions with the ability to synchronize them. Sounds like a good thing to me. You're asking for some of that functionality to be removed and I'm still not sure why. It also feels off topic for this MP.

[Chris Halse Rogers (raof)]

On Wed, Mar 18, 2015 at 2:29 PM, Daniel van Vugt
<email address hidden> wrote:
> "What I'm specifically blocking on here is mir_surface_set_*
> returning a wait handle that returns a value."
>
> You'll notice I haven't directly modified any of those functions
> here. It would be inconsistent to treat those MirWaitHandles
> differently to any other MirWaitHandle. It should also be remembered
> that this proposal solves the thread safety problem without resorting
> to callbacks. Nobody has proposed any other solution that achieves
> this.

I don't dispute that nobody has proposed a solution that doesn't have
callbacks. I equally don't think that this *is a solution*, and I think
that the reason nobody has proposed a solution that doesn't have
callbacks is because there *is* no solution that doesn't have callbacks
or language features equivalent to implicit callbacks.

This is a solution to a slightly different problem, but I don't think
it's a problem that is useful to solve.

>
> "You could resolve this by making all the setters return a null wait
> handle"
>
> You're asking me to break existing behaviour that has been in place
> for two years already.

The behaviour changes slightly, but it doesn't change correctness;
there is currently no correct way to use mir_wait_for on a wait handle
returned by one of the *_set functions.

> Admittedly the wait handle on setters is low value and will be
> ignored by most users, but we get it for free. And any corresponding
> _get call needs to be synchronized with the wait handle from the
> _set, because all our _get functions are non-blocking (they just
> return the local cache contents). So we presently have non-blocking
> _get/_set functions with the ability to synchronize them.

But we *don't* have an ability to synchronise them - if you call
mir_surface_set_state(fullscreen) and mir_wait_for_result returns true
then it is *not* guaranteed that an immediate call to
mir_surface_get_state == fullscreen.

> Sounds like a good thing to me. You're asking for some of that
> functionality to be removed and I'm still not sure why. It also feels
> off topic for this MP.

Because the functionality doesn't actually exist, but looks like it
does. This part of the API rates a -10 on Rusty's API scale - it's an
API that is *not possible* to use correctly.

I think it's not off-topic for this MP because this MP extends the
functionality that doesn't exist in a way that might tempt people to
use it and write incorrect programs.

[Daniel van Vugt (vanvugt)]

These statements are either false, or describe a bug I'm not yet aware of:

"The behaviour changes slightly, but it doesn't change correctness;
there is currently no correct way to use mir_wait_for on a wait handle
returned by one of the *_set functions.
  But we *don't* have an ability to synchronise them - if you call
mir_surface_set_state(fullscreen) and mir_wait_for_result returns true
then it is *not* guaranteed that an immediate call to
mir_surface_get_state == fullscreen."

When I implemented the attribute system in 2013, it did work. Correct usage looks like:

  w = mir_surface_set_X(surface, Y);
  mir_wait_for(w);
  y = mir_surface_get_X(surface);
  assert(y == Y);

If this doesn't work any more then we have a bug. But I suspect it does still work because I revisited the test cases for it as part of this proposal.

[Daniel van Vugt (vanvugt)]

Ah, what you might be talking about is multiple threads doing the same thing. Indeed in that case calling _get will be racy. There's no guarantee that w represents the request for Y in that case.

So yeah, we need something better that correctly matches result instance to request instance.

[Chris Halse Rogers (raof)]

On Thu, Mar 19, 2015 at 1:37 PM, Daniel van Vugt
<email address hidden> wrote:
> Ah, what you might be talking about is multiple threads doing the
> same thing. Indeed in that case calling _get will be racy. There's no
> guarantee that w represents the request for Y in that case.

Not even multiple threads doing the same thing; the server can change
the surface state without the client requesting it (obvious example is
drag-title-bar-to-unmaximise), and that event can be processed before
the thread executing mir_wait_for continues execution.

Surface states are event driven; attempts to make them appear otherwise
make the client API harder to use.

Unmerged revisions

2388. By Daniel van Vugt on 2015-03-10

Fix style.

2387. By Daniel van Vugt on 2015-03-10

More acceptance tests

2386. By Daniel van Vugt on 2015-03-10

And some acceptance-testing for the new client function.

2385. By Daniel van Vugt on 2015-03-10

Use more consistent and less ambiguous mir_wait_for_ naming.

2384. By Daniel van Vugt on 2015-03-10

Add unit test for MirResult

2383. By Daniel van Vugt on 2015-03-10

Treat attribute settings with values different to what was requested
as a failure (even if the attribute successfully changed value to something
else).

2382. By Daniel van Vugt on 2015-03-10

Merge latest trunk

2381. By Daniel van Vugt on 2015-03-09

Wire up result_received

2380. By Daniel van Vugt on 2015-03-09

Prototype client API.

2379. By Daniel van Vugt on 2015-03-09

Remove fields not yet in use

=== modified file 'include/client/mir_toolkit/mir_wait.h'
--- include/client/mir_toolkit/mir_wait.h	2014-03-31 14:36:08 +0000
+++ include/client/mir_toolkit/mir_wait.h	2015-03-10 08:02:21 +0000
@@ -46,6 +46,14 @@
  */
 void mir_wait_for_one(MirWaitHandle *wait_handle);
 
+/**
+ * Wait on the supplied handle until one instance of the associated request
+ * has completed. This is the same as mir_wait_for_one with the extra
+ * feature of returning a result.
+ *   \param [in] wait_handle  Handle returned by an asynchronous request
+ *   \returns                 Whether the request succeeded.
+ */
+bool mir_wait_for_result(MirWaitHandle* wait_handle);
 
 #ifdef __cplusplus
 }

=== modified file 'src/client/buffer_stream.cpp'
--- src/client/buffer_stream.cpp	2015-02-18 05:27:28 +0000
+++ src/client/buffer_stream.cpp	2015-03-10 08:02:21 +0000
@@ -226,7 +226,7 @@
 
     done();
 
-    next_buffer_wait_handle.result_received();
+    next_buffer_wait_handle.result_received(true);
 }
 
 /* mcl::EGLNativeSurface interface for EGLNativeWindow integration */
@@ -250,7 +250,7 @@
 void mcl::BufferStream::on_configured()
 {
     std::unique_lock<decltype(mutex)> lock(mutex);
-    configure_wait_handle.result_received();
+    configure_wait_handle.result_received(true);
 }
 
 // TODO: It's a little strange that we use SurfaceAttrib here for the swap interval

=== modified file 'src/client/mir_connection.cpp'
--- src/client/mir_connection.cpp	2015-02-18 05:27:28 +0000
+++ src/client/mir_connection.cpp	2015-03-10 08:02:21 +0000
@@ -191,7 +191,7 @@
         data.surface->handle_event(*unfocus);
     }
     data.callback(data.surface, data.context);
-    data.handle->result_received();
+    data.handle->result_received(true);
     delete data.surface;
 }
 
@@ -282,7 +282,7 @@
     }
 
     if (safe_to_callback) callback(this, context);
-    connect_wait_handle.result_received();
+    connect_wait_handle.result_received(!connect_result.has_error());
 }
 
 MirWaitHandle* MirConnection::connect(
@@ -318,7 +318,7 @@
 
     // Ensure no racy lifecycle notifications can happen after disconnect completes
     lifecycle_control->set_lifecycle_event_handler([](MirLifecycleState){});
-    disconnect_wait_handle.result_received();
+    disconnect_wait_handle.result_received(true);
 }
 
 MirWaitHandle* MirConnection::disconnect()
@@ -353,7 +353,8 @@
 
     callback(this, reply, context);
 
-    platform_operation_wait_handle.result_received();
+    platform_operation_wait_handle.result_received(
+        !platform_operation_reply.has_error());
 }
 
 MirWaitHandle* MirConnection::platform_operation(
@@ -529,10 +530,11 @@
 
     set_error_message(display_configuration_response.error());
 
-    if (!display_configuration_response.has_error())
+    bool ok = !display_configuration_response.has_error();
+    if (ok)
         display_configuration->set_configuration(display_configuration_response);
 
-    return configure_display_wait_handle.result_received();
+    return configure_display_wait_handle.result_received(ok);
 }
 
 MirWaitHandle* MirConnection::configure_display(MirDisplayConfiguration* config)

=== modified file 'src/client/mir_prompt_session.cpp'
--- src/client/mir_prompt_session.cpp	2015-02-18 05:27:28 +0000
+++ src/client/mir_prompt_session.cpp	2015-03-10 08:02:21 +0000
@@ -114,7 +114,7 @@
     }
 
     callback(this, context);
-    start_wait_handle.result_received();
+    start_wait_handle.result_received(!session.has_error());
 }
 
 void MirPromptSession::done_stop(mir_prompt_session_callback callback, void* context)
@@ -122,7 +122,7 @@
     set_state(mir_prompt_session_state_stopped);
 
     callback(this, context);
-    stop_wait_handle.result_received();
+    stop_wait_handle.result_received(true);
 }
 
 char const* MirPromptSession::get_error_message()
@@ -168,5 +168,5 @@
         fds.push_back(socket_fd_response.fd(i));
 
     callback(this, size, fds.data(), context);
-    fds_for_prompt_providers_wait_handle.result_received();
+    fds_for_prompt_providers_wait_handle.result_received(true);
 }

=== modified file 'src/client/mir_screencast.cpp'
--- src/client/mir_screencast.cpp	2015-02-18 05:27:28 +0000
+++ src/client/mir_screencast.cpp	2015-03-10 08:02:21 +0000
@@ -104,21 +104,22 @@
 void MirScreencast::screencast_created(
     mir_screencast_callback callback, void* context)
 {
-    if (!protobuf_screencast.has_error())
+    bool ok = !protobuf_screencast.has_error();
+    if (ok)
     {
         buffer_stream = buffer_stream_factory->make_consumer_stream(server,
             protobuf_screencast.buffer_stream(), "MirScreencast");
     }
 
     callback(this, context);
-    create_screencast_wait_handle.result_received();
+    create_screencast_wait_handle.result_received(ok);
 }
 
 void MirScreencast::released(
     mir_screencast_callback callback, void* context)
 {
     callback(this, context);
-    release_wait_handle.result_received();
+    release_wait_handle.result_received(true);
 }
 
 mir::client::ClientBufferStream* MirScreencast::get_buffer_stream()

=== modified file 'src/client/mir_surface.cpp'
--- src/client/mir_surface.cpp	2015-03-05 05:47:28 +0000
+++ src/client/mir_surface.cpp	2015-03-10 08:02:21 +0000
@@ -206,6 +206,7 @@
 
 void MirSurface::created(mir_surface_callback callback, void * context)
 {
+    bool ok = true;
     {
     std::lock_guard<decltype(mutex)> lock(mutex);
     if (!surface.has_id())
@@ -214,7 +215,7 @@
             surface.set_error("Error processing surface create response, no ID (disconnected?)");
 
         callback(this, context);
-        create_wait_handle.result_received();
+        create_wait_handle.result_received(false);
         return;
     }
     }
@@ -237,12 +238,13 @@
     }
     catch (std::exception const& error)
     {
+        ok = false;
         surface.set_error(std::string{"Error processing Surface creating response:"} +
                           boost::diagnostic_information(error));
     }
 
     callback(this, context);
-    create_wait_handle.result_received();
+    create_wait_handle.result_received(ok);
 }
 
 MirWaitHandle* MirSurface::release_surface(
@@ -313,7 +315,9 @@
 
     configure_wait_handle.expect_result();
     server->configure_surface(0, &setting, &configure_result,
-              google::protobuf::NewCallback(this, &MirSurface::on_configured));
+        google::protobuf::NewCallback(this,
+                                      &MirSurface::on_configured,
+                                      value));
 
     return &configure_wait_handle;
 }
@@ -322,7 +326,7 @@
 {
 void signal_response_received(MirWaitHandle* handle)
 {
-    handle->result_received();
+    handle->result_received(true);
 }
 }
 
@@ -361,7 +365,7 @@
     return !response.has_error();
 }
 
-void MirSurface::on_configured()
+void MirSurface::on_configured(int requested_value)
 {
     std::lock_guard<decltype(mutex)> lock(mutex);
 
@@ -370,6 +374,8 @@
         configure_result.has_attrib())
     {
         int a = configure_result.attrib();
+        bool ok = !configure_result.has_error() &&
+                  configure_result.ivalue() == requested_value;
 
         switch (a)
         {
@@ -384,17 +390,18 @@
                 assert(configure_result.has_error());
             break;
         default:
+            ok = false;
             assert(false);
             break;
         }
 
-        configure_wait_handle.result_received();
+        configure_wait_handle.result_received(ok);
     }
 }
 
 void MirSurface::on_cursor_configured()
 {
-    configure_cursor_wait_handle.result_received();
+    configure_cursor_wait_handle.result_received(true);
 }
 
 

=== modified file 'src/client/mir_surface.h'
--- src/client/mir_surface.h	2015-03-05 05:47:28 +0000
+++ src/client/mir_surface.h	2015-03-10 08:02:21 +0000
@@ -141,7 +141,7 @@
 private:
     mutable std::mutex mutex; // Protects all members of *this
 
-    void on_configured();
+    void on_configured(int requested_value);
     void on_cursor_configured();
     void created(mir_surface_callback callback, void* context);
     MirPixelFormat convert_ipc_pf_to_geometry(google::protobuf::int32 pf) const;

=== modified file 'src/client/mir_wait_api.cpp'
--- src/client/mir_wait_api.cpp	2014-03-31 14:36:08 +0000
+++ src/client/mir_wait_api.cpp	2015-03-10 08:02:21 +0000
@@ -30,3 +30,16 @@
     if (wait_handle)
         wait_handle->wait_for_one();
 }
+
+bool mir_wait_for_result(MirWaitHandle* wait_handle)
+{
+    if (!wait_handle)
+        return false;
+
+    /*
+     * As a MirWaitHandle may get recycled immediately, the structure cannot
+     * store a result beyond the lifetime of the mir_wait_for_ call. So any
+     * result needs to be copied out immediately. Even if it's ignored.
+     */
+    return wait_handle->wait_for_one().success;
+}

=== modified file 'src/client/mir_wait_handle.cpp'
--- src/client/mir_wait_handle.cpp	2015-01-21 07:34:50 +0000
+++ src/client/mir_wait_handle.cpp	2015-03-10 08:02:21 +0000
@@ -19,11 +19,14 @@
 
 #include "mir_wait_handle.h"
 
+MirResult::MirResult(bool ok) : success{ok}
+{
+}
+
 MirWaitHandle::MirWaitHandle() :
     guard(),
     wait_condition(),
-    expecting(0),
-    received(0)
+    expecting(0)
 {
 }
 
@@ -38,11 +41,11 @@
     expecting++;
 }
 
-void MirWaitHandle::result_received()
+void MirWaitHandle::result_received(MirResult const& r)
 {
     std::lock_guard<std::mutex> lock(guard);
 
-    received++;
+    received.push_back(r);
     wait_condition.notify_all();
 }
 
@@ -50,9 +53,9 @@
 {
     std::unique_lock<std::mutex> lock(guard);
 
-    wait_condition.wait(lock, [&]{ return received == expecting; });
+    wait_condition.wait(lock, [&]{ return received.size() == expecting; });
 
-    received = 0;
+    received.clear();
     expecting = 0;
 }
 
@@ -60,17 +63,18 @@
 {
     std::unique_lock<std::mutex> lock(guard);
 
-    wait_condition.wait_for(lock, limit, [&]{ return received == expecting; });
+    wait_condition.wait_for(lock, limit, [&]{ return received.size() == expecting; });
 }
 
-
-void MirWaitHandle::wait_for_one()  // wait for any single result
+MirResult MirWaitHandle::wait_for_one()  // wait for any single result
 {
     std::unique_lock<std::mutex> lock(guard);
 
-    wait_condition.wait(lock, [&]{ return received != 0; });
+    wait_condition.wait(lock, [&]{ return !received.empty(); });
 
-    --received;
+    auto head = received.front();
+    received.pop_front();
     --expecting;
+
+    return head;
 }
-

=== modified file 'src/client/mir_wait_handle.h'
--- src/client/mir_wait_handle.h	2013-10-03 03:44:08 +0000
+++ src/client/mir_wait_handle.h	2015-03-10 08:02:21 +0000
@@ -23,11 +23,24 @@
 #include <chrono>
 #include <condition_variable>
 #include <mutex>
+#include <deque>
 
 /**
  * \addtogroup mir_toolkit
  * @{
  */
+
+struct MirResult
+{
+    MirResult(bool ok);
+
+    bool success;
+    // Suggested future uses:
+    // const char* error;
+    // struct MirSurface* surface;
+    // struct MirConnection* connection;
+};
+
 struct MirWaitHandle
 {
 public:
@@ -36,16 +49,18 @@
 
     void expect_result();
     void result_received();
+    void result_received(MirResult const&);
     void wait_for_all();
-    void wait_for_one();
+    MirResult wait_for_one();
     void wait_for_pending(std::chrono::milliseconds limit);
 
 private:
     std::mutex guard;
     std::condition_variable wait_condition;
 
-    int expecting;
-    int received;
+    typedef std::deque<MirResult> Queue;
+    Queue::size_type expecting;
+    Queue received;
 };
 /**@}*/
 

=== modified file 'src/client/symbols.map'
--- src/client/symbols.map	2015-02-18 05:27:28 +0000
+++ src/client/symbols.map	2015-03-10 08:02:21 +0000
@@ -37,10 +37,15 @@
     mir_surface_spec_set_preferred_orientation;
 } MIR_CLIENT_8.1;
 
-MIR_CLIENT_8.3 {
+MIR_CLIENT_8.3 {  # Mir 0.12
   global:
     mir_connection_create_spec_for_menu;
     mir_connection_create_spec_for_tooltip;
     mir_connection_create_spec_for_dialog;
     mir_connection_platform_operation;
 } MIR_CLIENT_8.2;
+
+MIR_CLIENT_8.4 {  # Mir 0.13
+  global:
+    mir_wait_for_result;
+} MIR_CLIENT_8.3;

=== modified file 'tests/acceptance-tests/test_client_library.cpp'
--- tests/acceptance-tests/test_client_library.cpp	2015-02-18 05:27:28 +0000
+++ tests/acceptance-tests/test_client_library.cpp	2015-03-10 08:02:21 +0000
@@ -152,7 +152,7 @@
 {
     MirWaitHandle* wh = mir_connect(new_connection().c_str(), __PRETTY_FUNCTION__, connection_callback, this);
     EXPECT_THAT(wh, NotNull());
-    mir_wait_for(wh);
+    EXPECT_TRUE(mir_wait_for_result(wh));
 
     ASSERT_THAT(connection, NotNull());
     EXPECT_TRUE(mir_connection_is_valid(connection));
@@ -161,6 +161,19 @@
     mir_connection_release(connection);
 }
 
+TEST_F(ClientLibrary, client_library_cant_connect)
+{
+    MirWaitHandle* wh = mir_connect("/dev/null", __PRETTY_FUNCTION__, connection_callback, this);
+    EXPECT_THAT(wh, IsNull());
+    EXPECT_FALSE(mir_wait_for_result(wh));
+
+    ASSERT_THAT(connection, NotNull());
+    EXPECT_FALSE(mir_connection_is_valid(connection));
+    EXPECT_THAT(mir_connection_get_error_message(connection), StrNe(""));
+
+    mir_connection_release(connection);
+}
+
 TEST_F(ClientLibrary, synchronous_connection)
 {
     connection = mir_connect_sync(new_connection().c_str(), __PRETTY_FUNCTION__);
@@ -267,16 +280,16 @@
 
     EXPECT_THAT(mir_surface_get_state(surface), Eq(mir_surface_state_restored));
 
-    mir_wait_for(mir_surface_set_state(surface, mir_surface_state_fullscreen));
-    EXPECT_THAT(mir_surface_get_state(surface), Eq(mir_surface_state_fullscreen));
-
-    mir_wait_for(mir_surface_set_state(surface, static_cast<MirSurfaceState>(999)));
-    EXPECT_THAT(mir_surface_get_state(surface), Eq(mir_surface_state_fullscreen));
-
-    mir_wait_for(mir_surface_set_state(surface, mir_surface_state_minimized));
+    EXPECT_TRUE(mir_wait_for_result(mir_surface_set_state(surface, mir_surface_state_fullscreen)));
+    EXPECT_THAT(mir_surface_get_state(surface), Eq(mir_surface_state_fullscreen));
+
+    EXPECT_FALSE(mir_wait_for_result(mir_surface_set_state(surface, static_cast<MirSurfaceState>(999))));
+    EXPECT_THAT(mir_surface_get_state(surface), Eq(mir_surface_state_fullscreen));
+
+    EXPECT_TRUE(mir_wait_for_result(mir_surface_set_state(surface, mir_surface_state_minimized)));
     EXPECT_THAT(mir_surface_get_state(surface), Eq(mir_surface_state_minimized));
 
-    mir_wait_for(mir_surface_set_state(surface, static_cast<MirSurfaceState>(888)));
+    EXPECT_FALSE(mir_wait_for_result(mir_surface_set_state(surface, static_cast<MirSurfaceState>(888))));
     EXPECT_THAT(mir_surface_get_state(surface), Eq(mir_surface_state_minimized));
 
     // Stress-test synchronization logic with some flooding

=== modified file 'tests/unit-tests/client/test_wait_handle.cpp'
--- tests/unit-tests/client/test_wait_handle.cpp	2015-01-21 07:34:50 +0000
+++ tests/unit-tests/client/test_wait_handle.cpp	2015-03-10 08:02:21 +0000
@@ -33,7 +33,7 @@
 
     for (int i = 0; i < arbitrary_number_of_events; i++)
     {
-        w.result_received();
+        w.result_received(true);
     }
 
     w.wait_for_all();
@@ -51,7 +51,7 @@
             w.expect_result();
 
         for (int j = 1; j <= i; j++)
-            w.result_received();
+            w.result_received(true);
 
         w.wait_for_all();
     }
@@ -70,7 +70,7 @@
             in->expect_result();
             in->wait_for_all();
             symmetric_result = i;
-            out->result_received();
+            out->result_received(true);
         }
     }
 }
@@ -84,7 +84,7 @@
     for (int i = 1; i <= max; i++)
     {
         out.expect_result();
-        in.result_received();
+        in.result_received(true);
         out.wait_for_all();
         ASSERT_EQ(symmetric_result, i);
     }
@@ -103,7 +103,7 @@
             in->wait_for_all();
             asymmetric_result = i;
             for (int j = 1; j <= i; j++)
-                out->result_received();
+                out->result_received(true);
         }
     }
 }
@@ -116,7 +116,7 @@
 
     for (int i = 1; i <= max; i++)
     {
-        in.result_received();
+        in.result_received(true);
         for (int j = 1; j <= i; j++)
             out.expect_result();
         out.wait_for_all();
@@ -153,10 +153,30 @@
     std::thread c(crowded_thread, &w, max);
 
     for (int n = 0; n < max * 3; n++)
-        w.result_received();
+        w.result_received(true);
 
     a.join();
     b.join();
     c.join();
 }
 
+TEST(WaitHandle, returns_result_value)
+{
+    MirWaitHandle w;
+
+    w.result_received(true);
+    EXPECT_TRUE(w.wait_for_one().success);
+
+    w.result_received(false);
+    EXPECT_FALSE(w.wait_for_one().success);
+
+    for (int i = 0; i < 10; ++i)
+        w.result_received(true);
+    for (int i = 0; i < 10; ++i)
+        w.result_received(false);
+
+    for (int i = 0; i < 10; ++i)
+        EXPECT_TRUE(w.wait_for_one().success);
+    for (int i = 0; i < 10; ++i)
+        EXPECT_FALSE(w.wait_for_one().success);
+}