Merge lp:~andreas-pokorny/mir/cursor-position-on-rotated-output into lp:mir

[Andreas Pokorny]

This change updates the cursor position according to the orientation of the output.

Still outstanding is a rotation of the cursor image itself.

Uses the rotated rectangle for bounds checking in mesa cursor and transforms the position offset that falls within the rectangle according to the orientation. This still does not provide a rotated cursor image.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1440
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/969/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1094
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1092
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/683
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/701
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/701/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/706
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/706/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/684
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/684/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/652
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/4431

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/969/rebuild

review: Approve (continuous-integration)

[Robert Carr (robertcarr)]

LGTM.

The MesaCursor tests might look a little nicer if the constants in: 13 +
214 + EXPECT_CALL(*output_container.outputs[12], move_cursor(geom::Point{112,100}));
215 + EXPECT_CALL(*output_container.outputs[12], move_cursor(geom::Point{150,96}));
216 +
217 + cursor.move_to({766, 112});
218 + cursor.move_to({770, 150});

et al were written symbolically (i.e. in terms of output width and cursor movement vectors)

Whitespace at 239

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1441
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/972/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1097
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1095
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/686
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/704
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/704/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/709
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/709/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/687
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/687/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/655
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/4439

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/972/rebuild

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Nice. Works for me. :)

This function:
21 +geom::Displacement transform(geom::Rectangle const& rect, geom::Displacement const& vector, MirOrientation orientation)
appears complicated but I think it needs to be.

P.S. I lobbied for the wrapped int types to be pure ints a while back. That would eliminate all the "as_int()" calls. But I was out-voted.

review: Approve

[Daniel van Vugt (vanvugt)]

P.S. About rotating the cursor image -- Just rotating the image won't be enough. You would have to rotate it around the hotspot. Even right now while you can't change the hotspot (bug 1189775), simply rotating the image we load would put the cursor in the wrong place.

[Alan Griffiths (alan-griffiths)]

25 + case mir_orientation_left:
26 + return {vector.dy.as_int(), rect.size.width.as_int() -vector.dx.as_int()};
27 + case mir_orientation_inverted:
28 + return {rect.size.width.as_int() -vector.dx.as_int(), rect.size.height.as_int() - vector.dy.as_int()};
29 + case mir_orientation_right:
30 + return {rect.size.height.as_int() -vector.dy.as_int(), vector.dx.as_int()};

Maybe it is time we added enough matrix algebra to our geometry component to write:

   return geom::rotate(geom::degrees(90)) * vector;

etc. And stopped writing as_int() all over the domain code.

review: Approve

=== modified file 'src/platform/graphics/mesa/cursor.cpp'
--- src/platform/graphics/mesa/cursor.cpp	2014-01-22 10:03:11 +0000
+++ src/platform/graphics/mesa/cursor.cpp	2014-03-03 16:21:52 +0000
@@ -28,15 +28,32 @@
 #include <stdexcept>
 #include <vector>
 
+namespace mgm = mir::graphics::mesa;
+namespace geom = mir::geometry;
+
 namespace
 {
 #include "black_arrow.c"
 int const width = black_arrow.width;
 int const height = black_arrow.height;
-}
 
-namespace mgm = mir::graphics::mesa;
-namespace geom = mir::geometry;
+// Transforms a relative position within the display bounds described by \a rect which is rotated with \a orientation
+geom::Displacement transform(geom::Rectangle const& rect, geom::Displacement const& vector, MirOrientation orientation)
+{
+    switch(orientation)
+    {
+    case mir_orientation_left:
+        return {vector.dy.as_int(), rect.size.width.as_int() -vector.dx.as_int()};
+    case mir_orientation_inverted:
+        return {rect.size.width.as_int() -vector.dx.as_int(), rect.size.height.as_int() - vector.dy.as_int()};
+    case mir_orientation_right:
+        return {rect.size.height.as_int() -vector.dy.as_int(), vector.dx.as_int()};
+    default:
+    case mir_orientation_normal:
+        return vector;
+    }
+}
+}
 
 mgm::Cursor::GBMBOWrapper::GBMBOWrapper(gbm_device* gbm) :
     buffer(gbm_bo_create(
@@ -103,7 +120,7 @@
 }
 
 void mgm::Cursor::for_each_used_output(
-    std::function<void(KMSOutput&, geom::Rectangle const&)> const& f)
+    std::function<void(KMSOutput&, geom::Rectangle const&, MirOrientation orientation)> const& f)
 {
     current_configuration->with_current_configuration_do(
         [this,&f](KMSDisplayConfiguration const& kms_conf)
@@ -115,13 +132,7 @@
                     uint32_t const connector_id = kms_conf.get_kms_connector_id(conf_output.id);
                     auto output = output_container.get_kms_output_for(connector_id);
 
-                    // TODO: Cursor rotation support (conf_output.extents())
-                    geom::Rectangle output_rect
-                    {
-                        conf_output.top_left,
-                        conf_output.modes[conf_output.current_mode_index].size
-                    };
-                    f(*output, output_rect);
+                    f(*output, conf_output.extents(), conf_output.orientation);
                 }
             });
         });
