Merge lp:~andreas-pokorny/mir/fix-1664610 into lp:mir

[Andreas Pokorny]

mirclient: Offer a utf8 representation of the text a key press should produce (LP: #1664610)

The new client API mir_keyboard_event_key_text returns the text a key combination or key sequence generates in the current state of the keyboard.

To properly get the text of a key entry sequence the individual xkb key symbols do not contain enough information. The current xkb_state or xkb_compose_state is needed for that.

This change produces the key text within the XKBMapper and offers that information attached to each mir_keyboard event.

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

FAILED: Continuous integration, rev:4036
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~andreas-pokorny/mir/fix-1664610/+merge/317440/+edit-commit-message

https://mir-jenkins.ubuntu.com/job/mir-ci/3008/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/4010
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/4096
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/4086
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/4086
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/4086
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4037
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4037/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/4037
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/4037/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4037
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4037/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/4037
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/4037/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/4037
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/4037/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/4037
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/4037/artifact/output/*zip*/output.zip

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

review: Needs Fixing (continuous-integration)

[Brandon Schaefer (brandontschaefer)]

xkb_state_key_get_utf8(...)

All uses of this dont assign a null term on the end of the buffer when it is then assigned to a std::string which depends on a null term char*.

review: Needs Fixing

[Brandon Schaefer (brandontschaefer)]

Actually mis-read the API. It'll always be null terminated.

review: Approve

[Daniel van Vugt (vanvugt)]

Cool.

But why should Mir offer this at all?

Users could just call this:
   xkb_state_key_get_utf8(state, mir_keyboard_event_scan_code(e), buffer, sizeof(buffer));
if only they knew what 'state' was...?

review: Approve

[Daniel van Vugt (vanvugt)]

OK, I think you answered my question in the description.

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

review: Approve (continuous-integration)

=== modified file 'include/client/mir_toolkit/events/input/keyboard_event.h'
--- include/client/mir_toolkit/events/input/keyboard_event.h	2017-01-18 02:29:37 +0000
+++ include/client/mir_toolkit/events/input/keyboard_event.h	2017-02-16 10:52:53 +0000
@@ -82,6 +82,18 @@
 int mir_keyboard_event_scan_code(MirKeyboardEvent const* event);
 
 /**
+ * Retrieve the text the key press would emit as null terminated utf8 string.
+ *
+ * The text will only be available to key down and key repeat events.
+ * For mir_keyboard_action_up or key presses that do produce text an empty
+ * string will be returned.
+ *
+ *   \param [in] event The key event
+ *   \return           The text
+ */
+char const* mir_keyboard_event_key_text(MirKeyboardEvent const* event);
+
+/**
  * Retrieve the modifier keys pressed when the key action occured.
  *
  *   \param [in] event The key event

=== modified file 'include/test/mir/test/event_matchers.h'
--- include/test/mir/test/event_matchers.h	2017-01-27 04:57:53 +0000
+++ include/test/mir/test/event_matchers.h	2017-02-16 10:52:53 +0000
@@ -156,6 +156,18 @@
     return true;
 }
 
+MATCHER_P(KeyWithText, text, "")
+{
+    auto kev = maybe_key_event(to_address(arg));
+    if (kev == nullptr)
+        return false;
+
+    if(strcmp(mir_keyboard_event_key_text(kev), text))
+        return false;
+
+    return true;
+}
+
 MATCHER_P(KeyOfScanCode, code, "")
 {
     auto kev = maybe_key_event(to_address(arg));

=== modified file 'src/capnproto/mir_event.capnp'
--- src/capnproto/mir_event.capnp	2017-02-15 13:36:35 +0000
+++ src/capnproto/mir_event.capnp	2017-02-16 10:52:53 +0000
@@ -52,6 +52,7 @@
         down @1;
         repeat @2;
     }
+    text @3 :Text;
 }
 
 struct TouchScreenEvent

=== modified file 'src/client/input/input_event.cpp'
--- src/client/input/input_event.cpp	2017-02-15 13:36:35 +0000
+++ src/client/input/input_event.cpp	2017-02-16 10:52:53 +0000
@@ -143,6 +143,11 @@
     return kev->modifiers();
 })
 
+char const* mir_keyboard_event_key_text(MirKeyboardEvent const* kev) MIR_HANDLE_EVENT_EXCEPTION(
+{
+    return kev->text();
+})
+
 /* Touch event accessors */
 
 MirInputEventModifiers mir_touch_event_modifiers(MirTouchEvent const* tev) MIR_HANDLE_EVENT_EXCEPTION(

=== modified file 'src/client/input/xkb_mapper.cpp'
--- src/client/input/xkb_mapper.cpp	2017-02-15 07:38:33 +0000
+++ src/client/input/xkb_mapper.cpp	2017-02-16 10:52:53 +0000
@@ -313,13 +313,15 @@
     state = make_unique_state(keymap.get());
     modifier_state = mir_input_event_modifier_none;
     std::unordered_set<uint32_t> pressed_codes;
+    std::string t;
     for (uint32_t scan_code : key_state)
     {
         bool already_pressed = pressed_codes.count(scan_code) > 0;
 
         update_state(to_xkb_scan_code(scan_code),
                      (already_pressed) ? mir_keyboard_action_up : mir_keyboard_action_down,
-                     nullptr);
+                     nullptr,
+                     t);
 
         if (already_pressed)
             pressed_codes.erase(scan_code);
@@ -333,8 +335,12 @@
     auto& key_ev = *event.to_input()->to_keyboard();
     uint32_t xkb_scan_code = to_xkb_scan_code(key_ev.scan_code());
     auto old_state = modifier_state;
+    std::string key_text;
+    xkb_keysym_t key_sym;
+    key_sym = update_state(xkb_scan_code, key_ev.action(), compose_state, key_text);
 
-    key_ev.set_key_code(update_state(xkb_scan_code, key_ev.action(), compose_state));
+    key_ev.set_key_code(key_sym);
+    key_ev.set_text(key_text.c_str());
     // TODO we should also indicate effective/consumed modifier state to properly
     // implement short cuts with keys that are only reachable via modifier keys
     key_ev.set_modifiers(expand_modifiers(modifier_state));
@@ -342,13 +348,21 @@
     return old_state != modifier_state;
 }
 
-xkb_keysym_t mircv::XKBMapper::XkbMappingState::update_state(uint32_t scan_code, MirKeyboardAction action, mircv::XKBMapper::ComposeState* compose_state)
+xkb_keysym_t mircv::XKBMapper::XkbMappingState::update_state(uint32_t scan_code, MirKeyboardAction action, mircv::XKBMapper::ComposeState* compose_state, std::string& text)
 {
     auto key_sym = xkb_state_key_get_one_sym(state.get(), scan_code);
     auto mod_change = xkb_key_code_to_modifier(key_sym);
 
+    if(action == mir_keyboard_action_down || action == mir_keyboard_action_repeat)
+    {
+        char buffer[7];
+        // scan code? really? not keysym?
+        xkb_state_key_get_utf8(state.get(), scan_code, buffer, sizeof(buffer));
+        text = buffer;
+    }
+
     if (compose_state)
-        key_sym = compose_state->update_state(key_sym, action);
+        key_sym = compose_state->update_state(key_sym, action, text);
 
     if (action == mir_keyboard_action_up)
     {
@@ -405,25 +419,31 @@
 
     auto const key_sym = key_ev.key_code();
     auto const action = key_ev.action();
-    key_ev.set_key_code(update_state(key_sym, action));
+    std::string text;
+    key_ev.set_key_code(update_state(key_sym, action, text));
+    key_ev.set_text(text.c_str());
 }
 
-xkb_keysym_t mircv::XKBMapper::ComposeState::update_state(xkb_keysym_t mapped_key, MirKeyboardAction action)
+xkb_keysym_t mircv::XKBMapper::ComposeState::update_state(xkb_keysym_t mapped_key, MirKeyboardAction action, std::string& mapped_text)
 {
     // the state machine only cares about downs
     if (action == mir_keyboard_action_down)
     {
         if (xkb_compose_state_feed(state.get(), mapped_key) == XKB_COMPOSE_FEED_ACCEPTED)
         {
-            auto result =  xkb_compose_state_get_status(state.get());
+            auto result = xkb_compose_state_get_status(state.get());
             if (result == XKB_COMPOSE_COMPOSED)
             {
+                char buffer[7];
+                xkb_compose_state_get_utf8(state.get(), buffer, sizeof(buffer));
+                mapped_text = buffer;
                 auto composed_key_sym = xkb_compose_state_get_one_sym(state.get());
                 last_composed_key = std::make_tuple(mapped_key, composed_key_sym);
                 return composed_key_sym;
             }
             else if (result == XKB_COMPOSE_COMPOSING)
             {
+                mapped_text = "";
                 consumed_keys.insert(mapped_key);
                 return XKB_KEY_NoSymbol;
             }
@@ -440,12 +460,24 @@
             mapped_key == std::get<0>(last_composed_key.value()))
         {
             if (action == mir_keyboard_action_up)
+            {
+                mapped_text = "";
                 return std::get<1>(last_composed_key.consume());
+            }
             else
+            {
+                // on repeat get the text of the compose state
+                char buffer[7];
+                xkb_compose_state_get_utf8(state.get(), buffer, sizeof(buffer));
+                mapped_text = buffer;
                 return std::get<1>(last_composed_key.value());
+            }
         }
         if (consumed_keys.erase(mapped_key))
+        {
+            mapped_text = "";
             return XKB_KEY_NoSymbol;
+        }
     }
 
     return mapped_key;

=== modified file 'src/client/symbols.map'
--- src/client/symbols.map	2017-02-16 04:47:36 +0000
+++ src/client/symbols.map	2017-02-16 10:52:53 +0000
@@ -609,4 +609,5 @@
     mir_connection_apply_session_input_config;
     mir_connection_set_base_input_config;
     mir_get_client_api_version;
+    mir_keyboard_event_key_text;
 } MIR_CLIENT_0.26.1;

=== modified file 'src/common/events/keyboard_event.cpp'
--- src/common/events/keyboard_event.cpp	2017-01-18 02:29:37 +0000
+++ src/common/events/keyboard_event.cpp	2017-02-16 10:52:53 +0000
@@ -55,4 +55,12 @@
     event.getInput().getKey().setScanCode(scan_code);
 }
 
+char const* MirKeyboardEvent::text() const
+{
+    return event.asReader().getInput().getKey().getText().cStr();
+}
 
+void MirKeyboardEvent::set_text(char const* str)
+{
+    event.getInput().getKey().setText(str);
+}

=== modified file 'src/common/symbols.map'
--- src/common/symbols.map	2017-02-15 14:45:41 +0000
+++ src/common/symbols.map	2017-02-16 10:52:53 +0000
@@ -445,9 +445,11 @@
 MIR_COMMON_0.27 {
  global:
   extern "C++" {
+      MirInputDeviceStateEvent::set_window_id*;
+      MirInputDeviceStateEvent::window_id*;
+      MirInputEvent::set_window_id*;
       MirInputEvent::window_id*;
-      MirInputEvent::set_window_id*;
-      MirInputDeviceStateEvent::window_id*;
-      MirInputDeviceStateEvent::set_window_id*;
+      MirKeyboardEvent::set_text*;
+      MirKeyboardEvent::text*;
   };
 } MIR_COMMON_0.26;

=== modified file 'src/include/common/mir/events/keyboard_event.h'
--- src/include/common/mir/events/keyboard_event.h	2017-01-18 02:29:37 +0000
+++ src/include/common/mir/events/keyboard_event.h	2017-02-16 10:52:53 +0000
@@ -37,6 +37,9 @@
 
     int32_t scan_code() const;
     void set_scan_code(int32_t scan_code);
+
+    char const* text() const;
+    void set_text(char const* str);
 };
 
 #endif /* MIR_COMMON_KEYBOARD_EVENT_H_ */

