> -----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