[PATCH 2/3 wayland-fits] core: Add a wrapper for the keyboard
Eoff, Ullysses A
ullysses.a.eoff at intel.com
Wed Sep 25 14:08:37 PDT 2013
Comments inline.
> -----Original Message-----
> From: wayland-devel-bounces+ullysses.a.eoff=intel.com at lists.freedesktop.org [mailto:wayland-devel-
> bounces+ullysses.a.eoff=intel.com at lists.freedesktop.org] On Behalf Of Neil Roberts
> Sent: Wednesday, September 25, 2013 11:07 AM
> To: wayland-devel at lists.freedesktop.org
> Subject: [PATCH 2/3 wayland-fits] core: Add a wrapper for the keyboard
>
> This adds a wrapper for a wl_keyboard in a similar way to the pointer
> wrapper. It keeps track of the keys that are pressed so that they can
> be quickly verified. wayland-fits now depends on libxkbcommon so that
> the keyboard wrapper can pass the keymap to it and get the modifier
> indices.
> ---
> configure.ac | 2 +
> src/test/Makefile.am | 2 +
> src/test/core/Makefile.am | 3 +
> src/test/core/display.cpp | 13 +++
> src/test/core/display.h | 3 +
> src/test/core/keyboard.cpp | 213 +++++++++++++++++++++++++++++++++++++++++++++
> src/test/core/keyboard.h | 107 +++++++++++++++++++++++
> src/test/efl/Makefile.am | 2 +
> src/test/gtk+/Makefile.am | 2 +
> 9 files changed, 347 insertions(+)
> create mode 100644 src/test/core/keyboard.cpp
> create mode 100644 src/test/core/keyboard.h
>
> diff --git a/configure.ac b/configure.ac
> index de84adf..5ad59ca 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -61,6 +61,8 @@ PKG_CHECK_MODULES([WAYLAND],
> )
> PKG_CHECK_MODULES(WAYLAND_SERVER, [wayland-server >= wayland_req_version])
>
> +PKG_CHECK_MODULES([XKBCOMMON], xkbcommon)
> +
> have_weston="no"
> want_weston_extensions="auto"
> AC_ARG_ENABLE([weston-extensions],
> diff --git a/src/test/Makefile.am b/src/test/Makefile.am
> index ba5e5ce..e50c0fc 100644
> --- a/src/test/Makefile.am
> +++ b/src/test/Makefile.am
> @@ -14,6 +14,7 @@ wfits_SOURCES = \
> wfits_LDADD = \
> core/libwfits-core.la \
> $(CHECK_LIBS) \
> + $(XKBCOMMON_LIBS) \
> $(WAYLAND_LIBS) \
> $(BOOST_LIBS)
>
> @@ -23,6 +24,7 @@ wfits_LDFLAGS = \
> wfits_CPPFLAGS = \
> -I$(top_srcdir)/src \
> $(CHECK_CFLAGS) \
> + $(XKBCOMMON_CFLAGS) \
> $(WAYLAND_CFLAGS) \
> $(BOOST_CPPFLAGS)
>
> diff --git a/src/test/core/Makefile.am b/src/test/core/Makefile.am
> index bff1c5a..3a93a67 100644
> --- a/src/test/core/Makefile.am
> +++ b/src/test/core/Makefile.am
> @@ -3,6 +3,7 @@ noinst_LTLIBRARIES = libwfits-core.la
> libwfits_core_la_SOURCES = \
> display.cpp \
> compositor.cpp \
> + keyboard.cpp \
> shell.cpp \
> seat.cpp \
> pointer.cpp \
> @@ -23,11 +24,13 @@ libwfits_core_la_SOURCES = \
>
> libwfits_core_la_LIBADD = \
> $(WAYLAND_LIBS) \
> + $(XKBCOMMON_LIBS) \
> $(CHECK_LIBS)
>
> libwfits_core_la_CPPFLAGS = \
> -I$(top_srcdir)/src \
> $(WAYLAND_CFLAGS) \
> + $(XKBCOMMON_CFLAGS) \
> $(CHECK_CFLAGS)
>
> AM_CXXFLAGS = \
> diff --git a/src/test/core/display.cpp b/src/test/core/display.cpp
> index 1c29dbf..5c9a459 100644
> --- a/src/test/core/display.cpp
> +++ b/src/test/core/display.cpp
> @@ -30,6 +30,7 @@ Display::Display()
> : wl_display_(wl_display_connect(0))
> , wl_registry_(NULL)
> , globals_()
> + , xkbContext_(NULL)
> {
> ASSERT(wl_display_ != NULL);
>
> @@ -49,6 +50,8 @@ Display::Display()
>
> /*virtual*/ Display::~Display()
> {
> + if (xkbContext_)
> + xkb_context_unref(xkbContext_);
> wl_registry_destroy(wl_registry_);
> wl_display_disconnect(*this);
> }
> @@ -69,6 +72,16 @@ void Display::yield(const unsigned time) const
> usleep(time);
> }
>
> +struct xkb_context *Display::xkbContext() const
> +{
> + if (xkbContext_ == NULL) {
> + xkbContext_ = xkb_context_new((enum xkb_context_flags) 0);
> + ASSERT(xkbContext_ != NULL);
> + }
> +
> + return xkbContext_;
> +}
> +
> /*static*/ void Display::global(
> void *data, struct wl_registry *wl_registry, uint32_t id,
> const char* interface, uint32_t version)
> diff --git a/src/test/core/display.h b/src/test/core/display.h
> index 0a4c10a..bf09057 100644
> --- a/src/test/core/display.h
> +++ b/src/test/core/display.h
> @@ -25,6 +25,7 @@
>
> #include <map>
> #include <wayland-client.h>
> +#include <xkbcommon/xkbcommon.h>
> #include "test/tools.h"
>
> namespace wfits {
> @@ -63,6 +64,7 @@ public:
> void roundtrip() const;
> void dispatch() const;
> void yield(const unsigned = 0.001 * 1e6) const;
> + struct xkb_context *xkbContext() const;
>
> operator wl_display*() const { return wl_display_; }
> operator wl_registry*() const { return wl_registry_; }
> @@ -74,6 +76,7 @@ private:
> wl_display *wl_display_;
> wl_registry *wl_registry_;
> Globals globals_;
> + mutable struct xkb_context *xkbContext_;
> };
>
So we must only want one xkb context per display. Ok.
> } // namespace core
> diff --git a/src/test/core/keyboard.cpp b/src/test/core/keyboard.cpp
> new file mode 100644
> index 0000000..aa49413
> --- /dev/null
> +++ b/src/test/core/keyboard.cpp
> @@ -0,0 +1,213 @@
> +/*
> + * Copyright © 2013 Intel Corporation
> + *
> + * Permission to use, copy, modify, distribute, and sell this software and
> + * its documentation for any purpose is hereby granted without fee, provided
> + * that the above copyright notice appear in all copies and that both that
> + * copyright notice and this permission notice appear in supporting
> + * documentation, and that the name of the copyright holders not be used in
> + * advertising or publicity pertaining to distribution of the software
> + * without specific, written prior permission. The copyright holders make
> + * no representations about the suitability of this software for any
> + * purpose. It is provided "as is" without express or implied warranty.
> + *
> + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS
> + * SOFTWARE, INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND
> + * FITNESS, IN NO EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY
> + * SPECIAL, INDIRECT OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER
> + * RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF
> + * CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
> + * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> + */
> +
> +#include <sys/mman.h>
> +
> +#include "keyboard.h"
> +
> +namespace wfits {
> +namespace test {
> +namespace core {
> +
> +Keyboard::Keyboard(const Seat& seat)
> + : seat_(seat)
> + , focus_(NULL)
> + , focusSerial_(0)
> + , wl_keyboard_(NULL)
We should explicitly initialize keys_ here with some default:
, keys_()
> + , modsDepressed_(0)
> + , modsLatched_(0)
> + , modsLocked_(0)
> + , group_(0)
> + , keymap_(NULL)
> +{
> + ASSERT(seat.capabilities() & WL_SEAT_CAPABILITY_KEYBOARD);
> +
> + wl_keyboard_ = wl_seat_get_keyboard(seat);
> +
> + ASSERT(wl_keyboard_ != NULL);
> +
> + wl_keyboard_set_user_data(*this, this);
> +
> + static const wl_keyboard_listener listener = {
> + keymap, enter, leave, key, modifiers
> + };
> +
> + wl_keyboard_add_listener(*this, &listener, this);
> +}
> +
> +/*virtual*/ Keyboard::~Keyboard()
> +{
> + if (keymap_)
> + xkb_map_unref(keymap_);
> +
I prefer to always use brackets in all control structures even
if there is only one statement.
> + wl_keyboard_destroy(*this);
> +}
> +
> +bool Keyboard::hasFocus(wl_surface* surface)
> +{
> + return focus() == surface;
> +}
> +
> +bool Keyboard::keyPressed(uint32_t key) const
> +{
> + for (CKeyIterator it = keys_.begin(); it != keys_.end(); ++it)
> + if (*it == key)
> + return true;
> +
Prefer control structure brackets here, too.
In general, if the "end" iterator won't change during the loop execution
then prefer to assign it to a const variable and use that in the for-loop
condition:
const CKeyIterator endIt(keys_.end());
for ( CKeyIterator it(keys_.begin()); it != endIt; ++it)
{
...
}
But, instead of iterating here why not use std::find(...):
const CKeyIterator endIt(keys_.end());
return std::find(keys_.begin(), endIt, key) != endIt;
> + return false;
> +}
> +
> +void Keyboard::pressKey(uint32_t key)
> +{
> + for (KeyIterator it = keys_.begin(); it != keys_.end(); ++it)
> + if (*it == key)
> + return;
> +
> + keys_.push_back(key);
> +}
> +
How about:
if (not keyPressed(key))
{
keys_.push_back(key);
}
> +void Keyboard::releaseKey(uint32_t key)
> +{
> + for (KeyIterator it = keys_.begin(); it != keys_.end(); ++it) {
> + if (*it == key) {
> + *it = keys_.end()[-1];
> + keys_.pop_back();
> + break;
> + }
> + }
> +}
> +
This is an odd way to remove the key from the vector, and was a bit hard
to understand initially, but I suppose it's faster than keys_.erase(...). We
could at least avoid the RandomAccessIterator subscripting syntax and
use keys_.back() instead.
> +/* static */ void Keyboard::keymap(void *data,
> + struct wl_keyboard *wl_keyboard,
> + uint32_t format,
> + int32_t fd,
> + uint32_t size)
> +{
> + Keyboard* keyboard = static_cast<Keyboard*>(data);
> + struct xkb_keymap *keymap;
> + char *map_str;
> +
> + ASSERT(wl_keyboard == *keyboard);
> + ASSERT(format == WL_KEYBOARD_KEYMAP_FORMAT_XKB_V1);
> +
> + map_str = (char *) mmap(NULL, size, PROT_READ, MAP_SHARED, fd, 0);
> + ASSERT(map_str != MAP_FAILED);
> +
> + keymap = xkb_map_new_from_string(keyboard->seat_.display().xkbContext(),
> + map_str,
> + XKB_KEYMAP_FORMAT_TEXT_V1,
> + (enum xkb_keymap_compile_flags) 0);
> + munmap(map_str, size);
> + close(fd);
> +
> + ASSERT(keymap != NULL);
> +
> + if (keyboard->keymap_ != NULL)
> + xkb_map_unref(keyboard->keymap_);
> +
> + keyboard->keymap_ = keymap;
> +}
> +
> +/* static */ void Keyboard::enter(void *data,
> + struct wl_keyboard *wl_keyboard,
> + uint32_t serial,
> + struct wl_surface *surface,
> + struct wl_array *keys)
> +{
> + Keyboard* keyboard = static_cast<Keyboard*>(data);
> + ASSERT(wl_keyboard == *keyboard);
> +
> + keyboard->focus_ = surface;
> + keyboard->focusSerial_ = serial;
> +
> + keyboard->keys_.resize(keys->size / sizeof(uint32_t));
> + memcpy(&keyboard->keys_[0], keys->data, keys->size);
> +}
> +
> +/* static */ void Keyboard::leave(void *data,
> + struct wl_keyboard *wl_keyboard,
> + uint32_t serial,
> + struct wl_surface *surface)
> +{
> + Keyboard* keyboard = static_cast<Keyboard*>(data);
> + ASSERT(wl_keyboard == *keyboard);
> +
> + keyboard->focus_ = NULL;
> + keyboard->keys_.resize(0);
> +
> + /* We don't want to reset the modifiers here because the
> + * compositor may still send updated modifier state if the
> + * surface has pointer focus */
> +}
> +
> +/* static */ void Keyboard::key(void *data,
> + struct wl_keyboard *wl_keyboard,
> + uint32_t serial,
> + uint32_t time,
> + uint32_t key,
> + uint32_t state)
> +{
> + Keyboard* keyboard = static_cast<Keyboard*>(data);
> + ASSERT(wl_keyboard == *keyboard);
> +
> + if (state == 0)
> + keyboard->releaseKey(key);
> + else
> + keyboard->pressKey(key);
> +}
> +
> +/* static */ void Keyboard::modifiers(void *data,
> + struct wl_keyboard *wl_keyboard,
> + uint32_t serial,
> + uint32_t modsDepressed,
> + uint32_t modsLatched,
> + uint32_t modsLocked,
> + uint32_t group)
> +{
> + Keyboard* keyboard = static_cast<Keyboard*>(data);
> + ASSERT(wl_keyboard == *keyboard);
> +
> + keyboard->modsDepressed_ = modsDepressed;
> + keyboard->modsLatched_ = modsLatched;
> + keyboard->modsLocked_ = modsLocked;
> + keyboard->group_ = group;
> +}
> +
> +namespace wrapper {
> +
> + TEST(Keyboard)
> + {
> + Display display;
> + Seat seat(display);
> + Keyboard keyboard(seat);
> +
> + FAIL_UNLESS_EQUAL(&keyboard.seat(), &seat);
> + FAIL_IF((wl_keyboard*)keyboard == NULL);
> + FAIL_UNLESS_EQUAL(wl_keyboard_get_user_data(keyboard), &keyboard);
> + FAIL_UNLESS(keyboard.hasFocus(NULL));
> + }
> +
> +} // namespace wrapper
> +
> +} // namespace core
> +} // namespace test
> +} // namespace wfits
> diff --git a/src/test/core/keyboard.h b/src/test/core/keyboard.h
> new file mode 100644
> index 0000000..9bc5d33
> --- /dev/null
> +++ b/src/test/core/keyboard.h
> @@ -0,0 +1,107 @@
> +/*
> + * Copyright © 2013 Intel Corporation
> + *
> + * Permission to use, copy, modify, distribute, and sell this software and
> + * its documentation for any purpose is hereby granted without fee, provided
> + * that the above copyright notice appear in all copies and that both that
> + * copyright notice and this permission notice appear in supporting
> + * documentation, and that the name of the copyright holders not be used in
> + * advertising or publicity pertaining to distribution of the software
> + * without specific, written prior permission. The copyright holders make
> + * no representations about the suitability of this software for any
> + * purpose. It is provided "as is" without express or implied warranty.
> + *
> + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS
> + * SOFTWARE, INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND
> + * FITNESS, IN NO EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY
> + * SPECIAL, INDIRECT OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER
> + * RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF
> + * CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
> + * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> + */
> +
> +#ifndef __WFITS_CORE_KEYBOARD_H__
> +#define __WFITS_CORE_KEYBOARD_H__
> +
> +#include <xkbcommon/xkbcommon.h>
> +#include <vector>
> +#include "seat.h"
> +
> +namespace wfits {
> +namespace test {
> +namespace core {
> +
> +class Keyboard
> +{
> +public:
> + Keyboard(const Seat&);
> +
> + virtual ~Keyboard();
> +
> + operator wl_keyboard*() const { return wl_keyboard_; }
> + const Seat& seat() const { return seat_; }
> + wl_surface* focus() const { return focus_; }
> + xkb_keymap* keymap() const { return keymap_; }
> + const uint32_t focusSerial() const { return focusSerial_; }
> +
> + bool hasFocus(wl_surface*);
> +
Probably prefer const function here:
bool hasFocus(wl_surface*) const;
> + const uint32_t modsDepressed() const { return modsDepressed_; }
> + const uint32_t modsLatched() const { return modsLatched_; }
> + const uint32_t modsLocked() const { return modsLocked_; }
> + const uint32_t group() const { return group_; }
> + bool keyPressed(uint32_t key) const;
> +
> +private:
> + static void keymap(void *data,
> + struct wl_keyboard *wl_keyboard,
> + uint32_t format,
> + int32_t fd,
> + uint32_t size);
> + static void enter(void *data,
> + struct wl_keyboard *wl_keyboard,
> + uint32_t serial,
> + struct wl_surface *surface,
> + struct wl_array *keys);
> + static void leave(void *data,
> + struct wl_keyboard *wl_keyboard,
> + uint32_t serial,
> + struct wl_surface *surface);
> + static void key(void *data,
> + struct wl_keyboard *wl_keyboard,
> + uint32_t serial,
> + uint32_t time,
> + uint32_t key,
> + uint32_t state);
> + static void modifiers(void *data,
> + struct wl_keyboard *wl_keyboard,
> + uint32_t serial,
> + uint32_t modsDepressed,
> + uint32_t modsLatched,
> + uint32_t modsLocked,
> + uint32_t group);
> +
> + void pressKey(uint32_t key);
> + void releaseKey(uint32_t key);
> + void setKeys(const uint32_t *keys, int n_keys);
> +
> + const Seat& seat_;
> + wl_surface* focus_;
> + uint32_t focusSerial_;
> + wl_keyboard* wl_keyboard_;
> +
> + typedef std::vector<uint32_t>::iterator KeyIterator;
> + typedef std::vector<uint32_t>::const_iterator CKeyIterator;
> + std::vector<uint32_t> keys_;
I'd prefer to see all class scoped typedefs at the top of the class declaration,
just before the "public" keyword (which effectively keeps them private
by default). Also, for simplicity, we should typedef the vector, too:
Class Keyboard {
typedef std::vector<uint32_t> Keys;
typedef Keys::iterator KeyIterator;
typedef Keys::const_iterator ConstKeyIterator;
public:
...
private:
...
Keys keys_;
};
> + uint32_t modsDepressed_;
> + uint32_t modsLatched_;
> + uint32_t modsLocked_;
> + uint32_t group_;
> + struct xkb_keymap *keymap_;
> +};
> +
> +} // namespace core
> +} // namespace test
> +} // namespace wfits
> +
> +#endif
> diff --git a/src/test/efl/Makefile.am b/src/test/efl/Makefile.am
> index 104213d..85084be 100644
> --- a/src/test/efl/Makefile.am
> +++ b/src/test/efl/Makefile.am
> @@ -46,6 +46,7 @@ libwfits_efl_la_SOURCES = \
> test_window_fullscreen.cpp
>
> libwfits_efl_la_LIBADD = \
> + $(XKBCOMMON_LIBS) \
> $(WAYLAND_LIBS) \
> $(EFL_LIBS) \
> $(CHECK_LIBS)
> @@ -53,6 +54,7 @@ libwfits_efl_la_LIBADD = \
> libwfits_efl_la_CPPFLAGS = \
> -I$(top_srcdir)/src \
> -DMEDIA_PATH=$(MEDIA) \
> + $(XKBCOMMON_CFLAGS) \
> $(WAYLAND_CFLAGS) \
> $(EFL_CFLAGS) \
> $(CHECK_CFLAGS)
> diff --git a/src/test/gtk+/Makefile.am b/src/test/gtk+/Makefile.am
> index b19aa9c..aa12963 100644
> --- a/src/test/gtk+/Makefile.am
> +++ b/src/test/gtk+/Makefile.am
> @@ -10,12 +10,14 @@ libwfits_gtk_la_SOURCES = \
>
> libwfits_gtk_la_LIBADD = \
> $(WAYLAND_LIBS) \
> + $(XKBCOMMON_LIBS) \
> $(GTK_LIBS) \
> $(CHECK_LIBS)
>
> libwfits_gtk_la_CPPFLAGS = \
> -I$(top_srcdir)/src \
> $(WAYLAND_CFLAGS) \
> + $(XKBCOMMON_CFLAGS) \
> $(GTK_CFLAGS) \
> $(CHECK_CFLAGS)
>
> --
> 1.8.3.1
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
More information about the wayland-devel
mailing list