Merge lp:~attente/mir/basic-menu-surface-position into lp:mir

[William Hua]

Use attachment_rect for surface position when available.

Use attachment_rect for surface position when available.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2244
http://jenkins.qa.ubuntu.com/job/mir-ci/2795/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1043/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1042/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1004/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/792/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1004/console

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

review: Needs Fixing (continuous-integration)

[Alan Griffiths (alan-griffiths)]

1. Doesn't compile - see below
2. Is wrong - the placement should be relative to the parent window

[ 34%] Building CXX object src/server/shell/CMakeFiles/mirshell.dir/default_placement_strategy.cpp.o
/mir/src/server/shell/default_placement_strategy.cpp: In member function 'virtual mir::scene::SurfaceCreationParameters mir::shell::DefaultPlacementStrategy::place(const mir::scene::Session&, const mir::scene::SurfaceCreationParameters&)':
/mir/src/server/shell/default_placement_strategy.cpp:46:27: error: 'struct mir::scene::SurfaceCreationParameters' has no member named 'attachment_rect'
     if (placed_parameters.attachment_rect.is_set())
                           ^
/mir/src/server/shell/default_placement_strategy.cpp:47:54: error: 'struct mir::scene::SurfaceCreationParameters' has no member named 'attachment_rect'
       placed_parameters.top_left = placed_parameters.attachment_rect.value().bottom_left();
                                                      ^
src/server/shell/CMakeFiles/mirshell.dir/build.make:54: recipe for target 'src/server/shell/CMakeFiles/mirshell.dir/default_placement_strategy.cpp.o' failed
make[2]: *** [src/server/shell/CMakeFiles/mirshell.dir/default_placement_strategy.cpp.o] Error 1
CMakeFiles/Makefile2:3151: recipe for target 'src/server/shell/CMakeFiles/mirshell.dir/all' failed

review: Needs Fixing

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2248
http://jenkins.qa.ubuntu.com/job/mir-ci/2810/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1063
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1062
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1024
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/807
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/807/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1024
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1024/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4083
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/17475

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

review: Approve (continuous-integration)

[lp:~attente/mir/basic-menu-surface-position]

2249. By William Hua on 2015-01-26

Give surfaces a default depth.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2249
http://jenkins.qa.ubuntu.com/job/mir-ci/2813/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1068
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1067
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1029
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/810
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/810/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1029
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1029/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4089
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/17484

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Also keep in mind PlacementStrategy only represents initial surface placement. Most of the complexity is in what happens when a surface moves. So PlacementStrategy becomes less significant than coordinated move() logic for surfaces and their children.

In fact, move() policy could theoretically subsume PlacementStrategy in future. I don't think we need both.

[Daniel van Vugt (vanvugt)]

(1)-(2) What Alan said.

(3) I'm pretty sure this depth feature only exists to support the test framework. It shouldn't have a use in production (should it?) ...
34 + placed_parameters.of_depth(ms::DepthId(depth));

review: Needs Fixing

[William Hua (attente)]

Thanks, (1) and (2) should be fixed now.

For (3), I used the depth because child surfaces were ending up on the same layer as their parents, which led to them (child surfaces) being obscured. I'm not sure if there's a more proper way to do that though. I guess I just assumed that child surfaces were generally above their parents. Is there another way to specify the position of the surface in the stacking order?

[Alan Griffiths (alan-griffiths)]

I think this approach is doomed - DefaultPlacementStrategy only affects the initial placement of the window and, while abusing the depth does put the menu on top of the parent it also puts it on top of other surfaces too (even ones have focus).

To be effective there needs to be more "plumbing" in the window management to handle

It ignores client preferences as to where to locate the menu WRT to placement rectangle boundaries and allows both the parent and the menu to be moved independently.

review: Disapprove

[Cemil Azizoglu (cemil-azizoglu)]

Hi William,
Thanks for the MP. However, we (the Mir team) should probably carefully consider what sort of plumbing should be laid out first. I recommend you raise this as a bug (unless one already exists) describing what's not working for you and we'll take a look. Please help us prioritize by setting the bug importance according to your situation. Thanks.

Unmerged revisions

2249. By William Hua on 2015-01-26

Give surfaces a default depth.

2248. By William Hua on 2015-01-26

Position relative to parent.

2247. By William Hua on 2015-01-26

attachment_rect -> aux_rect

2246. By William Hua on 2015-01-26

Merge lp:~andreas-pokorny/mir/fix-1414630.

2245. By William Hua on 2015-01-26

Merge lp:mir.

2244. By William Hua on 2015-01-23

Use attachment_rect for menu surface position.

=== modified file 'src/server/shell/default_placement_strategy.cpp'
--- src/server/shell/default_placement_strategy.cpp	2015-01-14 06:39:13 +0000
+++ src/server/shell/default_placement_strategy.cpp	2015-01-26 23:10:43 +0000
@@ -17,6 +17,7 @@
  */
 
 #include "default_placement_strategy.h"
+#include "mir/scene/surface.h"
 #include "mir/scene/surface_creation_parameters.h"
 #include "mir/shell/display_layout.h"
 #include "mir/geometry/rectangle.h"
@@ -43,7 +44,27 @@
         mir_display_output_id_invalid};
     auto placed_parameters = request_parameters;
 
-    geom::Rectangle rect{request_parameters.top_left, request_parameters.size};
+    if (placed_parameters.aux_rect.is_set())
+        placed_parameters.top_left = placed_parameters.aux_rect.value().bottom_left();
+
+    if (auto parent = placed_parameters.parent.lock())
+    {
+        auto origin = parent->top_left();
+        placed_parameters.top_left = placed_parameters.top_left + geom::DeltaX(origin.x.as_int());
+        placed_parameters.top_left = placed_parameters.top_left + geom::DeltaY(origin.y.as_int());
+
+        auto depth = 1;
+
+        while (parent)
+        {
+            parent = parent->parent();
+            ++depth;
+        }
+
+        placed_parameters.of_depth(ms::DepthId(depth));
+    }
+
+    geom::Rectangle rect{placed_parameters.top_left, placed_parameters.size};
 
     if (request_parameters.output_id != output_id_invalid)
     {