Merge lp:~vanvugt/mir/fix-1522673 into lp:mir

[Daniel van Vugt]

Restore usable mouse wheel support (LP: #1522673)

With the introduction of libinput, mouse wheel (vscroll) events became
backwards and 15x larger than they were with Android Input. So this
reverses that change such that existing toolkits and apps using Mir
don't need modification.

Android Input: Up = +1.0 Down= -1.0
libinput: Up =-15.0 Down=+15.0

I have also modified mir_proving_server to become insensitive to any
similar magnitude changes in future.

[Alexandros Frantzis (afrantzis)]

Can we safely assume that the vscroll values (-15,+15) produced by libinput are the same for all devices?

review: Needs Information

[Daniel van Vugt (vanvugt)]

I think so, yes. Because:

(a) It was always [1,-1] before with all mice and always [-15,+15] now with all (my) mice; and
(b) If you use 'evtest' to look at the raw kernel events, they originate from button 4/5 down/up events. So there is no device-specific scale that changes.

I would like to dig into libinput still and find out why they chose 15...

[Alan Griffiths (alan-griffiths)]

I'd rather we understood the change than just work around it.

review: Abstain

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3177
http://jenkins.qa.ubuntu.com/job/mir-ci/5774/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5203
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4109
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5151
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-xenial-touch/99/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/103
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/103/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/103
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/103/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5151
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5151/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/7681
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/25811
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/97/console

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

review: Approve (continuous-integration)

[Andreas Pokorny (andreas-pokorny)]

This is the probably the wrong place to a apply a general compensating scaling. Since this can be changed by users soon.

I did not realize that android-input up-(right?) for positive values.
Down-Right is the positive direction in libinput.

A single tick on a mouse wheel on many mice relates to a 15 degree rotation, some scroll wheels do look different and may have finer grained ticks. In that case there is hopefully a udev rule to detect that and provide the property MOUSE_WHEEL_CLICK_ANGLE. Which libinput would then use instead of the default 15.

Libinput does not claim that the value provided is an angle, but rather 'scroll units'.

Because in the past we did forward the raw ticks qtmir applied a scaling factor of 15 to our events.
http://bazaar.launchpad.net/~mir-team/qtmir/trunk/view/head:/src/platforms/mirserver/qteventfeeder.cpp#L573

- note raw ticks that might relate to a different movement angle depending on the granularity of the wheel surface -

Gtk+ on the other hand forwarded the scroll value without scaling so they might work better now:
https://git.gnome.org/browse/gtk+/tree/gdk/mir/gdkmireventsource.c#n183

Even though we could get access to the raw ticks, by further checking the input device event. I suggest we stick with the scroll units - since then we would benefit from per device udev rules.

So we could do the following:
1) Scale every scroll event with (x:1/15, y:-1/15) not by affecting the use configured value, but by scalling libinputs result
2) change gtk to multiply with 15.0
3) keep qtmirs scaling
4) keep proving_server 1:1 mapping

Or
1) forward libinputs result
2) keep gtk 1:1 mapping
3) change qtmirs scaling to just forward
4) change proving servers interpretation of scroll events

review: Needs Fixing

[Andreas Pokorny (andreas-pokorny)]

I think we should take the second option from above because then qtmir would behave like the servers in mir -> no additional scaling is applied to events..

[Andreas Pokorny (andreas-pokorny)]

I went through our downstreams, and I have to correct some of the statements above
especially:

> Gtk+ on the other hand forwarded the scroll value without scaling so they
> might work better now:
> https://git.gnome.org/browse/gtk+/tree/gdk/mir/gdkmireventsource.c#n183

The back-end implementation is correct since gtk expects ticks here..
Additionally the qtmir frontend properly forwards only the 'ticks' to clients and not the scaled scroll values. So at least for 0.18 we should go this way:

1) Scale every scroll event with (horizontal:1/15, vertical:1/15) or take the original tick count value and scale the gesture/finger base scrolls.
2) keep gtk
3) keep qtmirs scaling
4) keep proving_server 1:1 mapping

In the long run it seems more correct to switch to scroll values. We could achieve that by adding additional axes to query.

[Daniel van Vugt (vanvugt)]

See bug 1522673 for two alternative branches that will probably replace this one.

Unmerged revisions

3177. By Daniel van Vugt on 2015-12-04

And protect Alt+mousewheel from magnitude changes too.

3176. By Daniel van Vugt on 2015-12-04

Restore usable mouse wheel support (LP: #1522673)

With the introduction of libinput, mouse wheel (vscroll) events became
backwards and 15x larger than they were with Android Input. So this
reverses that change such that existing toolkits and apps using Mir
don't need modification.

Android Input: Up = +1.0 Down= -1.0
libinput: Up =-15.0 Down=+15.0

I have also modified mir_proving_server to become insensitive to any
similar magnitude changes in future.

=== modified file 'include/test/mir/test/event_matchers.h'
--- include/test/mir/test/event_matchers.h	2015-11-30 16:34:46 +0000
+++ include/test/mir/test/event_matchers.h	2015-12-04 09:04:57 +0000
@@ -308,7 +308,7 @@
         return false;
     if (mir_pointer_event_action(pev) != mir_pointer_action_motion)
         return false;
-    if (mir_pointer_event_axis_value(pev, scroll_axis) != value)
+    if (fabs(mir_pointer_event_axis_value(pev, scroll_axis) - value) > 0.001f)
         return false;
     return true;
 }