=== modified file 'src/include/common/mir/input/xkb_mapper.h'
--- src/include/common/mir/input/xkb_mapper.h	2017-02-15 07:38:33 +0000
+++ src/include/common/mir/input/xkb_mapper.h	2017-02-16 10:52:53 +0000
@@ -80,7 +80,8 @@
     {
         ComposeState(XKBComposeTablePtr const& table);
         void update_and_map(MirEvent& event);
-        xkb_keysym_t update_state(xkb_keysym_t mapped_key, MirKeyboardAction action);
+        xkb_keysym_t update_state(xkb_keysym_t mapped_key, MirKeyboardAction action, std::string& text);
+    private:
         XKBComposeStatePtr state;
         std::unordered_set<xkb_keysym_t> consumed_keys;
         mir::optional_value<std::tuple<xkb_keysym_t,xkb_keysym_t>> last_composed_key;
@@ -92,7 +93,7 @@
         void set_key_state(std::vector<uint32_t> const& key_state);
 
         bool update_and_map(MirEvent& event, ComposeState* compose_state);
-        xkb_keysym_t update_state(uint32_t scan_code, MirKeyboardAction direction, ComposeState* compose_state);
+        xkb_keysym_t update_state(uint32_t scan_code, MirKeyboardAction direction, ComposeState* compose_state, std::string& text);
         MirInputEventModifiers modifiers() const;
     private:
         void press_modifier(MirInputEventModifiers mod);

=== modified file 'tests/acceptance-tests/test_client_input.cpp'
--- tests/acceptance-tests/test_client_input.cpp	2017-02-15 07:38:33 +0000
+++ tests/acceptance-tests/test_client_input.cpp	2017-02-16 10:52:53 +0000
@@ -1002,6 +1002,43 @@
     mir_input_config_release(config);
 }
 
