Merge lp:~vanvugt/mir/modifier-groups into lp:mir

[Daniel van Vugt]

Fix wasteful allocation of modifier bit flags that allowed for impossible combinations.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2232
http://jenkins.qa.ubuntu.com/job/mir-ci/2677/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/890
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/890
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/852/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/674
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/674/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/852
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/852/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3964/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/17147

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

review: Needs Fixing (continuous-integration)

[Cemil Azizoglu (cemil-azizoglu)]

Wouldn't

mir_input_event_modifier_alt = mir_input_event_modifier_alt_left | mir_input_event_modifier_alt_right,

have been better?

review: Approve

[Daniel van Vugt (vanvugt)]

I thought the same thing afterwards...

[Daniel van Vugt (vanvugt)]

Just did a quick audit. This stuff appeared in r2129 so first released in Mir 0.10.0. I think that makes it new enough that we can safely change the API without anyone complaining.

[lp:~vanvugt/mir/modifier-groups]

2233. By Daniel van Vugt on 2015-01-19

Merge latest trunk

2234. By Daniel van Vugt on 2015-01-19

Changed syntax per Cemil's comments.

[Daniel van Vugt (vanvugt)]

Changed to the syntax Cemil mentioned. It's a little bit more ugly but also more verbose. Don't know if that's better or worse.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2234
http://jenkins.qa.ubuntu.com/job/mir-ci/2690/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/905
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/905
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/867
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/687
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/687/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/867
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/867/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3978
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/17177

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

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

OK with this as long as there is no good reason for having separate alt/shift/ctrl values in the first place. I vaguely remember some discussions about this, but not the conclusion. Robert?

Also, since this is a breaking change anyway, why not reorganize the enum to avoid holes?

> This stuff appeared in r2129 so first released in Mir 0.10.0. I think that makes it
> new enough that we can safely change the API without anyone complaining.

That's only true as long as no one uses 0.10 and then moves to a later version. I don't think we have the luxury to skip bumps in the client ABI as we have been doing on some occasions on the server side.

review: Needs Information

[Alan Griffiths (alan-griffiths)]

*Needs Discussion*