=== modified file 'playground/demo-shell/window_manager.cpp'
--- playground/demo-shell/window_manager.cpp	2015-08-07 05:46:34 +0000
+++ playground/demo-shell/window_manager.cpp	2015-12-04 09:04:57 +0000
@@ -387,10 +387,11 @@
 
     float new_zoom_mag = 0.0f;  // zero means unchanged
 
-   if (modifiers & mir_input_event_modifier_meta &&
-       action == mir_pointer_action_motion)
-   {
-        zoom_exponent += vscroll;
+    if (modifiers & mir_input_event_modifier_meta &&
+        action == mir_pointer_action_motion &&
+        vscroll != 0.0f)
+    {
+        zoom_exponent += vscroll > 0.0f ? 1.0f : -1.0f;
 
         // Negative exponents do work too, but disable them until
         // there's a clear edge to the desktop.
@@ -449,7 +450,7 @@
         vscroll)
     {
         float alpha = surf->alpha();
-        alpha += 0.1f * vscroll;
+        alpha += vscroll > 0.0f ? 0.1f : -0.1f;
         if (alpha < 0.0f)
             alpha = 0.0f;
         else if (alpha > 1.0f)

=== modified file 'src/platforms/evdev/libinput_device.h'
--- src/platforms/evdev/libinput_device.h	2015-11-30 16:34:46 +0000
+++ src/platforms/evdev/libinput_device.h	2015-12-04 09:04:57 +0000
@@ -88,8 +88,8 @@
     InputDeviceInfo info;
     mir::geometry::Point pointer_pos;
     MirPointerButtons button_state;
-    double vertical_scroll_scale{1.0};
-    double horizontal_scroll_scale{1.0};
+    double vertical_scroll_scale{-0.0667f}; // <- Compatible with android input
+    double horizontal_scroll_scale{1.0};    // <- Might be wrong. Never tested.
 
     struct ContactData
     {

=== modified file 'tests/unit-tests/input/evdev/test_libinput_device.cpp'
--- tests/unit-tests/input/evdev/test_libinput_device.cpp	2015-11-30 16:34:46 +0000
+++ tests/unit-tests/input/evdev/test_libinput_device.cpp	2015-12-04 09:04:57 +0000
@@ -616,8 +616,8 @@
     InSequence seq;
     // expect two scroll events..
     EXPECT_CALL(mock_builder,
-                pointer_event(time_stamp_1, mir_pointer_action_motion, 0, 0.0f, 20.0f, 0.0f, 0.0f));
-    EXPECT_CALL(mock_sink, handle_input(mt::PointerAxisChange(mir_pointer_axis_vscroll, 20.0f)));
+                pointer_event(time_stamp_1, mir_pointer_action_motion, 0, 0.0f, FloatEq(-1.334f), 0.0f, 0.0f));
+    EXPECT_CALL(mock_sink, handle_input(mt::PointerAxisChange(mir_pointer_axis_vscroll, -1.334f)));
     EXPECT_CALL(mock_builder,
                 pointer_event(time_stamp_2, mir_pointer_action_motion, 0, 5.0f, 0.0f, 0.0f, 0.0f));
     EXPECT_CALL(mock_sink, handle_input(mt::PointerAxisChange(mir_pointer_axis_hscroll, 5.0f)));
@@ -755,6 +755,7 @@
 
 TEST_F(LibInputDeviceOnMouse, reads_pointer_settings_from_libinput)
 {
+    // XXX Is this test useful?
     setup_pointer_configuration(mouse.device(), 1, mir_pointer_handedness_right);
     auto optional_settings = mouse.get_pointer_settings();
 
@@ -764,7 +765,7 @@
     EXPECT_THAT(ptr_settings.handedness, Eq(mir_pointer_handedness_right));
     EXPECT_THAT(ptr_settings.cursor_acceleration_bias, Eq(1.0));
     EXPECT_THAT(ptr_settings.horizontal_scroll_scale, Eq(1.0));
-    EXPECT_THAT(ptr_settings.vertical_scroll_scale, Eq(1.0));
+    EXPECT_THAT(ptr_settings.vertical_scroll_scale, Lt(0.0f));
 
     setup_pointer_configuration(mouse.device(), 0.0, mir_pointer_handedness_left);
     optional_settings = mouse.get_pointer_settings();
@@ -775,7 +776,7 @@
     EXPECT_THAT(ptr_settings.handedness, Eq(mir_pointer_handedness_left));
     EXPECT_THAT(ptr_settings.cursor_acceleration_bias, Eq(0.0));
     EXPECT_THAT(ptr_settings.horizontal_scroll_scale, Eq(1.0));
-    EXPECT_THAT(ptr_settings.vertical_scroll_scale, Eq(1.0));
+    EXPECT_THAT(ptr_settings.vertical_scroll_scale, Lt(0.0f));
 }
 
 TEST_F(LibInputDeviceOnMouse, applies_pointer_settings)