Mir

Merge lp://qastaging/~afrantzis/mir/client-set-base-display-config into lp://qastaging/mir

client-set-base-display-config
Merge into development-branch

Proposed by Alexandros Frantzis on 2015-11-11

Status:	Merged
Approved by:	Alexandros Frantzis on 2015-11-26
Approved revision:	no longer in the source branch.
Merged at revision:	3129
Proposed branch:	lp://qastaging/~afrantzis/mir/client-set-base-display-config
Merge into:	lp://qastaging/mir
Diff against target:	428 lines (+210/-20) 12 files modified include/client/mir_toolkit/mir_connection.h (+25/-0) src/client/mir_connection.cpp (+48/-18) src/client/mir_connection.h (+6/-1) src/client/mir_connection_api.cpp (+15/-0) src/client/rpc/mir_display_server.cpp (+7/-0) src/client/rpc/mir_display_server.h (+4/-0) src/client/symbols.map (+1/-0) src/include/common/mir/protobuf/display_server.h (+4/-0) src/server/frontend/protobuf_message_processor.cpp (+4/-0) src/server/frontend/session_mediator.h (+1/-1) tests/acceptance-tests/test_display_configuration.cpp (+91/-0) tests/include/mir/test/doubles/stub_display_server.h (+4/-0)
To merge this branch:	bzr merge lp://qastaging/~afrantzis/mir/client-set-base-display-config
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration		Approve on 2015-11-26
Alberto Aguirre (community)			Approve on 2015-11-19
Alan Griffiths		2015-11-11	Approve on 2015-11-19
Review via email: mp+277234@code.qastaging.launchpad.net

Commit message

client: Add mir_connection_set_base_display_config() API

Description of the change

client: Add mir_connection_set_base_display_config() API

I went with 'set' instead of 'apply' because I find 'apply' to be too confusing/misleading in this context.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-11-11:

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

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-11-12:

- display_changer->set_base_configuration(config);
+ auto base_configuration_set = display_changer->set_base_configuration(config);
+ base_configuration_set.wait();

...

+TEST_F(DisplayConfigurationTest,
+ client_is_notified_of_new_base_config_before_set_base_configuration_returns)

Do we really want to guarantee this ordering by blocking the IPC thread? I would expect the future direction to be removing this legacy future: the client requests a config change and that may (or may not) happen at the server's discretion.

review: Needs Information

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2015-11-12:

> - display_changer->set_base_configuration(config);
> + auto base_configuration_set =
> display_changer->set_base_configuration(config);
> + base_configuration_set.wait();
>
> ...
>
> +TEST_F(DisplayConfigurationTest,
> + client_is_notified_of_new_base_config_before_set_base_configuration_returns)
>
> Do we really want to guarantee this ordering by blocking the IPC thread? I
> would expect the future direction to be removing this legacy future: the
> client requests a config change and that may (or may not) happen at the
> server's discretion.

The wait was introduced mainly to guarantee TEST_F(DisplayConfigurationTest, client_gets_updated_base_config_after_setting_it).

Since MirWaitHandle* mir_connection_set_base_display_config() is just setting (not necessarily applying) the base config, a user of the API will reasonably expect to be able to get the updated base config after a successful invocation (as long as no other source tries to change it in the meantime).

An alternative I considered is "void mir_connection_set_base_display_config(_async)()", which explicitly doesn't provide any guarantees about when it is "done" (note the lack of MirWaitHandle and the possible addition of 'async' in the name). Of course, this also makes it harder to check if the operation succeeded or not (a wait, possibly with a timeout, needs to be used, and there could also be some false positives if, e.g., the base config is changed in the meantime from another source).

The lack of a robust way to tell the client about operation success/failure is why I went with the approach in this MP. Having said that, if we don't think reporting success/failure is important in this case, I am fine with the alternative too.

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-11-13:

It isn't helpful to the client to know whether the operation succeeded: another client (or the shell) may set the base config before any action can be taken.

PS I went down this rabbit hole with the session configuration - and we've reverted much of that.

review: Needs Fixing

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2015-11-16:

I decided to take a middle-of-the-road approach to allow clients to get notified (at least) about authorization errors instead of leaving them in the dark. It's not the most elegant API (since it needs a comment to explain what to expect), but I think not notifying the client about such an error is even worse from an API usability perspective.

+ * When the wait handle returned by this function becomes ready, clients can use
+ * mir_connection_get_error_message() to check if an authorization error occured.
+ * Only authorization errors are guaranteed to return an error message for this
+ * operation.
+ *
+ * A successful result (i.e. no error) does not guarantee that the base display
+ * configuration has been changed to the desired value. Clients should register
+ * a callback with mir_connection_set_display_config_change_callback() to monitor
+ * actual base display configuration changes.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-11-16:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2015-11-16:

CI failure is caused by a CI infrastructure issue.

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-11-17:

+ * When the wait handle returned by this function becomes ready, clients can use
+ * mir_connection_get_error_message() to check if an authorization error occured.

s/occured/occurred/

review: Needs Fixing

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2015-11-17:

> s/occured/occurred/

Thanks, fixed.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-11-17:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-11-19:

I'm not sure that we need to report authorization errors like this, but don't have a strong objection. (I feel that it might be better to have an authorization "handshake" where the client states the authorizations it wants and the server replies with those it gets.)

review: Approve

Revision history for this message

Alberto Aguirre (albaguirre) wrote on 2015-11-19:

OK.

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-11-21:

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) on 2015-11-26:

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes:

Alexandros Frantzis

Emanuele Antonio Faraone

Gerry Boland

Mir development team