> -----Original Message-----
> From: wayland-devel-bounces+ullysses.a.eoff=intel....@lists.freedesktop.org
> [mailto:wayland-devel-
> bounces+ullysses.a.eoff=intel....@lists.freedesktop.org] On Behalf Of Neil
> Roberts
> Sent: Wednesday, September 25, 2013 4:33 PM
> To: wayland-devel@lists.freedesktop.org
> Subject: [PATCH wfits v2] core: Add a wrapper for the keyboard
>
> Ok, here's a second version of the patch with all of the suggested changes.
>
> > So we must only want one xkb context per display. Ok.
>
> Yes, I think we only really want one global xkb context but the
> display seemed like a convenient place to attach it and ensure that it
> will be destructed.
>
> > We should explicitly initialize keys_ here with some default:
>
> Ok
>
> > I prefer to always use brackets in all control structures even
> > if there is only one statement.
>
> Ok. I also changed places where I was using brace hugging because I
> had just assumed you were using the Wayland/Linux style.
>
I don't mind brace hugging. I'm not fully familiar with the
Wayland/Linux style yet; is there a document for it? Either way, it's
probably best if I add a coding style document to this project. I
use the style that I've always been taught... but of course I can't
expect everyone to know what that is ;). I'll try not to be too
critical or picky about style until I get it documented.
> > But, instead of iterating here why not use std::find(...):
>
> Ok, I changed it to use std::find().
>
That seems nicer ;)
> > How about:
> > if (not keyPressed(key))
> > {
> > keys_.push_back(key);
> > }
>
> Sure.
>
> > 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(...).
>
> Yeah, I copied this trick from Weston because I thought it was quite
> neat. But in this case I guess there's absolutely no point in
> optimising it so let's just use vector::erase() to make it more
> readable.
>
Cool, yeh optimization is not critical here.
> > Probably prefer const function here:
> > bool hasFocus(wl_surface*) const;
>
> Ok. You might want to fix the one in pointer.cpp too in that case.
>
Fair enough, I'll fix that. I'm sure there's likely other places where
const corrections need to be done too.
> > 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:
>
> Ok. I've added a typedef for the vector and moved it to the top. I've
> removed the iterator types because once you have the typedef for the
> vector then typing Keys::iterator doesn't seem such a pain.
>
Yeh that's not so bad.
> Regards,
> - Neil
>
Thanks Neil! I'll get these patches merged.
> ------- >8 --------------- (use git am --scissors to automatically chop here)
>
> 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 | 16 ++++
> src/test/core/display.h | 3 +
> src/test/core/keyboard.cpp | 216
> +++++++++++++++++++++++++++++++++++++++++++++
> src/test/core/keyboard.h | 106 ++++++++++++++++++++++
> src/test/efl/Makefile.am | 2 +
> src/test/gtk+/Makefile.am | 2 +
> 9 files changed, 352 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..55cb673 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,10 @@ Display::Display()
>
> /*virtual*/ Display::~Display()
> {
> + if (xkbContext_)
> + {
> + xkb_context_unref(xkbContext_);
> + }
> wl_registry_destroy(wl_registry_);
> wl_display_disconnect(*this);
> }
> @@ -69,6 +74,17 @@ 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_;
> };
>
> } // namespace core
> diff --git a/src/test/core/keyboard.cpp b/src/test/core/keyboard.cpp
> new file mode 100644
> index 0000000..2c766c6
> --- /dev/null
> +++ b/src/test/core/keyboard.cpp
> @@ -0,0 +1,216 @@
> +/*
> + * 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)
> + , 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_);
> + }
> +
> + wl_keyboard_destroy(*this);
> +}
> +
> +bool Keyboard::hasFocus(wl_surface* surface) const
> +{
> + return focus() == surface;
> +}
> +
> +bool Keyboard::keyPressed(uint32_t key) const
> +{
> + return std::find(keys_.begin(), keys_.end(), key) != keys_.end();
> +}
> +
> +void Keyboard::pressKey(uint32_t key)
> +{
> + if (not keyPressed(key))
> + {
> + keys_.push_back(key);
> + }
> +}
> +
> +void Keyboard::releaseKey(uint32_t key)
> +{
> + Keys::iterator it = std::find(keys_.begin(), keys_.end(), key);
> +
> + if (it != keys_.end())
> + {
> + keys_.erase(it);
> + }
> +}
> +
> +/* 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..53398a2
> --- /dev/null
> +++ b/src/test/core/keyboard.h
> @@ -0,0 +1,106 @@
> +/*
> + * 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
> +{
> + typedef std::vector<uint32_t> Keys;
> +
> +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*) 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);
> +
> + const Seat& seat_;
> + wl_surface* focus_;
> + uint32_t focusSerial_;
> + wl_keyboard* wl_keyboard_;
> +
> + std::vector<uint32_t> 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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
