Merge lp://staging/~alan-griffiths/mir/support-gdk_window_move_to_rect into lp://staging/mir

PASSED: Continuous integration, rev:3685
https://mir-jenkins.ubuntu.com/job/mir-ci/1586/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/1986
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2046
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2037
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2037
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2037
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2012
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2012/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2012
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2012/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2012
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2012/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2012
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2012/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/2012
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2012/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2012
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2012/artifact/output/*zip*/output.zip

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

190 + optional_value<MirPlacementHints> placement_hints;
191 + optional_value<MirPlacementGravity> surface_placement_gravity;
192 + optional_value<MirPlacementGravity> aux_rect_placement_gravity;
193 + optional_value<int> aux_rect_placement_offset_x;
194 + optional_value<int> aux_rect_placement_offset_y;

It seems like these values only ever make sense to be set together? Should this be rolled into a single
optional_value<SurfaceAttachmentParameters>?

Otherwise looks sensible to me. There's presumably a follow-up message where Mir tells the client where the surface was actually placed?

> 190 + optional_value<MirPlacementHints> placement_hints;
> 191 + optional_value<MirPlacementGravity> surface_placement_gravity;
> 192 + optional_value<MirPlacementGravity> aux_rect_placement_gravity;
> 193 + optional_value<int> aux_rect_placement_offset_x;
> 194 + optional_value<int> aux_rect_placement_offset_y;
>
> It seems like these values only ever make sense to be set together? Should
> this be rolled into a single
> optional_value<SurfaceAttachmentParameters>?

I did think about that, but there's an overlap with...

    optional_value<geometry::Rectangle> aux_rect;
    optional_value<MirEdgeAttachment> edge_attachment;

...which once might also have been said to only make sense together. In the end I decided
that this imposed fewer restrictions on future evolution of the code. It only affects a small part of the system.

> Otherwise looks sensible to me. There's presumably a follow-up message where
> Mir tells the client where the surface was actually placed?

I've intentionally left that out of scope for this MP as we don't currently notify the result of edge attachments and our policy on what placement information to disclose isn't clear to me.

Looks ok to me. API comprehensive enough for GTK's needs.

I guess the one-way requesting without confirmation or denial of the requests still is something that confuses me a bit in general about the placement/surface_spec api. Notwithstanding that, don't mind a request to place a surface in a certain way.

curiousity (since your testing opinions are typically more well-formed than mine)

+ EXPECT_TRUE(spec.placement_hints.is_set());
+ if (spec.placement_hints.is_set())
+ EXPECT_THAT(spec.placement_hints.value(), Eq(placement_hints));

why not:
ASSERT_TRUE(spec.placement_hints.is_set());
EXPECT_THAT(spec.placement_hints.value(), Eq(placement_hints));

nits/needs fixing:
extra indentation:
+ mir_placement_hints_flip_any = mir_placement_hints_flip_x|mir_placement_hints_flip_y,
+ mir_placement_hints_slide_any = mir_placement_hints_slide_x|mir_placement_hints_slide_y,
+ mir_placement_hints_resize_any = mir_placement_hints_resize_x|mir_placement_hints_resize_y,

+ optional_value<int> aux_rect_placement_offset_x;
+ optional_value<int> aux_rect_placement_offset_y;
why not optional_value<geom::Displacement>?

> I guess the one-way requesting without confirmation or denial of the requests
> still is something that confuses me a bit in general about the
> placement/surface_spec api. Notwithstanding that, don't mind a request to
> place a surface in a certain way.
>
> curiousity (since your testing opinions are typically more well-formed than
> mine)
>
> + EXPECT_TRUE(spec.placement_hints.is_set());
> + if (spec.placement_hints.is_set())
> + EXPECT_THAT(spec.placement_hints.value(), Eq(placement_hints));
>
> why not:
> ASSERT_TRUE(spec.placement_hints.is_set());
> EXPECT_THAT(spec.placement_hints.value(), Eq(placement_hints));

Because this callback is happening on the wrong thread, we want a report on any incorrect expectations and to ensure we raise the signal (not wait for a timeout).

> nits/needs fixing:
> extra indentation:
> + mir_placement_hints_flip_any =
> mir_placement_hints_flip_x|mir_placement_hints_flip_y,
> + mir_placement_hints_slide_any =
> mir_placement_hints_slide_x|mir_placement_hints_slide_y,
> + mir_placement_hints_resize_any =
> mir_placement_hints_resize_x|mir_placement_hints_resize_y,

Fixed.

> + optional_value<int> aux_rect_placement_offset_x;
> + optional_value<int> aux_rect_placement_offset_y;
> why not optional_value<geom::Displacement>?

We have more regular code if we match the protobuf structures:

+ COPY_IF_SET(aux_rect_placement_offset_x);
+ COPY_IF_SET(aux_rect_placement_offset_y);
...
+ COPY_IF_SET(aux_rect_placement_offset_x);
+ COPY_IF_SET(aux_rect_placement_offset_y);

The MirAL interface (which is what I want downstreams to use) does wrap this:

+ if (params.aux_rect_placement_offset_x.is_set() && params.aux_rect_placement_offset_y.is_set())
+ aux_rect_placement_offset = Displacement{params.aux_rect_placement_offset_x.value(), params.aux_rect_placement_offset_y.value()};

> Because this callback is happening on the wrong thread, we want a report on
> any incorrect expectations

I.e. Not just the first.

alright, lgtm then

Looks good, it isn't clear to me how the positioning feedback will work exactly. Will it be via a configure event? Or is that postponed to another branch?

> Looks good, it isn't clear to me how the positioning feedback will work
> exactly. Will it be via a configure event? Or is that postponed to another
> branch?

I left position feedback out of this MP because it will cause discussion. I need to do some investigation to determine the minimum information needed, and if there's prior art to follow in Weston.

FAILED: Continuous integration, rev:3686
https://mir-jenkins.ubuntu.com/job/mir-ci/1607/
Executed test runs:

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

FAILED: Continuous integration, rev:3688
https://mir-jenkins.ubuntu.com/job/mir-ci/1618/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/2026/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2087
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2078
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2078
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2078
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2052/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2052/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2052
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2052/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2052
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2052/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/2052
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2052/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2052
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2052/artifact/output/*zip*/output.zip

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

10:09:23 11: [ FAILED ] NestedServer.animated_cursor_image_changes_are_forwarded_to_host (6334 ms)

Known flaky test: lp:1523621

FAILED: Autolanding.
More details in the following jenkins job:
https://mir-jenkins.ubuntu.com/job/mir-autolanding/564/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/2046/console
    None: https://mir-jenkins.ubuntu.com/job/generic-land-mp/603/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2108/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2099/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2099/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2099/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2072/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2072/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2072/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2072/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2072/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2072/console

I'm good with this.

FAILED: Autolanding.
More details in the following jenkins job:
https://mir-jenkins.ubuntu.com/job/mir-autolanding/565/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/2047/console
    None: https://mir-jenkins.ubuntu.com/job/generic-land-mp/604/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2109/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2100/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2100/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2100/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2073/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2073/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2073/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2073/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2073/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2073/console