Merge lp:~alan-griffiths/mir/fix-1603922 into lp:mir

[Alan Griffiths]

Deprecate useless mir_connection_create_spec_for_tooltip() and add mir_connection_create_spec_for_tip()

[Mir CI Bot (mir-ci-bot)]

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

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

It's regrettable that the new function name is hard to understand, because the old good name is already taken.

Can we think of a better name? mir_connection_create_spec_for_tooltip2 ?

review: Needs Information

[Daniel van Vugt (vanvugt)]

mir_connection_create_spec_for_tooltip_edge ?
mir_connection_create_spec_for_tooltip_with_edge ?

[Daniel van Vugt (vanvugt)]

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

> It's regrettable that the new function name is hard to understand, because the
> old good name is already taken.
>
> Can we think of a better name? mir_connection_create_spec_for_tooltip2 ?

1. the corresponding surface type is: mir_surface_type_tip

2. From the (internal) discussion that led to this MP:

    On 17/07/16 13:17, Matthew Paul Thomas wrote:
    > Maybe that would be a good time to start calling the type “tip”, to
    > avoid the impression that it’s just for tooltips.

[Gerry Boland (gerboland)]

"tip" is what we're calling this surface type in the Mir surface spec. Makes sense to use that terminology IMO.

[Kevin DuBois (kdub)]

seems the naming discussion has a lot more context behind it, but my 2cents is that tooltip and tip are synonyms. (abstain on that point)

small needs fixing
+ int width,
+ int height,
we should check that its a positive number. (or use unsigned int and check that its not 0x0)

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

> small needs fixing
> + int width,
> + int height,
> we should check that its a positive number. (or use unsigned int and check
> that its not 0x0)

Curiously, we don't do that for mir_connection_create_spec_for_menu()

[Cemil Azizoglu (cemil-azizoglu)]

LGTM (modulo the width/height being positive).

review: Approve

[Daniel van Vugt (vanvugt)]

I know our design docs use the word 'tip' but that doesn't make it ideal. I'm aiming for something that existing toolkit developers will understand. They will grep for 'tooltip' and never find it.

Remember the Mir API should be a superset of the Unity design in order to support other shells and existing desktop concepts that are omitted from the Unity design.

We should not be betting on Unity8 being the winning shell. Because if it doesn't get sufficient traction and Mir is not more flexible than the Unity design, then Mir too will suffer as a platform that can't support other shells.

review: Abstain

[Daniel van Vugt (vanvugt)]

OK, too much tangential explanation. But you understand my concerns I hope. Abstain so we can make progress.

review: Abstain

[Alan Griffiths (alan-griffiths)]

> > small needs fixing
> > + int width,
> > + int height,
> > we should check that its a positive number. (or use unsigned int and check
> > that its not 0x0)
>
> Curiously, we don't do that for mir_connection_create_spec_for_menu()

We don't check this for *any* of the existing mir_connection_create_spec_for_XXX() APIs and they all use "int" too. It is unclear what you believe the client side should do for nonpositive values:

1. consider it a precondition violation and:
1.1 check and abort() or
1.2 don't check as its the caller's responsibility.

2. make the "contract" wide and return an error object

As things stand the window manager isn't constrained by the requested and does something sensible with it.

Note also that these attributes of the spec can be updated before calling the server.

[Alan Griffiths (alan-griffiths)]

Should I rebase on lp:~alan-griffiths/mir/add-mir_surface_spec_attach and use MirPlacement?

review: Needs Information

[Mir CI Bot (mir-ci-bot)]

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

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

review: Approve (continuous-integration)

[Kevin DuBois (kdub)]

re checking for 0x0, its just a concern that the user will hit #1597717-like bugs (where we can get stuck in surface creation. We should be doing this for all surfaces. Here seems like a good place to start, although if we want to fix all the surface creation stuff at once, I don't mind a followup MP.

rewriting to 0x0 to be 1x1 seems like the solution we've been using in the past, although 2) from the menu of options seems like the right way.

[Alan Griffiths (alan-griffiths)]

Personally I'm happy with the existing approach: it's a precondition & we don't need to check.

Agreed, the server should withstand such values if they are ever sent (in a way that applies to all surfaces). I *think* I've seen code that handles this for all surface types, but am not sure where it is.

Maybe a test is in order that goes through all the surface create functions with bad sizes?

If, hypothetically, we adopt your suggestion and client updates a valid spec with mir_surface_spec_set_width()/mir_surface_spec_set_height() - how would you handle bad values here? Turning the existing spec into an error object?

[Kevin DuBois (kdub)]

filed LP: #1604874, so no need to fix in this MP

review: Approve

[Alan Griffiths (alan-griffiths)]

> Should I rebase on lp:~alan-griffiths/mir/add-mir_surface_spec_attach and use
> MirPlacement?

I'll withdraw the question as we can handle this API along with other placement functions

review: Abstain

[Mir CI Bot (mir-ci-bot)]