+TEST_F(TestClientInput, key_event_contains_text_to_append)
+{
+    Client a_client(new_connection(), first);
+
+    EXPECT_CALL(a_client, handle_input(mt::KeyWithText("x")))
+        .WillOnce(mt::WakeUp(&a_client.all_events_received));
+
+    fake_keyboard->emit_event(mis::a_key_down_event().of_scancode(KEY_X));
+    a_client.all_events_received.wait_for(10s);
+}
+
+TEST_F(TestClientInput, key_event_text_applies_shift_modifiers)
+{
+    Client a_client(new_connection(), first);
+
+    EXPECT_CALL(a_client, handle_input(AllOf(mt::KeyWithText(""), mt::KeyOfSymbol(XKB_KEY_Shift_L))));
+    EXPECT_CALL(a_client, handle_input(mt::KeyWithText("W")))
+        .WillOnce(mt::WakeUp(&a_client.all_events_received));
+
+    fake_keyboard->emit_event(mis::a_key_down_event().of_scancode(KEY_LEFTSHIFT));
+    fake_keyboard->emit_event(mis::a_key_down_event().of_scancode(KEY_W));
+    a_client.all_events_received.wait_for(10s);
+}
+
+TEST_F(TestClientInput, on_ctrl_c_key_event_text_is_end_of_text)
+{
+    Client a_client(new_connection(), first);
+
+    EXPECT_CALL(a_client, handle_input(AllOf(mt::KeyWithText(""), mt::KeyOfSymbol(XKB_KEY_Control_R))));
+    EXPECT_CALL(a_client, handle_input(mt::KeyWithText("\003")))
+        .WillOnce(mt::WakeUp(&a_client.all_events_received));
+
+    fake_keyboard->emit_event(mis::a_key_down_event().of_scancode(KEY_RIGHTCTRL));
+    fake_keyboard->emit_event(mis::a_key_down_event().of_scancode(KEY_C));
+    a_client.all_events_received.wait_for(10s);
+}
+
 TEST_F(TestClientInput, num_lock_is_off_on_startup)
 {
     Client a_client(new_connection(), first);

=== modified file 'tests/unit-tests/client/input/test_xkb_mapper.cpp'
--- tests/unit-tests/client/input/test_xkb_mapper.cpp	2017-01-18 02:29:37 +0000
+++ tests/unit-tests/client/input/test_xkb_mapper.cpp	2017-02-16 10:52:53 +0000
@@ -438,3 +438,64 @@
     EXPECT_EQ(XKB_KEY_u, map_key(local_mapper, keyboard, mir_keyboard_action_down, KEY_U));
     EXPECT_EQ(XKB_KEY_u, map_key(local_mapper, keyboard, mir_keyboard_action_up, KEY_U));
 }