I know this example is a different enum, but do we plan to support the equivalent of:

            switch (event.modifiers & modifier_mask)
            {
            case mir_key_modifier_alt:
                wm->toggle(mir_surface_state_maximized);
                return true;

(from examples/server_example_window_management.cpp)

review: Needs Information

[Daniel van Vugt (vanvugt)]

Alexandros: Yeah we could fill the holes but I felt that would increase the risk of ABI breaks if all the bits moved instead of just four of them. Same kind of risk, but more limited.

Alan: Your example is certainly unusual and does expect mir_key_modifier_alt to be a single bit (or for the user to hold down both Alts at once :). I don't think that's common usage. More commonly mir_key_modifier_alt will be used as a mask. So in the long run I suspect the masks proposed here are going to be more useful. That said, this is not a critical change either.

[Daniel van Vugt (vanvugt)]

Alan's example code needs modification if this branch is to land. But I'm undecided now.

review: Needs Fixing

[Robert Carr (robertcarr)]

I'm not sure. I suspect both series of flags are inadequate and eventually we will have to use a dynamic modifier system similar to xkb_modmask_t (this can of course be backwards compatible with either this branch or the current devel branches).

I would say its not worth the churn but I didn't think very hard.

review: Abstain

Unmerged revisions

2234. By Daniel van Vugt on 2015-01-19

Changed syntax per Cemil's comments.

2233. By Daniel van Vugt on 2015-01-19

Merge latest trunk

2232. By Daniel van Vugt on 2015-01-16

Fix search/replace error, and improve formatting

2231. By Daniel van Vugt on 2015-01-16

Ensure each modifier with separate left/right keys doesn't get three
separate flag bits. So that one may test for:
    mods & mir_input_event_modifier_shift
without needing to know which shift key it is.

=== modified file 'client-ABI-sha1sums'
--- client-ABI-sha1sums	2015-01-15 13:07:37 +0000
+++ client-ABI-sha1sums	2015-01-19 03:53:36 +0000
@@ -15,7 +15,7 @@
 f4d39e9893ce6308bddd83a49b90f0051f565323  include/common/mir_toolkit/event.h
 2507f2929415aa423f9551d3c595c439fe1c6efd  include/common/mir_toolkit/events/event_deprecated.h
 cb2358a701db2b9c8d9ba1c877f29f834af259a7  include/common/mir_toolkit/events/event.h
-caaf95ea6c02570ab733ade70aba2ecea810b3a7  include/common/mir_toolkit/events/input/input_event.h
+3e86fb1c906b2c9b27760bfef2847b7cd4d79b36  include/common/mir_toolkit/events/input/input_event.h
 7748a12138474e9be218eeb8f14b372af0fcec7e  include/common/mir_toolkit/events/input/key_input_event.h
 489e8bae7c3e949ac449b09bd3bf554dae0e8986  include/common/mir_toolkit/events/input/pointer_input_event.h
 de7c23453e6d897296f32e49d9ba952a1baa0200  include/common/mir_toolkit/events/input/touch_input_event.h

=== modified file 'common-ABI-sha1sums'
--- common-ABI-sha1sums	2015-01-15 13:07:37 +0000
+++ common-ABI-sha1sums	2015-01-19 03:53:36 +0000
@@ -22,7 +22,7 @@
 f4d39e9893ce6308bddd83a49b90f0051f565323  include/common/mir_toolkit/event.h
 2507f2929415aa423f9551d3c595c439fe1c6efd  include/common/mir_toolkit/events/event_deprecated.h
 cb2358a701db2b9c8d9ba1c877f29f834af259a7  include/common/mir_toolkit/events/event.h
-caaf95ea6c02570ab733ade70aba2ecea810b3a7  include/common/mir_toolkit/events/input/input_event.h
+3e86fb1c906b2c9b27760bfef2847b7cd4d79b36  include/common/mir_toolkit/events/input/input_event.h
 7748a12138474e9be218eeb8f14b372af0fcec7e  include/common/mir_toolkit/events/input/key_input_event.h
 489e8bae7c3e949ac449b09bd3bf554dae0e8986  include/common/mir_toolkit/events/input/pointer_input_event.h
 de7c23453e6d897296f32e49d9ba952a1baa0200  include/common/mir_toolkit/events/input/touch_input_event.h

=== modified file 'include/common/mir_toolkit/events/input/input_event.h'
--- include/common/mir_toolkit/events/input/input_event.h	2015-01-05 23:06:03 +0000
+++ include/common/mir_toolkit/events/input/input_event.h	2015-01-19 03:53:36 +0000
@@ -44,20 +44,28 @@
  */
 typedef enum {
     mir_input_event_modifier_none        = 1 << 0,
-    mir_input_event_modifier_alt         = 1 << 1,
+    /* Unused                              1 << 1 */
     mir_input_event_modifier_alt_left    = 1 << 2,
     mir_input_event_modifier_alt_right   = 1 << 3,
-    mir_input_event_modifier_shift       = 1 << 4,
+    mir_input_event_modifier_alt         = mir_input_event_modifier_alt_left
+                                         | mir_input_event_modifier_alt_right,
+    /* Unused                              1 << 4 */
     mir_input_event_modifier_shift_left  = 1 << 5,
     mir_input_event_modifier_shift_right = 1 << 6,
+    mir_input_event_modifier_shift       = mir_input_event_modifier_shift_left
+                                         | mir_input_event_modifier_shift_right,
     mir_input_event_modifier_sym         = 1 << 7,
     mir_input_event_modifier_function    = 1 << 8,
-    mir_input_event_modifier_ctrl        = 1 << 9,
+    /* Unused                              1 << 9 */
     mir_input_event_modifier_ctrl_left   = 1 << 10,
     mir_input_event_modifier_ctrl_right  = 1 << 11,
-    mir_input_event_modifier_meta        = 1 << 12,
+    mir_input_event_modifier_ctrl        = mir_input_event_modifier_ctrl_left
+                                         | mir_input_event_modifier_ctrl_right,
+    /* Unused                              1 << 12 */
     mir_input_event_modifier_meta_left   = 1 << 13,
     mir_input_event_modifier_meta_right  = 1 << 14,
+    mir_input_event_modifier_meta        = mir_input_event_modifier_meta_left
+                                         | mir_input_event_modifier_meta_right,
     mir_input_event_modifier_caps_lock   = 1 << 15,
     mir_input_event_modifier_num_lock    = 1 << 16,
     mir_input_event_modifier_scroll_lock = 1 << 17

=== modified file 'platform-ABI-sha1sums'
--- platform-ABI-sha1sums	2015-01-15 13:07:37 +0000
+++ platform-ABI-sha1sums	2015-01-19 03:53:36 +0000
@@ -22,7 +22,7 @@
 f4d39e9893ce6308bddd83a49b90f0051f565323  include/common/mir_toolkit/event.h
 2507f2929415aa423f9551d3c595c439fe1c6efd  include/common/mir_toolkit/events/event_deprecated.h
 cb2358a701db2b9c8d9ba1c877f29f834af259a7  include/common/mir_toolkit/events/event.h
-caaf95ea6c02570ab733ade70aba2ecea810b3a7  include/common/mir_toolkit/events/input/input_event.h
+3e86fb1c906b2c9b27760bfef2847b7cd4d79b36  include/common/mir_toolkit/events/input/input_event.h
 7748a12138474e9be218eeb8f14b372af0fcec7e  include/common/mir_toolkit/events/input/key_input_event.h
 489e8bae7c3e949ac449b09bd3bf554dae0e8986  include/common/mir_toolkit/events/input/pointer_input_event.h
 de7c23453e6d897296f32e49d9ba952a1baa0200  include/common/mir_toolkit/events/input/touch_input_event.h

=== modified file 'server-ABI-sha1sums'
--- server-ABI-sha1sums	2015-01-15 13:07:37 +0000
+++ server-ABI-sha1sums	2015-01-19 03:53:36 +0000
@@ -22,7 +22,7 @@
 f4d39e9893ce6308bddd83a49b90f0051f565323  include/common/mir_toolkit/event.h
 2507f2929415aa423f9551d3c595c439fe1c6efd  include/common/mir_toolkit/events/event_deprecated.h
 cb2358a701db2b9c8d9ba1c877f29f834af259a7  include/common/mir_toolkit/events/event.h
-caaf95ea6c02570ab733ade70aba2ecea810b3a7  include/common/mir_toolkit/events/input/input_event.h
+3e86fb1c906b2c9b27760bfef2847b7cd4d79b36  include/common/mir_toolkit/events/input/input_event.h
 7748a12138474e9be218eeb8f14b372af0fcec7e  include/common/mir_toolkit/events/input/key_input_event.h
 489e8bae7c3e949ac449b09bd3bf554dae0e8986  include/common/mir_toolkit/events/input/pointer_input_event.h
 de7c23453e6d897296f32e49d9ba952a1baa0200  include/common/mir_toolkit/events/input/touch_input_event.h

=== modified file 'tests/unit-tests/input/test_input_event.cpp'
--- tests/unit-tests/input/test_input_event.cpp	2015-01-14 19:37:28 +0000
+++ tests/unit-tests/input/test_input_event.cpp	2015-01-19 03:53:36 +0000
@@ -88,6 +88,29 @@
     EXPECT_NE(nullptr, mir_event_get_input_event(&motion_ev));
 }
 
+TEST(InputEvent, modifier_families_are_inclusive)
+{
+    EXPECT_TRUE(mir_input_event_modifier_alt_left &
+                mir_input_event_modifier_alt);
+    EXPECT_TRUE(mir_input_event_modifier_alt_right &
+                mir_input_event_modifier_alt);
+
+    EXPECT_TRUE(mir_input_event_modifier_shift_left &
+                mir_input_event_modifier_shift);
+    EXPECT_TRUE(mir_input_event_modifier_shift_right &
+                mir_input_event_modifier_shift);
+
+    EXPECT_TRUE(mir_input_event_modifier_ctrl_left &
+                mir_input_event_modifier_ctrl);
+    EXPECT_TRUE(mir_input_event_modifier_ctrl_right &
+                mir_input_event_modifier_ctrl);
+
+    EXPECT_TRUE(mir_input_event_modifier_meta_left &
+                mir_input_event_modifier_meta);
+    EXPECT_TRUE(mir_input_event_modifier_meta_right &
+                mir_input_event_modifier_meta);
+}
+
 // MirInputEvent properties common to all events.
 
 TEST(CommonInputEventProperties, device_id_taken_from_old_style_event)