Merge into development-branch : error-callback-for-display-config : Code : Mir

Reviewer	Review Type	Date Requested	Status
Mir CI Bot	continuous-integration	2016-03-30	Approve on 2016-04-14
Daniel van Vugt		2016-03-30	Abstain on 2016-04-14
Alan Griffiths			Approve on 2016-04-13
Brandon Schaefer (community)			Approve on 2016-04-05
Cemil Azizoglu (community)		2016-03-30	Approve on 2016-04-01
Kevin DuBois (community)		2016-03-30	Approve on 2016-03-31
Review via email: mp+290384@code.staging.launchpad.net

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-03-16: Posted in a previous version of this proposal

#

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

review: Approve (continuous-integration)

Revision history for this message

Kevin DuBois (kdub) wrote on 2016-03-16: Posted in a previous version of this proposal

#

+void mir_connection_set_error_callback(

It would be better to set the display config, input config, and error callbacks when we mir_connect() so that we don't miss messages around connection time. I suppose that this is the way we're doing things at the moment though, so that's alright.

Is this needed down the road?
+ mir_error_domain_surface
Seems strange that we would say there's a surface error without an indication as to which surface has the error.

review: Needs Information

Revision history for this message

Cemil Azizoglu (cemil-azizoglu) wrote on 2016-03-16: Posted in a previous version of this proposal

#

No description (for autodocumentation) for these functions?

100 +MirErrorDomain mir_error_get_domain(MirError const* error);
101 +
102 +uint32_t mir_error_get_code(MirError const* error);
103
-----------
Did you mean to have them here?

353 +char const* mir_error_get_debug_message(MirError const* /*error*/)
354 +{
355 + return "Failed to frob";
356 +}
357 +
358 +char const* mir_error_get_display_message(MirError const* /*error*/)
359 +{
360 + return "I'm sorry Dave, I can't do that";
361 +}

If so, did you want to add a stub for the following also?
void* mir_error_get_detail()
----------------
Not really needed?

716 +
717 +

review: Needs Fixing

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-03-22: Posted in a previous version of this proposal

#

Inventing new error codes puts more responsibility for localizing the messages to arbitrary languages on us. Although I know Launchpad has facilities for crowd-sourcing that work... Please consider if the standard error codes of <errno.h> could be used instead (along with some specific named domain like "/tmp/mir_socket".

Example:
Error code = EACCES (Permissing denied)
Error domain = "/tmp/mir_socket" (This bit never gets localized)

That way we get free localization to all languages, using just strerror(). See LP: #1431190

review: Needs Fixing

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-03-22: Posted in a previous version of this proposal

#

^ This discussion should occur before the display API comes into it. So the proposal also needs to be broken up.

Revision history for this message

Chris Halse Rogers (raof) wrote on 2016-03-28: Posted in a previous version of this proposal

#

> Inventing new error codes puts more responsibility for localizing the messages
> to arbitrary languages on us. Although I know Launchpad has facilities for
> crowd-sourcing that work... Please consider if the standard error codes of
> <errno.h> could be used instead (along with some specific named domain like
> "/tmp/mir_socket".

Nothing in this API precludes the use of strerror() to produce the error strings. We can even have translated strings and fall back to strerror() when we don't have a translation, if desired.

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-03-29: Posted in a previous version of this proposal

#

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-03-29: Posted in a previous version of this proposal

#

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Chris Halse Rogers (raof) wrote on 2016-03-29: Posted in a previous version of this proposal

#

Once bzr has finished being stupid I'll resubmit this branch against lp:mir.

Revision history for this message

Chris Halse Rogers (raof) wrote on 2016-03-30:

#

I HAVE DEFEATED GIT!

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-03-30:

#

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

review: Approve (continuous-integration)

Revision history for this message

Kevin DuBois (kdub) wrote on 2016-03-31:

#

lgtm

review: Approve

Revision history for this message

Cemil Azizoglu (cemil-azizoglu) wrote on 2016-04-01:

#

LGTM.

Nit: s/in/is
37 + * Client in not permitted to change global display configuration

review: Approve

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-04-04:

#

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

FAILED: Continuous integration, rev:3440
https://mir-jenkins.ubuntu.com/job/mir-ci/741/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/713/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/750
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/741
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/741
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/722
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/722/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/722
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/722/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/722/console
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/722/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/722
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/722/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/722
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/722/artifact/output/*zip*/output.zip

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Brandon Schaefer (brandontschaefer) wrote on 2016-04-05:

#

LGTM, just checking on the:

+#pragma once

Since this branch:
https://code.launchpad.net/~raof/mir/use-pragma-once/+merge/290847

Revision history for this message

Brandon Schaefer (brandontschaefer) wrote on 2016-04-05:

#

lgtm otherwise

review: Approve

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-04-11:

#

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

FAILED: Continuous integration, rev:3441
https://mir-jenkins.ubuntu.com/job/mir-ci/802/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/797/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/834
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/825
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/825
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/806
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/806/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/806
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/806/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/806/console
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/806/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/806
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/806/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/806
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/806/artifact/output/*zip*/output.zip

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2016-04-11:

#

+ mir_connection_preview_base_display_configuration;
+ mir_connection_set_error_callback;
     mir_connection_set_input_config_change_callback;
     mir_display_config_get_max_simultaneous_outputs;
     mir_display_config_get_num_outputs;
     mir_display_config_get_output;
     mir_display_config_release;
+ mir_error_get_code;
+ mir_error_get_domain;

These belong in the unreleased 0.22 stanza.

review: Needs Fixing

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-04-12:

#

(0) What Alan said ^

Regarding:

524 +message StructuredError {
525 + optional uint32 domain = 1;
526 + optional uint32 code = 2;
527 +}

(1) If we do use libc for our error message localization then it needs to be decided and more obvious right now (signed int and also different name). So that makes 'uint32 code' more like 'int32 errnum' or 'int32 stderrcode'.

(2) Do we really need an enum domain? Seems cumbersome to maintain but also consider (3) ...

(3) I suspect a generic error solution will also require additional arg strings, but we can add those latter. Like EACCES (Permissing denied) with context="/tmp/mir_socket".

review: Needs Fixing

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-04-12:

#

... add those *later*

(4) As pointed out in my first review this proposal could also be split in two to make it smaller:
"This discussion should occur before the display API comes into it."

Revision history for this message

Chris Halse Rogers (raof) wrote on 2016-04-12:

#

> (0) What Alan said ^
>
> Regarding:
>
> 524 +message StructuredError {
> 525 + optional uint32 domain = 1;
> 526 + optional uint32 code = 2;
> 527 +}
>
> (1) If we do use libc for our error message localization then it needs to be
> decided and more obvious right now.

There's no conflict between using strerror for our message localisation and using our own opaque codes. For example:

char const* mir_error_get_description(MirError const* err)
{
    switch(err->code())
    {
    case mir_display_configuration_error_unauthorized:
        return strerror(EPERM);
    ...
    }
}

> (2) Do we really need an enum domain? Seems cumbersome to maintain but also
> consider (3) ...

There are (at least) three advantages to enum domain:
1) We can group errors into logical domains and have separate error enums for each. This means we can keep related error definitions together - if we had them all in a single enum you can only add new ones at the end, so you'll likely end up with logically related errors scattered throughout the range.
2) Clients can interpret any additional domain-specific information we attach without having to exhaustively match on the error codes
3) Clients can easily filter out errors they don't care about