FAILED: Autolanding.
More details in the following jenkins job:
https://mir-jenkins.ubuntu.com/job/mir-autolanding/414/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/1569/console
    None: https://mir-jenkins.ubuntu.com/job/generic-land-mp/444/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/1622
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/1613
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/1613
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/1613
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/1584
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/1584/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/1584
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/1584/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/1584/console
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/1584/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/1584/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/1584/console

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

Network problems but also a few of these (?!):
17:06:09 /home/phablet/mir-testrunner/mir-smoke-test-runner.sh: line 24: 11321 Segmentation fault (core dumped) ${root}mir_demo_server --test-timeout=$timeout --test-client $root$client

[Mir CI Bot (mir-ci-bot)]

FAILED: Autolanding.
More details in the following jenkins job:
https://mir-jenkins.ubuntu.com/job/mir-autolanding/415/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/1576/console
    None: https://mir-jenkins.ubuntu.com/job/generic-land-mp/445/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/1629
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/1620
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/1620
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/1620
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/1591
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/1591/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/1591
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/1591/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/1591/console
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/1591/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/1591
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/1591/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/1591
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/1591/artifact/output/*zip*/output.zip

review: Needs Fixing (continuous-integration)

[Alan Griffiths (alan-griffiths)]

Unrelated failure...

05:14:50 /home/phablet/mir-testrunner/mir-smoke-test-runner.sh: line 24: 10647 Segmentation fault (core dumped) ${root}mir_demo_server --test-timeout=$timeout --test-client $root$client
05:14:50 I: [FAILED] mir_demo_client_input_shapes

...but worrying

[Mir CI Bot (mir-ci-bot)]

review: Approve (continuous-integration)

=== modified file 'examples/tooltip.c'
--- examples/tooltip.c	2015-06-17 05:20:42 +0000
+++ examples/tooltip.c	2016-07-20 10:29:25 +0000
@@ -102,8 +102,8 @@
     MirRectangle zone = { 0, 0, 10, 10 };
     int const width = 50;
     int const height = 20;
-    MirSurfaceSpec* const spec = mir_connection_create_spec_for_tooltip(
-        connection, width, height, format, parent, &zone);
+    MirSurfaceSpec* const spec = mir_connection_create_spec_for_tip(
+        connection, width, height, format, parent, &zone, mir_edge_attachment_vertical);
 
     mir_surface_spec_set_buffer_usage(spec, mir_buffer_usage_software);
     mir_surface_spec_set_name(spec, "tooltip");

=== modified file 'include/client/mir_toolkit/mir_surface.h'
--- include/client/mir_toolkit/mir_surface.h	2016-06-09 00:38:26 +0000
+++ include/client/mir_toolkit/mir_surface.h	2016-07-20 10:29:25 +0000
@@ -103,9 +103,10 @@
  *                          return a surface of this height.
  * \param [in] format       Pixel format for the surface.
  * \param [in] parent       A valid parent surface for this tooltip.
- * \param [in] rect         A target zone relative to parent.
+ * \param [in] zone         A target zone relative to parent.
  * \return                  A handle that can be passed to mir_surface_create()
  *                          to complete construction.