+
+namespace
+{
+struct XKBMapperWithUsKeymap : XKBMapper
+{
+    MirInputDeviceId keyboard = MirInputDeviceId{0};
+    XKBMapperWithUsKeymap()
+    {
+        mapper.set_keymap_for_all_devices(mi::Keymap{"pc105", "us", "intl",""});
+    }
+};
+}
+
+TEST_F(XKBMapperWithUsKeymap, key_event_contains_text_to_append)
+{
+    EXPECT_CALL(*this, mapped_event(mt::KeyWithText("u")));
+    EXPECT_CALL(*this, mapped_event(mt::KeyWithText("")));
+
+    map_event(keyboard, mir_keyboard_action_down, KEY_U);
+    map_event(keyboard, mir_keyboard_action_up, KEY_U);
+}
+
+TEST_F(XKBMapperWithUsKeymap, key_event_text_applies_shift_modifiers)
+{
+    EXPECT_CALL(*this, mapped_event(AllOf(mt::KeyWithText(""), mt::KeyOfSymbol(XKB_KEY_Shift_L))));
+    EXPECT_CALL(*this, mapped_event(mt::KeyWithText("U")));
+    EXPECT_CALL(*this, mapped_event(AllOf(mt::KeyWithText(""), mt::KeyOfSymbol(XKB_KEY_U))));
+
+    map_event(keyboard, mir_keyboard_action_down, KEY_LEFTSHIFT);
+    map_event(keyboard, mir_keyboard_action_down, KEY_U);
+    map_event(keyboard, mir_keyboard_action_up, KEY_U);
+}
+
+TEST_F(XKBMapperWithUsKeymap, on_ctrl_c_key_event_text_is_u0003)
+{
+    EXPECT_CALL(*this, mapped_event(AllOf(mt::KeyWithText(""), mt::KeyOfSymbol(XKB_KEY_Control_R))));
+    EXPECT_CALL(*this, mapped_event(AllOf(mt::KeyWithText("\003"), mt::KeyOfSymbol(XKB_KEY_c))));
+    EXPECT_CALL(*this, mapped_event(AllOf(mt::KeyWithText(""), mt::KeyOfSymbol(XKB_KEY_c))));
+
+    map_event(keyboard, mir_keyboard_action_down, KEY_RIGHTCTRL);
+    map_event(keyboard, mir_keyboard_action_down, KEY_C);
+    map_event(keyboard, mir_keyboard_action_up, KEY_C);
+}
+
+TEST_F(XKBMapperWithUsKeymap, compose_key_sequence_contains_text_on_key_down)
+{
+    InSequence seq;
+    EXPECT_CALL(*this, mapped_event(AllOf(mt::KeyWithText(""),mt::KeyOfSymbol(XKB_KEY_Shift_R))));
+    EXPECT_CALL(*this, mapped_event(AllOf(mt::KeyWithText(""),mt::KeyOfSymbol(XKB_KEY_NoSymbol))));
+    EXPECT_CALL(*this, mapped_event(AllOf(mt::KeyWithText(""),mt::KeyOfSymbol(XKB_KEY_NoSymbol))));
+    EXPECT_CALL(*this, mapped_event(AllOf(mt::KeyWithText(""),mt::KeyOfSymbol(XKB_KEY_Shift_R))));
+    EXPECT_CALL(*this, mapped_event(AllOf(mt::KeyWithText("ü"),mt::KeyOfSymbol(XKB_KEY_udiaeresis))));
+    EXPECT_CALL(*this, mapped_event(AllOf(mt::KeyWithText(""),mt::KeyOfSymbol(XKB_KEY_udiaeresis))));
+
+    map_event(keyboard, mir_keyboard_action_down, KEY_RIGHTSHIFT);
+    map_event(keyboard, mir_keyboard_action_down, KEY_APOSTROPHE);
+    map_event(keyboard, mir_keyboard_action_up, KEY_APOSTROPHE);
+    map_event(keyboard, mir_keyboard_action_up, KEY_RIGHTSHIFT);
+    map_event(keyboard, mir_keyboard_action_down, KEY_U);
+    map_event(keyboard, mir_keyboard_action_up, KEY_U);
+}