> (4) As pointed out in my first review this proposal could also be split in two
> to make it smaller:
> "This discussion should occur before the display API comes into it."

While I could split it out, it's not currently big, and I belive the response to submitting *just* the error API with no users (and hence no useful tests) would be “how can this be used, please add an implementation that uses it”.

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-04-13:

#

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Chris Halse Rogers (raof) wrote on 2016-04-13:

#

CI failure looks like resource contention on our amd64 builder; retrying.

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-04-13:

#

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

review: Approve (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-04-13:

#

(1) Thanks for pointing that out... MirDisplayConfigurationError needs removing completely unless we can show the expected set of error codes is dramatically different to the standard set. We do not want to maintain switch statements to convert error codes. So just replace 'mir_display_configuration_error_unauthorized' with EACCES/EPERM. There's no need to reinvent that wheel when it's only adding unnecessary complexity (now and for future maintainers). Standard error codes combined with some domain/context information is all you need.

review: Needs Fixing

Revision history for this message

Chris Halse Rogers (raof) wrote on 2016-04-13:

#

I think that the expected set of error codes is¹ going to be
dramatically different to the standard set. For example, what error
would you map “Normal windows can't have parents” to, versus
“Dialogs must have parents” versus “You requested a 0-sized
surface”?

Or “You specified a display configuration with an invalid mode”
versus “You specified a display configuration that we can't actually
support”, versus “You specified a display configuration with an
output that can't be rendered to because of GL limit constraints”?

¹: Now, obviously, you can map *all* errors down to the standard set;
indeed, you can map all errors down to “false”. The question is
whether you lose sufficiently useful information.

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2016-04-13:

#

OK

review: Approve

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-04-14:

#

While standard error codes do loosely/generically cover all of those (ever read Linux kernel source?), they take some getting used to. And are slightly cryptic too.

Anyway, my job is done now. Just had to remind you to consider the existing (and simpler) solution that is already fully localized to many languages. If the team votes to not use that, I can't force them.

review: Abstain

Revision history for this message

Chris Halse Rogers (raof) wrote on 2016-04-14:

#

On Thu, Apr 14, 2016 at 1:35 PM, Daniel van Vugt
<email address hidden> wrote:
> Review: Abstain
>
> While standard error codes do loosely/generically cover all of those
> (ever read Linux kernel source?), they take some getting used to. And
> are slightly cryptic too.

Oh, yes. I *also* read the fairly common “damn, our error reporting
API doesn't provide very useful errors” threads :)

Revision history for this message

Mir CI Bot (mir-ci-bot) on 2016-04-14:

#

review: Approve (continuous-integration)

Mir

Merge lp://staging/~raof/mir/error-callback-for-display-config into lp://staging/mir

Commit message

Description of the change

Preview Diff

Subscribers