+ *\deprecated use mir_connection_create_spec_for_tip() instead
  */
 MirSurfaceSpec*
 mir_connection_create_spec_for_tooltip(MirConnection* connection,
@@ -113,7 +114,42 @@
                                        int height,
                                        MirPixelFormat format,
                                        MirSurface* parent,
-                                       MirRectangle* zone);
+                                       MirRectangle* zone)
+    __attribute__((deprecated));
+
+/**
+ * Create a surface specification for a tip surface.
+ *
+ * Positioning of the surface is specified with respect to the parent surface
+ * via an adjacency rectangle. The server will attempt to choose an edge of the
+ * adjacency rectangle on which to place the surface taking in to account
+ * screen-edge proximity or similar constraints. In addition, the server can use
+ * the edge affinity hint to consider only horizontal or only vertical adjacency
+ * edges in the given rectangle.
+ *
+ * \param [in] connection   Connection the surface will be created on
+ * \param [in] width        Requested width. The server is not guaranteed to
+ *                          return a surface of this width.
+ * \param [in] height       Requested height. The server is not guaranteed to
+ *                          return a surface of this height.
+ * \param [in] format       Pixel format for the surface.
+ * \param [in] parent       A valid parent surface for this tip.
+ * \param [in] rect         The adjacency rectangle. The server is not
+ *                          guaranteed to create a surface at the requested
+ *                          location.
+ * \param [in] edge         The preferred edge direction to attach to. Use
+ *                          mir_edge_attachment_any for no preference.
+ * \return                  A handle that can be passed to mir_surface_create()
+ *                          to complete construction.
+ */
+MirSurfaceSpec*
+mir_connection_create_spec_for_tip(MirConnection* connection,
+                                   int width,
+                                   int height,
+                                   MirPixelFormat format,
+                                   MirSurface* parent,
+                                   MirRectangle* rect,
+                                   MirEdgeAttachment edge);
 
 /**
  * Create a surface specification for a modal dialog surface.

=== modified file 'src/client/mir_surface_api.cpp'
--- src/client/mir_surface_api.cpp	2016-06-08 13:49:11 +0000
+++ src/client/mir_surface_api.cpp	2016-07-20 10:29:25 +0000
@@ -88,7 +88,25 @@
     auto spec = new MirSurfaceSpec{connection, width, height, format};
     spec->type = mir_surface_type_tip;
     spec->parent = parent;
+    return spec;
+}
+
+MirSurfaceSpec* mir_connection_create_spec_for_tip(MirConnection* connection,
+                                                   int width,
+                                                   int height,
+                                                   MirPixelFormat format,
+                                                   MirSurface* parent,
+                                                   MirRectangle* rect,
+                                                   MirEdgeAttachment edge)
+{
+    mir::require(mir_surface_is_valid(parent));
+    mir::require(rect != nullptr);
+
+    auto spec = new MirSurfaceSpec{connection, width, height, format};
+    spec->type = mir_surface_type_tip;
+    spec->parent = parent;
     spec->aux_rect = *rect;
+    spec->edge_attachment = edge;
     return spec;
 }
 

=== modified file 'src/client/symbols.map'
--- src/client/symbols.map	2016-07-07 09:54:02 +0000
+++ src/client/symbols.map	2016-07-20 10:29:25 +0000
@@ -415,3 +415,9 @@
     mir_input_device_state_event_device_pointer_buttons;
     mir_surface_spec_set_pointer_confinement;
 } MIR_CLIENT_0.22;
+
+MIR_CLIENT_0.25 {  # New functions in Mir 0.25
+  global:
+    mir_connection_create_spec_for_tip;
+} MIR_CLIENT_0.24;
+

=== modified file 'tests/acceptance-tests/test_client_surfaces.cpp'
--- tests/acceptance-tests/test_client_surfaces.cpp	2016-05-03 06:55:25 +0000
+++ tests/acceptance-tests/test_client_surfaces.cpp	2016-07-20 10:29:25 +0000
@@ -244,6 +244,8 @@
     mir_surface_release_sync(menu);
 }
 
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
 TEST_F(ClientSurfaces, can_be_tooltips)
 {
     auto parent = mtf::make_any_surface(connection);
@@ -262,6 +264,26 @@
     mir_surface_release_sync(parent);
     mir_surface_release_sync(tooltip);
 }
+#pragma GCC diagnostic pop
+
+TEST_F(ClientSurfaces, can_be_tips)
+{
+    auto parent = mtf::make_any_surface(connection);
+    MirRectangle rect{100, 200, 100, 100};
+
+    auto spec = mir_connection_create_spec_for_tip(connection, 640, 480,
+        mir_pixel_format_abgr_8888, parent, &rect, mir_edge_attachment_any);
+    ASSERT_THAT(spec, NotNull());
+
+    auto tooltip = mir_surface_create_sync(spec);
+    mir_surface_spec_release(spec);
+
+    ASSERT_THAT(tooltip, IsValid());
+    EXPECT_EQ(mir_surface_get_type(tooltip), mir_surface_type_tip);
+
+    mir_surface_release_sync(parent);
+    mir_surface_release_sync(tooltip);
+}
 
 TEST_F(ClientSurfaces, can_be_dialogs)
 {

=== modified file 'tests/integration-tests/client/test_mirsurface.cpp'
--- tests/integration-tests/client/test_mirsurface.cpp	2016-01-29 08:18:22 +0000
+++ tests/integration-tests/client/test_mirsurface.cpp	2016-07-20 10:29:25 +0000
@@ -196,25 +196,25 @@
     create_surface(menu_spec.get());
 }
 
-TEST_F(ClientMirSurface, as_tooltip_sends_correct_params)
+TEST_F(ClientMirSurface, as_tip_sends_correct_params)
 {
     EXPECT_CALL(*mock_shell, create_surface(_,_,_));
     auto parent = create_surface(&spec);
     ASSERT_THAT(parent.get(), IsValid());
 
-    MirRectangle target_zone_rect{100, 200, 300, 400};
+    MirRectangle placement_hint{100, 200, 300, 400};
 
     auto spec_deleter = [](MirSurfaceSpec* spec) {mir_surface_spec_release(spec);};
     std::unique_ptr<MirSurfaceSpec, decltype(spec_deleter)> tooltip_spec{
-        mir_connection_create_spec_for_tooltip(connection, 640, 480,
-            mir_pixel_format_abgr_8888, parent.get(), &target_zone_rect),
+        mir_connection_create_spec_for_tip(connection, 640, 480,
+            mir_pixel_format_abgr_8888, parent.get(), &placement_hint, mir_edge_attachment_vertical),
         spec_deleter
     };
 
     ASSERT_THAT(tooltip_spec, NotNull());
 
     EXPECT_CALL(*mock_shell, create_surface(_, AllOf(IsATooltip(),
-        HasParent(parent.get()), MatchesAuxRect(target_zone_rect)),_));
+        HasParent(parent.get()), MatchesAuxRect(placement_hint)),_));
     create_surface(tooltip_spec.get());
 }