@@ -131,15 +142,15 @@
     geometry::Point position,
     ForceCursorState force_state)
 {
-    for_each_used_output([&](KMSOutput& output, geom::Rectangle const& output_rect)
+    for_each_used_output([&](KMSOutput& output, geom::Rectangle const& output_rect, MirOrientation orientation)
     {
         if (output_rect.contains(position))
         {
-            auto dp = position - output_rect.top_left;
+            auto dp = transform(output_rect, position - output_rect.top_left, orientation);
             output.move_cursor({dp.dx.as_int(), dp.dy.as_int()});
-            if (force_state || !output.has_cursor())
+            if (force_state || !output.has_cursor()) // TODO - or if orientation had changed - then set buffer..
             {
-                output.set_cursor(buffer);
+                output.set_cursor(buffer);// TODO - select rotated buffer image
             }
         }
         else
 
=== modified file 'src/platform/graphics/mesa/cursor.h'
--- src/platform/graphics/mesa/cursor.h	2014-01-22 10:03:11 +0000
+++ src/platform/graphics/mesa/cursor.h	2014-03-03 16:21:52 +0000
@@ -21,6 +21,7 @@
 #define MIR_GRAPHICS_MESA_CURSOR_H_
 
 #include "mir/graphics/cursor.h"
+#include "mir_toolkit/common.h"
 
 #include <gbm.h>
 #include <memory>
@@ -73,7 +74,7 @@
 
 private:
     enum ForceCursorState { UpdateState, ForceState };
-    void for_each_used_output(std::function<void(KMSOutput&, geometry::Rectangle const&)> const& f);
+    void for_each_used_output(std::function<void(KMSOutput&, geometry::Rectangle const&, MirOrientation orientation)> const& f);
     void place_cursor_at(geometry::Point position, ForceCursorState force_state);
 
     KMSOutputContainer& output_container;
 
=== modified file 'tests/unit-tests/graphics/mesa/test_cursor.cpp'
--- tests/unit-tests/graphics/mesa/test_cursor.cpp	2014-02-13 09:38:32 +0000
+++ tests/unit-tests/graphics/mesa/test_cursor.cpp	2014-03-03 16:21:52 +0000
@@ -22,12 +22,14 @@
 #include "src/platform/graphics/mesa/kms_display_configuration.h"
 
 #include "mir_test_doubles/mock_gbm.h"
+#include "mir_test/fake_shared.h"
 #include "mock_kms_output.h"
 
 #include <gtest/gtest.h>
 #include <gmock/gmock.h>
 
 #include <unordered_map>
+#include <algorithm>
 
 namespace mg = mir::graphics;
 namespace mgm = mir::graphics::mesa;
@@ -43,7 +45,8 @@
     StubKMSOutputContainer()
         : outputs{
             {10, std::make_shared<testing::NiceMock<MockKMSOutput>>()},
-            {11, std::make_shared<testing::NiceMock<MockKMSOutput>>()}}
+            {11, std::make_shared<testing::NiceMock<MockKMSOutput>>()},
+            {12, std::make_shared<testing::NiceMock<MockKMSOutput>>()}}
     {
     }
 
@@ -112,6 +115,26 @@
                 mir_power_mode_on,
                 mir_orientation_normal
             });
+        outputs.push_back(
+            {
+                mg::DisplayConfigurationOutputId{12},
+                card_id,
+                mg::DisplayConfigurationOutputType::vga,
+                {},
+                {
+                    {geom::Size{800, 200}, 59.9},
+                    {geom::Size{100, 200}, 59.9},
+                },
+                0,
+                geom::Size{800, 200},
+                true,
+                true,
+                geom::Point{666, 0},
+                0,
+                mir_pixel_format_invalid,
+                mir_power_mode_on,
+                mir_orientation_right
+            });
     }
 
     void for_each_card(std::function<void(mg::DisplayConfigurationCard const&)> f) const override
