[PATCH weston] simple-shm: explain two initial roundtrips
From: Pekka Paalanen
Explain carefully why we need two roundtrips, not just one, not just
dispatch and roundtrip, but two roundtrips after creating the
wl_registry object.
Signed-off-by: Pekka Paalanen
---
clients/simple-shm.c | 30 ++
1 file changed, 30 insertions(+)
diff --git a/clients/simple-shm.c b/clients/simple-shm.c
index c1cb386..996a8a9 100644
--- a/clients/simple-shm.c
+++ b/clients/simple-shm.c
@@ -388,6 +388,36 @@ create_display(void)
wl_display_roundtrip(display->display);
+ /*
+* Why do we need two roundtrips here?
+*
+* wl_display_get_registry() sends a request to the server, to which
+* the server replies by emitting the wl_registry.global events.
+* The first wl_display_roundtrip() sends wl_display.sync. The server
+* first processes the wl_display.get_registry which includes sending
+* the global events, and then processes the sync. Therefore when the
+* sync (roundtrip) returns, we are guaranteed to have received and
+* processed all the global events.
+*
+* While we are inside the first wl_display_roundtrip(), incoming
+* events are dispatched, which causes registry_handle_global() to
+* be called for each global. One of these globals is wl_shm.
+* registry_handle_global() sends wl_registry.bind request for the
+* wl_shm global. However, wl_registry.bind request is sent only after
+* the first wl_display.sync, so the reply to the sync comes before
+* the initial events of the wl_shm object.
+*
+* When the reply to the first sync comes, the server may or may not
+* have sent initial wl_shm events. Therefore we need the second
+* wl_display_roundtrip() call here.
+*
+* The server processes the wl_registry.bind for wl_shm first, and
+* the second wl_display.sync next. During our second call to
+* wl_display_roundtrip() the initial wl_shm events are received and
+* processed. Finally, when the reply to the second wl_display.sync
+* arrives, it guarantees we have processed all wl_shm initial events.
+*/
+
if (!(display->formats & (1 << WL_SHM_FORMAT_XRGB))) {
fprintf(stderr, "WL_SHM_FORMAT_XRGB32 not available\n");
exit(1);
--
2.0.4
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput 2/2] test: Add test for mouse dpi tag parser
On Mon, Nov 24, 2014 at 03:53:37PM -0600, Derek Foreman wrote:
> Signed-off-by: Derek Foreman
> ---
> test/Makefile.am| 7 -
> test/mouse_dpi_parser.c | 69
> +
> 2 files changed, 75 insertions(+), 1 deletion(-)
> create mode 100644 test/mouse_dpi_parser.c
> diff --git a/test/Makefile.am b/test/Makefile.am
> index 0abd695..1f124a3 100644
> --- a/test/Makefile.am
> +++ b/test/Makefile.am
> @@ -41,7 +41,8 @@ run_tests = \
> test-trackpoint \
> test-misc \
> test-keyboard \
> - test-device
> + test-device \
> + test-mouse_dpi_parser
any reason you didn't add this to misc.c?
I prefer having the tests roughly grouped over a separate file for each
small test.
> build_tests = \
> test-build-cxx \
> test-build-linker \
> @@ -93,6 +94,10 @@ test_device_SOURCES = device.c
> test_device_LDADD = $(TEST_LIBS)
> test_device_LDFLAGS = -no-install
>
> +test_mouse_dpi_parser_SOURCES = mouse_dpi_parser.c
> +test_mouse_dpi_parser_LDADD = $(TEST_LIBS)
> +test_mouse_dpi_parser_LDFLAGS = -no-install
> +
> # build-test only
> test_build_pedantic_c99_SOURCES = build-pedantic.c
> test_build_pedantic_c99_CFLAGS = -std=c99 -pedantic -Werror
> diff --git a/test/mouse_dpi_parser.c b/test/mouse_dpi_parser.c
> new file mode 100644
> index 000..bde71cb
> --- /dev/null
> +++ b/test/mouse_dpi_parser.c
> @@ -0,0 +1,69 @@
> +/*
> + * Copyright © 2014 Samsung
> + *
> + * 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
> +
> +#include
> +#include
> +#include
> +
> +#include "litest.h"
> +
> +struct parser_test {
> + char *tag;
> + int expected_dpi;
> +};
> +
> +START_TEST(udev_dpi_parser)
> +{
> + struct parser_test tests[] = {
> + { "450 *1800 3200", 1800 },
> + { "*450 1800 3200", 450 },
> + { "450 1800 *3200", 3200 },
> + { "450 1800 *failboat", 0 },
> + { "0 450 1800 *3200", 0 },
> + { "450@37 1800@12 *3200@6", 3200 },
> + { "450@125 1800@125 *3200@125 ", 3200 },
> + { "450@125 *1800@125 3200@125", 1800 },
> + { "*this @string fails", 0 },
> + { "12@34 *45@", 0 },
> + { " * 12, 450, 800", 0 },
> + { " *12, 450, 800", 12 },
> + { "*12, *450, 800", 12 },
pls add the empty string "" too, and tests for negative numbers.
Cheers,
Peter
> + { NULL }
> + };
> + int i, dpi;
> +
> + for (i = 0; tests[i].tag != NULL; i++) {
> + dpi = udev_parse_mouse_dpi_property(tests[i].tag);
> + ck_assert(dpi == tests[i].expected_dpi);
> + }
> +}
> +END_TEST
> +
> +int
> +main(int argc, char **argv)
> +{
> + litest_add_no_device("udev:dpi parser", udev_dpi_parser);
> +
> + return litest_run(argc, argv);
> +}
> --
> 2.1.3
>
> ___
> 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
Re: [PATCH libinput 1/2] evdev: Query mouse DPI from udev
On Mon, Nov 24, 2014 at 03:53:36PM -0600, Derek Foreman wrote:
> Instead of using a hard coded mouse DPI value, we query it from udev.
> If it's not present or the property is obviously broken we fall back
> to default.
for the archives: the thing we're struggling with on high-DPI mice is that
our pointer acceleration becomes rather useless. the DPI setting is
something we can't get from the device though, so our goal is to build a
udev database of "all" devices and go from there. once that is in place, we
expect the MOUSE_DPI property to be set and use that to calibrate our
pointer acceleration.
> Signed-off-by: Derek Foreman
> ---
> src/evdev.c | 18 ++
> src/libinput-util.c | 41 +
> src/libinput-util.h | 2 ++
> 3 files changed, 61 insertions(+)
>
> diff --git a/src/evdev.c b/src/evdev.c
> index 36c6859..a2547c7 100644
> --- a/src/evdev.c
> +++ b/src/evdev.c
> @@ -1218,8 +1218,11 @@ evdev_need_mtdev(struct evdev_device *device)
> static void
> evdev_tag_device(struct evdev_device *device)
> {
> + struct libinput *libinput = device->base.seat->libinput;
> + const char *mouse_dpi;
> struct udev *udev;
> struct udev_device *udev_device = NULL;
> + int dpi;
>
> udev = udev_new();
> if (!udev)
> @@ -1229,6 +1232,21 @@ evdev_tag_device(struct evdev_device *device)
> if (udev_device) {
> if (device->dispatch->interface->tag_device)
> device->dispatch->interface->tag_device(device,
> udev_device);
> + mouse_dpi = udev_device_get_property_value(udev_device,
> "MOUSE_DPI");
> + if (mouse_dpi) {
> + dpi = udev_parse_mouse_dpi_property(mouse_dpi);
> + if (dpi)
> + device->dpi = dpi;
> + else {
> + log_error(libinput, "Mouse DPI property for"
> + " '%s' is present but "
> + "invalid, using %d DPI "
> + "instead\n",
> + device->devname,
> + DEFAULT_MOUSE_DPI);
> + device->dpi = DEFAULT_MOUSE_DPI;
> + }
> + }
> udev_device_unref(udev_device);
> }
> udev_unref(udev);
> diff --git a/src/libinput-util.c b/src/libinput-util.c
> index 34d5549..5600424 100644
> --- a/src/libinput-util.c
> +++ b/src/libinput-util.c
> @@ -28,7 +28,9 @@
>
> #include "config.h"
>
> +#include
> #include
> +#include
> #include
> #include
>
> @@ -113,3 +115,42 @@ ratelimit_test(struct ratelimit *r)
>
> return RATELIMIT_EXCEEDED;
> }
> +
> +int
> +udev_parse_mouse_dpi_property(const char *prop)
I'd prefer this not be named udev_... if it's a general utility. just drop
udev_ would be enough.
> +{
> + bool is_default = false;
> + int rd, dpi;
this is nitpicking, but I think we can afford the extra character storage to
have 'nread' instead of the harder-to-interpret 'rd' :)
> +
> + /* When parsing the mouse DPI property, if we find an error we
> + * just return 0 since it's obviously invalid, the caller will
> + * treat that as an error and use a reasonable default instead.
> + * If the property contains multiple DPI settings but none flagged
> + * as default, we return the last because we're lazy and that's
> + * a silly way to set the property anyway.
> + */
it'd be nice to add a comment here on how the property _should_ look like.
otherwise, looks good, thanks. I'll get the udev hwdb patches to the list
asap, just a heads up: I'm going to wait until those are merged before we
merge this just in case there are any last-minute changes.
Cheers,
Peter
> + while (*prop != 0) {
> + if (*prop == ' ') {
> + prop++;
> + continue;
> + }
> + if (*prop == '*') {
> + prop++;
> + is_default = true;
> + if (!isdigit(prop[0]))
> + return 0;
> + }
> +
> + rd = 0;
> + sscanf(prop, "%d@%*d%n", &dpi, &rd);
> + if (!rd)
> + sscanf(prop, "%d%n", &dpi, &rd);
> + if (!rd || dpi == 0 || prop[rd] == '@')
> + return 0;
check for negative dpi/frequency here pls
otherwise, looks good, thanks.
Cheers,
Peter
> +
> + if (is_default)
> + break;
> + prop += rd;
> + }
> + return dpi;
> +}
> diff --git a/src/libinput-util.h b/src/libinput-util.h
> index 909c9db..62d7ac1 100644
> --- a/src/libinput-util.h
> +++ b/src/libinput-util.h
> @@ -296,4 +296,6 @@ struct ratelimit {
>
Re: [PATCH 2/2] Support for adjusting socket access rights to allow group of users to connect to the socket.
On 19.11.2014 16:22, Pekka Paalanen wrote: When a session compositor is started, you can already use WAYLAND_SOCKET environment variable to pass an opened connection to it. If your system compositor forks session compositors, no problem. If something else starts your session compositors, you need a wrapper program to pre-create a connection to the socket file in the shared directory (that is not your XDG_RUNTIME_DIR) and then exec the session compositor. We are running system compositor under one special user session, in this case user "genivi". This is normal session other than it is not visible to logind (pam_systemd is not called). Then, in our login manager (TLM), other seats are set to wait for the system compositor lock file to appear before logging in default (guest) user sessions. These are normal sessions visible to logind and weston for each seat runs inside the user session. When user is logged out, the weston instance is terminated and restarted for the next user session. Passing WAYLAND_SOCKET environment from a user session of user X to another newly created user sessions Y and Z is not very straightforward operation... Also lifetime of the Y and Z vary and new sessions will be spawned after one is terminated while the system compositor persists. ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput v3 2/3] touchpad: Add code to get the touchpad model / manufacturer
On Mon, Nov 24, 2014 at 12:16:05PM +0100, Hans de Goede wrote:
> This is useful to know in some cases, it is e.g. necessary to figure out
> which percentage of a touchpads range to use as edge for edge-scrolling.
>
> Note this is a slightly cleaned up copy of the same code in
> xf86-input-synaptics.
>
> Signed-off-by: Hans de Goede
pushed, thanks
92d178f..6a4ceed master -> master
Cheers,
Peter
> ---
> src/evdev-mt-touchpad.c | 36
> src/evdev-mt-touchpad.h | 10 ++
> 2 files changed, 46 insertions(+)
>
> diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c
> index 7a1c32d..6d4b583 100644
> --- a/src/evdev-mt-touchpad.c
> +++ b/src/evdev-mt-touchpad.c
> @@ -1146,6 +1146,40 @@ tp_change_to_left_handed(struct evdev_device *device)
> device->buttons.left_handed = device->buttons.want_left_handed;
> }
>
> +struct model_lookup_t {
> + uint16_t vendor;
> + uint16_t product_start;
> + uint16_t product_end;
> + enum touchpad_model model;
> +};
> +
> +static struct model_lookup_t model_lookup_table[] = {
> + { 0x0002, 0x0007, 0x0007, MODEL_SYNAPTICS },
> + { 0x0002, 0x0008, 0x0008, MODEL_ALPS },
> + { 0x0002, 0x000e, 0x000e, MODEL_ELANTECH },
> + { 0x05ac, 0, 0x0222, MODEL_APPLETOUCH },
> + { 0x05ac, 0x0223, 0x0228, MODEL_UNIBODY_MACBOOK },
> + { 0x05ac, 0x0229, 0x022b, MODEL_APPLETOUCH },
> + { 0x05ac, 0x022c, 0x, MODEL_UNIBODY_MACBOOK },
> + { 0, 0, 0, 0 }
> +};
> +
> +static enum touchpad_model
> +tp_get_model(struct evdev_device *device)
> +{
> + struct model_lookup_t *lookup;
> + uint16_t vendor = libevdev_get_id_vendor(device->evdev);
> + uint16_t product = libevdev_get_id_product(device->evdev);
> +
> + for (lookup = model_lookup_table; lookup->vendor; lookup++) {
> + if (lookup->vendor == vendor &&
> + lookup->product_start <= product &&
> + product <= lookup->product_end)
> + return lookup->model;
> + }
> + return MODEL_UNKNOWN;
> +}
> +
> struct evdev_dispatch *
> evdev_mt_touchpad_create(struct evdev_device *device)
> {
> @@ -1155,6 +1189,8 @@ evdev_mt_touchpad_create(struct evdev_device *device)
> if (!tp)
> return NULL;
>
> + tp->model = tp_get_model(device);
> +
> if (tp_init(tp, device) != 0) {
> tp_destroy(&tp->base);
> return NULL;
> diff --git a/src/evdev-mt-touchpad.h b/src/evdev-mt-touchpad.h
> index 11c4d49..7f3ce49 100644
> --- a/src/evdev-mt-touchpad.h
> +++ b/src/evdev-mt-touchpad.h
> @@ -42,6 +42,15 @@ enum touchpad_event {
> TOUCHPAD_EVENT_BUTTON_RELEASE = (1 << 2),
> };
>
> +enum touchpad_model {
> + MODEL_UNKNOWN = 0,
> + MODEL_SYNAPTICS,
> + MODEL_ALPS,
> + MODEL_APPLETOUCH,
> + MODEL_ELANTECH,
> + MODEL_UNIBODY_MACBOOK
> +};
> +
> enum touch_state {
> TOUCH_NONE = 0,
> TOUCH_BEGIN,
> @@ -156,6 +165,7 @@ struct tp_dispatch {
> unsigned int slot; /* current slot */
> bool has_mt;
> bool semi_mt;
> + enum touchpad_model model;
>
> unsigned int real_touches; /* number of slots */
> unsigned int ntouches; /* no slots inc. fakes */
> --
> 2.1.0
>
> ___
> 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
Re: [PATCH v1] Added string conversion utility functions
On Tue, Nov 25, 2014 at 1:15 AM, Bill Spitzak wrote: > > > On 11/24/2014 11:32 AM, Imran Zaman wrote: > >> [IZ2] This is the case where endptr is used in patch. I can remove >> endptr but it seems its used so i have to keep the existing >> functionality in place. >> + if (!weston_strtoi(pid, &end, 0, &other) || end != pid + 10) > > > Use strlen(pid) != 10 instead. This will not exactly replace the implemented logic e.g. pid="123abcdefg". strlen(pid) = 10 end = 3 (end != pid + 10) So cant use the solution which u propose... > >> [IZ2] Sorry still not able to grasp what you want to highlight. Can >> you please give an example? > > > If I write the following code: > > if (strtol(string, 0, 0, 0)) ... > > I want it to crash so I notice that I passed 0 for the val output pointer. > There is no reason to every pass null here so it must be a programmer > mistake. As you have written it this will act like there is a syntax error > in string, which could lead a person trying to debug their code down the > wrong path. > Sorry, why is it a programmer mistake and how would it crash? Let me add the exact test code and will come back to you. It should not crash for sure IMO. > Also if the pointer is passed as &(structure->member) it is going to crash > anyway if structure is null and the offset to member is non-zero. I would > think most errors causing the pointer to be invalid will be of this form so > this is not preventing any crash. > This cant be prevented anyway. we are using pointers everywhere in the whole weston code base. So its the user of the function who have to pass valid pointer or NULL. > I suppose null could be used as "I am only testing if it parses and don't > want the number" but it is not implemented this way, and I kind of doubt > much code is interested in that test. At least one case above is using endptr.. if we can replace it with exactly similarly logic, I can remove it otherwies I dont see any harm in its usage ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH wayland 2/2] support specifying custom directories for the client and server
On 19.11.2014 12:56, Pekka Paalanen wrote: I have very hard time deciding if we should allow the environment to overwrite the server and client assumptions on where the socket is. It would be easier for me to accept new API functions that operate with absolute paths or existing fds explicitly, but those of course require both servers and clients to be written to use them. A bit tricky part in current Weston is case where you use wayland-backend. In this case Weston is client to another Weston and in addition server providing a socket to it's client. In this setup the server is sort of proxy between lower level server and the client. Since both instances solely use XDG_RUNTIME_DIR, the wayland-backend client weston is trying to connect to a socket there are create a new socket with same name in the same place... The change is intended to allow a way to tell this second weston where to look for the client socket and where to place the server socket. Usually these two are not the same place... ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: vc_dispmanx_set_wl_buffer_in_use not found
On Tue, Nov 18, 2014 at 3:45 PM, Pekka Paalanen wrote: > well, with so little information, all I can ask is: are you sure you > had Wayland-enabled version of a player, and the player is using GLESv2 > instead of anything else like desktop OpenGL? # ldd mplayer libGLESv2.so.2 => /opt/vc/lib/libGLESv2.so.2 (0xf67b7000) libEGL.so.1 => /opt/vc/lib/libEGL.so.1 (0xf6785000) libncurses.so.5 => /lib/libncurses.so.5 (0xf6737000) libwayland-client.so.0 => /usr/way/lib/libwayland-client.so.0 (0xf6727000) libwayland-egl.so => /opt/vc/lib/libwayland-egl.so (0xf671e000) libwayland-cursor.so.0 => /usr/way/lib/libwayland-cursor.so.0 (0xf670f000) libxkbcommon.so.0 => /usr/way/lib/libxkbcommon.so.0 (0xf66d) ... > Also, does weston-simple-egl work? If that doesn't work, then nothing > else EGL/Wayland will, and there is still something wrong with the EGL, > driver, or Weston installation or such. weston-simple-egl works. > I don't really know in what state the Mesa vc4 is in terms of running > apps, but I do know that in the past, Tomeu's branch did allow > weston-simple-egl to run. Not smoothly and reliably as we had some > open issues, but it did run. Yes, weston-simple-egl works. I'm using Tomeu's branch. > I'm not sure what you are looking for even exists, if XBMC is not > suitable (I mean any of the popular rpi media apps, I don't really > know those either). Still keen on getting mplayer to work. Thanks, Jeff ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v1 weston 07/11] tests: Add a fadein test
On Mon, Nov 24, 2014 at 01:19:46PM +0200, Pekka Paalanen wrote:
> On Wed, 19 Nov 2014 15:06:22 -0800
> Bryce Harrington wrote:
>
> > This also serves as a proof of concept of the screen capture
> > functionality and as a demo for snapshot-based rendering verification.
> >
> > Signed-off-by: Bryce Harrington
> > ---
> > Makefile.am | 7 +-
> > tests/fadein-test.c | 64
> > +
> > 2 files changed, 70 insertions(+), 1 deletion(-)
> > create mode 100644 tests/fadein-test.c
> >
> > diff --git a/Makefile.am b/Makefile.am
> > index 26dd473..c3dfa10 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -852,7 +852,8 @@ weston_tests = \
> > text.weston \
> > presentation.weston \
> > roles.weston\
> > - subsurface.weston
> > + subsurface.weston \
> > + fadein.weston
> >
> >
> > AM_TESTS_ENVIRONMENT = \
> > @@ -954,6 +955,10 @@ subsurface_weston_SOURCES = tests/subsurface-test.c
> > subsurface_weston_CFLAGS = $(AM_CFLAGS) $(TEST_CLIENT_CFLAGS)
> > subsurface_weston_LDADD = libtest-client.la
> >
> > +fadein_weston_SOURCES = tests/fadein-test.c
> > +fadein_weston_CFLAGS = $(AM_CFLAGS) $(TEST_CLIENT_CFLAGS)
> > +fadein_weston_LDADD = libtest-client.la
> > +
> > presentation_weston_SOURCES = tests/presentation-test.c
> > nodist_presentation_weston_SOURCES = \
> > protocol/presentation_timing-protocol.c \
> > diff --git a/tests/fadein-test.c b/tests/fadein-test.c
> > new file mode 100644
> > index 000..a6c284f
> > --- /dev/null
> > +++ b/tests/fadein-test.c
> > @@ -0,0 +1,64 @@
> > +/*
> > + * Copyright © 2014 Samsung
> > + *
> > + * 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 "config.h"
> > +
> > +#include
> > +#include
> > +
> > +#include "weston-test-client-helper.h"
> > +
> > +char *server_parameters="--use-pixman --width=320 --height=240";
> > +
> > +static char*
> > +output_filename(const char* basename) {
> > + static const char *path = "./";
> > + char *filename;
> > +
> > +if (asprintf(&filename, "%s%s", path, basename) < 0)
> > + filename = NULL;
> > +
> > + return filename;
> > +}
> > +
> > +TEST(fadein)
> > +{
> > + struct client *client;
> > + char basename[32];
> > + char *out_path;
> > + int i;
> > +
> > + client = client_create(100, 100, 100, 100);
> > + assert(client);
> > +
> > + for (i = 0; i < 6; i++) {
> > + snprintf(basename, sizeof basename, "fadein-%02d", i);
> > + out_path = output_filename(basename);
> > +
> > + wl_test_record_screenshot(client->test->wl_test, out_path);
> > + client_roundtrip(client);
> > + free (out_path);
> > +
> > + usleep(25);
> > + }
> > +
> > +}
>
> Hi,
>
> where is the verification promised in the commit message? ;-)
>
> I think it is bad to rely on delays/timers. We need something
> more deterministic: a protocol to drive the (headless) repaint.
>
> We probably need the headless repaint to not run on its own, but
> strictly when requested by the test client. Additionally, we probably
> want to carry a "page flip completion" timestamp in that request, and
> have the compositor use the timestamp. That way the test client can
> actually drive the in-compositor animations, because it is in control
> of the clock and framerate.
Yes, that would definitely be helpful.
Can you elaborate on how this should be implemented?
> That should make the fade-in test reliable, and it will help with
> Presentation testing too.
>
> The client driven repaint likely needs to be enabled per-test. If a
> test attempts to enable it, but the c
[PATCH] Ignore devices that have joystick buttons
This patch allows libinput to ignore devices that have joystick buttons.
Signed-off-by: Krzysztof Sobiecki
---
src/evdev.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/src/evdev.c b/src/evdev.c
index 36c6859..5f6cc32 100644
--- a/src/evdev.c
+++ b/src/evdev.c
@@ -1274,6 +1274,15 @@ evdev_configure_device(struct evdev_device *device)
has_keyboard = 0;
has_touch = 0;
+for (i = BTN_JOYSTICK; i < BTN_DIGI; i++) {
+if (libevdev_has_event_code(evdev, EV_KEY, i)) {
+log_info(libinput,
+ "input device '%s', %s is a joystick, ignoring\n",
+ device->devname, device->devnode);
+return -1;
+}
+}
+
if (libevdev_has_event_type(evdev, EV_ABS)) {
if ((absinfo = libevdev_get_abs_info(evdev, ABS_X))) {
--
2.2.0.rc0.207.ga3a616c
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v1 weston 06/11] tests: Add screenshot recording to weston-test
On Mon, Nov 24, 2014 at 04:31:01PM -0600, Derek Foreman wrote: > On 24/11/14 05:01 AM, Pekka Paalanen wrote: > > On Wed, 19 Nov 2014 15:06:21 -0800 > > Bryce Harrington wrote: > > > >> From: Derek Foreman > >> > >> Adds wl_test_record_screenshot() to weston test. This commit also > >> adds a dependency on cairo to weston-test to use it for writing PNG > >> files. > >> > >> Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=83981 > >> Signed-off-by: Bryce Harrington > > > > Hi, > > > > could we use a wl_shm-based wl_buffer instead of a file, please? > > > > That way we can simply relay the pixels as is to the test client, which > > can then compare without compressing and decompressing from PNG first. > > Ok - it (the client) will still need to be able to write and read files > though, so it can create or read the reference images. I agree it will run faster and save on disk space if we can skip writing the images to files. Yes, we'll still need to read the reference images, but reading is faster than writing so we'll be ahead. Most of the time we don't care what the images look like anyway. That said, it can be beneficial to write the output out to files for troubleshooting purposes. So we still need a write mode. But perhaps it should be off by default with some env var or switch to make it show its work. > > Synchronization cannot be done with wl_display_roundtrip, so you need > > an explicit event for that. The best would be to create a new protocol > > object on record_screenshot request, which then delivers the event and > > self-destructs, like wl_callback. You could just use wl_callback, but > > in principle we should avoid new use cases for wl_callback due to > > the design issues. > > > > Please also document carefully in which orientation the screenshot is > > taken and how the clip coords are interpreted, in case some > > output_scale/transform are in effect. Or if you rely on the renderer's > > read_pixels() convention, I hope that is documented somewhere... > > Not yet ;) > > (fwiw it's done so an unrotated display's screenshot looks normal in an > image viewer - so the opposite of gl convention :) > > > Btw. there is a small danger in linking Cairo to the compositor. > > GL-renderer uses GLESv2, but if CAIRO_LIBS contains cairo-gl.so linking > > to libGL, it might explode... or maybe not because of no RTLD_GLOBAL... > > or maybe it could, I don't know. So I'd just avoid that. :-) > > Sgh, I was afraid someone would come up with a reason to actually > deal with libpng directly. Oh, wait, only the client has to deal with > png files - I think it's safe to link cairo there? :) Most distros don't ship cairo with gl enabled anyway. You'd have to do that intentionally. Even with it enabled, if we keep png generation client-side then we're not going to risk interfering with the compositor. Although, maybe some tests link in some compositor code and could hit the issue... not sure. At least here the scope of the problem will be limited to the one test. And there's probably a few paths to solve it. Bryce ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v1] Added string conversion utility functions
On 11/24/2014 11:32 AM, Imran Zaman wrote: [IZ2] This is the case where endptr is used in patch. I can remove endptr but it seems its used so i have to keep the existing functionality in place. + if (!weston_strtoi(pid, &end, 0, &other) || end != pid + 10) Use strlen(pid) != 10 instead. [IZ2] Sorry still not able to grasp what you want to highlight. Can you please give an example? If I write the following code: if (strtol(string, 0, 0, 0)) ... I want it to crash so I notice that I passed 0 for the val output pointer. There is no reason to every pass null here so it must be a programmer mistake. As you have written it this will act like there is a syntax error in string, which could lead a person trying to debug their code down the wrong path. Also if the pointer is passed as &(structure->member) it is going to crash anyway if structure is null and the offset to member is non-zero. I would think most errors causing the pointer to be invalid will be of this form so this is not preventing any crash. I suppose null could be used as "I am only testing if it parses and don't want the number" but it is not implemented this way, and I kind of doubt much code is interested in that test. ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v1 weston 06/11] tests: Add screenshot recording to weston-test
On 24/11/14 05:01 AM, Pekka Paalanen wrote:
> On Wed, 19 Nov 2014 15:06:21 -0800
> Bryce Harrington wrote:
>
>> From: Derek Foreman
>>
>> Adds wl_test_record_screenshot() to weston test. This commit also
>> adds a dependency on cairo to weston-test to use it for writing PNG
>> files.
>>
>> Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=83981
>> Signed-off-by: Bryce Harrington
>
> Hi,
>
> could we use a wl_shm-based wl_buffer instead of a file, please?
>
> That way we can simply relay the pixels as is to the test client, which
> can then compare without compressing and decompressing from PNG first.
Ok - it (the client) will still need to be able to write and read files
though, so it can create or read the reference images.
> For an example how to do this, see Weston's screenshooter protocol and
> implementation.
Thanks - I'd actually considered dumping screenshots in screenshooter's
wcap format, but I figured it would be a bit of a pain to work with that.
> I think you also want to specify in the record_screenshot request:
> - which output to capture (in case there are multiple)
> - the rectangular sub-region that must be contained in the output
> (which you already do with the clip coords in a later patch, so that
> should be squashed here)
That makes sense too.
> Synchronization cannot be done with wl_display_roundtrip, so you need
> an explicit event for that. The best would be to create a new protocol
> object on record_screenshot request, which then delivers the event and
> self-destructs, like wl_callback. You could just use wl_callback, but
> in principle we should avoid new use cases for wl_callback due to
> the design issues.
>
> Please also document carefully in which orientation the screenshot is
> taken and how the clip coords are interpreted, in case some
> output_scale/transform are in effect. Or if you rely on the renderer's
> read_pixels() convention, I hope that is documented somewhere...
Not yet ;)
(fwiw it's done so an unrotated display's screenshot looks normal in an
image viewer - so the opposite of gl convention :)
> Btw. there is a small danger in linking Cairo to the compositor.
> GL-renderer uses GLESv2, but if CAIRO_LIBS contains cairo-gl.so linking
> to libGL, it might explode... or maybe not because of no RTLD_GLOBAL...
> or maybe it could, I don't know. So I'd just avoid that. :-)
Sgh, I was afraid someone would come up with a reason to actually
deal with libpng directly. Oh, wait, only the client has to deal with
png files - I think it's safe to link cairo there? :)
> I see the test interface has been extended before without bumping the
> revision... but we probably should bump it, to not give a bad example.
Hehe, that's why I thought it was ok. I'll bump it in the next revision.
Thanks for the review!
>> ---
>> Makefile.am | 4 +--
>> protocol/wayland-test.xml | 3 +++
>> tests/weston-test.c | 68
>> +++
>> 3 files changed, 73 insertions(+), 2 deletions(-)
>>
>> diff --git a/Makefile.am b/Makefile.am
>> index 1e7cc81..26dd473 100644
>> --- a/Makefile.am
>> +++ b/Makefile.am
>> @@ -881,7 +881,7 @@ noinst_PROGRAMS += \
>> matrix-test
>>
>> test_module_ldflags = \
>> --module -avoid-version -rpath $(libdir) $(COMPOSITOR_LIBS)
>> +-module -avoid-version -rpath $(libdir) $(COMPOSITOR_LIBS) $(CAIRO_LIBS)
>>
>> surface_global_test_la_SOURCES = tests/surface-global-test.c
>> surface_global_test_la_LDFLAGS = $(test_module_ldflags)
>> @@ -893,7 +893,7 @@ surface_test_la_CFLAGS = $(GCC_CFLAGS)
>> $(COMPOSITOR_CFLAGS)
>>
>> weston_test_la_LIBADD = $(COMPOSITOR_LIBS) libshared.la
>> weston_test_la_LDFLAGS = $(test_module_ldflags)
>> -weston_test_la_CFLAGS = $(GCC_CFLAGS) $(COMPOSITOR_CFLAGS)
>> +weston_test_la_CFLAGS = $(GCC_CFLAGS) $(COMPOSITOR_CFLAGS) $(CAIRO_CFLAGS)
>> weston_test_la_SOURCES = tests/weston-test.c
>> nodist_weston_test_la_SOURCES = \
>> protocol/wayland-test-protocol.c\
>> diff --git a/protocol/wayland-test.xml b/protocol/wayland-test.xml
>> index 18b6625..a22a6ac 100644
>> --- a/protocol/wayland-test.xml
>> +++ b/protocol/wayland-test.xml
>> @@ -58,5 +58,8 @@
>>
>>
>>
>> +
>> +
>> +
>>
>>
>> diff --git a/tests/weston-test.c b/tests/weston-test.c
>> index f1e45c1..16f20c6 100644
>> --- a/tests/weston-test.c
>> +++ b/tests/weston-test.c
>> @@ -35,6 +35,8 @@
>> #include
>> #endif /* ENABLE_EGL */
>>
>> +#include
>> +
>> struct weston_test {
>> struct weston_compositor *compositor;
>> struct weston_layer layer;
>> @@ -235,6 +237,71 @@ get_n_buffers(struct wl_client *client, struct
>> wl_resource *resource)
>> wl_test_send_n_egl_buffers(resource, n_buffers);
>> }
>>
>> +static void
>> +dump_image(const char *filename, int x, int y, uint32_t *image)
>> +{
>> +cairo_surface_t *surface, *flipped;
>> +cairo_t *cr;
>> +
>>
[PATCH libinput 2/2] test: Add test for mouse dpi tag parser
Signed-off-by: Derek Foreman
---
test/Makefile.am| 7 -
test/mouse_dpi_parser.c | 69 +
2 files changed, 75 insertions(+), 1 deletion(-)
create mode 100644 test/mouse_dpi_parser.c
diff --git a/test/Makefile.am b/test/Makefile.am
index 0abd695..1f124a3 100644
--- a/test/Makefile.am
+++ b/test/Makefile.am
@@ -41,7 +41,8 @@ run_tests = \
test-trackpoint \
test-misc \
test-keyboard \
- test-device
+ test-device \
+ test-mouse_dpi_parser
build_tests = \
test-build-cxx \
test-build-linker \
@@ -93,6 +94,10 @@ test_device_SOURCES = device.c
test_device_LDADD = $(TEST_LIBS)
test_device_LDFLAGS = -no-install
+test_mouse_dpi_parser_SOURCES = mouse_dpi_parser.c
+test_mouse_dpi_parser_LDADD = $(TEST_LIBS)
+test_mouse_dpi_parser_LDFLAGS = -no-install
+
# build-test only
test_build_pedantic_c99_SOURCES = build-pedantic.c
test_build_pedantic_c99_CFLAGS = -std=c99 -pedantic -Werror
diff --git a/test/mouse_dpi_parser.c b/test/mouse_dpi_parser.c
new file mode 100644
index 000..bde71cb
--- /dev/null
+++ b/test/mouse_dpi_parser.c
@@ -0,0 +1,69 @@
+/*
+ * Copyright © 2014 Samsung
+ *
+ * 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
+
+#include
+#include
+#include
+
+#include "litest.h"
+
+struct parser_test {
+ char *tag;
+ int expected_dpi;
+};
+
+START_TEST(udev_dpi_parser)
+{
+ struct parser_test tests[] = {
+ { "450 *1800 3200", 1800 },
+ { "*450 1800 3200", 450 },
+ { "450 1800 *3200", 3200 },
+ { "450 1800 *failboat", 0 },
+ { "0 450 1800 *3200", 0 },
+ { "450@37 1800@12 *3200@6", 3200 },
+ { "450@125 1800@125 *3200@125 ", 3200 },
+ { "450@125 *1800@125 3200@125", 1800 },
+ { "*this @string fails", 0 },
+ { "12@34 *45@", 0 },
+ { " * 12, 450, 800", 0 },
+ { " *12, 450, 800", 12 },
+ { "*12, *450, 800", 12 },
+ { NULL }
+ };
+ int i, dpi;
+
+ for (i = 0; tests[i].tag != NULL; i++) {
+ dpi = udev_parse_mouse_dpi_property(tests[i].tag);
+ ck_assert(dpi == tests[i].expected_dpi);
+ }
+}
+END_TEST
+
+int
+main(int argc, char **argv)
+{
+ litest_add_no_device("udev:dpi parser", udev_dpi_parser);
+
+ return litest_run(argc, argv);
+}
--
2.1.3
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH libinput 1/2] evdev: Query mouse DPI from udev
Instead of using a hard coded mouse DPI value, we query it from udev.
If it's not present or the property is obviously broken we fall back
to default.
Signed-off-by: Derek Foreman
---
src/evdev.c | 18 ++
src/libinput-util.c | 41 +
src/libinput-util.h | 2 ++
3 files changed, 61 insertions(+)
diff --git a/src/evdev.c b/src/evdev.c
index 36c6859..a2547c7 100644
--- a/src/evdev.c
+++ b/src/evdev.c
@@ -1218,8 +1218,11 @@ evdev_need_mtdev(struct evdev_device *device)
static void
evdev_tag_device(struct evdev_device *device)
{
+ struct libinput *libinput = device->base.seat->libinput;
+ const char *mouse_dpi;
struct udev *udev;
struct udev_device *udev_device = NULL;
+ int dpi;
udev = udev_new();
if (!udev)
@@ -1229,6 +1232,21 @@ evdev_tag_device(struct evdev_device *device)
if (udev_device) {
if (device->dispatch->interface->tag_device)
device->dispatch->interface->tag_device(device,
udev_device);
+ mouse_dpi = udev_device_get_property_value(udev_device,
"MOUSE_DPI");
+ if (mouse_dpi) {
+ dpi = udev_parse_mouse_dpi_property(mouse_dpi);
+ if (dpi)
+ device->dpi = dpi;
+ else {
+ log_error(libinput, "Mouse DPI property for"
+ " '%s' is present but "
+ "invalid, using %d DPI "
+ "instead\n",
+ device->devname,
+ DEFAULT_MOUSE_DPI);
+ device->dpi = DEFAULT_MOUSE_DPI;
+ }
+ }
udev_device_unref(udev_device);
}
udev_unref(udev);
diff --git a/src/libinput-util.c b/src/libinput-util.c
index 34d5549..5600424 100644
--- a/src/libinput-util.c
+++ b/src/libinput-util.c
@@ -28,7 +28,9 @@
#include "config.h"
+#include
#include
+#include
#include
#include
@@ -113,3 +115,42 @@ ratelimit_test(struct ratelimit *r)
return RATELIMIT_EXCEEDED;
}
+
+int
+udev_parse_mouse_dpi_property(const char *prop)
+{
+ bool is_default = false;
+ int rd, dpi;
+
+ /* When parsing the mouse DPI property, if we find an error we
+* just return 0 since it's obviously invalid, the caller will
+* treat that as an error and use a reasonable default instead.
+* If the property contains multiple DPI settings but none flagged
+* as default, we return the last because we're lazy and that's
+* a silly way to set the property anyway.
+*/
+ while (*prop != 0) {
+ if (*prop == ' ') {
+ prop++;
+ continue;
+ }
+ if (*prop == '*') {
+ prop++;
+ is_default = true;
+ if (!isdigit(prop[0]))
+ return 0;
+ }
+
+ rd = 0;
+ sscanf(prop, "%d@%*d%n", &dpi, &rd);
+ if (!rd)
+ sscanf(prop, "%d%n", &dpi, &rd);
+ if (!rd || dpi == 0 || prop[rd] == '@')
+ return 0;
+
+ if (is_default)
+ break;
+ prop += rd;
+ }
+ return dpi;
+}
diff --git a/src/libinput-util.h b/src/libinput-util.h
index 909c9db..62d7ac1 100644
--- a/src/libinput-util.h
+++ b/src/libinput-util.h
@@ -296,4 +296,6 @@ struct ratelimit {
void ratelimit_init(struct ratelimit *r, uint64_t ival_ms, unsigned int burst);
enum ratelimit_state ratelimit_test(struct ratelimit *r);
+int udev_parse_mouse_dpi_property(const char *prop);
+
#endif /* LIBINPUT_UTIL_H */
--
2.1.3
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: FreeBSD libinput
Hey, as far as I know epoll is only available on freeBSD via the linux compatibility kernel + libraries layer, which is not installed by default. There is kqueue which is an arguably better event handling mechanism. There's an existing patch for such a rewrite here: http://lists.freedesktop.org/archives/wayland-devel/2013-February/007442.html from Philip Withnall. I'm not sure where this patch is supposed to be applied to as the files in it are not corresponding to libinput sources, but what he did could be done here with current libinput. Best method would probably be to write a basic abstraction on top of the polling mechanism and have individual mechanisms implementing it in stand-alone .c files. My biggest hurdle right now is the mtdev dependency since that library is linux specific. Thanks, Ales 2014-11-23 16:05 GMT-07:00 Peter Hutterer : > On Fri, Nov 21, 2014 at 09:10:24PM -0700, Ales Katona wrote: > > I wanted to try and compile libinput under freeBSD seeing how epoll has > > been patched into kqueue on the platform, but I'm getting stopped by > > configure saying it can't find EPOLL_CLOEXEC. Is this just a configure > > block or is the code still linux specific? > > how is epoll implemented in freebsd? I checked out the source, poked > around a bit and even managed to find the commit (r255672) but that got > reverted soon after (r255675). Best I can tell so far from the > syscalls.master file is that the syscalls are just occupying space, they > don't seem to do much (all the calls are simply epoll_foo(void) now). > > There are some references to EPOLL_CLOEXEC in the tree but svn is far to > painful for me to figure out how everything fits together here, sorry. > > Cheers, >Peter > > -- Feel the power of Opensource. Feel the power of Free Pascal. ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v1] Added string conversion utility functions
Hi Please see the comments below. BR imran On Mon, Nov 24, 2014 at 8:58 PM, Bill Spitzak wrote: > > > On 11/24/2014 05:08 AM, Imran Zaman wrote: >> >> Thanks Bill for your comments. Please see my comments inline. >> >> On Fri, Nov 21, 2014 at 9:31 PM, Bill Spitzak wrote: >>> >>> There is no need to pass endptr, because your functions will always fail >>> if >>> it is not set to point to the terminating null. >>> >> [IZ] endptr is passed other than null; please see the whole patch. why >> do u think it will fail? > > > What I meant was that if this function returns success, endptr will ALWAYS > be set to the string+strlen(string), ie at the NULL. As the caller can > easily create this answer itself it seems useless to pass a pointer to store > it. > > Although endptr might not point at the null when this fails, it seems hard > for the caller to do anything useful with this. It would have to first > determine if the error was due to something other than trailing characters, > which would involve replicating the entire function locally. > > Therefore I think this argument is useless and there is no reason to support > it. > > Also the grep below shows that nobody uses the endptr except to do the error > checking that this function is removing the need for. > [IZ2] This is the case where endptr is used in patch. I can remove endptr but it seems its used so i have to keep the existing functionality in place. + if (!weston_strtoi(pid, &end, 0, &other) || end != pid + 10) >>> I also suspect that the base is never used. In addition a possible future >>> fix would be to accept 0x for hex but a leading zero is decimal, not >>> octal, >>> which means this function would generate the base. >>> >> [IZ] Base is used in weston code as well, please see the whole patch. > > > Out of 14 calls (found with "git grep strto"), 6 use 10, 1 uses 16, and 7 > use 0. > > I believe all the "10"s are to avoid the unexpected-octal problem. I can't > be absolutely certain, but I suspect the reason this is done is to prevent > unexpected octal conversion, and in fact there is no desire to make 0x > prefix not work. > > This is a good indication that perhaps your functions should do this as > well. Basically only base 10 and 0x for base 16 work. > > wscreensaver-glue uses 16 to read the color map out of an xpm file. I cannot > find any code in weston that calls this xpm converter. > >>> I am also worried about passing the out ptr. Depending on the sizes of >>> int, >>> long, and long long this will easily result in code that compiles on one >>> platform but not on another. Returning the value would avoid this and >>> reduce >>> the number of functions. Instead pass a pointer to a variable to store >>> the >>> error into, or use errno? >>> >>> Would prefer you not test the addresses of out parameters for null. It >>> should crash at this point, as this is a programming error, not a runtime >>> error. Returning false will make it hard to figure out what is wrong. >>> >> [IZ] can u please elaborate it bit more as I can add more test cases >> to cover the corner case if I understand clearly what do you want to >> highlight? > > > I want it it CRASH if null is passed for the output pointer. The easiest way > to do this is to not check if it is null. > > Please do not return zero or do anything other than crash right at that > point. This is not an environment or input error, passing null for this is a > programming error that can be fixed at compile time. Hiding the error so it > is not obvious the first time the program is run is completely > non-productive. > [IZ2] Sorry still not able to grasp what you want to highlight. Can you please give an example? >>> It is useful to pass -1 for strtoul and this should continue to work. It >>> is >>> the only easily-recognized value for "largest number possible". May be ok >>> to >>> not allow other negative numbers however. > > > Yes I don't think preserving -1 is a big deal, but I have used this in the > past. The caller can certainly check for this string before calling the > converter. BR imran ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston 5/6] xdg-shell: Rewrite documentation
On 11/22/2014 12:28 PM, Jasper St. Pierre wrote: + xdg_shell allows clients to turn a wl_surface into a "real window" + which can be dragged, resized, stacked, and moved around by the + user. Everything about this interface is suited towards traditional + desktop environments. I think this information should be moved to xdg_surface, which is much more the object the user is going to be thinking about. xdg_shell is just the factory used to create xdg_surface objects. + Destroying a bound xdg_shell object while there are surfaces + still alive with roles from this interface is illegal and will + result in a protocol error. Make sure to destroy all surfaces + before destroying this object. There does not appear to be a way to destroy a xdg_shell, actually. + + Set the "parent" of this surface. This window should be stacked + above a parent. The parent surface must be mapped as long as this + surface is mapped. + + Parent windows should be set on dialogs, toolboxes, or other + "auxilliary" surfaces, so that the parent is raised when the dialog + is raised. I think (or I hope) you mean "the child will be raised when the parent is raised"). You certainly should be able to raise the dialog without raising the parent, otherwise you have something identical to a subsurface. ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston 4/6] xdg-shell: Send an error when the client uses the not-topmost popup
Are you sure this supports a client replacing the top popup with a different one? It seems to set the top to null rather than the next one down when one is removed. I would also be concerned that it may be difficult for a client to insure that the previous popup is destroyed before the new one is created, some possible toolkit designs may like to do all the creation first, then the destruction, to allow reuse of shared data. ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v1] Added string conversion utility functions
On 11/24/2014 05:08 AM, Imran Zaman wrote: Thanks Bill for your comments. Please see my comments inline. On Fri, Nov 21, 2014 at 9:31 PM, Bill Spitzak wrote: There is no need to pass endptr, because your functions will always fail if it is not set to point to the terminating null. [IZ] endptr is passed other than null; please see the whole patch. why do u think it will fail? What I meant was that if this function returns success, endptr will ALWAYS be set to the string+strlen(string), ie at the NULL. As the caller can easily create this answer itself it seems useless to pass a pointer to store it. Although endptr might not point at the null when this fails, it seems hard for the caller to do anything useful with this. It would have to first determine if the error was due to something other than trailing characters, which would involve replicating the entire function locally. Therefore I think this argument is useless and there is no reason to support it. Also the grep below shows that nobody uses the endptr except to do the error checking that this function is removing the need for. I also suspect that the base is never used. In addition a possible future fix would be to accept 0x for hex but a leading zero is decimal, not octal, which means this function would generate the base. [IZ] Base is used in weston code as well, please see the whole patch. Out of 14 calls (found with "git grep strto"), 6 use 10, 1 uses 16, and 7 use 0. I believe all the "10"s are to avoid the unexpected-octal problem. I can't be absolutely certain, but I suspect the reason this is done is to prevent unexpected octal conversion, and in fact there is no desire to make 0x prefix not work. This is a good indication that perhaps your functions should do this as well. Basically only base 10 and 0x for base 16 work. wscreensaver-glue uses 16 to read the color map out of an xpm file. I cannot find any code in weston that calls this xpm converter. I am also worried about passing the out ptr. Depending on the sizes of int, long, and long long this will easily result in code that compiles on one platform but not on another. Returning the value would avoid this and reduce the number of functions. Instead pass a pointer to a variable to store the error into, or use errno? Would prefer you not test the addresses of out parameters for null. It should crash at this point, as this is a programming error, not a runtime error. Returning false will make it hard to figure out what is wrong. [IZ] can u please elaborate it bit more as I can add more test cases to cover the corner case if I understand clearly what do you want to highlight? I want it it CRASH if null is passed for the output pointer. The easiest way to do this is to not check if it is null. Please do not return zero or do anything other than crash right at that point. This is not an environment or input error, passing null for this is a programming error that can be fixed at compile time. Hiding the error so it is not obvious the first time the program is run is completely non-productive. It is useful to pass -1 for strtoul and this should continue to work. It is the only easily-recognized value for "largest number possible". May be ok to not allow other negative numbers however. Yes I don't think preserving -1 is a big deal, but I have used this in the past. The caller can certainly check for this string before calling the converter. ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v2] Increase listen queue to 128
On Mon, 24 Nov 2014 16:10:49 +0200
Imran Zaman wrote:
> This will allow more than 1 simultaneous client connections to the server
> without the possibility of connection refused error.
>
> Signed-off-by: Imran Zaman
> ---
> src/wayland-server.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/wayland-server.c b/src/wayland-server.c
> index c40d300..c845dd6 100644
> --- a/src/wayland-server.c
> +++ b/src/wayland-server.c
> @@ -1139,7 +1139,7 @@ _wl_display_add_socket(struct wl_display *display,
> struct wl_socket *s)
> return -1;
> }
>
> - if (listen(s->fd, 1) < 0) {
> + if (listen(s->fd, 128) < 0) {
> wl_log("listen() failed with error: %m\n");
> return -1;
> }
Pushed, I added the links to the commit message just in case.
Thanks,
pq
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: Wayland and Weston in Patchwork
On Mon, 24 Nov 2014 15:59:31 +0200 Imran Zaman wrote: > Exactly git send-email did ask for utf-8 and i guess i entered y.. shall I > not? > and no idea why git is making it multipart.. > > Shall i try to resend the patch with utf-8 question as 'no'? Actually you should just hit enter, so that it will use the default utf-8. If you write something, that needs to be the charset the text is using. No need to resend unless you have changes. It's still unread in my inbox, so I probably won't skip it. Thanks, pq > > BR > imran > > On Mon, Nov 24, 2014 at 3:50 PM, Pekka Paalanen wrote: > > On Mon, 24 Nov 2014 15:33:51 +0200 > > Imran Zaman wrote: > > > >> Hi > >> > >> Somehow I dont see the patch below in patch work: > >> http://lists.freedesktop.org/archives/wayland-devel/2014-November/018410.html > >> > >> Any idea what is wrong with it or there is some sort of filtering or ? > > > > Hi, yeah, I tried to ask you about it in IRC. > > > > The strange things I see in that email are: > > > > Content-Type: multipart/mixed; boundary="===1810149695==" > > > > (why multipart?) and > > > > Content-Type: text/plain; charset=y > > > > I wonder if one of those caused Patchwork to ignore it. Patchwork also > > seems to ignore patches that contain only binary diffs: > > http://lists.freedesktop.org/archives/wayland-devel/2014-November/018362.html > > > > Imran, did git-send-email ask you what charset to use for these > > patches, suggesting UTF-8 by default, and you perhaps replied "y"? > > That would put the "y" as charset there, I think. :-) > > > > Nothing else comes to mind... > > > > > > Thanks, > > pq ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH v2] Increase listen queue to 128
This will allow more than 1 simultaneous client connections to the server
without the possibility of connection refused error.
Signed-off-by: Imran Zaman
---
src/wayland-server.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/wayland-server.c b/src/wayland-server.c
index c40d300..c845dd6 100644
--- a/src/wayland-server.c
+++ b/src/wayland-server.c
@@ -1139,7 +1139,7 @@ _wl_display_add_socket(struct wl_display *display, struct
wl_socket *s)
return -1;
}
- if (listen(s->fd, 1) < 0) {
+ if (listen(s->fd, 128) < 0) {
wl_log("listen() failed with error: %m\n");
return -1;
}
--
1.9.1
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston] input: don't send to clients key events eaten by bindings
On Fri, 21 Nov 2014 18:34:32 + Daniel Stone wrote: > Hi, > > On 21 November 2014 08:57, Pekka Paalanen wrote: > > > > > I agree 100%. Again, think of nesting compositors: why should our > > compositor > > > > behave completely different if it's nested? The more difficult we make > > it to > > > > sensibly nest compositors, the more we're shooting ourselves in the > > foot. > > > > > > Ok, now you got me confused. Wasn't this what you said would be the > > > correct way to handle bindings? > > > > Yeah, I'm confused too. > > > > I tried to think of a stack of: > > evdev <- Weston <- nested (compositor) <- app > > and the different cases of how Weston behaves towards nested vs. OS > > (e.g. VT-switching) behaves towards Weston, and I'm not sure I see what > > nested would need to do differently from weston with what I thought. > > > > I tried to write out the case that made me nervous in my head, but > couldn't. I'm not sure if that's because it's not actually a real problem, > or I'm just far too tired. > > > > So how should it work? > > > > I would still assume, that if I press Mod+s to take a screenshot, I > > wouldn't want the 's' to appear on the terminal that happened to have > > the keyboard focus when Mod went down. If the terminal had a Mod+s > > shortcut too, should it run or not? > > > > That is, if all of Weston, nested, and app happened to have the same > > shortcut, should all three fire, or only Weston's? > > > > Only Weston. > > All three need to obey the same rule: > - when triggering a hotkey, steal the focus and do _not_ send key events > - when the grab is ended, send the focus back to the client with the > true, correct, and full keyboard state in enter > - on an enter event, update internal state but do _not_ trigger actions > or hotkeys because of it Makes perfect sense, this and your other replies. "Steal focus" means sending wl_keyboard.leave to the current focus, right? Thanks, pq ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: Wayland and Weston in Patchwork
Exactly git send-email did ask for utf-8 and i guess i entered y.. shall I not? and no idea why git is making it multipart.. Shall i try to resend the patch with utf-8 question as 'no'? BR imran On Mon, Nov 24, 2014 at 3:50 PM, Pekka Paalanen wrote: > On Mon, 24 Nov 2014 15:33:51 +0200 > Imran Zaman wrote: > >> Hi >> >> Somehow I dont see the patch below in patch work: >> http://lists.freedesktop.org/archives/wayland-devel/2014-November/018410.html >> >> Any idea what is wrong with it or there is some sort of filtering or ? > > Hi, yeah, I tried to ask you about it in IRC. > > The strange things I see in that email are: > > Content-Type: multipart/mixed; boundary="===1810149695==" > > (why multipart?) and > > Content-Type: text/plain; charset=y > > I wonder if one of those caused Patchwork to ignore it. Patchwork also > seems to ignore patches that contain only binary diffs: > http://lists.freedesktop.org/archives/wayland-devel/2014-November/018362.html > > Imran, did git-send-email ask you what charset to use for these > patches, suggesting UTF-8 by default, and you perhaps replied "y"? > That would put the "y" as charset there, I think. :-) > > Nothing else comes to mind... > > > Thanks, > pq ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH wayland] increase server listen queue to 8
Actually we were connecting multiple simultaneous child/session
westons to system weston and sometime it didn't succeed. Nevertheless
we are not sure that this can be one of the reasons and we couldnt
spot the problem precisely (we were testing it in Tizen with kernel
version: 3.14.19-10.4)
We thought this can be one of the reasons if multiple clients ask for
simultaneous connections.
As you said its harmless, its better to increase the length anyways..
Thanks for the links. I can push the patch again after rebasing and
changing queue length to 128.
BR
imran
On Wed, Nov 19, 2014 at 12:22 PM, Pekka Paalanen wrote:
> On Wed, 15 Oct 2014 16:43:34 +0300
> Imran Zaman wrote:
>
>> Hi
>>
>> This will allow more than 1 simultaneous client connections to the server
>> without the possibility of connection refused error. possible use case
>> is multiple session
>> compositors can connect to the system compositor simultaneously.
>>
>>
>>
>> diff --git a/src/wayland-server.c b/src/wayland-server.c
>> index 674aeca..e3b7d9f 100644
>> --- a/src/wayland-server.c
>> +++ b/src/wayland-server.c
>> @@ -1126,7 +1126,7 @@ _wl_display_add_socket(struct wl_display
>> *display, struct wl_socket *s)
>> return -1;
>> }
>>
>> - if (listen(s->fd, 1) < 0) {
>> + if (listen(s->fd, 8) < 0) {
>> wl_log("listen() failed with error: %m\n");
>> return -1;
>> }
>
> This patch is obviously format-broken, but the change itself seems
> harmless to me. Since no-one has commented on it, I suppose no-one is
> against it.
>
> I also found:
> http://utcc.utoronto.ca/~cks/space/blog/unix/ListenBacklogMeaning
> which suggests that the number is fairly arbitrary.
>
> I couldn't find a clear explanation what the backlog argument means for
> unix domain sockets, either. However, I did find a random comment:
> http://stackoverflow.com/questions/19221105/connect-with-unix-domain-socket-and-full-backlog
> That suggests that on Linux you might rather see EAGAIN than
> ECONNREFUSED. Sounds like it's not completely true?
>
> So, since we have no reason to artificially limit the queue, let's go
> with something big. Say, 128.
>
> Would you like to spin a proper patch, or shall I just rewrite this in
> my own name?
>
> Would be nice to hear how you actually hit the connection refused, and
> on what OS/kernel.
>
>
> Thanks,
> pq
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: Wayland and Weston in Patchwork
On Mon, 24 Nov 2014 15:33:51 +0200 Imran Zaman wrote: > Hi > > Somehow I dont see the patch below in patch work: > http://lists.freedesktop.org/archives/wayland-devel/2014-November/018410.html > > Any idea what is wrong with it or there is some sort of filtering or ? Hi, yeah, I tried to ask you about it in IRC. The strange things I see in that email are: Content-Type: multipart/mixed; boundary="===1810149695==" (why multipart?) and Content-Type: text/plain; charset=y I wonder if one of those caused Patchwork to ignore it. Patchwork also seems to ignore patches that contain only binary diffs: http://lists.freedesktop.org/archives/wayland-devel/2014-November/018362.html Imran, did git-send-email ask you what charset to use for these patches, suggesting UTF-8 by default, and you perhaps replied "y"? That would put the "y" as charset there, I think. :-) Nothing else comes to mind... Thanks, pq ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston 0/6] Fixups and rewrites for xdg-shell
On Sat, 22 Nov 2014 12:28:24 -0800 "Jasper St. Pierre" wrote: > This is the last round of fixups I'm going to to the existing > xdg-shell interface. We'll be adding on other APIs before it > goes fully stable, but before that, let's focus on making > sure what we already have is good enough. > > Jasper St. Pierre (6): > xdg-shell: Take a xdg_surface as the parent surface > xdg-shell: Remove the serial from popup_done > xdg-shell: Remove the flags from get_xdg_popup > xdg-shell: Send an error when the client uses the not-topmost popup > xdg-shell: Rewrite documentation > xdg-shell: Bump unstable version > > clients/simple-damage.c | 2 +- > clients/simple-egl.c| 2 +- > clients/simple-shm.c| 2 +- > clients/window.c| 11 +- > desktop-shell/shell.c | 28 - > protocol/xdg-shell.xml | 269 > +--- > 6 files changed, 196 insertions(+), 118 deletions(-) > Patches 1, 2, 3, and 6 are Reviewed-by: Pekka Paalanen On patch 4 I had one question. For patch 5 I had some comments, but in general very good improvements. I like the "serial of the user event", for instance, and the xdg_popup general explanation is very nice. Obviously this series should land all at once to minimize the disruption, so I'll wait for my comments to be addressed and also comments from others. Thanks, pq ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: Wayland and Weston in Patchwork
Hi Somehow I dont see the patch below in patch work: http://lists.freedesktop.org/archives/wayland-devel/2014-November/018410.html Any idea what is wrong with it or there is some sort of filtering or ? BR imran On Thu, Nov 20, 2014 at 12:47 AM, Bill Spitzak wrote: > On 11/19/2014 07:30 AM, Daniel Stone wrote: > >> I don't think there's any reason to be as precious with Patchwork access >> as we are with others. We need all the help with triage we can get, all >> the changes are cosmetic and logged, and if anyone screws around with it >> and misrepresents mailing list consensus for any reason, then we can >> gently slap them around with a wet trout. >> >> Bill, I've added you as an admin now, so you should be able to change >> these. > > > I took a look and that certainly looks like it will work. > > What I want to do is mass-change my patches to superseded. Since everybody > seems to contribute large sets of patches I think everybody is interested in > this. > > It does seem it should not be too hard of a patchwork modification to make > this box work for everybody, just skipping changes on patches that don't > belong to them, or popping up an error if they are not allowed. > > But it seems pretty safe to make everybody an administrator too. > > > ___ > 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
Re: [PATCH weston 5/6] xdg-shell: Rewrite documentation
On Sat, 22 Nov 2014 12:28:29 -0800 "Jasper St. Pierre" wrote: > This rewrites basically all of the text inside xdg-shell to be up to > date, clearer, and rid of wl_shell and X11 terminology. > --- > protocol/xdg-shell.xml | 256 > ++--- > 1 file changed, 156 insertions(+), 100 deletions(-) > > diff --git a/protocol/xdg-shell.xml b/protocol/xdg-shell.xml > index 360179d..3359cf7 100644 > --- a/protocol/xdg-shell.xml > +++ b/protocol/xdg-shell.xml > @@ -31,11 +31,15 @@ > > > > - This interface is implemented by servers that provide > - desktop-style user interfaces. > - > - It allows clients to associate a xdg_surface with > - a basic surface. > + xdg_shell allows clients to turn a wl_surface into a "real window" > + which can be dragged, resized, stacked, and moved around by the > + user. Everything about this interface is suited towards traditional > + desktop environments. > + > + Destroying a bound xdg_shell object while there are surfaces > + still alive with roles from this interface is illegal and will > + result in a protocol error. Make sure to destroy all surfaces > + before destroying this object. Ok. We don't have a destroy request on xdg_shell interface currently, and you are not adding one in this series, so this cannot be enforced. Is that intentional, or should there be a destroy request? I think having a destroy request on global interfaces should be mandatory, so I'd recommend adding one. What about clients that create multiple xdg_shell objects and then further xdg_* objects from each? Do xdg_shell objects track which xdg_* objects have been created from them? > > > > @@ -65,33 +69,26 @@ > > > > - Create a shell surface for an existing surface. > - > - This request gives the surface the role of xdg_surface. If the > - surface already has another role, it raises a protocol error. > - > - Only one shell or popup surface can be associated with a given > - surface. > + This creates an xdg_surface for the given surface and gives it the > + xdg_surface role. See the documentation of xdg_surface for more details. > > > > > > > - > - Create a popup surface for an existing surface. > - > - This request gives the surface the role of xdg_popup. If the > - surface already has another role, it raises a protocol error. > + > + This creates an xdg_popup for the given surface and gives it the > + xdg_popup role. See the documentation of xdg_surface for more details. ITYM xdg_popup instead of xdg_surface. > > - Only one shell or popup surface can be associated with a given > - surface. > + This request must be used in response to some sort of user action > + like a button press, key press, or touch down event. > > > > > - > - > + > + > > > > @@ -107,7 +104,7 @@ > respond to the ping request, or in what timeframe. Clients should > try to respond in a reasonable amount of time. > > - > + > > > > @@ -120,8 +117,7 @@ > > > > - > - > + >An interface that may be implemented by a wl_surface, for >implementations that provide a desktop-style user interface. > > @@ -136,20 +132,22 @@ > > > > - > - The xdg_surface interface is removed from the wl_surface object > - that was turned into a xdg_surface with > - xdg_shell.get_xdg_surface request. The xdg_surface properties, > - like maximized and fullscreen, are lost. The wl_surface loses > - its role as a xdg_surface. The wl_surface is unmapped. > + > + Unmap and destroy the window. The window will be effectively > + hidden from the user's point of view, and all state like > + maximization, fullscreen, and so on, will be lost. This is ok, but above this there is a paragraph that says: "On the server side the object is automatically destroyed when the related wl_surface is destroyed. On client side, xdg_surface.destroy() must be called before destroying the wl_surface object." I think that paragraph needs to have "On the server side the object is automatically destroyed when the related wl_surface is destroyed." deleted, because it contradicts with the existence of the destroy request. This is likely a left-over from wl_shell_surface, where there is no destroy request. > > > > > - > - Child surfaces are stacked above their parents, and will be > - unmapped if the parent is unmapped too. They should not appear > - on task bars and alt+tab. > + > + Set the "parent" of this surface. This window should be stacked > + above a parent. The parent surface must be mapped as long as this > + surfa
Re: [PATCH v1] Added string conversion utility functions
Thanks Bill for your comments. Please see my comments inline.
On Fri, Nov 21, 2014 at 9:31 PM, Bill Spitzak wrote:
> There is no need to pass endptr, because your functions will always fail if
> it is not set to point to the terminating null.
>
[IZ] endptr is passed other than null; please see the whole patch. why
do u think it will fail?
> I also suspect that the base is never used. In addition a possible future
> fix would be to accept 0x for hex but a leading zero is decimal, not octal,
> which means this function would generate the base.
>
[IZ] Base is used in weston code as well, please see the whole patch.
> I am also worried about passing the out ptr. Depending on the sizes of int,
> long, and long long this will easily result in code that compiles on one
> platform but not on another. Returning the value would avoid this and reduce
> the number of functions. Instead pass a pointer to a variable to store the
> error into, or use errno?
>
> Would prefer you not test the addresses of out parameters for null. It
> should crash at this point, as this is a programming error, not a runtime
> error. Returning false will make it hard to figure out what is wrong.
>
[IZ] can u please elaborate it bit more as I can add more test cases
to cover the corner case if I understand clearly what do you want to
highlight?
> It is useful to pass -1 for strtoul and this should continue to work. It is
> the only easily-recognized value for "largest number possible". May be ok to
> not allow other negative numbers however.
>
[IZ] IMO its good to keep the APIs simple and homogeneous with
appropriate data structures needed for the input
BR
imran
> On 11/21/2014 07:38 AM, Imran Zaman wrote:
>
>> +static bool
>> +convert_strtol(const char *str, char **endptr, int base, long *val)
>> +{
>> + char *end = NULL;
>> + long v;
>> + int prev_errno = errno;
>> +
>
>
>> + if (!str || !val)
>> + return false;
>
> Please do not do the above tests!
>
>> + if (!endptr)
>> + endptr = &end;
>> +
>> + errno = 0;
>> + v = strtol(str, endptr, base);
>> + if (errno != 0 || *endptr == str || **endptr != '\0')
>> + return false;
>> +
>> + errno = prev_errno;
>> + *val = v;
>> + return true;
>> +}
>> +
>> +static bool
>> +convert_strtoul (const char *str, char **endptr, int base, unsigned long
>> *val)
>> +{
>> + char *end = NULL;
>> + unsigned long v;
>> + int i = 0;
>> + int prev_errno = errno;
>> +
>> + if (!str || !val)
>> + return false;
>
>
>> + /* check for negative numbers */
>> + while (str[i]) {
>> + if (!isspace(str[i])) {
>> + if (str[i] == '-')
>> + return false;
>> + else
>> + break;
>> + }
>> + i++;
>> + }
>
> You could do the above test afterwards and check for and allow the -1 value.
>
>
>> +
>> + if (!endptr)
>> + endptr = &end;
>> +
>> + errno = 0;
>> + v = strtoul(str, endptr, base);
>> + if (errno != 0 || *endptr == str || **endptr != '\0')
>> + return false;
>> +
>> + errno = prev_errno;
>> + *val = v;
>> + return true;
>> +}
>> +
>> +WL_EXPORT bool
>> +weston_strtoi(const char *str, char **endptr, int base, int *val)
>> +{
>> + long v;
>> +
>> + if (!convert_strtol(str, endptr, base, &v) || v > INT_MAX
>> + || v < INT_MIN)
>> + return false;
>> +
>> + *val = (int)v;
>> + return true;
>> +}
>> +
>> +WL_EXPORT bool
>> +weston_strtol(const char *str, char **endptr, int base, long *val)
>> +{
>> + return convert_strtol(str, endptr, base, val);
>> +}
>> +
>> +WL_EXPORT bool
>> +weston_strtoui(const char *str, char **endptr, int base, unsigned int
>> *val)
>> +{
>> + unsigned long v;
>> +
>> + if (!convert_strtoul(str, endptr, base, &v) || v > UINT_MAX)
>> + return false;
>> +
>> + *val = (unsigned int)v;
>> + return true;
>> +}
>> +
>> +WL_EXPORT bool
>> +weston_strtoul(const char *str, char **endptr, int base, unsigned long
>> *val)
>> +{
>> + return convert_strtoul(str, endptr, base, val);
>> +}
>
>
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston 4/6] xdg-shell: Send an error when the client uses the not-topmost popup
On Sat, 22 Nov 2014 12:28:28 -0800
"Jasper St. Pierre" wrote:
> Either in destroy or get_xdg_popup.
> ---
> desktop-shell/shell.c | 21 -
> protocol/xdg-shell.xml | 7 +++
> 2 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
> index 7650884..9a69657 100644
> --- a/desktop-shell/shell.c
> +++ b/desktop-shell/shell.c
> @@ -224,6 +224,7 @@ struct shell_seat {
> struct {
> struct weston_pointer_grab grab;
> struct weston_touch_grab touch_grab;
> + struct shell_surface *top_surface;
So 'top_surface' is per shell_seat...
You intend to support nested menus (popups), will these be an xdg_popup
each? I assume they are and the (new) docs seem to say so.
> struct wl_list surfaces_list;
> struct wl_client *client;
> int32_t initial_up;
> @@ -3231,6 +3232,7 @@ popup_grab_end(struct weston_pointer *pointer)
> }
> wl_list_init(&prev->popup.grab_link);
> wl_list_init(&shseat->popup_grab.surfaces_list);
> + shseat->popup_grab.top_surface = NULL;
...shouldn't this restore the previous top_surface rather than NULL?
That is, shouldn't top_surface act like a stack?
Thanks,
pq
> }
> }
>
> @@ -3291,6 +3293,8 @@ add_popup_grab(struct shell_surface *shsurf, struct
> shell_seat *shseat, int32_t
> } else {
> wl_list_insert(&shseat->popup_grab.surfaces_list,
> &shsurf->popup.grab_link);
> }
> +
> + shseat->popup_grab.top_surface = shsurf;
> }
>
> static void
> @@ -3298,6 +3302,13 @@ remove_popup_grab(struct shell_surface *shsurf)
> {
> struct shell_seat *shseat = shsurf->popup.shseat;
>
> + if (shell_surface_is_xdg_popup(shsurf) &&
> shseat->popup_grab.top_surface != shsurf) {
> + wl_resource_post_error(shsurf->resource,
> +XDG_POPUP_ERROR_NOT_THE_TOPMOST_POPUP,
> +"xdg_popup was destroyed while it was
> not the topmost popup.");
> + return;
> + }
> +
> wl_list_remove(&shsurf->popup.grab_link);
> wl_list_init(&shsurf->popup.grab_link);
> if (wl_list_empty(&shseat->popup_grab.surfaces_list)) {
> @@ -3945,7 +3956,15 @@ create_xdg_popup(struct shell_client *owner, void
> *shell,
>uint32_t serial,
>int32_t x, int32_t y)
> {
> - struct shell_surface *shsurf;
> + struct shell_surface *shsurf, *parent_shsurf;
> +
> + parent_shsurf = get_shell_surface(parent);
> + if (seat->popup_grab.top_surface != NULL &&
> seat->popup_grab.top_surface != parent_shsurf) {
> + wl_resource_post_error(owner->resource,
> +XDG_POPUP_ERROR_NOT_THE_TOPMOST_POPUP,
> +"xdg_popup was not created on the
> topmost popup");
> + return NULL;
> + }
>
> shsurf = create_common_surface(owner, shell, surface, client);
> if (!shsurf)
> diff --git a/protocol/xdg-shell.xml b/protocol/xdg-shell.xml
> index 4414d46..360179d 100644
> --- a/protocol/xdg-shell.xml
> +++ b/protocol/xdg-shell.xml
> @@ -398,6 +398,13 @@
>xdg_popup surfaces are always transient for another surface.
>
>
> +
> +
> + These errors can be emitted in response to xdg_popup requests.
> +
> +
> +
> +
>
>
> The xdg_surface interface is removed from the wl_surface object
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston] desktop-shell: don't crash input-panel if no kbd focus
On Thu, 20 Nov 2014 10:25:17 +0200
Pekka Paalanen wrote:
> From: Pekka Paalanen
>
> If a keyboard exists but it has no current focus, yet something asks the
> input-panel to come up, we would crash here. Check that there is a focus
> before attempting to use it.
>
> Maybe there should not even exist a case where input-panel tries to come
> up without a keyboard focus, but I am not sure there is no race where it
> could happen.
>
> In any case, this fix was brought up by the ivi-shell work, where I
> suppose you can somehow hit it.
>
> Signed-off-by: Pekka Paalanen
> Cc: Tanibata, Nobuhiko
> ---
> desktop-shell/input-panel.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/desktop-shell/input-panel.c b/desktop-shell/input-panel.c
> index 435cd5d..0b42c2f 100644
> --- a/desktop-shell/input-panel.c
> +++ b/desktop-shell/input-panel.c
> @@ -66,7 +66,7 @@ show_input_panel_surface(struct input_panel_surface *ipsurf)
> float x, y;
>
> wl_list_for_each(seat, &shell->compositor->seat_list, link) {
> - if (!seat->keyboard)
> + if (!seat->keyboard || !seat->keyboard->focus)
> continue;
> focus = weston_surface_get_main_surface(seat->keyboard->focus);
> ipsurf->output = focus->output;
No objetions, so pushed.
Thanks,
pq
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston] input: don't run the key bindings on focus in
On Fri, 21 Nov 2014 13:45:04 +
Daniel Stone wrote:
> Yep, this is correct.
>
> (I'll get back to the other thread shortly.)
>
> -d
>
> On Thursday, November 20, 2014, Giulio Camuffo
> wrote:
>
> > When getting the focus we get the list of pressed keys, but we are
> > not supposed to run the key binding on them.
> > ---
> > src/input.c | 6 --
> > 1 file changed, 6 deletions(-)
> >
> > diff --git a/src/input.c b/src/input.c
> > index 80aa34e..15ff6ed 100644
> > --- a/src/input.c
> > +++ b/src/input.c
> > @@ -1404,12 +1404,6 @@ notify_keyboard_focus_in(struct weston_seat *seat,
> > struct wl_array *keys,
> >
> > WL_KEYBOARD_KEY_STATE_PRESSED);
> > }
> >
> > - /* Run key bindings after we've updated the state. */
> > - wl_array_for_each(k, &keyboard->keys) {
> > - weston_compositor_run_key_binding(compositor, seat, 0, *k,
> > -
> > WL_KEYBOARD_KEY_STATE_PRESSED);
> > - }
> > -
> > surface = seat->saved_kbd_focus;
> >
> > if (surface) {
> > --
Pushed!
Thanks,
pq
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v1 weston 00/11] Headless rendering testing
On Wed, 19 Nov 2014 15:06:15 -0800 Bryce Harrington wrote: > This set of patches by Derek and I implement functionality to do > screenshot-based testing of wayland. > > The idea is to allow tests to render the desktop to memory using Pixman, > apply an optional clipping mask via Cairo, and store the image to disk > as a PNG file. A pixel checker routine is included which just checks > that the files are bit-for-bit equivalent. The choice of checker is > under the control of the test case itself, so tests that need a more > fuzzy checker can provide one as needed. > > A clipping mechanism is included to permit exclusion of areas of the > screen the test doesn't care about. For example, the desktop clock's > time will vary from run to run, and thus differ from reference images so > you can clip out a subregion that excludes it. Clipping also makes the > reference images smaller in git. > > This patchset is intended to close the following bug reports as fixed: > > https://bugs.freedesktop.org/show_bug.cgi?id=83981 > https://bugs.freedesktop.org/show_bug.cgi?id=83987 > > > Bryce Harrington (7): > configure.ac: Indicate headless compositor presence in config.h > compositor: Document options for headless compositor > tests: Add a fadein test > tests: Add test reference directory and images for fadein > tests: Add support for comparing output against reference images > tests: Use only a clipped portion of screenshot for comparisons > tests: Construct the filename external to wl_test_record_screenshot > > Derek Foreman (4): > compositor-headless: allow rendering with pixman > compositor-headless: add support for transforms set on command line > tests: Allow tests to use customized command line parameters > tests: Add screenshot recording to weston-test > > Makefile.am | 11 +++-- > configure.ac | 3 ++ > protocol/wayland-test.xml | 8 > src/compositor-headless.c | 73 +++ > src/compositor.c | 10 + > tests/fadein-test.c | 76 > tests/reference/fadein-00-0.png | Bin 0 -> 737 bytes > tests/reference/fadein-01-0.png | Bin 0 -> 3453 bytes > tests/reference/fadein-02-0.png | Bin 0 -> 3499 bytes > tests/reference/fadein-03-0.png | Bin 0 -> 3461 bytes > tests/reference/fadein-04-0.png | Bin 0 -> 3461 bytes > tests/reference/fadein-05-0.png | Bin 0 -> 3461 bytes > tests/weston-test-client-helper.c | 67 + > tests/weston-test-client-helper.h | 11 + > tests/weston-test-runner.c| 8 > tests/weston-test.c | 88 > ++ > tests/weston-tests-env| 1 + > 17 files changed, 344 insertions(+), 12 deletions(-) > create mode 100644 tests/fadein-test.c > create mode 100644 tests/reference/fadein-00-0.png > create mode 100644 tests/reference/fadein-01-0.png > create mode 100644 tests/reference/fadein-02-0.png > create mode 100644 tests/reference/fadein-03-0.png > create mode 100644 tests/reference/fadein-04-0.png > create mode 100644 tests/reference/fadein-05-0.png > Hi, this is a good start, and I pushed patches 1-5. I also did some changes to patch 4, I hope you don't mind. On patch 6 and onwards I suggested some improvements, so I marked them as "changes requested" in patchwork. Btw. I think Patchwork ignored patch 8, maybe because it was a pure binary patch. Thanks, pq ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v1 weston 11/11] tests: Construct the filename external to wl_test_record_screenshot
On Wed, 19 Nov 2014 15:06:26 -0800 Bryce Harrington wrote: > We need to use the output filename when we do a comparison with the > appropriate reference image, so move the filename construction to the > caller of record_screenshot() instead. > > Doing this also requires externalizing the output iteration loop since > we want the output number to be recorded in the filename. In most cases > this won't matter since we will just have a single head anyway; plus, in > cases where we do have multiple heads we could well be caring only about > the contents of the Nth head rather than all of them. > > The fadein test doesn't care about multi-head so no need to traverse the > list; we'll look just at head #0. > > Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=83987 > Signed-off-by: Bryce Harrington > --- > protocol/wayland-test.xml | 3 +- > tests/fadein-test.c | 40 ++--- > tests/weston-test-client-helper.c | 34 + > tests/weston-test-client-helper.h | 6 > tests/weston-test.c | 62 > --- > 5 files changed, 83 insertions(+), 62 deletions(-) > > diff --git a/protocol/wayland-test.xml b/protocol/wayland-test.xml > index 93fbc16..4c289a6 100644 > --- a/protocol/wayland-test.xml > +++ b/protocol/wayland-test.xml > @@ -59,7 +59,8 @@ > > > > - > + > + I wonder if using the head index is stable enough. How about using a wl_output object instead? That would be more predictable, if we one day start testing output hotplug, with protocol in the test interface to add and remove outputs. Thanks, pq > > > ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v1 weston 09/11] tests: Add support for comparing output against reference images
On Wed, 19 Nov 2014 15:06:24 -0800
Bryce Harrington wrote:
> Steal Cairo's files_equal() routine to do byte comparison of the rendered
> files. Note that since the clock time will change run to run we can
> only compare against the first frame (which will be black).
Not even the first frame is actually guaranteed to be black, I think.
It's just luck at this point.
> Signed-off-by: Bryce Harrington
> ---
> tests/fadein-test.c | 31 +++
> tests/weston-test-client-helper.c | 33 +
> tests/weston-test-client-helper.h | 5 +
> 3 files changed, 65 insertions(+), 4 deletions(-)
>
> diff --git a/tests/fadein-test.c b/tests/fadein-test.c
> index a6c284f..965bb0a 100644
> --- a/tests/fadein-test.c
> +++ b/tests/fadein-test.c
> @@ -24,27 +24,40 @@
>
> #include
> #include
> +#include
>
> #include "weston-test-client-helper.h"
>
> char *server_parameters="--use-pixman --width=320 --height=240";
>
> static char*
> -output_filename(const char* basename) {
> +output_filename(const char* basename, int head) {
> static const char *path = "./";
> char *filename;
>
> -if (asprintf(&filename, "%s%s", path, basename) < 0)
> +if (asprintf(&filename, "%s%s-%d.png", path, basename, head) < 0)
> filename = NULL;
>
> return filename;
> }
>
> +static char*
> +reference_filename(const char* basename, int head) {
> +static const char *path = "./tests/reference/";
> +char *filename;
> +
> +if (asprintf(&filename, "%s%s-%d.png", path, basename, head) < 0)
> +filename = NULL;
> +
> +return filename;
> +}
> +
> TEST(fadein)
> {
> struct client *client;
> char basename[32];
> char *out_path;
> + char *ref_path;
> int i;
>
> client = client_create(100, 100, 100, 100);
> @@ -52,11 +65,21 @@ TEST(fadein)
>
> for (i = 0; i < 6; i++) {
> snprintf(basename, sizeof basename, "fadein-%02d", i);
> - out_path = output_filename(basename);
> + // FIXME: Iterate over all heads
> + out_path = output_filename(basename, 0);
> + ref_path = reference_filename(basename, 0);
>
> - wl_test_record_screenshot(client->test->wl_test, out_path);
> + // FIXME: Would be preferred to pass in out_path rather than
> basename here...
> + wl_test_record_screenshot(client->test->wl_test, basename);
> client_roundtrip(client);
> + if (i == 0) {
> + if (files_equal(out_path, ref_path))
> + printf("%s is correct\n", out_path);
> + else
> + printf("%s doesn't match reference %s\n",
> out_path, ref_path);
> + }
> free (out_path);
> + free (ref_path);
>
> usleep(25);
> }
> diff --git a/tests/weston-test-client-helper.c
> b/tests/weston-test-client-helper.c
> index 79097fa..72834e4 100644
> --- a/tests/weston-test-client-helper.c
> +++ b/tests/weston-test-client-helper.c
> @@ -22,6 +22,7 @@
>
> #include
>
> +#include
> #include
> #include
> #include
> @@ -624,3 +625,35 @@ client_create(int x, int y, int width, int height)
>
> return client;
> }
> +
> +bool
> +files_equal(const char *test_filename, const char* ref_filename)
> +{
> +FILE *test, *ref;
> +int t, p;
> +
> +if (test_filename == NULL || ref_filename == NULL)
> +return false;
> +
> +test = fopen (test_filename, "rb");
> +if (test == NULL)
> +return false;
> +
> +ref = fopen (ref_filename, "rb");
> +if (ref == NULL) {
> +fclose (test);
> +return false;
> +}
> +
> +do {
> +t = getc (test);
> +p = getc (ref);
> +if (t != p)
> +break;
> +} while (t != EOF && p != EOF);
> +
> +fclose (test);
> +fclose (ref);
> +
> +return t == p; /* both EOF */
> +}
Byte by byte comparison on the raw PNG data, I didn't expect that. :-)
Well, this needs a rewrite with wl_buffer screenshots, and there we
naturally get a chance to apply some fuzz for matching as needed.
Thanks,
pq
> diff --git a/tests/weston-test-client-helper.h
> b/tests/weston-test-client-helper.h
> index 684afc6..20e08b1 100644
> --- a/tests/weston-test-client-helper.h
> +++ b/tests/weston-test-client-helper.h
> @@ -26,6 +26,8 @@
> #include "config.h"
>
> #include
> +#include
> +
> #include "weston-test-runner.h"
> #include "wayland-test-client-protocol.h"
>
> @@ -135,4 +137,7 @@ void
> expect_protocol_error(struct client *client,
> const struct wl_interface *intf, uint32_t code);
>
> +bool
> +files_equal(const char *file_1, const char *file_2);
> +
Re: [PATCH v1 weston 07/11] tests: Add a fadein test
On Wed, 19 Nov 2014 15:06:22 -0800
Bryce Harrington wrote:
> This also serves as a proof of concept of the screen capture
> functionality and as a demo for snapshot-based rendering verification.
>
> Signed-off-by: Bryce Harrington
> ---
> Makefile.am | 7 +-
> tests/fadein-test.c | 64
> +
> 2 files changed, 70 insertions(+), 1 deletion(-)
> create mode 100644 tests/fadein-test.c
>
> diff --git a/Makefile.am b/Makefile.am
> index 26dd473..c3dfa10 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -852,7 +852,8 @@ weston_tests =\
> text.weston \
> presentation.weston \
> roles.weston\
> - subsurface.weston
> + subsurface.weston \
> + fadein.weston
>
>
> AM_TESTS_ENVIRONMENT = \
> @@ -954,6 +955,10 @@ subsurface_weston_SOURCES = tests/subsurface-test.c
> subsurface_weston_CFLAGS = $(AM_CFLAGS) $(TEST_CLIENT_CFLAGS)
> subsurface_weston_LDADD = libtest-client.la
>
> +fadein_weston_SOURCES = tests/fadein-test.c
> +fadein_weston_CFLAGS = $(AM_CFLAGS) $(TEST_CLIENT_CFLAGS)
> +fadein_weston_LDADD = libtest-client.la
> +
> presentation_weston_SOURCES = tests/presentation-test.c
> nodist_presentation_weston_SOURCES = \
> protocol/presentation_timing-protocol.c \
> diff --git a/tests/fadein-test.c b/tests/fadein-test.c
> new file mode 100644
> index 000..a6c284f
> --- /dev/null
> +++ b/tests/fadein-test.c
> @@ -0,0 +1,64 @@
> +/*
> + * Copyright © 2014 Samsung
> + *
> + * 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 "config.h"
> +
> +#include
> +#include
> +
> +#include "weston-test-client-helper.h"
> +
> +char *server_parameters="--use-pixman --width=320 --height=240";
> +
> +static char*
> +output_filename(const char* basename) {
> + static const char *path = "./";
> + char *filename;
> +
> +if (asprintf(&filename, "%s%s", path, basename) < 0)
> + filename = NULL;
> +
> + return filename;
> +}
> +
> +TEST(fadein)
> +{
> + struct client *client;
> + char basename[32];
> + char *out_path;
> + int i;
> +
> + client = client_create(100, 100, 100, 100);
> + assert(client);
> +
> + for (i = 0; i < 6; i++) {
> + snprintf(basename, sizeof basename, "fadein-%02d", i);
> + out_path = output_filename(basename);
> +
> + wl_test_record_screenshot(client->test->wl_test, out_path);
> + client_roundtrip(client);
> + free (out_path);
> +
> + usleep(25);
> + }
> +
> +}
Hi,
where is the verification promised in the commit message? ;-)
I think it is bad to rely on delays/timers. We need something
more deterministic: a protocol to drive the (headless) repaint.
We probably need the headless repaint to not run on its own, but
strictly when requested by the test client. Additionally, we probably
want to carry a "page flip completion" timestamp in that request, and
have the compositor use the timestamp. That way the test client can
actually drive the in-compositor animations, because it is in control
of the clock and framerate.
That should make the fade-in test reliable, and it will help with
Presentation testing too.
The client driven repaint likely needs to be enabled per-test. If a
test attempts to enable it, but the compositor still does not advertise
the support(!) in protocol, the test should skip. This way real
hardware backends can skip these tests automatically, as they cannot
(or not implemented yet) support client driven repaint.
How would that sound?
As a first proof of concept screenshooting test, I'd prefer something
that doesn't depend on timings at all, so we can add th
Re: [PATCH libinput v2 3/3] touchpad: Add edge-scrolling support
Hi,
On 11/21/2014 01:29 AM, Peter Hutterer wrote:
On Thu, Nov 20, 2014 at 10:34:51AM +0100, Hans de Goede wrote:
[...]
+int
+tp_edge_scroll_init(struct tp_dispatch *tp, struct evdev_device *device)
+{
+ struct tp_touch *t;
+ int width, height;
+ int edge_width, edge_height;
+
+ width = device->abs.absinfo_x->maximum - device->abs.absinfo_x->minimum;
+ height = device->abs.absinfo_y->maximum -
device->abs.absinfo_y->minimum;
+
+ switch (tp->model) {
+ case MODEL_SYNAPTICS:
+ edge_width = width * .07;
+ edge_height = height * .07;
+ break;
+ case MODEL_ALPS:
+ edge_width = width * .15;
+ edge_height = height * .15;
+ break;
+ case MODEL_APPLETOUCH:
+ case MODEL_UNIBODY_MACBOOK:
unless there's one I didn't find in my quick search, the unibodies all had
clickpads so we should skip this here and maybe leave a comment for that.
But keep the APPLETOUCH ?
yes, from what I remember (and quick googling seems to confirm this), the
ones with appletouch were e.g. the Core 2 Duo macbooks. Which only had one
mouse button but weren't clickpads yet.
+ edge_width = width * .085;
+ edge_height = height * .085;
+ break;
+ default:
+ edge_width = width * .04;
+ edge_height = height * .054;
make MODEL_SYNAPTICS the same as default please
So use .04 and .054 for synaptics too, and drop the SYNAPTICS case ?
yes please.
It might be worth mentioning in a comment that for the *40 series, the edges
are the absolute edges (not the recommended edges), but since libinput
doesn't care about clickpad edges we ignore them here.
Ok, both fixed for v3, including adding the suggested comments.
Regards,
Hans
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH libinput v3 3/3] touchpad: Add edge-scrolling support
Add edge-scrolling support for non multi-touch touchpads as well as for users who prefer edge-scrolling (as long as they don't have a clickpad). Note the percentage to use of the width / height as scroll-edge differs from one manufacturer to the next, the various per model percentages were taken from xf86-input-synaptics. BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=85635 Signed-off-by: Hans de Goede --- doc/touchpad-edge-scrolling-state-machine.svg | 262 ++ src/Makefile.am | 1 + src/evdev-mt-touchpad-edge-scroll.c | 374 ++ src/evdev-mt-touchpad.c | 95 ++- src/evdev-mt-touchpad.h | 46 5 files changed, 767 insertions(+), 11 deletions(-) create mode 100644 doc/touchpad-edge-scrolling-state-machine.svg create mode 100644 src/evdev-mt-touchpad-edge-scroll.c diff --git a/doc/touchpad-edge-scrolling-state-machine.svg b/doc/touchpad-edge-scrolling-state-machine.svg new file mode 100644 index 000..7a82d88 --- /dev/null +++ b/doc/touchpad-edge-scrolling-state-machine.svg @@ -0,0 +1,262 @@ + +http://www.w3.org/2000/svg"; xmlns:xlink="http://www.w3.org/1999/xlink"; width="1490px" height="1399px" version="1.1"> + + + + + NONE + on-entry: + edge = none + threshold = def + + + + EDGE_NEW + on-entry: + edge = get_edge() + set_timer() + + + + AREA + on-entry: + edge = none + set_pointer() + + + + release + + + + touch + + + + + + touch, + edge &= get_edge() + + + + + + + + + + + + + + + tp_edge_scroll_post_events() + + + + dirty? + + + + + + no + + + + + + yes + + + + + + current = buttons.state & 0x01 + old = buttons.old_state & 0x01 + button = 0 + is_top = 0 + + + + + + notify_axis(last_axis, 0.0) + last_axis = -1 + + + + edge == right + + + + + + yes + + + + axis = scroll_vertical + delta = dy + + + + + + edge == none + + + + + + no + + + + edge == bottom + + + + + + yes + + + + + + no + + + + axis = scroll_horizontal + delta = dx + + + + + + no + + + + get_delta() + + + + + + + + notify_axis(axis, delta) + last_axis = axis + emit(scroll_event_posted) + + + + delta < threshold + + + + + + yes + + + + + + no + + + + + + + + last_axis != -1 + + + + EDGE + on-entry: + threshold = 0.01 + + + + + + + + timeout || + scroll_event_posted + + + + + + + + + + yes + + + + + + yes + + + + + + no + + + + + + + + + + + + + + + + + + yes + + + + + + no + + + + get_edge() + + + + edge + + + + + + no + + + + + + yes + + + diff --git a/src/Makefile.am b/src/Makefile.am index 5cc52a6..027e08c 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -15,6 +15,7 @@ libinput_la_SOURCES = \ evdev-mt-touchpad.h \ evdev-mt-touchpad-tap.c \ evdev-mt-touchpad-buttons.c \ + evdev-mt-touchpad-edge-scroll.c \ filter.c\ filter.h\ filter-private.h\ diff --git a/src/evdev-mt-touchpad-edge-scroll.c b/src/evdev-mt-touchpad-edge-scroll.c new file mode 100644 index 000..1dca0ea --- /dev/null +++ b/src/evdev-mt-touchpad-edge-scroll.c @@ -0,0 +1,374 @@ +/* + * Copyright © 2014 Red Hat, Inc. + * + * 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 implie
[PATCH libinput v3 2/3] touchpad: Add code to get the touchpad model / manufacturer
This is useful to know in some cases, it is e.g. necessary to figure out
which percentage of a touchpads range to use as edge for edge-scrolling.
Note this is a slightly cleaned up copy of the same code in
xf86-input-synaptics.
Signed-off-by: Hans de Goede
---
src/evdev-mt-touchpad.c | 36
src/evdev-mt-touchpad.h | 10 ++
2 files changed, 46 insertions(+)
diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c
index 7a1c32d..6d4b583 100644
--- a/src/evdev-mt-touchpad.c
+++ b/src/evdev-mt-touchpad.c
@@ -1146,6 +1146,40 @@ tp_change_to_left_handed(struct evdev_device *device)
device->buttons.left_handed = device->buttons.want_left_handed;
}
+struct model_lookup_t {
+ uint16_t vendor;
+ uint16_t product_start;
+ uint16_t product_end;
+ enum touchpad_model model;
+};
+
+static struct model_lookup_t model_lookup_table[] = {
+ { 0x0002, 0x0007, 0x0007, MODEL_SYNAPTICS },
+ { 0x0002, 0x0008, 0x0008, MODEL_ALPS },
+ { 0x0002, 0x000e, 0x000e, MODEL_ELANTECH },
+ { 0x05ac, 0, 0x0222, MODEL_APPLETOUCH },
+ { 0x05ac, 0x0223, 0x0228, MODEL_UNIBODY_MACBOOK },
+ { 0x05ac, 0x0229, 0x022b, MODEL_APPLETOUCH },
+ { 0x05ac, 0x022c, 0x, MODEL_UNIBODY_MACBOOK },
+ { 0, 0, 0, 0 }
+};
+
+static enum touchpad_model
+tp_get_model(struct evdev_device *device)
+{
+ struct model_lookup_t *lookup;
+ uint16_t vendor = libevdev_get_id_vendor(device->evdev);
+ uint16_t product = libevdev_get_id_product(device->evdev);
+
+ for (lookup = model_lookup_table; lookup->vendor; lookup++) {
+ if (lookup->vendor == vendor &&
+ lookup->product_start <= product &&
+ product <= lookup->product_end)
+ return lookup->model;
+ }
+ return MODEL_UNKNOWN;
+}
+
struct evdev_dispatch *
evdev_mt_touchpad_create(struct evdev_device *device)
{
@@ -1155,6 +1189,8 @@ evdev_mt_touchpad_create(struct evdev_device *device)
if (!tp)
return NULL;
+ tp->model = tp_get_model(device);
+
if (tp_init(tp, device) != 0) {
tp_destroy(&tp->base);
return NULL;
diff --git a/src/evdev-mt-touchpad.h b/src/evdev-mt-touchpad.h
index 11c4d49..7f3ce49 100644
--- a/src/evdev-mt-touchpad.h
+++ b/src/evdev-mt-touchpad.h
@@ -42,6 +42,15 @@ enum touchpad_event {
TOUCHPAD_EVENT_BUTTON_RELEASE = (1 << 2),
};
+enum touchpad_model {
+ MODEL_UNKNOWN = 0,
+ MODEL_SYNAPTICS,
+ MODEL_ALPS,
+ MODEL_APPLETOUCH,
+ MODEL_ELANTECH,
+ MODEL_UNIBODY_MACBOOK
+};
+
enum touch_state {
TOUCH_NONE = 0,
TOUCH_BEGIN,
@@ -156,6 +165,7 @@ struct tp_dispatch {
unsigned int slot; /* current slot */
bool has_mt;
bool semi_mt;
+ enum touchpad_model model;
unsigned int real_touches; /* number of slots */
unsigned int ntouches; /* no slots inc. fakes */
--
2.1.0
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH libinput v3 1/3] touchpad: Move 2 finger scrolling functions to above tp_process_state()
This is purely a code move, this is a preparation patch for adding edge
scrolling support.
Signed-off-by: Hans de Goede
---
src/evdev-mt-touchpad.c | 118
1 file changed, 59 insertions(+), 59 deletions(-)
diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c
index c3d1d9e..7a1c32d 100644
--- a/src/evdev-mt-touchpad.c
+++ b/src/evdev-mt-touchpad.c
@@ -399,6 +399,65 @@ tp_palm_detect(struct tp_dispatch *tp, struct tp_touch *t,
uint64_t time)
}
static void
+tp_post_twofinger_scroll(struct tp_dispatch *tp, uint64_t time)
+{
+ struct tp_touch *t;
+ int nchanged = 0;
+ double dx = 0, dy =0;
+ double tmpx, tmpy;
+
+ tp_for_each_touch(tp, t) {
+ if (tp_touch_active(tp, t) && t->dirty) {
+ nchanged++;
+ tp_get_delta(t, &tmpx, &tmpy);
+
+ dx += tmpx;
+ dy += tmpy;
+ }
+ /* Stop spurious MOTION events at the end of scrolling */
+ t->is_pointer = false;
+ }
+
+ if (nchanged == 0)
+ return;
+
+ dx /= nchanged;
+ dy /= nchanged;
+
+ tp_filter_motion(tp, &dx, &dy, time);
+
+ evdev_post_scroll(tp->device, time, dx, dy);
+}
+
+static int
+tp_post_scroll_events(struct tp_dispatch *tp, uint64_t time)
+{
+ struct tp_touch *t;
+ int nfingers_down = 0;
+
+ if (tp->scroll.method != LIBINPUT_CONFIG_SCROLL_2FG)
+ return 0;
+
+ /* No scrolling during tap-n-drag */
+ if (tp_tap_dragging(tp))
+ return 0;
+
+ /* Only count active touches for 2 finger scrolling */
+ tp_for_each_touch(tp, t) {
+ if (tp_touch_active(tp, t))
+ nfingers_down++;
+ }
+
+ if (nfingers_down != 2) {
+ evdev_stop_scroll(tp->device, time);
+ return 0;
+ }
+
+ tp_post_twofinger_scroll(tp, time);
+ return 1;
+}
+
+static void
tp_process_state(struct tp_dispatch *tp, uint64_t time)
{
struct tp_touch *t;
@@ -467,65 +526,6 @@ tp_post_process_state(struct tp_dispatch *tp, uint64_t
time)
}
static void
-tp_post_twofinger_scroll(struct tp_dispatch *tp, uint64_t time)
-{
- struct tp_touch *t;
- int nchanged = 0;
- double dx = 0, dy =0;
- double tmpx, tmpy;
-
- tp_for_each_touch(tp, t) {
- if (tp_touch_active(tp, t) && t->dirty) {
- nchanged++;
- tp_get_delta(t, &tmpx, &tmpy);
-
- dx += tmpx;
- dy += tmpy;
- }
- /* Stop spurious MOTION events at the end of scrolling */
- t->is_pointer = false;
- }
-
- if (nchanged == 0)
- return;
-
- dx /= nchanged;
- dy /= nchanged;
-
- tp_filter_motion(tp, &dx, &dy, time);
-
- evdev_post_scroll(tp->device, time, dx, dy);
-}
-
-static int
-tp_post_scroll_events(struct tp_dispatch *tp, uint64_t time)
-{
- struct tp_touch *t;
- int nfingers_down = 0;
-
- if (tp->scroll.method != LIBINPUT_CONFIG_SCROLL_2FG)
- return 0;
-
- /* No scrolling during tap-n-drag */
- if (tp_tap_dragging(tp))
- return 0;
-
- /* Only count active touches for 2 finger scrolling */
- tp_for_each_touch(tp, t) {
- if (tp_touch_active(tp, t))
- nfingers_down++;
- }
-
- if (nfingers_down != 2) {
- evdev_stop_scroll(tp->device, time);
- return 0;
- }
-
- tp_post_twofinger_scroll(tp, time);
- return 1;
-}
-
-static void
tp_post_events(struct tp_dispatch *tp, uint64_t time)
{
struct tp_touch *t = tp_current_touch(tp);
--
2.1.0
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v1 weston 06/11] tests: Add screenshot recording to weston-test
On Wed, 19 Nov 2014 15:06:21 -0800
Bryce Harrington wrote:
> From: Derek Foreman
>
> Adds wl_test_record_screenshot() to weston test. This commit also
> adds a dependency on cairo to weston-test to use it for writing PNG
> files.
>
> Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=83981
> Signed-off-by: Bryce Harrington
Hi,
could we use a wl_shm-based wl_buffer instead of a file, please?
That way we can simply relay the pixels as is to the test client, which
can then compare without compressing and decompressing from PNG first.
For an example how to do this, see Weston's screenshooter protocol and
implementation.
I think you also want to specify in the record_screenshot request:
- which output to capture (in case there are multiple)
- the rectangular sub-region that must be contained in the output
(which you already do with the clip coords in a later patch, so that
should be squashed here)
Synchronization cannot be done with wl_display_roundtrip, so you need
an explicit event for that. The best would be to create a new protocol
object on record_screenshot request, which then delivers the event and
self-destructs, like wl_callback. You could just use wl_callback, but
in principle we should avoid new use cases for wl_callback due to
the design issues.
Please also document carefully in which orientation the screenshot is
taken and how the clip coords are interpreted, in case some
output_scale/transform are in effect. Or if you rely on the renderer's
read_pixels() convention, I hope that is documented somewhere...
Btw. there is a small danger in linking Cairo to the compositor.
GL-renderer uses GLESv2, but if CAIRO_LIBS contains cairo-gl.so linking
to libGL, it might explode... or maybe not because of no RTLD_GLOBAL...
or maybe it could, I don't know. So I'd just avoid that. :-)
I see the test interface has been extended before without bumping the
revision... but we probably should bump it, to not give a bad example.
Thanks,
pq
> ---
> Makefile.am | 4 +--
> protocol/wayland-test.xml | 3 +++
> tests/weston-test.c | 68
> +++
> 3 files changed, 73 insertions(+), 2 deletions(-)
>
> diff --git a/Makefile.am b/Makefile.am
> index 1e7cc81..26dd473 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -881,7 +881,7 @@ noinst_PROGRAMS +=\
> matrix-test
>
> test_module_ldflags = \
> - -module -avoid-version -rpath $(libdir) $(COMPOSITOR_LIBS)
> + -module -avoid-version -rpath $(libdir) $(COMPOSITOR_LIBS) $(CAIRO_LIBS)
>
> surface_global_test_la_SOURCES = tests/surface-global-test.c
> surface_global_test_la_LDFLAGS = $(test_module_ldflags)
> @@ -893,7 +893,7 @@ surface_test_la_CFLAGS = $(GCC_CFLAGS)
> $(COMPOSITOR_CFLAGS)
>
> weston_test_la_LIBADD = $(COMPOSITOR_LIBS) libshared.la
> weston_test_la_LDFLAGS = $(test_module_ldflags)
> -weston_test_la_CFLAGS = $(GCC_CFLAGS) $(COMPOSITOR_CFLAGS)
> +weston_test_la_CFLAGS = $(GCC_CFLAGS) $(COMPOSITOR_CFLAGS) $(CAIRO_CFLAGS)
> weston_test_la_SOURCES = tests/weston-test.c
> nodist_weston_test_la_SOURCES = \
> protocol/wayland-test-protocol.c\
> diff --git a/protocol/wayland-test.xml b/protocol/wayland-test.xml
> index 18b6625..a22a6ac 100644
> --- a/protocol/wayland-test.xml
> +++ b/protocol/wayland-test.xml
> @@ -58,5 +58,8 @@
>
>
>
> +
> +
> +
>
>
> diff --git a/tests/weston-test.c b/tests/weston-test.c
> index f1e45c1..16f20c6 100644
> --- a/tests/weston-test.c
> +++ b/tests/weston-test.c
> @@ -35,6 +35,8 @@
> #include
> #endif /* ENABLE_EGL */
>
> +#include
> +
> struct weston_test {
> struct weston_compositor *compositor;
> struct weston_layer layer;
> @@ -235,6 +237,71 @@ get_n_buffers(struct wl_client *client, struct
> wl_resource *resource)
> wl_test_send_n_egl_buffers(resource, n_buffers);
> }
>
> +static void
> +dump_image(const char *filename, int x, int y, uint32_t *image)
> +{
> + cairo_surface_t *surface, *flipped;
> + cairo_t *cr;
> +
> + surface = cairo_image_surface_create_for_data((unsigned char *)image,
> + CAIRO_FORMAT_ARGB32,
> + x, y, x * 4);
> + flipped = cairo_surface_create_similar_image(surface,
> CAIRO_FORMAT_ARGB32, x, y);
> +
> + cr = cairo_create(flipped);
> + cairo_translate(cr, 0.0, y);
> + cairo_scale(cr, 1.0, -1.0);
> + cairo_set_source_surface(cr, surface, 0, 0);
> + cairo_paint(cr);
> + cairo_destroy(cr);
> + cairo_surface_destroy(surface);
> +
> + cairo_surface_write_to_png(flipped, filename);
> + cairo_surface_destroy(flipped);
> +}
> +
> +static void
> +record_screenshot(struct wl_client *client, struct wl_resource *resource,
> + const char *basename)
> +{
> + struct weston_output *o;
> + struct weston_test *test = wl_res
Re: [PATCH 2/2] touchpad: Add edge-scrolling support
Hi,
On 11/20/2014 07:19 AM, Peter Hutterer wrote:
On Wed, Nov 19, 2014 at 11:50:13AM +0100, Hans de Goede wrote:
[...]
+
+ switch (tp->model) {
+ case MODEL_SYNAPTICS:
+ edge_width = width * .07;
+ edge_height = height * .07;
+ break;
+ case MODEL_ALPS:
+ edge_width = width * .15;
+ edge_height = height * .15;
+ break;
+ case MODEL_APPLETOUCH:
+ case MODEL_UNIBODY_MACBOOK:
the unibody macbooks already had a clickpad, so this isn't needed
+ edge_width = width * .085;
+ edge_height = height * .085;
+ break;
+ default:
+ edge_width = width * .04;
+ edge_height = height * .054;
+ }
fwiw, there is a bit of history there and many oddities are based on a
the edges in synaptics being 'wrong'. the kernel exports the synaptics
dimensions as the coordinates produced by 'an average finger', but it's not
hard to go beyond min/max. iirc for alps and other devices the edges are the
actual edges (and for the T440-style synaptics pads that we manually fixed
up). Hence the need for different edge zones on synaptics.
And the default numbers come from the synaptics user interface guide.
Typical bezel limits: x 1472–5472 y 1408–4448
Typical edge margins: x 1632–5312 y 1568–4288
i.e. 4% and 5.4% are interpolated from those
so in short, we don't need MODEL_SYNAPTICS, it's covered by the defaults.
that just leaves APPLETOUCH and ALPS, I wonder if these have resolutions
set (the synaptics code pre-dates resolution support in the kernel). if so,
just defining a sensible size for the edge is enough (10mm?). or based on
the diagonal.
As also mentioned in a private mail not all alps devices have resolution set,
so at least for some devices we will need to take a percentage of width / height
(no idea why you mention diagonal here).
pls see comments in the other email
static int
@@ -1096,6 +1133,9 @@ tp_init(struct tp_dispatch *tp,
if (tp_init_scroll(tp) != 0)
return -1;
+ if (tp_edge_scroll_init(tp, device) != 0)
+ return -1;
tp_scroll_init() -> tp_edge_scroll_init()
+
device->seat_caps |= EVDEV_DEVICE_POINTER;
return 0;
@@ -1239,6 +1279,7 @@ evdev_mt_touchpad_create(struct evdev_device *device)
tp->model = tp_get_model(device);
+ device->dispatch = &tp->base;
I guess this is needed for some check or another? if so, can we either pass
in the data that's needed, or alternatively make it consistent that the
create method sets device->dispatch for the fallback interface as well.
This is needed because the initial value for tp->scroll.mode is set
by calling tp_scroll_config_scroll_mode_get_default_mode(device).
yeah, I think we need to wrap this differently. The current approach works,
but it's a bit of a cardhouse. specifically, in a failure case the order of
events is:
evdev_mt_touchpad_create() sets device->dispatch but returns NULL
evdev_configure_device() takes the NULL and overwrites device->dispatch
evdev_configure_device() calls evdev_device_destroy() which frees
device->dispatch
so again, it works right now but if we ever rearrange
evdev_configure_device() we'll be dealing with a freed pointer here. and the
failure case isn't easy to test for.
simple way to split this is to have an inline that takes the tp_dispatch and
is called from get_default_mode() and tp_init_touch().
Ok, fixed as suggested for V3.
Regards,
Hans
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v1 weston 05/11] tests: Allow tests to use customized command line parameters
On Fri, 21 Nov 2014 12:38:35 -0800
Bryce Harrington wrote:
> On Fri, Nov 21, 2014 at 04:56:03PM +0200, Pekka Paalanen wrote:
> > On Wed, 19 Nov 2014 15:06:20 -0800
> > Bryce Harrington wrote:
> >
> > > From: Derek Foreman
> > >
> > > Tests will now return the extra command line parameters they need
> > > when run with --params
> > >
> > > Signed-off-by: Bryce Harrington
> > > ---
> > > tests/weston-test-runner.c | 8
> > > tests/weston-tests-env | 1 +
> > > 2 files changed, 9 insertions(+)
> > >
> > > diff --git a/tests/weston-test-runner.c b/tests/weston-test-runner.c
> > > index ef45bae..ce0a670 100644
> > > --- a/tests/weston-test-runner.c
> > > +++ b/tests/weston-test-runner.c
> > > @@ -34,6 +34,8 @@
> > >
> > > #define SKIP 77
> > >
> > > +char __attribute__((weak)) *server_parameters="";
> > > +
> > > extern const struct weston_test __start_test_section,
> > > __stop_test_section;
> > >
> > > static const struct weston_test *
> > > @@ -154,6 +156,12 @@ int main(int argc, char *argv[])
> > > exit(EXIT_SUCCESS);
> > > }
> > >
> > > + if (strcmp(testname, "--params") == 0 ||
> > > + strcmp(testname, "-p") == 0) {
> > > + printf("%s", server_parameters);
> > > + exit(EXIT_SUCCESS);
> > > + }
> > > +
> > > t = find_test(argv[1]);
> > > if (t == NULL) {
> > > fprintf(stderr, "unknown test: \"%s\"\n", argv[1]);
> > > diff --git a/tests/weston-tests-env b/tests/weston-tests-env
> > > index e332354..aaf3ee1 100755
> > > --- a/tests/weston-tests-env
> > > +++ b/tests/weston-tests-env
> > > @@ -46,5 +46,6 @@ case $TESTNAME in
> > > --shell=$SHELL_PLUGIN \
> > > --log="$SERVERLOG" \
> > > --modules=$TEST_PLUGIN,$XWAYLAND_PLUGIN \
> > > + $($abs_builddir/$TESTNAME --params) \
> > > &> "$OUTLOG"
> > > esac
> >
> > Hi,
> >
> > this is pretty clever. I do wonder though if we are going to need a
> > test-specific weston.ini at some point, too. If we do, do we also need
> > to control command line options? Well, apart from specifying the config
> > file location.
>
> There are three different external, direct mechanisms for adjusting
> weston's behavior:
>
> * command line options and parameters
> * weston.ini config file
> * environment variables
>
> Now, afaik we don't have any policies that one of these should be
> preferred and/or a superset of the others, so presumably future tests
> are going to want to test settings against any of the three mechanisms.
> Which means we will need to enable tests to have control over server
> command line options, environment variables, and weston.ini.
>
> If we were to set as a policy that, say, all command line options (and
> maybe environment variables) must have an equivalent setting in
> weston.ini, then we could provide only that interface inside tests.
> This would simplify things from a testing perspective, but it would
> require the rest of the project to adhere to this constraint.
>
> OTOH, we could take the inverse route, and add a command line option
> that allows setting/overriding any arbitrary parameter that can be set
> in weston.ini. (We took this approach in Inkscape with a --verb
> command, that ended up handy both for testing and for other mechanized
> uses of Inkscape.)
Mm, yes, I'm not sure which way to go.
Currently Weston does not have a command line option to read a specific
config file, but there is --no-config. Adding a standard mapping from
all weston.ini options to command line options would be fairly
straightforward I assume, something like '-c/--conf=section:key=value'
and easy to document.
The problem with command line options is that we do not pass them to
helper clients. Helpers like weston-desktop-shell just parse weston.ini
on their own. I'm not sure even --no-config gets propagated to the
helpers.
Something should probably be done to tie the helpers better to the
compositor's config, however that is received.
From security perspective, there is no difference between command line
options and config file, is there?
OTOH, crafting custom config files at test runtime is a bit labourious.
Environment variables we probably need to check case by case. I think
few are because we didn't have a way to give any custom options to
Weston from the tests.
> With respect to the environment variables, fortunately there's not too
> many of them so far. I've started compiling a list and documenting what
> they do.
Excellent!
> > While this patch is fine for now, we are probably going to need
> > something more flexible soon. Currently we only run tests on a single
> > backend at a time, and a single shell at a time. There will become time
> > when we need to test different shell plugins. Oh hey, but this could
> > also work for shell plugins, as tests would likely be shell-specific.
> >
> >
Re: [PATCH libinput 9/9] test: add seat changing tests
Hi,
On 11/24/2014 01:46 AM, Peter Hutterer wrote:
Signed-off-by: Peter Hutterer
Looks good:
Reviewed-by: Hans de Goede
Regards,
Hans
---
test/path.c | 71 ++-
test/udev.c | 78 -
2 files changed, 147 insertions(+), 2 deletions(-)
diff --git a/test/path.c b/test/path.c
index ecb7839..3a2bf2f 100644
--- a/test/path.c
+++ b/test/path.c
@@ -162,6 +162,74 @@ START_TEST(path_added_seat)
}
END_TEST
+START_TEST(path_seat_change)
+{
+ struct litest_device *dev = litest_current_device();
+ struct libinput *li = dev->libinput;
+ struct libinput_event *event;
+ struct libinput_device *device;
+ struct libinput_seat *seat1, *seat2;
+ const char *seat1_name;
+ const char *seat2_name = "new seat";
+ int rc;
+
+ libinput_dispatch(li);
+
+ event = libinput_get_event(li);
+ ck_assert_int_eq(libinput_event_get_type(event),
+LIBINPUT_EVENT_DEVICE_ADDED);
+
+ device = libinput_event_get_device(event);
+ libinput_device_ref(device);
+
+ seat1 = libinput_device_get_seat(device);
+ libinput_seat_ref(seat1);
+
+ seat1_name = libinput_seat_get_logical_name(seat1);
+ libinput_event_destroy(event);
+
+ litest_drain_events(li);
+
+ rc = libinput_device_set_seat_logical_name(device,
+ seat2_name);
+ ck_assert_int_eq(rc, 0);
+
+ libinput_dispatch(li);
+
+ event = libinput_get_event(li);
+ ck_assert(event != NULL);
+
+ ck_assert_int_eq(libinput_event_get_type(event),
+LIBINPUT_EVENT_DEVICE_REMOVED);
+
+ ck_assert(libinput_event_get_device(event) == device);
+ libinput_event_destroy(event);
+
+ event = libinput_get_event(li);
+ ck_assert(event != NULL);
+ ck_assert_int_eq(libinput_event_get_type(event),
+LIBINPUT_EVENT_DEVICE_ADDED);
+ ck_assert(libinput_event_get_device(event) != device);
+ libinput_device_unref(device);
+
+ device = libinput_event_get_device(event);
+ seat2 = libinput_device_get_seat(device);
+
+ ck_assert_str_ne(libinput_seat_get_logical_name(seat2),
+seat1_name);
+ ck_assert_str_eq(libinput_seat_get_logical_name(seat2),
+seat2_name);
+ libinput_event_destroy(event);
+
+ libinput_seat_unref(seat1);
+
+ /* litest: swap the new device in, so cleanup works */
+ libinput_device_unref(dev->libinput_device);
+ libinput_device_ref(device);
+ dev->libinput_device = device;
+}
+END_TEST
+
START_TEST(path_added_device)
{
struct litest_device *dev = litest_current_device();
@@ -805,7 +873,8 @@ main(int argc, char **argv)
litest_add_no_device("path:suspend", path_add_device_suspend_resume);
litest_add_no_device("path:suspend",
path_add_device_suspend_resume_fail);
litest_add_no_device("path:suspend",
path_add_device_suspend_resume_remove_device);
- litest_add_for_device("path:seat events", path_added_seat,
LITEST_SYNAPTICS_CLICKPAD);
+ litest_add_for_device("path:seat", path_added_seat,
LITEST_SYNAPTICS_CLICKPAD);
+ litest_add_for_device("path:seat", path_seat_change,
LITEST_SYNAPTICS_CLICKPAD);
litest_add("path:device events", path_added_device, LITEST_ANY,
LITEST_ANY);
litest_add("path:device events", path_device_sysname, LITEST_ANY,
LITEST_ANY);
litest_add_for_device("path:device events", path_add_device,
LITEST_SYNAPTICS_CLICKPAD);
diff --git a/test/udev.c b/test/udev.c
index 9520663..f5d2c88 100644
--- a/test/udev.c
+++ b/test/udev.c
@@ -175,6 +175,81 @@ START_TEST(udev_added_seat_default)
}
END_TEST
+/**
+ * This test only works if there's at least one device in the system that is
+ * assigned the default seat. Should cover the 99% case.
+ */
+START_TEST(udev_change_seat)
+{
+ struct libinput *li;
+ struct udev *udev;
+ struct libinput_event *event;
+ struct libinput_device *device;
+ struct libinput_seat *seat1, *seat2;
+ const char *seat1_name;
+ const char *seat2_name = "new seat";
+ int rc;
+
+ udev = udev_new();
+ ck_assert(udev != NULL);
+
+ li = libinput_udev_create_context(&simple_interface, NULL, udev);
+ ck_assert(li != NULL);
+ ck_assert_int_eq(libinput_udev_assign_seat(li, "seat0"), 0);
+ libinput_dispatch(li);
+
+ event = libinput_get_event(li);
+ ck_assert(event != NULL);
+
+ ck_assert_int_eq(libinput_event_get_type(event),
+LIBINPUT_EVENT_DEVICE_ADDED);
+
+ device = libinput_event_get_device(event);
+ libinput_device_ref(device);
+
+ seat1 = libinput_device_get_seat(device);
+ libinput_seat_ref(seat1);
+
+ seat1_name = libinput_s
Re: [PATCH libinput 8/9] Add libinput_device_set_seat_logical_name() to change seats at runtime
Hi,
On 11/24/2014 01:46 AM, Peter Hutterer wrote:
The seat of a device is currently immutable, but a device may (in a
multi-pointer case) move between different logical seats. Moving it between
seats is akin to removing it and re-plugging it, so let's do exactly that.
The physical seat name stays immutable.
Pro:
- device handling after changing a seat remains identical as handling any
other device.
Con:
- tracking a device across seat changes is difficult
- this is not an atomic operation, if re-adding the device fails it stays
removed from the original seat and is now dead
Signed-off-by: Peter Hutterer
Looks good:
Reviewed-by: Hans de Goede
Regards,
Hans
---
src/libinput-private.h | 2 ++
src/libinput.c | 13 +
src/libinput.h | 26 ++
src/path.c | 20
src/udev-seat.c| 19 +++
5 files changed, 80 insertions(+)
diff --git a/src/libinput-private.h b/src/libinput-private.h
index 8c75d1f..07db7fd 100644
--- a/src/libinput-private.h
+++ b/src/libinput-private.h
@@ -36,6 +36,8 @@ struct libinput_interface_backend {
int (*resume)(struct libinput *libinput);
void (*suspend)(struct libinput *libinput);
void (*destroy)(struct libinput *libinput);
+ int (*device_change_seat)(struct libinput_device *device,
+ const char *seat_name);
};
struct libinput {
diff --git a/src/libinput.c b/src/libinput.c
index 3f5370f..7b94320 100644
--- a/src/libinput.c
+++ b/src/libinput.c
@@ -1213,6 +1213,19 @@ libinput_device_get_seat(struct libinput_device *device)
return device->seat;
}
+LIBINPUT_EXPORT int
+libinput_device_set_seat_logical_name(struct libinput_device *device,
+ const char *name)
+{
+ struct libinput *libinput = device->seat->libinput;
+
+ if (name == NULL)
+ return -1;
+
+ return libinput->interface_backend->device_change_seat(device,
+ name);
+}
+
LIBINPUT_EXPORT void
libinput_device_led_update(struct libinput_device *device,
enum libinput_led leds)
diff --git a/src/libinput.h b/src/libinput.h
index 092b0e7..9c9a313 100644
--- a/src/libinput.h
+++ b/src/libinput.h
@@ -1373,6 +1373,32 @@ libinput_device_get_seat(struct libinput_device *device);
/**
* @ingroup device
*
+ * Change the logical seat associated with this device by removing the
+ * device and adding it to the new seat.
+ *
+ * This command is identical to physically unplugging the device, then
+ * re-plugging it as member of the new seat,
+ * @ref LIBINPUT_EVENT_DEVICE_REMOVED and @ref LIBINPUT_EVENT_DEVICE_ADDED
+ * events are sent accordingly. Those events mark the end of the lifetime
+ * of this device and the start of a new device.
+ *
+ * If the logical seat name already exists in the device's physical seat,
+ * the device is added to this seat. Otherwise, a new seat is created.
+ *
+ * @note This change applies to this device until removal or @ref
+ * libinput_suspend(), whichever happens earlier.
+ *
+ * @param device A previously obtained device
+ * @param name The new logical seat name
+ * @return 0 on success, non-zero on error
+ */
+int
+libinput_device_set_seat_logical_name(struct libinput_device *device,
+ const char *name);
+
+/**
+ * @ingroup device
+ *
* Update the LEDs on the device, if any. If the device does not have
* LEDs, or does not have one or more of the LEDs given in the mask, this
* function does nothing.
diff --git a/src/path.c b/src/path.c
index fef1d46..57f4cc6 100644
--- a/src/path.c
+++ b/src/path.c
@@ -235,10 +235,30 @@ path_create_device(struct libinput *libinput,
return device;
}
+static int
+path_device_change_seat(struct libinput_device *device,
+ const char *seat_name)
+{
+ struct libinput *libinput = device->seat->libinput;
+ struct evdev_device *evdev_device = (struct evdev_device *)device;
+ struct udev_device *udev_device = NULL;
+ int rc = -1;
+
+ udev_device = evdev_device->udev_device;
+ udev_device_ref(udev_device);
+ libinput_path_remove_device(device);
+
+ if (path_create_device(libinput, udev_device, seat_name) != NULL)
+ rc = 0;
+ udev_device_unref(udev_device);
+ return rc;
+}
+
static const struct libinput_interface_backend interface_backend = {
.resume = path_input_enable,
.suspend = path_input_disable,
.destroy = path_input_destroy,
+ .device_change_seat = path_device_change_seat,
};
LIBINPUT_EXPORT struct libinput *
diff --git a/src/udev-seat.c b/src/udev-seat.c
index c69d175..f7a3df3 100644
--- a/src/udev-seat.c
+++ b/src/udev-seat.c
@@ -332,10 +332,29 @@ udev_seat_get_named(struct udev_input *input, const char
*seat_name)
return
Re: [PATCH libinput 7/9] path: optionally pass the seat name into path_device_enable()
Hi,
On 11/24/2014 01:46 AM, Peter Hutterer wrote:
Prep work for changing seat names on devices. No functional changes.
Signed-off-by: Peter Hutterer
Looks good:
Reviewed-by: Hans de Goede
Regards,
Hans
---
src/path.c | 30 ++
1 file changed, 22 insertions(+), 8 deletions(-)
diff --git a/src/path.c b/src/path.c
index 9d0632a..fef1d46 100644
--- a/src/path.c
+++ b/src/path.c
@@ -111,7 +111,8 @@ path_seat_get_named(struct path_input *input,
static struct libinput_device *
path_device_enable(struct path_input *input,
- struct udev_device *udev_device)
+ struct udev_device *udev_device,
+ const char *seat_logical_name_override)
{
struct path_seat *seat;
struct evdev_device *device = NULL;
@@ -119,12 +120,24 @@ path_device_enable(struct path_input *input,
const char *seat_prop;
const char *devnode;
+ devnode = udev_device_get_devnode(udev_device);
+
seat_prop = udev_device_get_property_value(udev_device, "ID_SEAT");
seat_name = strdup(seat_prop ? seat_prop : default_seat);
- seat_prop = udev_device_get_property_value(udev_device, "WL_SEAT");
- seat_logical_name = strdup(seat_prop ? seat_prop : default_seat_name);
- devnode = udev_device_get_devnode(udev_device);
+ if (seat_logical_name_override) {
+ seat_logical_name = strdup(seat_logical_name_override);
+ } else {
+ seat_prop = udev_device_get_property_value(udev_device,
"WL_SEAT");
+ seat_logical_name = strdup(seat_prop ? seat_prop :
default_seat_name);
+ }
+
+ if (!seat_logical_name) {
+ log_error(&input->base,
+ "failed to create seat name for device '%s'.\n",
+ devnode);
+ goto out;
+ }
seat = path_seat_get_named(input, seat_name, seat_logical_name);
@@ -170,7 +183,7 @@ path_input_enable(struct libinput *libinput)
struct path_device *dev;
list_for_each(dev, &input->path_list, link) {
- if (path_device_enable(input, dev->udev_device) == NULL) {
+ if (path_device_enable(input, dev->udev_device, NULL) == NULL) {
path_input_disable(libinput);
return -1;
}
@@ -196,7 +209,8 @@ path_input_destroy(struct libinput *input)
static struct libinput_device *
path_create_device(struct libinput *libinput,
- struct udev_device *udev_device)
+ struct udev_device *udev_device,
+ const char *seat_name)
{
struct path_input *input = (struct path_input*)libinput;
struct path_device *dev;
@@ -210,7 +224,7 @@ path_create_device(struct libinput *libinput,
list_insert(&input->path_list, &dev->link);
- device = path_device_enable(input, udev_device);
+ device = path_device_enable(input, udev_device, seat_name);
if (!device) {
udev_device_unref(udev_device);
@@ -287,7 +301,7 @@ libinput_path_add_device(struct libinput *libinput,
return NULL;
}
- device = path_create_device(libinput, udev_device);
+ device = path_create_device(libinput, udev_device, NULL);
udev_device_unref(udev_device);
return device;
}
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput 6/9] udev: optionally pass the seat name into device_added()
Hi,
On 11/24/2014 01:46 AM, Peter Hutterer wrote:
Prep work for changing seat names on devices. No functional changes.
Signed-off-by: Peter Hutterer
Looks good:
Reviewed-by: Hans de Goede
Regards,
Hans
---
src/udev-seat.c | 13 -
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/src/udev-seat.c b/src/udev-seat.c
index 49c8f47..c69d175 100644
--- a/src/udev-seat.c
+++ b/src/udev-seat.c
@@ -42,11 +42,13 @@ static struct udev_seat *
udev_seat_get_named(struct udev_input *input, const char *seat_name);
static int
-device_added(struct udev_device *udev_device, struct udev_input *input)
+device_added(struct udev_device *udev_device,
+struct udev_input *input,
+const char *seat_name)
{
struct evdev_device *device;
const char *devnode;
- const char *device_seat, *seat_name, *output_name;
+ const char *device_seat, *output_name;
const char *calibration_values;
float calibration[6];
struct udev_seat *seat;
@@ -61,7 +63,8 @@ device_added(struct udev_device *udev_device, struct
udev_input *input)
devnode = udev_device_get_devnode(udev_device);
/* Search for matching logical seat */
- seat_name = udev_device_get_property_value(udev_device, "WL_SEAT");
+ if (!seat_name)
+ seat_name = udev_device_get_property_value(udev_device,
"WL_SEAT");
if (!seat_name)
seat_name = default_seat_name;
@@ -161,7 +164,7 @@ udev_input_add_devices(struct udev_input *input, struct
udev *udev)
continue;
}
- if (device_added(device, input) < 0) {
+ if (device_added(device, input, NULL) < 0) {
udev_device_unref(device);
udev_enumerate_unref(e);
return -1;
@@ -193,7 +196,7 @@ evdev_udev_handler(void *data)
goto out;
if (!strcmp(action, "add"))
- device_added(udev_device, input);
+ device_added(udev_device, input, NULL);
else if (!strcmp(action, "remove"))
device_removed(udev_device, input);
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput 5/9] evdev: remove a race condition opening the wrong device
Hi,
On 11/24/2014 01:46 AM, Peter Hutterer wrote:
Potential race condition:
- udev notifies us that a udev_device became available
- we go for a coffee and chat to the neighbours on the way
- the device is unplugged
- a new device is plugged in, gets the same devnode
- we finish our coffee and come back
- open(udev_device_get_devnode())
- new device is now opened as the old device
To avoid the above race, we compare the syspath of the device at the open fd
with the syspath of the device we originally wanted. If they differ, we fail.
evdev_compare_syspath was simply moved up.
Signed-off-by: Peter Hutterer
Looks good:
Reviewed-by: Hans de Goede
Regards,
Hans
---
src/evdev.c | 49 ++---
1 file changed, 26 insertions(+), 23 deletions(-)
diff --git a/src/evdev.c b/src/evdev.c
index 8d457f2..8897cd4 100644
--- a/src/evdev.c
+++ b/src/evdev.c
@@ -1449,6 +1449,29 @@ evdev_notify_added_device(struct evdev_device *device)
notify_added_device(&device->base);
}
+static int
+evdev_device_compare_syspath(struct udev_device *udev_device, int fd)
+{
+ struct udev *udev = udev_device_get_udev(udev_device);
+ struct udev_device *udev_device_new;
+ struct stat st;
+ int rc = 1;
+
+ if (fstat(fd, &st) < 0)
+ goto out;
+
+ udev_device_new = udev_device_new_from_devnum(udev, 'c', st.st_rdev);
+ if (!udev_device_new)
+ goto out;
+
+ rc = strcmp(udev_device_get_syspath(udev_device_new),
+ udev_device_get_syspath(udev_device));
+out:
+ if (udev_device_new)
+ udev_device_unref(udev_device_new);
+ return rc;
+}
+
struct evdev_device *
evdev_device_create(struct libinput_seat *seat,
struct udev_device *udev_device)
@@ -1471,6 +1494,9 @@ evdev_device_create(struct libinput_seat *seat,
return NULL;
}
+ if (evdev_device_compare_syspath(udev_device, fd) != 0)
+ goto err;
+
device = zalloc(sizeof *device);
if (device == NULL)
goto err;
@@ -1909,29 +1935,6 @@ evdev_device_suspend(struct evdev_device *device)
return 0;
}
-static int
-evdev_device_compare_syspath(struct udev_device *udev_device, int fd)
-{
- struct udev *udev = udev_device_get_udev(udev_device);
- struct udev_device *udev_device_new;
- struct stat st;
- int rc = 1;
-
- if (fstat(fd, &st) < 0)
- goto out;
-
- udev_device_new = udev_device_new_from_devnum(udev, 'c', st.st_rdev);
- if (!udev_device_new)
- goto out;
-
- rc = strcmp(udev_device_get_syspath(udev_device_new),
- udev_device_get_syspath(udev_device));
-out:
- if (udev_device_new)
- udev_device_unref(udev_device_new);
- return rc;
-}
-
int
evdev_device_resume(struct evdev_device *device)
{
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput 4/9] evdev: use a udev_device instead of separate sysname/syspath/devnode
Hi,
On 11/24/2014 01:46 AM, Peter Hutterer wrote:
Using a udev_device instead of the various bits separately safes us
re-initializing udev contexts whenever we need to compare the device. And
having the actual udev device makes it a bit easier to ensure that we're not
re-initializing a different device as a current one.
Signed-off-by: Peter Hutterer
---
src/evdev.c | 85 +++--
src/evdev.h | 9 ++
src/path.c | 11 ++--
src/udev-seat.c | 16 +--
4 files changed, 46 insertions(+), 75 deletions(-)
diff --git a/src/evdev.c b/src/evdev.c
index fed66cb..8d457f2 100644
--- a/src/evdev.c
+++ b/src/evdev.c
@@ -240,7 +240,8 @@ evdev_flush_pending_event(struct evdev_device *device,
uint64_t time)
if (device->mt.slots[slot].seat_slot != -1) {
log_bug_kernel(libinput,
"%s: Driver sent multiple touch down for the
"
- "same slot", device->devnode);
+ "same slot",
+
udev_device_get_devnode(device->udev_device));
break;
}
@@ -292,7 +293,8 @@ evdev_flush_pending_event(struct evdev_device *device,
uint64_t time)
if (device->abs.seat_slot != -1) {
log_bug_kernel(libinput,
"%s: Driver sent multiple touch down for the
"
- "same slot", device->devnode);
+ "same slot",
+
udev_device_get_devnode(device->udev_device));
break;
}
@@ -1218,20 +1220,11 @@ evdev_need_mtdev(struct evdev_device *device)
static void
evdev_tag_device(struct evdev_device *device)
{
- struct udev *udev;
- struct udev_device *udev_device = NULL;
+ struct udev_device *udev_device;
- udev = udev_new();
- if (!udev)
- return;
-
- udev_device = udev_device_new_from_syspath(udev, device->syspath);
- if (udev_device) {
- if (device->dispatch->interface->tag_device)
- device->dispatch->interface->tag_device(device,
udev_device);
- udev_device_unref(udev_device);
- }
- udev_unref(udev);
+ udev_device = device->udev_device;
+ if (device->dispatch->interface->tag_device)
+ device->dispatch->interface->tag_device(device, udev_device);
Maybe reduce the function to just these 2 lines ? :
if (device->dispatch->interface->tag_device)
device->dispatch->interface->tag_device(device,
device->udev_device);
Otherwise looks good:
Reviewed-by: Hans de Goede
Regards,
Hans
}
static inline int
@@ -1266,6 +1259,7 @@ evdev_configure_device(struct evdev_device *device)
int active_slot;
int slot;
unsigned int i;
+ const char *devnode = udev_device_get_devnode(device->udev_device);
has_rel = 0;
has_abs = 0;
@@ -1364,7 +1358,7 @@ evdev_configure_device(struct evdev_device *device)
device->dispatch = evdev_mt_touchpad_create(device);
log_info(libinput,
"input device '%s', %s is a touchpad\n",
-device->devname, device->devnode);
+device->devname, devnode);
return device->dispatch == NULL ? -1 : 0;
}
@@ -1397,7 +1391,7 @@ evdev_configure_device(struct evdev_device *device)
log_info(libinput,
"input device '%s', %s is a pointer caps =%s%s%s\n",
-device->devname, device->devnode,
+device->devname, devnode,
has_abs ? " absolute-motion" : "",
has_rel ? " relative-motion": "",
has_button ? " button" : "");
@@ -1417,13 +1411,13 @@ evdev_configure_device(struct evdev_device *device)
device->seat_caps |= EVDEV_DEVICE_KEYBOARD;
log_info(libinput,
"input device '%s', %s is a keyboard\n",
-device->devname, device->devnode);
+device->devname, devnode);
}
if (has_touch && !has_button) {
device->seat_caps |= EVDEV_DEVICE_TOUCH;
log_info(libinput,
"input device '%s', %s is a touch device\n",
-device->devname, device->devnode);
+device->devname, devnode);
}
return 0;
@@ -1457,15 +1451,14 @@ evdev_notify_added_device(struct evdev_device *device)
struct evdev_device *
evdev_device_create(struct libinput_seat *seat,
- con
Re: [PATCH libinput 3/9] path: store the udev device instead of just the devnode
Hi,
On 11/24/2014 01:46 AM, Peter Hutterer wrote:
Long-term plan to use more of udev_device here is to better protect us against
re-opening a different device that happens to have the same devnode.
This now also prints an error message for invalid devices, the log tests are
adjusted.
Signed-off-by: Peter Hutterer
---
src/path.c | 109 ++---
src/path.h | 2 +-
test/log.c | 8 +++--
3 files changed, 53 insertions(+), 66 deletions(-)
diff --git a/src/path.c b/src/path.c
index 5900775..175366b 100644
--- a/src/path.c
+++ b/src/path.c
@@ -109,62 +109,26 @@ path_seat_get_named(struct path_input *input,
return NULL;
}
-static int
-path_get_udev_properties(struct udev *udev,
-const char *path,
-char **sysname,
-char **syspath,
-char **seat_name,
-char **seat_logical_name)
-{
- struct udev_device *device = NULL;
- struct stat st;
- const char *seat;
- int rc = -1;
-
- if (stat(path, &st) < 0)
- goto out;
-
- device = udev_device_new_from_devnum(udev, 'c', st.st_rdev);
- if (!device)
- goto out;
-
- *sysname = strdup(udev_device_get_sysname(device));
- *syspath = strdup(udev_device_get_syspath(device));
-
- seat = udev_device_get_property_value(device, "ID_SEAT");
- *seat_name = strdup(seat ? seat : default_seat);
-
- seat = udev_device_get_property_value(device, "WL_SEAT");
- *seat_logical_name = strdup(seat ? seat : default_seat_name);
-
- rc = 0;
-
-out:
- if (device)
- udev_device_unref(device);
- return rc;
-}
-
static struct libinput_device *
-path_device_enable(struct path_input *input, const char *devnode)
+path_device_enable(struct path_input *input,
+ struct udev_device *udev_device)
{
struct path_seat *seat;
struct evdev_device *device = NULL;
char *sysname = NULL, *syspath = NULL;
char *seat_name = NULL, *seat_logical_name = NULL;
+ const char *seat_prop;
+ const char *devnode;
- if (path_get_udev_properties(input->udev,
-devnode,
-&sysname,
-&syspath,
-&seat_name,
-&seat_logical_name) == -1) {
- log_info(&input->base,
-"failed to obtain sysname for device '%s'.\n",
-devnode);
- return NULL;
- }
+ sysname = strdup(udev_device_get_sysname(udev_device));
+ syspath = strdup(udev_device_get_syspath(udev_device));
+
+ seat_prop = udev_device_get_property_value(udev_device, "ID_SEAT");
+ seat_name = strdup(seat_prop ? seat_prop : default_seat);
+
+ seat_prop = udev_device_get_property_value(udev_device, "WL_SEAT");
+ seat_logical_name = strdup(seat_prop ? seat_prop : default_seat_name);
+ devnode = udev_device_get_devnode(udev_device);
seat = path_seat_get_named(input, seat_name, seat_logical_name);
@@ -212,7 +176,7 @@ path_input_enable(struct libinput *libinput)
struct path_device *dev;
list_for_each(dev, &input->path_list, link) {
- if (path_device_enable(input, dev->path) == NULL) {
+ if (path_device_enable(input, dev->udev_device) == NULL) {
path_input_disable(libinput);
return -1;
}
@@ -230,7 +194,7 @@ path_input_destroy(struct libinput *input)
udev_unref(path_input->udev);
list_for_each_safe(dev, tmp, &path_input->path_list, link) {
- free(dev->path);
+ udev_device_unref(dev->udev_device);
free(dev);
}
@@ -238,7 +202,7 @@ path_input_destroy(struct libinput *input)
static struct libinput_device *
path_create_device(struct libinput *libinput,
- const char *path)
+ struct udev_device *udev_device)
{
struct path_input *input = (struct path_input*)libinput;
struct path_device *dev;
@@ -248,19 +212,15 @@ path_create_device(struct libinput *libinput,
if (!dev)
return NULL;
- dev->path = strdup(path);
- if (!dev->path) {
- free(dev);
- return NULL;
- }
+ dev->udev_device = udev_device_ref(udev_device);
list_insert(&input->path_list, &dev->link);
- device = path_device_enable(input, dev->path);
+ device = path_device_enable(input, udev_device);
if (!device) {
+ udev_device_unref(udev_device);
Nitpick, it would be better to use: udev_device_unref(dev->udev_device);
(same thing, but clearer signals intent).
Otherwise looks good:
Reviewed-by: Hans
Re: [PATCH libinput 2/9] path: split out creating a device into a helper function
Hi,
On 11/24/2014 01:46 AM, Peter Hutterer wrote:
No functional changes
Signed-off-by: Peter Hutterer
Looks good:
Reviewed-by: Hans de Goede
Regards,
Hans
---
src/path.c | 57 -
1 file changed, 32 insertions(+), 25 deletions(-)
diff --git a/src/path.c b/src/path.c
index f986afd..5900775 100644
--- a/src/path.c
+++ b/src/path.c
@@ -236,6 +236,37 @@ path_input_destroy(struct libinput *input)
}
+static struct libinput_device *
+path_create_device(struct libinput *libinput,
+ const char *path)
+{
+ struct path_input *input = (struct path_input*)libinput;
+ struct path_device *dev;
+ struct libinput_device *device;
+
+ dev = zalloc(sizeof *dev);
+ if (!dev)
+ return NULL;
+
+ dev->path = strdup(path);
+ if (!dev->path) {
+ free(dev);
+ return NULL;
+ }
+
+ list_insert(&input->path_list, &dev->link);
+
+ device = path_device_enable(input, dev->path);
+
+ if (!device) {
+ list_remove(&dev->link);
+ free(dev->path);
+ free(dev);
+ }
+
+ return device;
+}
+
static const struct libinput_interface_backend interface_backend = {
.resume = path_input_enable,
.suspend = path_input_disable,
@@ -275,36 +306,12 @@ LIBINPUT_EXPORT struct libinput_device *
libinput_path_add_device(struct libinput *libinput,
const char *path)
{
- struct path_input *input = (struct path_input*)libinput;
- struct path_device *dev;
- struct libinput_device *device;
-
if (libinput->interface_backend != &interface_backend) {
log_bug_client(libinput, "Mismatching backends.\n");
return NULL;
}
- dev = zalloc(sizeof *dev);
- if (!dev)
- return NULL;
-
- dev->path = strdup(path);
- if (!dev->path) {
- free(dev);
- return NULL;
- }
-
- list_insert(&input->path_list, &dev->link);
-
- device = path_device_enable(input, dev->path);
-
- if (!device) {
- list_remove(&dev->link);
- free(dev->path);
- free(dev);
- }
-
- return device;
+ return path_create_device(libinput, path);
}
LIBINPUT_EXPORT void
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput 1/9] path: keep the udev context around
Hi,
On 11/24/2014 01:46 AM, Peter Hutterer wrote:
We need it for each device anyway, keep the ref around. Makes error handling a
bit easier, we don't need to handle failing udev_new() and reduce the danger
of mis-refcounting it.
Signed-off-by: Peter Hutterer
Looks good:
Reviewed-by: Hans de Goede
Regards,
Hans
---
src/path.c | 33 +++--
src/path.h | 1 +
2 files changed, 20 insertions(+), 14 deletions(-)
diff --git a/src/path.c b/src/path.c
index 3752751..f986afd 100644
--- a/src/path.c
+++ b/src/path.c
@@ -110,22 +110,18 @@ path_seat_get_named(struct path_input *input,
}
static int
-path_get_udev_properties(const char *path,
+path_get_udev_properties(struct udev *udev,
+const char *path,
char **sysname,
char **syspath,
char **seat_name,
char **seat_logical_name)
{
- struct udev *udev = NULL;
struct udev_device *device = NULL;
struct stat st;
const char *seat;
int rc = -1;
- udev = udev_new();
- if (!udev)
- goto out;
-
if (stat(path, &st) < 0)
goto out;
@@ -147,8 +143,6 @@ path_get_udev_properties(const char *path,
out:
if (device)
udev_device_unref(device);
- if (udev)
- udev_unref(udev);
return rc;
}
@@ -160,8 +154,12 @@ path_device_enable(struct path_input *input, const char
*devnode)
char *sysname = NULL, *syspath = NULL;
char *seat_name = NULL, *seat_logical_name = NULL;
- if (path_get_udev_properties(devnode, &sysname, &syspath,
-&seat_name, &seat_logical_name) == -1) {
+ if (path_get_udev_properties(input->udev,
+devnode,
+&sysname,
+&syspath,
+&seat_name,
+&seat_logical_name) == -1) {
log_info(&input->base,
"failed to obtain sysname for device '%s'.\n",
devnode);
@@ -229,6 +227,8 @@ path_input_destroy(struct libinput *input)
struct path_input *path_input = (struct path_input*)input;
struct path_device *dev, *tmp;
+ udev_unref(path_input->udev);
+
list_for_each_safe(dev, tmp, &path_input->path_list, link) {
free(dev->path);
free(dev);
@@ -247,20 +247,25 @@ libinput_path_create_context(const struct
libinput_interface *interface,
void *user_data)
{
struct path_input *input;
+ struct udev *udev;
if (!interface)
return NULL;
+ udev = udev_new();
+ if (!udev)
+ return NULL;
+
input = zalloc(sizeof *input);
- if (!input)
- return NULL;
-
- if (libinput_init(&input->base, interface,
+ if (!input ||
+ libinput_init(&input->base, interface,
&interface_backend, user_data) != 0) {
+ udev_unref(udev);
free(input);
return NULL;
}
+ input->udev = udev;
list_init(&input->path_list);
return &input->base;
diff --git a/src/path.h b/src/path.h
index 779d823..999601b 100644
--- a/src/path.h
+++ b/src/path.h
@@ -28,6 +28,7 @@
struct path_input {
struct libinput base;
+ struct udev *udev;
struct list path_list;
};
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
