Mir

Merge lp://qastaging/~raof/mir/report-invalid-window-state into lp://qastaging/mir

report-invalid-window-state
Merge into development-branch

Proposed by Chris Halse Rogers on 2017-01-30

Status:	Rejected
Rejected by:	Chris Halse Rogers on 2017-07-04
Proposed branch:	lp://qastaging/~raof/mir/report-invalid-window-state
Merge into:	lp://qastaging/mir
Diff against target:	863 lines (+389/-21) (has conflicts) 31 files modified include/client/mir/events/event_builders.h (+3/-0) include/client/mir_toolkit/client_types.h (+24/-0) include/client/mir_toolkit/events/error_event.h (+42/-0) include/client/mir_toolkit/events/event.h (+5/-0) include/common/mir/mir_error.h (+3/-3) include/server/mir/scene/surface_observer.h (+2/-0) src/capnproto/mir_event.capnp (+7/-0) src/client/CMakeLists.txt (+1/-2) src/client/event.cpp (+14/-0) src/client/events/event_builders.cpp (+10/-0) src/client/mir_connection.cpp (+1/-1) src/client/mir_error_api.cpp (+1/-1) src/client/mir_surface.cpp (+0/-2) src/client/rpc/mir_protobuf_rpc_channel.cpp (+1/-1) src/client/symbols.map (+6/-0) src/common/CMakeLists.txt (+5/-1) src/common/events/CMakeLists.txt (+4/-1) src/common/events/error_event.cpp (+52/-0) src/common/events/event.cpp (+8/-0) src/common/mir_error.cpp (+1/-1) src/common/symbols.map (+15/-0) src/include/common/mir/events/error_event.h (+32/-0) src/include/common/mir/events/event.h (+25/-1) src/include/common/mir/events/event_private.h (+1/-0) src/server/input/android/input_sender.cpp (+2/-1) src/server/scene/basic_surface.cpp (+4/-0) src/server/shell/CMakeLists.txt (+10/-4) src/server/shell/abstract_shell.cpp (+37/-0) tests/acceptance-tests/test_client_library.cpp (+71/-0) tests/mir_test_framework/headless_test.cpp (+1/-1) tests/unit-tests/client/test_client_mir_error.cpp (+1/-1) Text conflict in include/client/mir_toolkit/client_types.h Text conflict in tests/acceptance-tests/test_client_library.cpp
To merge this branch:	bzr merge lp://qastaging/~raof/mir/report-invalid-window-state
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Alan Griffiths		2017-01-30	Disapprove on 2017-01-30
Mir CI Bot	continuous-integration		Needs Fixing on 2017-01-30
Review via email: mp+315864@code.qastaging.launchpad.net

Commit message

(Asynchronously) Report invalid window-state errors.

Description of the change

This is the first part of a replacement for MirWaitHandle in mir_window_set_sate() - ensuring the client either gets a mir_event_type_window event changing it to the requested state or an error event.

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2017-01-30:

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/2894/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2017-01-30:

"This is the first part of a replacement for MirWaitHandle in mir_window_set_sate()..."

The legacy wait handle in mir_surface_set_state() is useless, because the only guarantee it can provide is that the server has received the request. In particular, Unity8 passes the decision off to another thread while the RPC completes.

Because it would be useless mir_window_set_state() does *not* provide a wait handle.

"...ensuring the client either gets a mir_event_type_window event changing it to the requested state or an error event."

Is this a reasonable expectation?

The validation introduced in msh::AbstractShell::set_surface_attribute() happens before invoking the window management policy. Window managers can decide to reinterpret the request as neither the requested state nor an error. Or even chose a different state.

In any case, msh::AbstractShell::set_surface_attribute() is the wrong place to detect an "error" as acting on the request may occur on another thread (e.g. Unity8). There is no simple way to know that a surface state change actioned by Unity8 is related to a client request.

Finally, the client can make a similar request using a WindowSpec, that doesn't give the same guarantees.

review: Disapprove

Revision history for this message

Chris Halse Rogers (raof) wrote on 2017-01-30:

> "This is the first part of a replacement for MirWaitHandle in
> mir_window_set_sate()..."
>
> The legacy wait handle in mir_surface_set_state() is useless, because the only
> guarantee it can provide is that the server has received the request. In
> particular, Unity8 passes the decision off to another thread while the RPC
> completes.
>
> Because it would be useless mir_window_set_state() does *not* provide a wait
> handle.
>
> "...ensuring the client either gets a mir_event_type_window event changing it
> to the requested state or an error event."
>
> Is this a reasonable expectation?

A bunch of our acceptance tests require that they know when a set_state call has succeeded or failed. It's plausible to me that non-test clients would also want to know this information.

>
> The validation introduced in msh::AbstractShell::set_surface_attribute()
> happens before invoking the window management policy. Window managers can
> decide to reinterpret the request as neither the requested state nor an error.
> Or even chose a different state.

The current validation is ‘is this a valid MirWindowState value’, which the window manager should not be able to ignore, and which we should filter out before the window manager gets to act.

>
> In any case, msh::AbstractShell::set_surface_attribute() is the wrong place to
> detect an "error" as acting on the request may occur on another thread (e.g.
> Unity8). There is no simple way to know that a surface state change actioned
> by Unity8 is related to a client request.

So, this suggests two things:
1) set_surface_attribute() incorrectly returns a value, and
2) My follow-up branch to send a state_change_rejected event needs to be to unity8 as well as Mir.

>
> Finally, the client can make a similar request using a WindowSpec, that
> doesn't give the same guarantees.

That is indeed a required follow up branch, yes :)