@@ -148,6 +171,16 @@
     {
     }
 
+    void set_orentation_of_output(mg::DisplayConfigurationOutputId id, MirOrientation orientation)
+    {
+        auto output = std::find_if(outputs.begin(), outputs.end(),
+                                   [id] (mg::DisplayConfigurationOutput const& output) -> bool {return output.id == id;});
+        if (output != outputs.end())
+        {
+            output->orientation = orientation;
+        }
+    }
+
     mg::DisplayConfigurationCardId card_id;
     std::vector<mg::DisplayConfigurationOutput> outputs;
 };
@@ -167,10 +200,11 @@
 {
     MesaCursorTest()
         : cursor{mock_gbm.fake_gbm.device, output_container,
-                 std::make_shared<StubCurrentConfiguration>()}
+            mir::test::fake_shared(current_configuration)}
     {
     }
 
+    StubCurrentConfiguration current_configuration;
     testing::NiceMock<mtd::MockGBM> mock_gbm;
     StubKMSOutputContainer output_container;
     mgm::Cursor cursor;
@@ -224,10 +258,12 @@
     EXPECT_CALL(*output_container.outputs[10], move_cursor(geom::Point{0,0}));
     EXPECT_CALL(*output_container.outputs[10], set_cursor(_));
     EXPECT_CALL(*output_container.outputs[11], clear_cursor());
+    EXPECT_CALL(*output_container.outputs[12], clear_cursor());
 
     /* No checking of existing cursor state */
     EXPECT_CALL(*output_container.outputs[10], has_cursor()).Times(0);
     EXPECT_CALL(*output_container.outputs[11], has_cursor()).Times(0);
+    EXPECT_CALL(*output_container.outputs[12], has_cursor()).Times(0);
 
     mgm::Cursor cursor_tmp{mock_gbm.fake_gbm.device, output_container,
                               std::make_shared<StubCurrentConfiguration>()};
@@ -328,12 +364,60 @@
     output_container.verify_and_clear_expectations();
 }
 
+TEST_F(MesaCursorTest, moves_properly_to_and_inside_left_rotated_output)
+{
+    using namespace testing;
+
+    current_configuration.conf.set_orentation_of_output(mg::DisplayConfigurationOutputId{12}, mir_orientation_left);
+
+    EXPECT_CALL(*output_container.outputs[12], move_cursor(geom::Point{112,100}));
+    EXPECT_CALL(*output_container.outputs[12], move_cursor(geom::Point{150,96}));
+
+    cursor.move_to({766, 112});
+    cursor.move_to({770, 150});
+
+    output_container.verify_and_clear_expectations();
+}
+
+
+TEST_F(MesaCursorTest, moves_properly_to_and_inside_right_rotated_output)
+{
+    using namespace testing;
+
+    current_configuration.conf.set_orentation_of_output(mg::DisplayConfigurationOutputId{12}, mir_orientation_right);
+
+
+    EXPECT_CALL(*output_container.outputs[12], move_cursor(geom::Point{688,100}));
+    EXPECT_CALL(*output_container.outputs[12], move_cursor(geom::Point{650,104}));
+
+    cursor.move_to({766, 112});
+    cursor.move_to({770, 150});
+
+    output_container.verify_and_clear_expectations();
+}
+
+TEST_F(MesaCursorTest, moves_properly_to_and_inside_inverted_output)
+{
+    using namespace testing;
+
+    current_configuration.conf.set_orentation_of_output(mg::DisplayConfigurationOutputId{12}, mir_orientation_inverted);
+
+    EXPECT_CALL(*output_container.outputs[12], move_cursor(geom::Point{700,88}));
+    EXPECT_CALL(*output_container.outputs[12], move_cursor(geom::Point{696,50}));
+
+    cursor.move_to({766, 112});
+    cursor.move_to({770, 150});
+
+    output_container.verify_and_clear_expectations();
+}
+
 TEST_F(MesaCursorTest, hides_cursor_in_all_outputs)
 {
     using namespace testing;
 
     EXPECT_CALL(*output_container.outputs[10], clear_cursor());
     EXPECT_CALL(*output_container.outputs[11], clear_cursor());
+    EXPECT_CALL(*output_container.outputs[12], clear_cursor());
 
     cursor.hide();
 
@@ -346,4 +430,5 @@
 
     EXPECT_CALL(*output_container.outputs[10], clear_cursor());
     EXPECT_CALL(*output_container.outputs[11], clear_cursor());
+    EXPECT_CALL(*output_container.outputs[12], clear_cursor());
 }