Re: [PATCH libinput 0/8] Add libinput_device_suspend() to disable devices
On Thu, Aug 21, 2014 at 06:35:57PM -0700, Bill Spitzak wrote: > Seems to me you only need two states: what you call disabled and what you > are calling "smart disable". > > The fully disabled is so a button the hardware does not know about can > disable the device. Or a wayland compositor could use a mouse being added to > disable the device. the compositor doesn't know which device is a mouse, which devices are virtual, or even if the touchpad is an external touchpad (which wouldn't need that feature). > But the "smart disable" sounds like part of the device driver / libinput > driver, just like palm detection. There is no reason to turn it off. Maybe > to "tune" it but that would require a much more complex api. ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH wayland v4 1/5] protocol: define the concept of wl_surface role
On Thu, Aug 21, 2014 at 9:29 PM, Bill Spitzak wrote: > On 08/21/2014 04:30 PM, Jasper St. Pierre wrote: > > How do you create more than one copy of the role object in the first >> place? >> > > I thought you could create any number of wl_shell_surface objects for the > same wl_surface. > No, you cannot. You get an error. Source code ref: http://cgit.freedesktop.org/wayland/weston/tree/desktop-shell/shell.c#n3415 > May also be really nasty if a former object-less role is updated >> with a new api that returns a role object. Now all users of that >> role have to be updated to store this object they didn't use to need >> so it won't get deleted... >> >> Methods don't return parameters. Clients allocate new object IDs and >> then pass those to the server. You cannot add new API that returns a >> role object without breaking API in the first place, since it's >> modifying the number of parameters of the method. >> > > I am adding a new global object. Lets say that new api is added to > cursors. To support this add a new wl_cursor global object that you use to > create wl_cursor_surface api objects from wl_surfaces. > > Right now a client that uses a surface as a cursor only has to store the > wl_surface (and the wl_pointer that it calls set_cursor on). > > But as soon as any part of the client wants to use the new > wl_cursor_surface api, it creates an object that must remain in existence > (and thus stored somewhere) until the wl_surface is destroyed, otherwise it > will "lose the cursor role". This may require considerable rewriting of > existing client code since it currently does not store this object. To use a cursor, you must already store a wl_buffer and a wl_surface, and presumably also your SHM data of the cursor. You have to put that in a struct somewhere. If you're already storing three things, storing four should not be harder. > It would be much easier if the client could throw away the api object > after using it, the same way I throw away the result of dynamic_cast after > using it. In particular if it is only going to call the api once. > Throwing away the object means you will leak it. Do not leak objects. > A client subroutine that uses a role the caller does not know about >> will have to use a static map to look up cached role objects by >> wl_surface id. And some scheme has to be figured out to delete them >> when the wl_surface is deleted. >> >> That doesn't make any sense. What kind of client subroutine are you >> talking about? >> > > One that knows about xdg_shell or ivi_shell while the caller only knows > about wl_shell or wl_surface. Or knows about this new wl_cursor_surface api. > > xdg_shell, ivi_shell and wl_cursor_surface are three separate APIs for three separate purposes: that's why they're separate APIs. What kind of client API are you imagining that could act on xdg_shell and ivi_shell, but not on wl_cursor_surface? I have a hard time imagining these hypothetical client APIs that you always bring up in opposition to everything, because I can't honestly imagine what you could expect them to do. I'm getting really frustrated with you Bill. Have you written any Wayland code yet? -- Jasper ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput 0/8] Add libinput_device_suspend() to disable devices
Seems to me you only need two states: what you call disabled and what you are calling "smart disable". The fully disabled is so a button the hardware does not know about can disable the device. Or a wayland compositor could use a mouse being added to disable the device. But the "smart disable" sounds like part of the device driver / libinput driver, just like palm detection. There is no reason to turn it off. Maybe to "tune" it but that would require a much more complex api. ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH wayland v4 1/5] protocol: define the concept of wl_surface role
On 08/21/2014 04:30 PM, Jasper St. Pierre wrote: How do you create more than one copy of the role object in the first place? I thought you could create any number of wl_shell_surface objects for the same wl_surface. May also be really nasty if a former object-less role is updated with a new api that returns a role object. Now all users of that role have to be updated to store this object they didn't use to need so it won't get deleted... Methods don't return parameters. Clients allocate new object IDs and then pass those to the server. You cannot add new API that returns a role object without breaking API in the first place, since it's modifying the number of parameters of the method. I am adding a new global object. Lets say that new api is added to cursors. To support this add a new wl_cursor global object that you use to create wl_cursor_surface api objects from wl_surfaces. Right now a client that uses a surface as a cursor only has to store the wl_surface (and the wl_pointer that it calls set_cursor on). But as soon as any part of the client wants to use the new wl_cursor_surface api, it creates an object that must remain in existence (and thus stored somewhere) until the wl_surface is destroyed, otherwise it will "lose the cursor role". This may require considerable rewriting of existing client code since it currently does not store this object. It would be much easier if the client could throw away the api object after using it, the same way I throw away the result of dynamic_cast after using it. In particular if it is only going to call the api once. A client subroutine that uses a role the caller does not know about will have to use a static map to look up cached role objects by wl_surface id. And some scheme has to be figured out to delete them when the wl_surface is deleted. That doesn't make any sense. What kind of client subroutine are you talking about? One that knows about xdg_shell or ivi_shell while the caller only knows about wl_shell or wl_surface. Or knows about this new wl_cursor_surface api. ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput 3/8] Add libinput_device_suspend() and libinput_device_resume()
On Thu, Aug 21, 2014 at 09:36:57PM +0200, Jonas Ådahl wrote:
> On Wed, Aug 20, 2014 at 01:18:51PM +1000, Peter Hutterer wrote:
> > Does what it says on the box, preventing events from the device without
> > actually
> > dropping the device from the context.
> >
> > Signed-off-by: Peter Hutterer
> > ---
> > See my notes in the coverletter. For the T440 case I specifically did not
> > mention that _suspend() closes the file descriptors.
> >
> > src/evdev.c| 23 +++
> > src/evdev.h| 3 +++
> > src/libinput.c | 16
> > src/libinput.h | 45 +
> > 4 files changed, 87 insertions(+)
> >
> > diff --git a/src/evdev.c b/src/evdev.c
> > index d4a4a07..78d9985 100644
> > --- a/src/evdev.c
> > +++ b/src/evdev.c
> > @@ -1082,6 +1082,29 @@ evdev_device_suspend(struct evdev_device *device)
> > return 0;
> > }
> >
> > +int
> > +evdev_device_resume(struct evdev_device *device)
> > +{
> > + struct libinput *libinput = device->base.seat->libinput;
> > + int fd;
> > +
> > + if (device->fd != -1)
> > + return 0;
> > +
> > + fd = open_restricted(libinput, device->devnode, O_RDWR | O_NONBLOCK);
> > +
> > + if (fd < 0)
> > + return fd;
> > +
>
> We'd need to reset the button/key bitmask here, as it is only updated
> when reading actual evdev events. Note to self: that mask and its
> friends should probably be prefixed with hw_ to make it easier to
> understand that they represent the observed hardware state.
will do, got the hw prefix patch locally now too.
Cheers,
Peter
>
> > + device->fd = fd;
> > + device->source =
> > + libinput_add_fd(libinput, fd, evdev_device_dispatch, device);
> > + if (!device->source)
> > + return -ENOMEM;
> > +
> > + return 0;
> > +}
> > +
> > void
> > evdev_device_remove(struct evdev_device *device)
> > {
> > diff --git a/src/evdev.h b/src/evdev.h
> > index 05125b7..b4749b6 100644
> > --- a/src/evdev.h
> > +++ b/src/evdev.h
> > @@ -182,6 +182,9 @@ evdev_device_transform_y(struct evdev_device *device,
> > int
> > evdev_device_suspend(struct evdev_device *device);
> >
> > +int
> > +evdev_device_resume(struct evdev_device *device);
> > +
> > void
> > evdev_keyboard_notify_key(struct evdev_device *device,
> > uint32_t time,
> > diff --git a/src/libinput.c b/src/libinput.c
> > index 90b6a13..d18d9b8 100644
> > --- a/src/libinput.c
> > +++ b/src/libinput.c
> > @@ -1142,6 +1142,22 @@ libinput_suspend(struct libinput *libinput)
> > libinput->interface_backend->suspend(libinput);
> > }
> >
> > +LIBINPUT_EXPORT int
> > +libinput_device_suspend(struct libinput_device *device)
> > +{
> > + struct evdev_device *dev = (struct evdev_device*)device;
> > +
> > + return evdev_device_suspend(dev);
> > +}
> > +
> > +LIBINPUT_EXPORT int
> > +libinput_device_resume(struct libinput_device *device)
> > +{
> > + struct evdev_device *dev = (struct evdev_device*)device;
> > +
> > + return evdev_device_resume(dev);
> > +}
> > +
> > LIBINPUT_EXPORT void
> > libinput_device_set_user_data(struct libinput_device *device, void
> > *user_data)
> > {
> > diff --git a/src/libinput.h b/src/libinput.h
> > index 9296a35..338a08f 100644
> > --- a/src/libinput.h
> > +++ b/src/libinput.h
> > @@ -1226,6 +1226,51 @@ libinput_device_unref(struct libinput_device
> > *device);
> > /**
> > * @ingroup device
> > *
> > + * Suspend the device but do not remove the device from the context.
> > + * Suspending a device stops the device from generating events until it is
> > + * either resumed with libinput_device_resume() or removed from the
> > context.
> > + * Suspending a device does not generate a @ref
> > + * LIBINPUT_EVENT_DEVICE_REMOVED event.
> > + *
> > + * Events already received and processed from this device are unaffected
> > and
> > + * will be passed to the caller on the next call to libinput_get_event().
> > + * When the device is suspended, it may generate events to reset it into a
> > + * neutral state (e.g. releasing currently pressed buttons).
> > + *
> > + * If the device is already suspended, this function does nothing.
> > + *
> > + * @param device A previously obtained device
> > + * @return 0 on success or a negative errno on failure
> > + *
> > + * @see libinput_device_resume
> > + */
> > +int
> > +libinput_device_suspend(struct libinput_device *device);
> > +
> > +/**
> > + * @ingroup device
> > + *
> > + * Resume a previously suspended device. Events from this device will be
> > + * processed in the next call of libinput_dispatch().
> > + * Resuming a device does not generate a @ref LIBINPUT_EVENT_DEVICE_ADDED
> > + * event.
> > + *
> > + * When the device is resumed, it may generate events to match the logical
> > + * state with the current physical state of the device.
> > + *
> > + * If the device is not currently suspended, this function does nothing.
> > + *
> > + * @param device A previously suspend
Re: [PATCH libinput 0/8] Add libinput_device_suspend() to disable devices
On Thu, Aug 21, 2014 at 10:16:49PM +0200, Jonas Ådahl wrote: > On Thu, Aug 21, 2014 at 04:18:41PM +1000, Peter Hutterer wrote: > > replying to myself, now that I've had a bit of a think about this all. > > > > On Wed, Aug 20, 2014 at 01:18:48PM +1000, Peter Hutterer wrote: > > > This patchset adds two new API hooks, libinput_device_suspend() and > > > libinput_device_resume() which do what it says on the box. No special > > > event > > > is sent when suspending or resuming, so this is really the hook to ignore > > > a > > > specific device. > > > > > > The main idea here is two-fold: > > > some devices simply shouldn't send events, in which case suspending them > > > means fewer wakeup calls. The other use-case is touchpads. GNOME and other > > > DE's provide hooks to disable the touchpad altogether - suspending it > > > achieves that. > > > > > Now, there are some issues that we should discuss: > > > * the touchpad use-case seems to be mainly caused by bad palm detection in > > > the synaptics driver. Presumably having near-perfect palm detection > > > would remove the need for that toggle (except on the > > > devices that have that magic "disable touchpad button" on the touchpad > > > itself). So there's an argument that we don't necessarily need this, > > > but I > > > don't know yet how good palm detection can be. > > There is also the case where the user presses the "disable touchpad" key > on the keyboard which should simply result in the touchpad being > disabled. true, so at least the disabled state is needed then. > > > * the second problem: I don't think we provide enough information to > > > callers to identify which device we don't need. Short of name matching > > > there is no way currently to a touchpad before disabling it. Same goes > > > for > > > identifying any other device we don't care about. > > > To allow that, we'd need some sort of device identification system > > > beyond > > > simple capabilities. > > > > I think we need this sooner or later. Another example is devices that > can emit key events but are not real keyboards. For setups with only > such key emitting devices, for example keyboard layout or key repeat > need not to be configured. yeah, I'm mostly delaying the implementation until we have a couple of definitive use-cases we cover with that approach. Right now, it's all a bit speculative, but we'll likely end up with something similar to the capabilities: libinput_device_has_tag(LIBINPUT_DEVICE_TAG_TOUCHPAD) etc. The trick is getting a sensible set of tags, as the litest.h feature tags show it's quite tricky, especially if they're just added on-demand. > > [...] > > > > I think the suspend/resume approach is not the right one, at least not in > > this form. A better option is to make this a config setting on the device, > > specifically something like: > > > > libinput_device_config_set_disabled_mode(device, ) > > with the parameter being one of: > > - ...ENABLED > > - ...DISABLED > > - ...TOUCHPAD_SMART_DISABLE > > I think using the config API for disabling a device sounds reasonable, > but I'd prefer a bit better terminology here. Set disabled mode enabled > could sound like its either enabled the 'disabled' state, or the state > the device is enabled. Maybe simply make it into a general "mode", i.e. > > libinput_device_config_set_mode(device, ) > with the options identical to those above. reading your comment now I realised that 'mode' has been used for absolute vs relative in X, so we probably shouldn't use that here (I've got a patch somewhere to add that to tablet devices too). 'state' is probably too generic and 'disabled_state' has the same problems. How about: libinput_device_config_set_send_events(device, ) > > ENABLED is what it is, DISABLED still leaves the top button enabled, routed > > through the trackstick where applicable. > > > > SMART_DISABLE solves the above use-case nicely. Identifying which device has > > that feature becomes relatively simple (is the flag set in > > libinput_device_config_get_disabled_modes()?) and what exactly is the > > SMART_DISABLE is left to the implementation. The default would be to disable > > when an external mouse is connected, otherwise leave enabled. > > Is it really useful with such a mode if the meaning of it that vague? > How will it be presented to the user, other than that its "smart" and > depending on what was plugged in it could mean anything. > > A mode like 'disable if mouse connected' could possibly be applicable to > other devices as well, for example the little nob on some Lenovo > laptops. Don't know if that is a feature that is ever asked for however, > so might not be relevant. I don't think I've ever seen that request, mostly because it's a lot harder to accidentally trigger. From a quick survey on G+ it looks like the accidental mouse movement/tap/scroll events are the main reason to disable the touchpad. I suspect we can get that down with better palm
Re: [PATCH wayland v4 1/5] protocol: define the concept of wl_surface role
On Thu, Aug 21, 2014 at 7:18 PM, Bill Spitzak wrote: > > > On 08/21/2014 02:52 AM, Pekka Paalanen wrote: > > + Destroying the role object does not remove the role from the >> + wl_surface, but it may stop the wl_surface from "playing the role". >> + For instance, if a wl_subsurface object is destroyed, the >> wl_surface >> + it was created for will be unmapped and forget its position and >> + z-order. It is allowed to create a wl_subsurface for the same >> + wl_surface again... >> > > I really think destroying the role object should have no visible effect. > > This is inconsistent with roles that don't create a role object. They have > zero role objects too, but somehow the role keeps working. > > It is also inconsistent with the ability to create more than one copy of > the role object. For some reason destruction of the last one is different > than destruction of all the others... > How do you create more than one copy of the role object in the first place? > May also be really nasty if a former object-less role is updated with a > new api that returns a role object. Now all users of that role have to be > updated to store this object they didn't use to need so it won't get > deleted... > Methods don't return parameters. Clients allocate new object IDs and then pass those to the server. You cannot add new API that returns a role object without breaking API in the first place, since it's modifying the number of parameters of the method. > A client subroutine that uses a role the caller does not know about will > have to use a static map to look up cached role objects by wl_surface id. > And some scheme has to be figured out to delete them when the wl_surface is > deleted. > That doesn't make any sense. What kind of client subroutine are you talking about? > It also just seems like it would be easier to implement the compositor > this way, too. > > ___ > wayland-devel mailing list > wayland-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/wayland-devel > -- Jasper ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH wayland v4 1/5] protocol: define the concept of wl_surface role
On 08/21/2014 02:52 AM, Pekka Paalanen wrote: + Destroying the role object does not remove the role from the + wl_surface, but it may stop the wl_surface from "playing the role". + For instance, if a wl_subsurface object is destroyed, the wl_surface + it was created for will be unmapped and forget its position and + z-order. It is allowed to create a wl_subsurface for the same + wl_surface again... I really think destroying the role object should have no visible effect. This is inconsistent with roles that don't create a role object. They have zero role objects too, but somehow the role keeps working. It is also inconsistent with the ability to create more than one copy of the role object. For some reason destruction of the last one is different than destruction of all the others... May also be really nasty if a former object-less role is updated with a new api that returns a role object. Now all users of that role have to be updated to store this object they didn't use to need so it won't get deleted... A client subroutine that uses a role the caller does not know about will have to use a static map to look up cached role objects by wl_surface id. And some scheme has to be figured out to delete them when the wl_surface is deleted. It also just seems like it would be easier to implement the compositor this way, too. ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston 3/3] shell: constrain resize grabs so titlebars don't go under the panel
Constrain it so that both the grabbed point and the nearest point on the edge cannot go under the panel. A radius about the cursor does not work because if they grab the titlebar too close to the top they can't put the window all the way up against the edge (Unity has this bug and it triggers full-screen when you try to move windows, which is incredibly annoying!). Conversely if you grab too far away you can put the edge of the titlebar under the panel. If the window is rotated it is possible the nearest point is already under the panel, in which case it is probably best to pretend it's position is the panel edge instead (ie the user can always drop the window back where they grabbed it, and move it horizontally). I think this will work ok for windows that are rotated upside down, though the user can stick almost all the contents up under the panel. All this applies to the output edges, too. After comparing mine and Jonny's patches the more carefully, I found the patches' idea are different. Jonny's patch puts a "hard requirement" that when expanding in height on the top edge, titlebar shouldn't go under top panel or the resizing won't take effect (quite tricky here). While mine will clamp the y-coordinate of cursor position if it's on the panel. I admit that my approach is somewhat too conservative. But there are some tricky edge case under which Jonny's approach doesn't work very well. Rotate a window clockwise 60-75 degrees, move it up until it can't be moved further. You will find the top edge can't expand (actually expanding the top edge here does no harm I think). More seriously, You'll find that if you shrink the top edge a little, it can't resize back! This behavior, in my opinion, is unacceptable, since too much constraint is put on rotated windows. Also, if you rotate a window 90 degrees and expand the edge that is now on the top. It will go under the panel. This behavior isn't change by Jonny's patch but will be changed by mine since I believe the cursor position in panel shouldn't be used in resizing, regardless of which edge we are resizing, at least a clamped coordinate should be used. I think an ideal solution should surely ensure an unrotated window not to be expanded under panel, but don't place too much restriction on rotated windows. What's your idea? ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston 00/19] Basic tablet support in Weston
On Tue, 2014-08-19 at 16:57 +0300, Pekka Paalanen wrote: > On Tue, 19 Aug 2014 13:21:54 +0200 > Jonas Ådahl wrote: > > > On Tue, Aug 19, 2014 at 01:03:52PM +0300, Pekka Paalanen wrote: > > > On Wed, 6 Aug 2014 19:07:50 -0400 > > > Stephen Chandler Paul wrote: > > > > > > > Hi! As some of you have been aware, I have been working on implementing > > > > tablet > > > > ssupport in libinput, the wayland protocol and weston. This patchset > > > > adds basic > > > > tablet support to weston, along with support in the shell and the window > > > > manager. It should be noted that these patches rely on one of the > > > > earlier > > > > patches to libinput that I posted on the mailing list: "tablet: Add > > > > libinput_tablet_has_axis()", along with the tablet-support branch in > > > > the git repository for libinput. > > > > > > > > As of right now, the following things are still missing/haven't been > > > > finished > > > > yet: > > > > * Tablet objects don't ever send a removed event, right now it's up to > > > > the > > > > clients to free the resources for each tool object. > > > > * Tablet tool objects should be separate for each tablet connected that > > > > doesn't > > > > report serial numbers. Right now tablet objects are shared by all > > > > tablets > > > > connected to the system unconditionally. > > > > * The tablet cursor disappears occasionally when moving a window and > > > > causing > > > > the mouse pointer to change images, but reappears the next time a > > > > client sets > > > > a cursor image. > > > > * weston-flower can crash the shell for some reason. It seems that if a > > > > tablet > > > > is turned on, and the first surface the tool comes into focus on > > > > belongs to a > > > > weston-flower instance, the desktop-shell runs into an error and > > > > exits. I > > > > haven't noticed this with anything other then weston-flower though. > > > > * Held down buttons are released when the tool changes focus from one > > > > surface > > > > to another, instead of being held down until they're released. > > > > * Some of the declarations for structs in my code might need to be moved > > > > around, I'm not sure what the expected guidelines for this in > > > > weston's code > > > > are though. > > > > * A surface argument needs to be added to the proximity_out event, so > > > > we can > > > > tell if the tablet tool is leaving proximity to switch focus to > > > > another > > > > surface, or if it's just exiting proximity. > > > > > > > > Review/critique would be appreciated, thank you! ^^ > > > > > > > > Cheers, > > > > Lyude > > > > > > > > Stephen Chandler Paul (19): > > > > tablet: Add XML for wl_tablet and wl_tablet_manager > > > > tablet: Add initial tablet support to weston > > > > client: Add support for handling motion events in toytoolkit > > > > client: Add support for handling proximity_in/out events in > > > > libtoytoolkit > > > > tablet: Add support for setting/changing the tablet cursor in weston > > > > client: Add tablet cursor support into libtoytoolkit > > > > tablet: Add support for tablet tool objects > > > > client: Add support for tool objects in libtoytoolkit > > > > client: Add support for tablet cursor motion to window frames in > > > > libtoytoolkit > > > > tablet: Add support for button presses to weston > > > > client: Add support for handling button presses to libtoytoolkit > > > > tablet: Add support for up/down events to weston > > > > client: Add up/down event support into libtoytoolkit > > > > tablet: Add support for moving windows around using the stylus > > > > tablet: Add support for reporting pressure, distance, and tilt in > > > > weston > > > > client: Add support for pressure, distance, and tilt into > > > > libtoytoolkit > > > > tablet: Add tablet support to the top panel of the desktop shell > > > > tablet: Add binding to activate surfaces using the tablet tool > > > > tablet: Add demo application for tablets > > > > > > > > .gitignore | 1 + > > > > Makefile.am | 23 +- > > > > clients/desktop-shell.c | 55 > > > > clients/tablet.c| 341 > > > > clients/window.c| 607 > > > > clients/window.h| 102 ++ > > > > desktop-shell/shell.c | 282 + > > > > protocol/wayland-tablet.xml | 310 +++ > > > > shared/cairo-util.h | 4 + > > > > shared/frame.c | 38 +++ > > > > src/bindings.c | 37 +++ > > > > src/compositor.c| 1 + > > > > src/compositor.h| 171 ++ > > > > src/input.c | 736 > > > > > > > > src/libinput-device.c | 317 +++ > > > > src/libinput-device.h | 4 +- > > > > 16 files changed, 3024 inser
Re: [PATCH libinput 0/8] Add libinput_device_suspend() to disable devices
On Thu, Aug 21, 2014 at 04:18:41PM +1000, Peter Hutterer wrote: > replying to myself, now that I've had a bit of a think about this all. > > On Wed, Aug 20, 2014 at 01:18:48PM +1000, Peter Hutterer wrote: > > This patchset adds two new API hooks, libinput_device_suspend() and > > libinput_device_resume() which do what it says on the box. No special event > > is sent when suspending or resuming, so this is really the hook to ignore a > > specific device. > > > > The main idea here is two-fold: > > some devices simply shouldn't send events, in which case suspending them > > means fewer wakeup calls. The other use-case is touchpads. GNOME and other > > DE's provide hooks to disable the touchpad altogether - suspending it > > achieves that. > > > Now, there are some issues that we should discuss: > > * the touchpad use-case seems to be mainly caused by bad palm detection in > > the synaptics driver. Presumably having near-perfect palm detection > > would remove the need for that toggle (except on the > > devices that have that magic "disable touchpad button" on the touchpad > > itself). So there's an argument that we don't necessarily need this, but I > > don't know yet how good palm detection can be. There is also the case where the user presses the "disable touchpad" key on the keyboard which should simply result in the touchpad being disabled. > > > > * the second problem: I don't think we provide enough information to > > callers to identify which device we don't need. Short of name matching > > there is no way currently to a touchpad before disabling it. Same goes for > > identifying any other device we don't care about. > > To allow that, we'd need some sort of device identification system beyond > > simple capabilities. > I think we need this sooner or later. Another example is devices that can emit key events but are not real keyboards. For setups with only such key emitting devices, for example keyboard layout or key repeat need not to be configured. > [...] > > I think the suspend/resume approach is not the right one, at least not in > this form. A better option is to make this a config setting on the device, > specifically something like: > > libinput_device_config_set_disabled_mode(device, ) > with the parameter being one of: > - ...ENABLED > - ...DISABLED > - ...TOUCHPAD_SMART_DISABLE I think using the config API for disabling a device sounds reasonable, but I'd prefer a bit better terminology here. Set disabled mode enabled could sound like its either enabled the 'disabled' state, or the state the device is enabled. Maybe simply make it into a general "mode", i.e. libinput_device_config_set_mode(device, ) with the options identical to those above. > > ENABLED is what it is, DISABLED still leaves the top button enabled, routed > through the trackstick where applicable. > > SMART_DISABLE solves the above use-case nicely. Identifying which device has > that feature becomes relatively simple (is the flag set in > libinput_device_config_get_disabled_modes()?) and what exactly is the > SMART_DISABLE is left to the implementation. The default would be to disable > when an external mouse is connected, otherwise leave enabled. Is it really useful with such a mode if the meaning of it that vague? How will it be presented to the user, other than that its "smart" and depending on what was plugged in it could mean anything. A mode like 'disable if mouse connected' could possibly be applicable to other devices as well, for example the little nob on some Lenovo laptops. Don't know if that is a feature that is ever asked for however, so might not be relevant. > > Finally, from a semantic point of view, having this as configuration is more > logical as we expect this to be pretty much set once and done with it. Agreed. "suspending" and "resuming" sounds more like something related to sessions switching than user configuration. Jonas > > Cheers, >Peter ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH libinput 3/8] Add libinput_device_suspend() and libinput_device_resume()
On Wed, Aug 20, 2014 at 01:18:51PM +1000, Peter Hutterer wrote:
> Does what it says on the box, preventing events from the device without
> actually
> dropping the device from the context.
>
> Signed-off-by: Peter Hutterer
> ---
> See my notes in the coverletter. For the T440 case I specifically did not
> mention that _suspend() closes the file descriptors.
>
> src/evdev.c| 23 +++
> src/evdev.h| 3 +++
> src/libinput.c | 16
> src/libinput.h | 45 +
> 4 files changed, 87 insertions(+)
>
> diff --git a/src/evdev.c b/src/evdev.c
> index d4a4a07..78d9985 100644
> --- a/src/evdev.c
> +++ b/src/evdev.c
> @@ -1082,6 +1082,29 @@ evdev_device_suspend(struct evdev_device *device)
> return 0;
> }
>
> +int
> +evdev_device_resume(struct evdev_device *device)
> +{
> + struct libinput *libinput = device->base.seat->libinput;
> + int fd;
> +
> + if (device->fd != -1)
> + return 0;
> +
> + fd = open_restricted(libinput, device->devnode, O_RDWR | O_NONBLOCK);
> +
> + if (fd < 0)
> + return fd;
> +
We'd need to reset the button/key bitmask here, as it is only updated
when reading actual evdev events. Note to self: that mask and its
friends should probably be prefixed with hw_ to make it easier to
understand that they represent the observed hardware state.
Jonas
> + device->fd = fd;
> + device->source =
> + libinput_add_fd(libinput, fd, evdev_device_dispatch, device);
> + if (!device->source)
> + return -ENOMEM;
> +
> + return 0;
> +}
> +
> void
> evdev_device_remove(struct evdev_device *device)
> {
> diff --git a/src/evdev.h b/src/evdev.h
> index 05125b7..b4749b6 100644
> --- a/src/evdev.h
> +++ b/src/evdev.h
> @@ -182,6 +182,9 @@ evdev_device_transform_y(struct evdev_device *device,
> int
> evdev_device_suspend(struct evdev_device *device);
>
> +int
> +evdev_device_resume(struct evdev_device *device);
> +
> void
> evdev_keyboard_notify_key(struct evdev_device *device,
> uint32_t time,
> diff --git a/src/libinput.c b/src/libinput.c
> index 90b6a13..d18d9b8 100644
> --- a/src/libinput.c
> +++ b/src/libinput.c
> @@ -1142,6 +1142,22 @@ libinput_suspend(struct libinput *libinput)
> libinput->interface_backend->suspend(libinput);
> }
>
> +LIBINPUT_EXPORT int
> +libinput_device_suspend(struct libinput_device *device)
> +{
> + struct evdev_device *dev = (struct evdev_device*)device;
> +
> + return evdev_device_suspend(dev);
> +}
> +
> +LIBINPUT_EXPORT int
> +libinput_device_resume(struct libinput_device *device)
> +{
> + struct evdev_device *dev = (struct evdev_device*)device;
> +
> + return evdev_device_resume(dev);
> +}
> +
> LIBINPUT_EXPORT void
> libinput_device_set_user_data(struct libinput_device *device, void
> *user_data)
> {
> diff --git a/src/libinput.h b/src/libinput.h
> index 9296a35..338a08f 100644
> --- a/src/libinput.h
> +++ b/src/libinput.h
> @@ -1226,6 +1226,51 @@ libinput_device_unref(struct libinput_device *device);
> /**
> * @ingroup device
> *
> + * Suspend the device but do not remove the device from the context.
> + * Suspending a device stops the device from generating events until it is
> + * either resumed with libinput_device_resume() or removed from the context.
> + * Suspending a device does not generate a @ref
> + * LIBINPUT_EVENT_DEVICE_REMOVED event.
> + *
> + * Events already received and processed from this device are unaffected and
> + * will be passed to the caller on the next call to libinput_get_event().
> + * When the device is suspended, it may generate events to reset it into a
> + * neutral state (e.g. releasing currently pressed buttons).
> + *
> + * If the device is already suspended, this function does nothing.
> + *
> + * @param device A previously obtained device
> + * @return 0 on success or a negative errno on failure
> + *
> + * @see libinput_device_resume
> + */
> +int
> +libinput_device_suspend(struct libinput_device *device);
> +
> +/**
> + * @ingroup device
> + *
> + * Resume a previously suspended device. Events from this device will be
> + * processed in the next call of libinput_dispatch().
> + * Resuming a device does not generate a @ref LIBINPUT_EVENT_DEVICE_ADDED
> + * event.
> + *
> + * When the device is resumed, it may generate events to match the logical
> + * state with the current physical state of the device.
> + *
> + * If the device is not currently suspended, this function does nothing.
> + *
> + * @param device A previously suspended device
> + * @return 0 on success or a negative errno on failure
> + *
> + * @see libinput_device_suspend
> + */
> +int
> +libinput_device_resume(struct libinput_device *device);
> +
> +/**
> + * @ingroup device
> + *
> * Set caller-specific data associated with this input device. libinput does
> * not manage, look at, or modify this data. The caller must ensure the
> *
Re: [PATCH libinput 7/8] evdev: ignore excessive releases of a button or key
On Wed, Aug 20, 2014 at 01:18:55PM +1000, Peter Hutterer wrote:
> When we're suspending a touchpad, several events triggered by timers may still
> be waiting in the background. We still need to release all buttons/keys
> immediately though so we get an uneven number of release events: one from
> release_pressed_keys() and one when the timers expire and send the release
> event through the normal channels.
>
> Don't assert if we're already on a press-count of 0, simply ignore it and
> discard the event.
>
> Signed-off-by: Peter Hutterer
> ---
> I'm not 100% sure on this one. It's the easy solution, but until we know the
> general direction I didn't want to implement the full solution.
> What's needed here are hooks for suspending/resuming in the dispatch structs
> so that the touchpad code can cancel anything currently happening and reset
> the state. For the normal fallback_dispatch the hooks can just release keys
> and touches.
If suspending of the dispatch modules would guarantee to be graceful
(i.e. guarantee symmetric button presses), the auto-release function
could, partly as what you did in a previous patch, use evdev_notify_...
but do so by checking the bitmask instead of the counter in order to
only deal with the physical button presses it tracks. It'd be the
cleaner solution IMHO.
Jonas
>
> src/evdev.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/src/evdev.c b/src/evdev.c
> index 6bbea92..755e7a1 100644
> --- a/src/evdev.c
> +++ b/src/evdev.c
> @@ -74,8 +74,10 @@ update_key_down_count(struct evdev_device *device, int
> code, int pressed)
> if (pressed) {
> key_count = ++device->key_count[code];
> } else {
> - assert(device->key_count[code] > 0);
> - key_count = --device->key_count[code];
> + if (device->key_count[code] > 0)
> + key_count = --device->key_count[code];
> + else
> + return -1;
> }
>
> if (key_count > 32) {
> --
> 1.9.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 6/8] evdev: drop the button count when releasing keys on remove
On Wed, Aug 20, 2014 at 01:18:54PM +1000, Peter Hutterer wrote: > We only called this function before device removal, so failing to update the > button state didn't matter. To make this function generic for the upcoming > device suspend/resume, we need to keep track of the button/key count properly. > > Signed-off-by: Peter Hutterer > --- > src/evdev.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/src/evdev.c b/src/evdev.c > index 74632c8..6bbea92 100644 > --- a/src/evdev.c > +++ b/src/evdev.c > @@ -1042,15 +1042,15 @@ release_pressed_keys(struct evdev_device *device) > case EVDEV_KEY_TYPE_NONE: > break; > case EVDEV_KEY_TYPE_KEY: > - keyboard_notify_key( > - &device->base, > + evdev_keyboard_notify_key( > + device, This might not necessarily result in a key event is queued as it would only do so if the count reaches 0. I think right now, as the 'touchpad: Only break out of tap FSM for clickpad button presses' patch has not been applied, we only ever have one concurrent button press with the same code, so it the problem won't show. > time, > code, > LIBINPUT_KEY_STATE_RELEASED); > break; > case EVDEV_KEY_TYPE_BUTTON: > - pointer_notify_button( > - &device->base, > + evdev_pointer_notify_button( > + device, Same as above. Jonas > time, > code, > LIBINPUT_BUTTON_STATE_RELEASED); > -- > 1.9.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] tests: allow running make check without make install
And here's a second round with free()s for the asprintf()s.
On 21/08/14 11:46 AM, Derek Foreman wrote:
> This is my first attempt at a patch to allow running weston-keyboard,
> desktop-shell and screenshooter out of the build directory so make check
> doesn't do funny things...
>
>
>
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
>From 7a5bcfaf48693018c5bbe3779bbd9d080d4e436f Mon Sep 17 00:00:00 2001
From: Derek Foreman
Date: Thu, 21 Aug 2014 11:32:38 -0500
Subject: [PATCH] tests: allow running make check without make install
desktop shell and weston keyboard both refer to themselves prefixed by
LIBEXECDIR, however this is only valid once installed. make check will
currently either fail or run pre-existing versions.
This patch adds a way to override that location by setting the env var
WESTON_BUILD_DIR - which is then set by the test env script so make check
will test the versions in the build directory regardless of whether they're
installed or not.
---
desktop-shell/shell.c | 7 +--
shared/config-parser.c | 12
shared/config-parser.h | 3 +++
src/screenshooter.c| 6 +-
src/text-backend.c | 6 +-
tests/weston-tests-env | 2 ++
6 files changed, 32 insertions(+), 4 deletions(-)
diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
index e3abaad..1e7e7ed 100644
--- a/desktop-shell/shell.c
+++ b/desktop-shell/shell.c
@@ -567,7 +567,7 @@ shell_configuration(struct desktop_shell *shell)
{
struct weston_config_section *section;
int duration;
- char *s;
+ char *s, *client;
section = weston_config_get_section(shell->compositor->config,
"screensaver", NULL, NULL);
@@ -578,8 +578,11 @@ shell_configuration(struct desktop_shell *shell)
section = weston_config_get_section(shell->compositor->config,
"shell", NULL, NULL);
+ asprintf(&client, "%s/%s", weston_config_get_libexec_dir(),
+ WESTON_SHELL_CLIENT);
weston_config_section_get_string(section,
- "client", &s, LIBEXECDIR "/" WESTON_SHELL_CLIENT);
+ "client", &s, client);
+ free(client);
shell->client = s;
weston_config_section_get_string(section,
"binding-modifier", &s, "super");
diff --git a/shared/config-parser.c b/shared/config-parser.c
index 8defbbb..4542ca6 100644
--- a/shared/config-parser.c
+++ b/shared/config-parser.c
@@ -282,6 +282,18 @@ weston_config_section_get_bool(struct weston_config_section *section,
return 0;
}
+WL_EXPORT
+const char *
+weston_config_get_libexec_dir(void)
+{
+ const char *path = getenv("WESTON_BUILD_DIR");
+
+ if (path)
+ return path;
+
+ return LIBEXECDIR;
+}
+
static struct weston_config_section *
config_add_section(struct weston_config *config, const char *name)
{
diff --git a/shared/config-parser.h b/shared/config-parser.h
index 745562b..1ecc8cc 100644
--- a/shared/config-parser.h
+++ b/shared/config-parser.h
@@ -92,6 +92,9 @@ int
weston_config_section_get_bool(struct weston_config_section *section,
const char *key,
int *value, int default_value);
+const char *
+weston_config_get_libexec_dir(void);
+
struct weston_config *
weston_config_parse(const char *name);
diff --git a/src/screenshooter.c b/src/screenshooter.c
index 4403933..af2c754 100644
--- a/src/screenshooter.c
+++ b/src/screenshooter.c
@@ -286,12 +286,16 @@ screenshooter_binding(struct weston_seat *seat, uint32_t time, uint32_t key,
void *data)
{
struct screenshooter *shooter = data;
- const char *screenshooter_exe = LIBEXECDIR "/weston-screenshooter";
+ char *screenshooter_exe;
+
+ asprintf(&screenshooter_exe, "%s/%s", weston_config_get_libexec_dir(),
+ "/weston-screenshooter");
if (!shooter->client)
shooter->client = weston_client_launch(shooter->ec,
&shooter->process,
screenshooter_exe, screenshooter_sigchld);
+ free(screenshooter_exe);
}
struct weston_recorder {
diff --git a/src/text-backend.c b/src/text-backend.c
index 1d549d4..7d2a064 100644
--- a/src/text-backend.c
+++ b/src/text-backend.c
@@ -937,12 +937,16 @@ static void
text_backend_configuration(struct text_backend *text_backend)
{
struct weston_config_section *section;
+ char *client;
section = weston_config_get_section(text_backend->compositor->config,
"input-method", NULL, NULL);
+ asprintf(&client, "%s/weston-keyboard",
+ weston_config_get_libexec_dir());
weston_config_section_get_string(section, "path",
&text_backend->input_method.path,
- LIBEXECDIR "/weston-keyboard");
+ client);
+ free(client);
}
static void
diff --git a/tests/weston-tests-env b/tests/weston-tests-env
index 473e092..e332354 100755
--- a/tests/weston-tests-env
+++ b/tests/weston-tests-env
@@ -28,6 +28,7 @@ XWAYLAND_PLUGIN=$abs_builddir/.libs/xwayland.so
case $TESTNAME in
*.la|*.so)
+ WESTON_BUILD_DIR=$abs_builddir \
$WESTON --backend=$BACKEND \
--no-config
[PATCH] tests: allow running make check without make install
This is my first attempt at a patch to allow running weston-keyboard,
desktop-shell and screenshooter out of the build directory so make check
doesn't do funny things...
>From a97352451bff6398edace74defca67e18c92bb98 Mon Sep 17 00:00:00 2001
From: Derek Foreman
Date: Thu, 21 Aug 2014 11:32:38 -0500
Subject: [PATCH] tests: allow running make check without make install
desktop shell and weston keyboard both refer to themselves prefixed by
LIBEXECDIR, however this is only valid once installed. make check will
currently either fail or run pre-existing versions.
This patch adds a way to override that location by setting the env var
WESTON_BUILD_DIR - which is then set by the test env script so make check
will test the versions in the build directory regardless of whether they're
installed or not.
---
desktop-shell/shell.c | 6 --
shared/config-parser.c | 12
shared/config-parser.h | 3 +++
src/screenshooter.c| 5 -
src/text-backend.c | 5 -
tests/weston-tests-env | 2 ++
6 files changed, 29 insertions(+), 4 deletions(-)
diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
index e3abaad..f78bdc9 100644
--- a/desktop-shell/shell.c
+++ b/desktop-shell/shell.c
@@ -567,7 +567,7 @@ shell_configuration(struct desktop_shell *shell)
{
struct weston_config_section *section;
int duration;
- char *s;
+ char *s, *client;
section = weston_config_get_section(shell->compositor->config,
"screensaver", NULL, NULL);
@@ -578,8 +578,10 @@ shell_configuration(struct desktop_shell *shell)
section = weston_config_get_section(shell->compositor->config,
"shell", NULL, NULL);
+ asprintf(&client, "%s/%s", weston_config_get_libexec_dir(),
+ WESTON_SHELL_CLIENT);
weston_config_section_get_string(section,
- "client", &s, LIBEXECDIR "/" WESTON_SHELL_CLIENT);
+ "client", &s, client);
shell->client = s;
weston_config_section_get_string(section,
"binding-modifier", &s, "super");
diff --git a/shared/config-parser.c b/shared/config-parser.c
index 8defbbb..4542ca6 100644
--- a/shared/config-parser.c
+++ b/shared/config-parser.c
@@ -282,6 +282,18 @@ weston_config_section_get_bool(struct weston_config_section *section,
return 0;
}
+WL_EXPORT
+const char *
+weston_config_get_libexec_dir(void)
+{
+ const char *path = getenv("WESTON_BUILD_DIR");
+
+ if (path)
+ return path;
+
+ return LIBEXECDIR;
+}
+
static struct weston_config_section *
config_add_section(struct weston_config *config, const char *name)
{
diff --git a/shared/config-parser.h b/shared/config-parser.h
index 745562b..1ecc8cc 100644
--- a/shared/config-parser.h
+++ b/shared/config-parser.h
@@ -92,6 +92,9 @@ int
weston_config_section_get_bool(struct weston_config_section *section,
const char *key,
int *value, int default_value);
+const char *
+weston_config_get_libexec_dir(void);
+
struct weston_config *
weston_config_parse(const char *name);
diff --git a/src/screenshooter.c b/src/screenshooter.c
index 4403933..78c2d0e 100644
--- a/src/screenshooter.c
+++ b/src/screenshooter.c
@@ -286,7 +286,10 @@ screenshooter_binding(struct weston_seat *seat, uint32_t time, uint32_t key,
void *data)
{
struct screenshooter *shooter = data;
- const char *screenshooter_exe = LIBEXECDIR "/weston-screenshooter";
+ char *screenshooter_exe;
+
+ asprintf(&screenshooter_exe, "%s/%s", weston_config_get_libexec_dir(),
+ "/weston-screenshooter");
if (!shooter->client)
shooter->client = weston_client_launch(shooter->ec,
diff --git a/src/text-backend.c b/src/text-backend.c
index 1d549d4..f295457 100644
--- a/src/text-backend.c
+++ b/src/text-backend.c
@@ -937,12 +937,15 @@ static void
text_backend_configuration(struct text_backend *text_backend)
{
struct weston_config_section *section;
+ char *client;
section = weston_config_get_section(text_backend->compositor->config,
"input-method", NULL, NULL);
+ asprintf(&client, "%s/weston-keyboard",
+ weston_config_get_libexec_dir());
weston_config_section_get_string(section, "path",
&text_backend->input_method.path,
- LIBEXECDIR "/weston-keyboard");
+ client);
}
static void
diff --git a/tests/weston-tests-env b/tests/weston-tests-env
index 473e092..e332354 100755
--- a/tests/weston-tests-env
+++ b/tests/weston-tests-env
@@ -28,6 +28,7 @@ XWAYLAND_PLUGIN=$abs_builddir/.libs/xwayland.so
case $TESTNAME in
*.la|*.so)
+ WESTON_BUILD_DIR=$abs_builddir \
$WESTON --backend=$BACKEND \
--no-config \
--shell=$SHELL_PLUGIN \
@@ -37,6 +38,7 @@ case $TESTNAME in
&> "$OUTLOG"
;;
*)
+ WESTON_BUILD_DIR=$abs_builddir \
WESTON_TEST_CLIENT_PATH=$abs_builddir/$TESTNAME $WESTON \
--socket=test-$(basename $TESTNAME) \
--backend=$BACKEND \
--
2.1.0.rc1
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH wayland] server: Don't expose wl_display as a global
On Thu, Aug 21, 2014 at 4:00 AM, Pekka Paalanen wrote:
> On Thu, 7 Aug 2014 09:55:49 -0400
> "Jasper St. Pierre" wrote:
>
> > The idea here was that once upon a time, clients could rebind wl_display
> > to a higher version, so we offered the ability to rebind it
> > here. However, this is particularly broken. The existing bind
> > implementation actually still hardcodes version numbers, and it leaks
> > previous resources, overwriting the existing one.
> >
> > The newly bound resource *also* won't have any listeners attached by the
> > client, meaning that the error and delete_id events won't get delivered
> > correctly. Unless the client poked into libwayland internals, it also
> > can't possibly set up these handlers correctly either, so the client
> > will sustain errors and leak all deleted globals.
> >
> > Since this never worked correctly in the first place, we can feel safe
> > removing it.
> > ---
> > src/wayland-server.c | 28
> > 1 file changed, 8 insertions(+), 20 deletions(-)
> >
> > diff --git a/src/wayland-server.c b/src/wayland-server.c
> > index 3c162d4..dc3f502 100644
> > --- a/src/wayland-server.c
> > +++ b/src/wayland-server.c
> > @@ -380,9 +380,8 @@ wl_client_get_display(struct wl_client *client)
> > return client->display;
> > }
> >
> > -static void
> > -bind_display(struct wl_client *client,
> > - void *data, uint32_t version, uint32_t id);
> > +static int
> > +bind_display(struct wl_client *client, struct wl_display *display);
> >
> > /** Create a client for the given file descriptor
> > *
> > @@ -440,9 +439,7 @@ wl_client_create(struct wl_display *display, int fd)
> > goto err_map;
> >
> > wl_signal_init(&client->destroy_signal);
> > - bind_display(client, display, 1, 1);
> > -
> > - if (!client->display_resource)
> > + if (bind_display(client, display) < 0)
> > goto err_map;
> >
> > wl_list_insert(display->client_list.prev, &client->link);
> > @@ -772,22 +769,20 @@ destroy_client_display_resource(struct wl_resource
> *resource)
> > resource->client->display_resource = NULL;
> > }
> >
> > -static void
> > -bind_display(struct wl_client *client,
> > - void *data, uint32_t version, uint32_t id)
> > +static int
> > +bind_display(struct wl_client *client, struct wl_display *display)
> > {
> > - struct wl_display *display = data;
> > -
> > client->display_resource =
> > - wl_resource_create(client, &wl_display_interface, 1, id);
> > + wl_resource_create(client, &wl_display_interface, 1, 1);
> > if (client->display_resource == NULL) {
> > wl_client_post_no_memory(client);
> > - return;
> > + return -1;
> > }
> >
> > wl_resource_set_implementation(client->display_resource,
> > &display_interface, display,
> > destroy_client_display_resource);
> > + return 0;
> > }
> >
> > /** Create Wayland display object.
> > @@ -831,13 +826,6 @@ wl_display_create(void)
> >
> > wl_array_init(&display->additional_shm_formats);
> >
> > - if (!wl_global_create(display, &wl_display_interface, 1,
> > - display, bind_display)) {
> > - wl_event_loop_destroy(display->loop);
> > - free(display);
> > - return NULL;
> > - }
> > -
> > return display;
> > }
> >
>
> I asked krh about this in irc, and he eventually agreed that exposing
> wl_display as a global is not so useful.
>
> I reviewed the patch, and the alleged problems the current code has,
> and I agree they are real.
>
> If we ever end up needing wl_display as a global, we can easily add it
> back. Since the implementation was indeed broken, no client should ever
> try to bind to wl_display global with version 1.
>
> Therefore, pushed with ack from Jason and R-b from me.
>
> In fact, I think this warrants this patch as a condidate for the stable
> branch.
>
Yeah, it probably counts as a bug-fix. It does change behaviour though, so
I'm not sure I really want to call it a stable thing. Unless, of course,
re-binding wl_display could cause a crash; in which case, push it to 1.5
--Jason
>
>
> Thanks,
> pq
>
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH wayland v4 1/5] protocol: define the concept of wl_surface role
That applies to the whole series, BTW On Thu, Aug 21, 2014 at 9:03 AM, Jason Ekstrand wrote: > LGTM > Reviewed-by: Jason Ekstrand > > > On Thu, Aug 21, 2014 at 2:52 AM, Pekka Paalanen > wrote: > >> From: Pekka Paalanen >> >> Define what a role is, and what restrictions there are. >> >> A change to existing behaviour is that a role cannot be changed at all >> once set. However, this is unlikely to cause problems, as there is no >> reason to re-use wl_surfaces in clients. >> >> v2: give more concrete examples of roles, define losing a role, Jasper >> rewrote the paragraph on how a role is set. >> >> v3: make role permanent, there is no such thing as "losing a role". >> Re-issuing the same role again must be allowed for wl_pointer.set_cursor >> et al. to work. >> >> v4: clarify the semantics of destroying a role object. >> >> Signed-off-by: Pekka Paalanen >> --- >> protocol/wayland.xml | 33 +++-- >> 1 file changed, 31 insertions(+), 2 deletions(-) >> >> diff --git a/protocol/wayland.xml b/protocol/wayland.xml >> index bb457bc..621c64d 100644 >> --- a/protocol/wayland.xml >> +++ b/protocol/wayland.xml >> @@ -973,8 +973,37 @@ >>local coordinates of the pixel content, in case a buffer_transform >>or a buffer_scale is used. >> >> - Surfaces are also used for some special purposes, e.g. as >> - cursor images for pointers, drag icons, etc. >> + A surface without a "role" is fairly useless, a compositor does >> + not know where, when or how to present it. The role is the >> + purpose of a wl_surface. Examples of roles are a cursor for a >> + pointer (as set by wl_pointer.set_cursor), a drag icon >> + (wl_data_device.start_drag), a sub-surface >> + (wl_subcompositor.get_subsurface), and a window as defined by a >> + shell protocol (e.g. wl_shell.get_shell_surface). >> + >> + A surface can have only one role at a time. Initially a >> + wl_surface does not have a role. Once a wl_surface is given a >> + role, it is set permanently for the whole lifetime of the >> + wl_surface object. Giving the current role again is allowed, >> + unless explicitly forbidden by the relevant interface >> + specification. >> + >> + Surface roles are given by requests in other interfaces such as >> + wl_pointer.set_cursor. The request should explicitly mention >> + that this request gives a role to a wl_surface. Often, this >> + request also creates a new protocol object that represents the >> + role and adds additional functionality to wl_surface. When a >> + client wants to destroy a wl_surface, they must destroy this 'role >> + object' before the wl_surface. >> + >> + Destroying the role object does not remove the role from the >> + wl_surface, but it may stop the wl_surface from "playing the role". >> + For instance, if a wl_subsurface object is destroyed, the >> wl_surface >> + it was created for will be unmapped and forget its position and >> + z-order. It is allowed to create a wl_subsurface for the same >> + wl_surface again, but it is not allowed to use the wl_surface as >> + a cursor (cursor is a different role than sub-surface, and role >> + switching is not allowed). >> >> >> >> -- >> 1.8.5.5 >> >> > ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH wayland v4 1/5] protocol: define the concept of wl_surface role
LGTM Reviewed-by: Jason Ekstrand On Thu, Aug 21, 2014 at 2:52 AM, Pekka Paalanen wrote: > From: Pekka Paalanen > > Define what a role is, and what restrictions there are. > > A change to existing behaviour is that a role cannot be changed at all > once set. However, this is unlikely to cause problems, as there is no > reason to re-use wl_surfaces in clients. > > v2: give more concrete examples of roles, define losing a role, Jasper > rewrote the paragraph on how a role is set. > > v3: make role permanent, there is no such thing as "losing a role". > Re-issuing the same role again must be allowed for wl_pointer.set_cursor > et al. to work. > > v4: clarify the semantics of destroying a role object. > > Signed-off-by: Pekka Paalanen > --- > protocol/wayland.xml | 33 +++-- > 1 file changed, 31 insertions(+), 2 deletions(-) > > diff --git a/protocol/wayland.xml b/protocol/wayland.xml > index bb457bc..621c64d 100644 > --- a/protocol/wayland.xml > +++ b/protocol/wayland.xml > @@ -973,8 +973,37 @@ >local coordinates of the pixel content, in case a buffer_transform >or a buffer_scale is used. > > - Surfaces are also used for some special purposes, e.g. as > - cursor images for pointers, drag icons, etc. > + A surface without a "role" is fairly useless, a compositor does > + not know where, when or how to present it. The role is the > + purpose of a wl_surface. Examples of roles are a cursor for a > + pointer (as set by wl_pointer.set_cursor), a drag icon > + (wl_data_device.start_drag), a sub-surface > + (wl_subcompositor.get_subsurface), and a window as defined by a > + shell protocol (e.g. wl_shell.get_shell_surface). > + > + A surface can have only one role at a time. Initially a > + wl_surface does not have a role. Once a wl_surface is given a > + role, it is set permanently for the whole lifetime of the > + wl_surface object. Giving the current role again is allowed, > + unless explicitly forbidden by the relevant interface > + specification. > + > + Surface roles are given by requests in other interfaces such as > + wl_pointer.set_cursor. The request should explicitly mention > + that this request gives a role to a wl_surface. Often, this > + request also creates a new protocol object that represents the > + role and adds additional functionality to wl_surface. When a > + client wants to destroy a wl_surface, they must destroy this 'role > + object' before the wl_surface. > + > + Destroying the role object does not remove the role from the > + wl_surface, but it may stop the wl_surface from "playing the role". > + For instance, if a wl_subsurface object is destroyed, the wl_surface > + it was created for will be unmapped and forget its position and > + z-order. It is allowed to create a wl_subsurface for the same > + wl_surface again, but it is not allowed to use the wl_surface as > + a cursor (cursor is a different role than sub-surface, and role > + switching is not allowed). > > > > -- > 1.8.5.5 > > ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH weston] xdg-shell: bump experimental protocol version
From: Pekka Paalanen
The experimental versioning has not been updated when it was supposed
to. Let's try to be better at it now, as xdg-shell is close to have its
first stable version.
Bump the version now to bring the world into the same exact version.
There may be some protocol changes still coming, but we try to land them
before 1.6 gets out. Those changes will bump the experimental version
again as needed.
When 1.6.0 is released, the experimental version will no longer be
bumped, and no incompatible protocol changes will be made. Xdg-shell.xml
file will move to Wayland in 1.7.0, drop the experimental versioning,
and become stable.
Cc: Jasper St. Pierre
Signed-off-by: Pekka Paalanen
---
clients/simple-damage.c | 2 +-
clients/simple-egl.c| 2 +-
clients/simple-shm.c| 2 +-
clients/window.c| 2 +-
desktop-shell/shell.c | 2 +-
protocol/xdg-shell.xml | 2 +-
6 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/clients/simple-damage.c b/clients/simple-damage.c
index d7a7c70..fe532fe 100644
--- a/clients/simple-damage.c
+++ b/clients/simple-damage.c
@@ -642,7 +642,7 @@ static const struct xdg_shell_listener xdg_shell_listener =
{
xdg_shell_ping,
};
-#define XDG_VERSION 3 /* The version of xdg-shell that we implement */
+#define XDG_VERSION 4 /* The version of xdg-shell that we implement */
#ifdef static_assert
static_assert(XDG_VERSION == XDG_SHELL_VERSION_CURRENT,
"Interface version doesn't match implementation version");
diff --git a/clients/simple-egl.c b/clients/simple-egl.c
index f23fb8c..69d28ea 100644
--- a/clients/simple-egl.c
+++ b/clients/simple-egl.c
@@ -670,7 +670,7 @@ static const struct xdg_shell_listener xdg_shell_listener =
{
xdg_shell_ping,
};
-#define XDG_VERSION 3 /* The version of xdg-shell that we implement */
+#define XDG_VERSION 4 /* The version of xdg-shell that we implement */
#ifdef static_assert
static_assert(XDG_VERSION == XDG_SHELL_VERSION_CURRENT,
"Interface version doesn't match implementation version");
diff --git a/clients/simple-shm.c b/clients/simple-shm.c
index 29abb8b..b1c311f 100644
--- a/clients/simple-shm.c
+++ b/clients/simple-shm.c
@@ -321,7 +321,7 @@ static const struct xdg_shell_listener xdg_shell_listener =
{
xdg_shell_ping,
};
-#define XDG_VERSION 3 /* The version of xdg-shell that we implement */
+#define XDG_VERSION 4 /* The version of xdg-shell that we implement */
#ifdef static_assert
static_assert(XDG_VERSION == XDG_SHELL_VERSION_CURRENT,
"Interface version doesn't match implementation version");
diff --git a/clients/window.c b/clients/window.c
index 90f45d3..5d64022 100644
--- a/clients/window.c
+++ b/clients/window.c
@@ -5103,7 +5103,7 @@ static const struct xdg_shell_listener xdg_shell_listener
= {
xdg_shell_ping,
};
-#define XDG_VERSION 3 /* The version of xdg-shell that we implement */
+#define XDG_VERSION 4 /* The version of xdg-shell that we implement */
#ifdef static_assert
static_assert(XDG_VERSION == XDG_SHELL_VERSION_CURRENT,
"Interface version doesn't match implementation version");
diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
index e3abaad..20536d7 100644
--- a/desktop-shell/shell.c
+++ b/desktop-shell/shell.c
@@ -3883,7 +3883,7 @@ xdg_shell_unversioned_dispatch(const void *implementation,
return 0;
}
-#define XDG_SERVER_VERSION 3
+#define XDG_SERVER_VERSION 4
static_assert(XDG_SERVER_VERSION == XDG_SHELL_VERSION_CURRENT,
"shell implementation doesn't match protocol version");
diff --git a/protocol/xdg-shell.xml b/protocol/xdg-shell.xml
index bd36231..275837f 100644
--- a/protocol/xdg-shell.xml
+++ b/protocol/xdg-shell.xml
@@ -45,7 +45,7 @@
they implement using static_assert to ensure the protocol and
implementation versions match.
-
+
--
1.8.5.5
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH v3] client: drop unused event queue cond and list variables
From: Olivier Blin
The wl_event_queue cond variable has been replaced by the wl_display
reader_cond variable (commit 3c7e8bfbb4745315b7bcbf69fa746c3d6718c305).
This cond variable is never waited for anymore, just
signaled/broadcasted, and thus can be safely removed.
The wl_display event_queue_list and link from wl_event_queue
can be removed as well, since it was only used to iterate over
the event queue list in order to broadcast the now unused cond.
No regression on queue unit tests.
Signed-off-by: Olivier Blin
v2: fixed and rebased after 886b09c9a3a9d8672039f09fe7eaf3f2b2b012ca
added signed-off-by
v3: removed link from wl_event_queue
Signed-off-by: Marek Chalupa
---
src/wayland-client.c | 22 --
1 file changed, 22 deletions(-)
diff --git a/src/wayland-client.c b/src/wayland-client.c
index 2252424..4b184d5 100644
--- a/src/wayland-client.c
+++ b/src/wayland-client.c
@@ -69,10 +69,8 @@ struct wl_global {
};
struct wl_event_queue {
- struct wl_list link;
struct wl_list event_list;
struct wl_display *display;
- pthread_cond_t cond;
};
struct wl_display {
@@ -100,7 +98,6 @@ struct wl_display {
struct wl_map objects;
struct wl_event_queue display_queue;
struct wl_event_queue default_queue;
- struct wl_list event_queue_list;
pthread_mutex_t mutex;
int reader_count;
@@ -123,8 +120,6 @@ static int debug_client = 0;
static void
display_fatal_error(struct wl_display *display, int error)
{
- struct wl_event_queue *iter;
-
if (display->last_error)
return;
@@ -132,9 +127,6 @@ display_fatal_error(struct wl_display *display, int error)
error = EFAULT;
display->last_error = error;
-
- wl_list_for_each(iter, &display->event_queue_list, link)
- pthread_cond_broadcast(&iter->cond);
}
/**
@@ -153,7 +145,6 @@ static void
display_protocol_error(struct wl_display *display, uint32_t code,
uint32_t id, const struct wl_interface *intf)
{
- struct wl_event_queue *iter;
int err;
if (display->last_error)
@@ -184,9 +175,6 @@ display_protocol_error(struct wl_display *display, uint32_t
code,
display->protocol_error.id = id;
display->protocol_error.interface = intf;
- wl_list_for_each(iter, &display->event_queue_list, link)
- pthread_cond_broadcast(&iter->cond);
-
pthread_mutex_unlock(&display->mutex);
}
@@ -194,7 +182,6 @@ static void
wl_event_queue_init(struct wl_event_queue *queue, struct wl_display *display)
{
wl_list_init(&queue->event_list);
- pthread_cond_init(&queue->cond, NULL);
queue->display = display;
}
@@ -209,7 +196,6 @@ wl_event_queue_release(struct wl_event_queue *queue)
wl_list_remove(&closure->link);
wl_closure_destroy(closure);
}
- pthread_cond_destroy(&queue->cond);
}
/** Destroy an event queue
@@ -231,7 +217,6 @@ wl_event_queue_destroy(struct wl_event_queue *queue)
struct wl_display *display = queue->display;
pthread_mutex_lock(&display->mutex);
- wl_list_remove(&queue->link);
wl_event_queue_release(queue);
free(queue);
pthread_mutex_unlock(&display->mutex);
@@ -256,10 +241,6 @@ wl_display_create_queue(struct wl_display *display)
wl_event_queue_init(queue, display);
- pthread_mutex_lock(&display->mutex);
- wl_list_insert(&display->event_queue_list, &queue->link);
- pthread_mutex_unlock(&display->mutex);
-
return queue;
}
@@ -767,7 +748,6 @@ wl_display_connect_to_fd(int fd)
wl_map_init(&display->objects, WL_MAP_CLIENT_SIDE);
wl_event_queue_init(&display->default_queue, display);
wl_event_queue_init(&display->display_queue, display);
- wl_list_init(&display->event_queue_list);
pthread_mutex_init(&display->mutex, NULL);
pthread_cond_init(&display->reader_cond, NULL);
display->reader_count = 0;
@@ -1046,8 +1026,6 @@ queue_event(struct wl_display *display, int len)
else
queue = proxy->queue;
- if (wl_list_empty(&queue->event_list))
- pthread_cond_signal(&queue->cond);
wl_list_insert(queue->event_list.prev, &closure->link);
return size;
--
2.0.4
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH v2] tests: use test compositor in queue-test
Most of the code of the queue-test is covered by the test compositor,
so we can save few lines and use the test compositor instead.
I think it's also more readable.
This patch removes timeout from the test. We plan to add timeout
to all tests later, though.
v2.
rebased to master
Signed-off-by: Marek Chalupa
---
tests/queue-test.c | 182 +
1 file changed, 30 insertions(+), 152 deletions(-)
diff --git a/tests/queue-test.c b/tests/queue-test.c
index a7b54d9..96f2100 100644
--- a/tests/queue-test.c
+++ b/tests/queue-test.c
@@ -31,33 +31,17 @@
#include "wayland-client.h"
#include "wayland-server.h"
#include "test-runner.h"
-
-#define SOCKET_NAME "wayland-queue-test"
+#include "test-compositor.h"
#define ARRAY_LENGTH(a) (sizeof (a) / sizeof (a)[0])
-#define client_assert(expr)\
- do {\
- if (!(expr)) { \
- fprintf(stderr, "%s:%d: " \
- "Assertion `%s' failed.\n", \
- __FILE__, __LINE__, #expr); \
- exit(EXIT_FAILURE); \
- } \
- } while (0)
-
-struct display {
- struct wl_display *display;
- int child_exit_status;
-};
-
static void
registry_handle_global(void *data, struct wl_registry *registry,
uint32_t id, const char *interface, uint32_t version)
{
int *pcounter = data;
(*pcounter)++;
- client_assert(*pcounter == 1);
+ assert(*pcounter == 1);
wl_registry_destroy(registry);
}
@@ -68,15 +52,15 @@ static const struct wl_registry_listener registry_listener
= {
/* Test that destroying a proxy object doesn't result in any more
* callback being invoked, even though were many queued. */
-static int
+static void
client_test_proxy_destroy(void)
{
struct wl_display *display;
struct wl_registry *registry;
int counter = 0;
- display = wl_display_connect(SOCKET_NAME);
- client_assert(display);
+ display = wl_display_connect(NULL);
+ assert(display);
registry = wl_display_get_registry(display);
assert(registry != NULL);
@@ -84,13 +68,11 @@ client_test_proxy_destroy(void)
&counter);
wl_display_roundtrip(display);
- client_assert(counter == 1);
+ assert(counter == 1);
/* don't destroy the registry, we have already destroyed them
* in the global handler */
wl_display_disconnect(display);
-
- return 0;
}
struct multiple_queues_state {
@@ -119,7 +101,7 @@ static const struct wl_callback_listener sync_listener = {
/* Test that when receiving the first of two synchronization
* callback events, destroying the second one doesn't cause any
* errors even if the delete_id event is handled out of order. */
-static int
+static void
client_test_multiple_queues(void)
{
struct wl_event_queue *queue;
@@ -127,11 +109,11 @@ client_test_multiple_queues(void)
struct multiple_queues_state state;
int ret = 0;
- state.display = wl_display_connect(SOCKET_NAME);
- client_assert(state.display);
+ state.display = wl_display_connect(NULL);
+ assert(state.display);
queue = wl_display_create_queue(state.display);
- client_assert(queue);
+ assert(queue);
state.done = false;
callback1 = wl_display_sync(state.display);
@@ -152,7 +134,7 @@ client_test_multiple_queues(void)
wl_event_queue_destroy(queue);
wl_display_disconnect(state.display);
- return ret == -1 ? -1 : 0;
+ exit(ret == -1 ? -1 : 0);
}
static void
@@ -168,7 +150,7 @@ static const struct wl_callback_listener
sync_listener_roundtrip = {
/* Test that doing a roundtrip on a queue only the events on that
* queue get dispatched. */
-static int
+static void
client_test_queue_roundtrip(void)
{
struct wl_event_queue *queue;
@@ -178,11 +160,11 @@ client_test_queue_roundtrip(void)
bool done1 = false;
bool done2 = false;
- display = wl_display_connect(SOCKET_NAME);
- client_assert(display);
+ display = wl_display_connect(NULL);
+ assert(display);
queue = wl_display_create_queue(display);
- client_assert(queue);
+ assert(queue);
/* arm a callback on the default queue */
callback1 = wl_display_sync(display);
@@ -197,8 +179,8 @@ client_test_queue_roundtrip(void)
/* roundtrip on default queue must not dispatch the other queue. */
wl_display_roundtrip(display);
- client_assert(done1 == true);
- client_assert(done2 == false);
+ assert(done1 == true);
+ assert(done2 == false);
/* re-arm t
[PATCH v2] tests: add test-compositor
This patch introduces a set of functions that can create a display
and clients for tests.
On server side the user can use functions:
display_create()
display_destroy()
create_client()
display_run()
display_resume()
and on client side the user can use:
client_connect()
client_disconnect()
stop_display()
The stop_display() and display_resume() are functions that serve as a barrier
and also allow the display to take some action after the display_run() was
called,
because after the display is stopped, it can run arbitrary code until it calls
display_resume().
client_connect() function connects to wayland display and creates a proxy to
test_compositor global object, so it can ask for stopping the display later
using stop_display().
An example:
void
client_main()
{
/* or client can use wl_display_connect(NULL)
* and do all the stuff manually */
struct client *c = client_connect();
/* do some stuff, ... */
/* stop the display so that it can
* do some other stuff */
stop_display(c, 1);
/* ... */
client_disconnect(c);
}
TEST(dummy_tst)
{
struct display *d = display_create();
/* set up the display */
wl_global_create(d->wl_display, ...);
/* ... */
create_client(d, client_main);
display_run();
/* if we are here, the display has been stopped
* and we can do some code, i. e. create another global or so */
wl_global_create(d->wl_display, ...);
/* ... */
display_resume(d); /* resume display and clients */
display_destroy(d);
}
v2:
added/changed message in few asserts that were not clear
fixed codying style issues and typo
client_create_with_name: fixed a condition in an assert
get_socket_name: use also pid
check_error: fix errno -> err
Signed-off-by: Marek Chalupa
---
Makefile.am | 3 +-
tests/test-compositor.c | 467
tests/test-compositor.h | 97 ++
3 files changed, 566 insertions(+), 1 deletion(-)
create mode 100644 tests/test-compositor.c
create mode 100644 tests/test-compositor.h
diff --git a/Makefile.am b/Makefile.am
index fee19ab..dee95c2 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -142,7 +142,8 @@ check_LTLIBRARIES = libtest-runner.la
libtest_runner_la_SOURCES =\
tests/test-runner.c \
tests/test-runner.h \
- tests/test-helpers.c
+ tests/test-helpers.c\
+ tests/test-compositor.c
libtest_runner_la_LIBADD = \
libwayland-util.la \
libwayland-client.la\
diff --git a/tests/test-compositor.c b/tests/test-compositor.c
new file mode 100644
index 000..2f54276
--- /dev/null
+++ b/tests/test-compositor.c
@@ -0,0 +1,467 @@
+/*
+ * Copyright (c) 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 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
+#include
+#include
+#include
+#include
+#include
+
+#include "test-compositor.h"
+
+/* --- Protocol --- */
+struct test_compositor;
+
+static const struct wl_message tc_requests[] = {
+ /* this request serves as a barrier for synchronizing*/
+ { "stop_display", "u", NULL }
+};
+
+static const struct wl_message tc_events[] = {
+ { "display_resumed", "", NULL }
+};
+
+const struct wl_interface test_compositor_interface = {
+ "test", 1,
+ 1, tc_requests,
+ 1, tc_events
+};
+
+struct test_compositor_interface {
+ void (*stop_display)(struct wl_client *client,
+struct wl_resource *resource,
+uint32_t num);
+};
+
+struct test_compositor_listener {
+ void (*display_resumed)(void
Re: [PATCH 2/2] compositor: add a way to change the keyboard leds
On Thu, 21 Aug 2014 08:37:17 +0100 Daniel Stone wrote: > Hi, > Last nitpick, sorry ... > > On Wednesday, August 20, 2014, Giulio Camuffo > wrote: > > > > + mods_depressed = > > xkb_state_serialize_mods(keyboard->xkb_state.state, > > + XKB_STATE_DEPRESSED); > > + mods_latched = xkb_state_serialize_mods(keyboard->xkb_state.state, > > + XKB_STATE_LATCHED); > > + mods_locked = xkb_state_serialize_mods(keyboard->xkb_state.state, > > + XKB_STATE_LOCKED); > > + group = xkb_state_serialize_group(keyboard->xkb_state.state, > > + XKB_STATE_EFFECTIVE); > > + > > + num = (1 << keyboard->xkb_info->mod2_mod); > > + caps = (1 << keyboard->xkb_info->caps_mod); > > > I still don't really believe the non-xkbcommon build should exist, but > shouldn't all this be #ifdef'ed? Looks like that to me, too, we have #ifdef ENABLE_XKBCOMMON it seems, and input.c is full of it. So a simple ifdef around this function would be needed. FWIW, I think the --disable-xkbcommon build is broken anyway, compositor.h uses e.g. xkb_mod_index_t and unconditionally includes xkbcommon.h, so if xkbcommon is not in the standard system path, weston won't build at all. But apparently it works for runtime to not link to libxkbcommon.so, so let's keep it that way. I just wonder... weston_keyboard_set_leds()... but that also changes the XKB state to match the leds, right? Should it be called weston_keyboard_set_locks() or something? The "compositor: add an option to set the default numlock value" is fine by me, and the "evdev/libinput: sync the leds of keyboards with the xkb state" you both already seemed to agree to leave for later. Thanks, pq ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston] screen-share: Add screen-share command to weston.ini man page
Hi Pekka, On Thu, 21 Aug 2014, Pekka Paalanen wrote: On Thu, 26 Jun 2014 16:31:43 +0100 Andrew Wedgbury wrote: This adds a description of the screen-share command configuration key to the weston.ini man page. Signed-off-by: Andrew Wedgbury --- man/weston.ini.man | 10 ++ 1 file changed, 10 insertions(+) diff --git a/man/weston.ini.man b/man/weston.ini.man index 667f70a..86f7e37 100644 --- a/man/weston.ini.man +++ b/man/weston.ini.man @@ -78,6 +78,7 @@ The section headers are: .BR "keyboard " "Keyboard layouts" .BR "terminal " "Terminal application options" .BR "xwayland " "XWayland options" +.BR "screen-share " "Screen sharing options" .fi .RE .PP @@ -115,6 +116,7 @@ directory are: .nf .BR xwayland.so .BR cms-colord.so +.BR screen-share.so .fi .RE .TP 7 @@ -421,6 +423,14 @@ The terminal shell (string). Sets the $TERM variable. sets the path to the xserver to run (string). .RE .RE +.SH "SCREEN-SHARE SECTION" +.TP 7 +.BI "command=" "/usr/bin/weston --backend=rdp-backend.so \ +--shell=fullscreen-shell.so --no-clients-resize" +sets the command to start a fullscreen-shell server for screen sharing (string). +The default value starts weston with the RDP backend. +.RE +.RE .SH "SEE ALSO" .BR weston (1), .BR weston-launch (1), Hi, pushed, but I removed the sentence about the default value. You have a value set in weston.ini.in which is the example .ini file, yes, but that is not the default. The default is set on the line: weston_config_section_get_string(section, "command", &ss->command, ""); and so the default is the empty string. I also think the default won't run at all. I don't think we even install the example weston.ini anywhere, even though we do generate it. Yes, I had assumed that most people would be using the generated weston.ini file, so that would be a better place to put this value. It just seems like quite a specific value to hard code, and given that people might want to tweak it, having a 'default' value in the ini file would be more useful and informative. Thanks, pq --- Andrew Wedgbury ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH] client: check for error before prepare/read
On Tue, 5 Aug 2014 11:43:35 +0200
Marek Chalupa wrote:
> This prevents from blocking shown in one display test. Also, it
> makes sense to not proceed further in the code of these function
> when an error occurred.
> ---
> src/wayland-client.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/src/wayland-client.c b/src/wayland-client.c
> index 68a91f6..f176dd4 100644
> --- a/src/wayland-client.c
> +++ b/src/wayland-client.c
> @@ -1183,6 +1183,9 @@ wl_display_read_events(struct wl_display *display)
> {
> int ret;
>
> + if (display->last_error)
> + return -1;
> +
> pthread_mutex_lock(&display->mutex);
>
> ret = read_events(display);
> @@ -1229,6 +1232,9 @@ wl_display_prepare_read_queue(struct wl_display
> *display,
> {
> int ret;
>
> + if (display->last_error)
> + return -1;
> +
> pthread_mutex_lock(&display->mutex);
>
> if (!wl_list_empty(&queue->event_list)) {
Yup, this looks good to me. I just want to merge the tests before this
one.
Very good work.
Thanks,
pq
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH] desktop-shell: Replace magic constants with named ones
Signed-off-by: Ondřej Majerech
---
desktop-shell/shell.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
index e3abaad..02e180d 100644
--- a/desktop-shell/shell.c
+++ b/desktop-shell/shell.c
@@ -1884,13 +1884,20 @@ surface_resize(struct shell_surface *shsurf,
struct weston_seat *seat, uint32_t edges)
{
struct weston_resize_grab *resize;
+ const unsigned resize_topbottom =
+ WL_SHELL_SURFACE_RESIZE_TOP | WL_SHELL_SURFACE_RESIZE_BOTTOM;
+ const unsigned resize_leftright =
+ WL_SHELL_SURFACE_RESIZE_LEFT | WL_SHELL_SURFACE_RESIZE_RIGHT;
+ const unsigned resize_any = resize_topbottom | resize_leftright;
if (shsurf->grabbed ||
shsurf->state.fullscreen || shsurf->state.maximized)
return 0;
- if (edges == 0 || edges > 15 ||
- (edges & 3) == 3 || (edges & 12) == 12)
+ /* Check for invalid edge combinations. */
+ if (edges == WL_SHELL_SURFACE_RESIZE_NONE || edges > resize_any ||
+ (edges & resize_topbottom) == resize_topbottom ||
+ (edges & resize_leftright) == resize_leftright)
return 0;
resize = malloc(sizeof *resize);
--
2.0.4
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH] tests: test if thread can block on error
On Tue, 5 Aug 2014 11:39:49 +0200
Marek Chalupa wrote:
> wl_display_read_events() can make a thread wait until some other thread
> ends reading. Normally it wakes up all threads after the reading is
> done. But there's a place when it does not get to waking up the threads
> - when an error occurs. This test reveals bug that can block programs.
>
> If a thread is waiting in wl_display_read_events() and another thread
> calls wl_display_read_events and the reading fails,
> then the sleeping thread is not woken up. This is because
> display_handle_error is using old pthread_cond instead of new
> display->reader_cond, that was added along with wl_display_read_events().
> ---
> tests/display-test.c | 90
>
> 1 file changed, 90 insertions(+)
>
> diff --git a/tests/display-test.c b/tests/display-test.c
> index e212942..26f946b 100644
> --- a/tests/display-test.c
> +++ b/tests/display-test.c
> @@ -31,6 +31,7 @@
> #include
> #include
> #include
> +#include
>
> #include "wayland-private.h"
> #include "wayland-server.h"
> @@ -323,3 +324,92 @@ TEST(post_nomem_tst)
>
> display_destroy(d);
> }
> +
> +static void
> +register_reading(struct wl_display *display)
> +{
> + while(wl_display_prepare_read(display) != 0 && errno == EAGAIN)
> + assert(wl_display_dispatch_pending(display) >= 0);
> + assert(wl_display_flush(display) >= 0);
> +}
> +
> +static void *
> +thread_read_error(void *data)
> +{
> + struct client *c = data;
> +
> + register_reading(c->wl_display);
> +
> + /*
> + * Calling the read right now will block this thread
> + * until the other thread will read the data.
> + * However, after invoking an error, this
> + * thread should be woken up or it will block indefinitely.
> + */
> + c->display_stopped = 1;
> + assert(wl_display_read_events(c->wl_display) == 0);
> +
> + wl_display_dispatch_pending(c->wl_display);
> + assert(wl_display_get_error(c->wl_display));
> +
> + pthread_exit(NULL);
> +}
> +
> +/* test posting an error in multi-threaded environment. */
> +static void
> +threading_post_err(void)
> +{
> + struct client *c = client_connect();
> + pthread_t thread;
> +
> + /* register read intention */
> + register_reading(c->wl_display);
> +
> + /* use this var as an indicator that thread is sleeping */
> + c->display_stopped = 0;
> +
> + /* create new thread that will register its intention too */
> + assert(pthread_create(&thread, NULL, thread_read_error, c) == 0);
> +
> + /* make sure thread is sleeping. It's a little bit racy
> + * (setting display_stopped to 1 and calling wl_display_read_events)
> + * so call usleep once again after the loop ends - it should
> + * be sufficient... */
> + while (c->display_stopped == 0)
> + usleep(500);
> +
> + usleep(1);
> +
> + /* so now we have sleeping thread waiting for a pthread_cond signal.
> + * The main thread must call wl_display_read_events().
> + * If this call fails, then it won't call broadcast at the
> + * end of the function and the sleeping thread will block indefinitely.
> + * Make the call fail and watch if libwayland will unblock the thread!
> */
> +
> + /* create error on fd, so that wl_display_read_events will fail.
> + * The same can happen when server hangs up */
> + close(wl_display_get_fd(c->wl_display));
> + /* this read events will fail and will
> + * post an error that should wake the sleeping thread
> + * and dispatch the incoming events */
> + assert(wl_display_read_events(c->wl_display) == -1);
> +
> + /* kill test in 3 seconds. This should be enough time for the
> + * thread to exit if it's not blocking. If everything is OK, than
> + * the thread was woken up and the test will end before the SIGALRM */
> + alarm(3);
> + pthread_join(thread, NULL);
> +
> + wl_proxy_destroy((struct wl_proxy *) c->tc);
> + wl_display_disconnect(c->wl_display);
> +}
> +
> +TEST(threading_errors_tst)
> +{
> + struct display *d = display_create();
> +
> + client_create(d, threading_post_err);
> + display_run(d);
> +
> + display_destroy(d);
> +}
This looks good. I'll be merging this and likely the others after I get
the refreshed series adding the test-compositor framework, as discussed
in irc.
Thanks,
pq
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH] tests: add multiple-queues testcases
On Wed, 25 Jun 2014 14:35:16 +0200
Marek Chalupa wrote:
> Test if events are going to the right queue and if the queue
> is interrupted from polling when an error to the main queue comes.
> The last one is failing.
> ---
> tests/queue-test.c | 128
> +
> 1 file changed, 128 insertions(+)
>
> diff --git a/tests/queue-test.c b/tests/queue-test.c
> index 36d7a69..307bae9 100644
> --- a/tests/queue-test.c
> +++ b/tests/queue-test.c
> @@ -27,6 +27,7 @@
> #include
> #include
> #include
> +#include
>
> #include "wayland-client.h"
> #include "wayland-server.h"
> @@ -139,6 +140,53 @@ client_test_multiple_queues(void)
> exit(ret == -1 ? -1 : 0);
> }
>
> +/* test that events will come to the right queue */
> +static void
> +client_test_multiple_queues2(void)
> +{
> + struct wl_event_queue *queue[5], *q;
Instead of using 5 everwhere, how about a #define?
> + struct wl_callback *callback;
> + struct wl_display *display;
> + int i, j;
> +
> + display = wl_display_connect(NULL);
> + assert(display);
> +
> + for (i = 0; i < 5; ++i) {
> + queue[i] = wl_display_create_queue(display);
> + assert(queue[i]);
> + }
> +
> + for (i = 0; i < 100; ++i) {
> + q = queue[i % 5];
> + callback = wl_display_sync(display);
Isn't this 'callback' leaked?
> + assert(callback != NULL);
> + wl_proxy_set_queue((struct wl_proxy *) callback, q);
> +
> + assert(wl_display_flush(display) > 0);
> +
> + /* the callback should come to the q queue */
> + assert(wl_display_dispatch_pending(display) == 0);
> + assert(wl_display_dispatch_queue(display, q) > 0);
> + /* now all queues should be empty */
> + for (j = 0; j < 5; ++j)
> + assert(wl_display_dispatch_queue_pending(display,
> + queue[j]) ==
> 0);
> + }
> +
> + callback = wl_display_sync(display);
And this?
> + assert(callback != NULL);
> +
> + assert(wl_display_flush(display) > 0);
> + assert(wl_display_dispatch(display) > 0);
> + for (j = 0; j < 5; ++j) {
> + assert(wl_display_dispatch_queue_pending(display, queue[j]) ==
> 0);
> + wl_event_queue_destroy(queue[j]);
> + }
> +
> + wl_display_disconnect(display);
> +}
> +
> static void
> dummy_bind(struct wl_client *client,
> void *data, uint32_t version, uint32_t id)
> @@ -169,5 +217,85 @@ TEST(queue)
> client_create(d, client_test_multiple_queues);
> display_run(d);
>
> + client_create(d, client_test_multiple_queues2);
> + display_run(d);
> +
> + display_destroy(d);
> +}
> +
> +struct thread_data {
> + struct wl_display *display;
> + struct wl_event_queue *queue;
> +};
> +
> +static void *
> +run_new_queue(void *arg)
> +{
> + struct thread_data *data = arg;
> + int ret;
> +
> + /* XXX shouldn't it return -1 in the first dispatch?
> + * Anyway - the queue keeps polling atm and is not interrupted by
> + * the error */
Hmm, yes, I think would probably be good that if wl_display goes into
error state, all the dispatch calls should return with error, rather
than hang.
> + do {
> + ret = wl_display_dispatch_queue(data->display, data->queue);
> + /* it definitely cannot dispatch any event */
> + assert(ret <= 0);
> + } while (ret != -1);
> +
> + pthread_exit(NULL);
> +}
> +
> +static void
> +client_test_multiple_queues_errors(void)
> +{
> + struct client *c;
> + struct wl_event_queue *queue;
> + struct thread_data data;
> + pthread_t thr;
> +
> + c = client_connect();
> +
> + queue = wl_display_create_queue(c->wl_display);
> + assert(queue);
> +
> + data.display = c->wl_display;
> + data.queue = queue;
> + /* run new thread that will wait for event in
> + * the new queue. Then post an error and check
> + * if the dispatching was cancelled... */
> + assert(pthread_create(&thr, NULL, run_new_queue, &data) == 0);
> +
> + stop_display(c, 1);
> +
> + /* make sure we got the error */
> + alarm(3); /* timeout */
> + wl_display_roundtrip(c->wl_display);
> + assert(wl_display_get_error(c->wl_display) != 0);
> +
> + /* wait for the thread to finish */
> + assert(pthread_join(thr, NULL) == 0);
> +
> + /* do not use client_disconnect(c), because it would
> + * fail the test since we posted an error */
> + wl_event_queue_destroy(queue);
> + wl_proxy_destroy((struct wl_proxy *) c->tc);
> + wl_display_disconnect(c->wl_display);
> +}
> +
> +TEST(queue_errors)
> +{
> + struct display *d = display_create();
> +
> + client_create(d, client_test_multiple_queues_errors);
> + display_run(d);
> +
> + /* get the client */
> + struct client_info *ci
Re: [PATCH weston 1/3] evdev/libinput: sync the leds of keyboards with the xkb state
On 21 August 2014 13:17, Giulio Camuffo wrote: > 2014-08-21 15:06 GMT+03:00 Daniel Stone : > > On 21 August 2014 08:58, Giulio Camuffo wrote: > >> Switching VT is another matter, because all the keyboard devices are > >> removed so the xkb state is lost, so when returning to weston's vt we > >> don't know anymore which leds are supposed to be on. > > > > Sure we do: the usual way is to release all keys on VT leave, resetting > > latches but leaving locks as they are. So when they came back, just apply > > the state that occurred as a result of that, rather than trying to > maintain > > a totally unknown state from whilst you were switched away, or reset it > > completely. > > I mean the current weston code doesn't know the state anymore. Sure > that can be fixed, but I think it should be another patch. Oh yes, to be totally clear, that was really just an aside. It might be the correct way to solve the problem if the LEDs get clobbered on VT switches though, mind. ;) > >> This patch just fixes keyboard hotplugging, besides turning the leds > >> off at startup. > > > > Sure. I really, really don't like that timer though ... I'd rather just > sit > > this one out whilst we work out the correct thing to do though. If you've > > got some time on your hands, looking at the VT-enter path would be good I > > think; even if it doesn't make the first release, I think it'd make a > good > > stable-branch candidate. > > I agree the timer is ugly, but I have other priorities at the moment > honestly, so I'm not going to spend big amount of time on this, at > least yet. I'm fine with leaving this one be and getting only the > other two in. > No problem. We can come back to it later. Thanks for dealing with all my bikeshedding! -d ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston 1/3] evdev/libinput: sync the leds of keyboards with the xkb state
2014-08-21 15:06 GMT+03:00 Daniel Stone : > Hi, > > On 21 August 2014 08:58, Giulio Camuffo wrote: >> >> 2014-08-21 10:34 GMT+03:00 Daniel Stone : >> > Ugh. If you've your own kernel to hand, I'd hack it to WARN_ON(1) on LED >> > updates, so you can track where the rogue update is coming from. My >> > guess is >> > that switching VT breaks it, so it might work if you instead trigger an >> > update of all LED state on every VT enter? >> >> Switching VT is another matter, because all the keyboard devices are >> removed so the xkb state is lost, so when returning to weston's vt we >> don't know anymore which leds are supposed to be on. > > > Sure we do: the usual way is to release all keys on VT leave, resetting > latches but leaving locks as they are. So when they came back, just apply > the state that occurred as a result of that, rather than trying to maintain > a totally unknown state from whilst you were switched away, or reset it > completely. I mean the current weston code doesn't know the state anymore. Sure that can be fixed, but I think it should be another patch. > >> >> This patch just fixes keyboard hotplugging, besides turning the leds >> off at startup. > > > Sure. I really, really don't like that timer though ... I'd rather just sit > this one out whilst we work out the correct thing to do though. If you've > got some time on your hands, looking at the VT-enter path would be good I > think; even if it doesn't make the first release, I think it'd make a good > stable-branch candidate. I agree the timer is ugly, but I have other priorities at the moment honestly, so I'm not going to spend big amount of time on this, at least yet. I'm fine with leaving this one be and getting only the other two in. Thanks, Giulio > > Cheers, > Daniel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston 1/3] evdev/libinput: sync the leds of keyboards with the xkb state
Hi, On 21 August 2014 08:58, Giulio Camuffo wrote: > 2014-08-21 10:34 GMT+03:00 Daniel Stone : > > Ugh. If you've your own kernel to hand, I'd hack it to WARN_ON(1) on LED > > updates, so you can track where the rogue update is coming from. My > guess is > > that switching VT breaks it, so it might work if you instead trigger an > > update of all LED state on every VT enter? > > Switching VT is another matter, because all the keyboard devices are > removed so the xkb state is lost, so when returning to weston's vt we > don't know anymore which leds are supposed to be on. > Sure we do: the usual way is to release all keys on VT leave, resetting latches but leaving locks as they are. So when they came back, just apply the state that occurred as a result of that, rather than trying to maintain a totally unknown state from whilst you were switched away, or reset it completely. > This patch just fixes keyboard hotplugging, besides turning the leds > off at startup. > Sure. I really, really don't like that timer though ... I'd rather just sit this one out whilst we work out the correct thing to do though. If you've got some time on your hands, looking at the VT-enter path would be good I think; even if it doesn't make the first release, I think it'd make a good stable-branch candidate. Cheers, Daniel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH 3/3] client: remove unused variable
On Thu, 21 Aug 2014 12:07:08 +0200
Marek Chalupa wrote:
> display_thread variable is unused since
> 3c7e8bfbb4745315b7bcbf69fa746c3d6718c305
>
> Signed-off-by: Marek Chalupa
> ---
> src/wayland-client.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/src/wayland-client.c b/src/wayland-client.c
> index 15a5acc..2252424 100644
> --- a/src/wayland-client.c
> +++ b/src/wayland-client.c
> @@ -97,7 +97,6 @@ struct wl_display {
> uint32_t id;
> } protocol_error;
> int fd;
> - pthread_t display_thread;
> struct wl_map objects;
> struct wl_event_queue display_queue;
> struct wl_event_queue default_queue;
All three patches pushed, thanks,
pq
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH] weston-test: check if resource is not NULL
On Fri, 11 Jul 2014 12:33:02 +0200
Marek Chalupa wrote:
> and post client_no_memory if is...
> ---
> tests/weston-test.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/tests/weston-test.c b/tests/weston-test.c
> index 44875a6..cc937e5 100644
> --- a/tests/weston-test.c
> +++ b/tests/weston-test.c
> @@ -260,6 +260,11 @@ bind_test(struct wl_client *client, void *data, uint32_t
> version, uint32_t id)
> struct wl_resource *resource;
>
> resource = wl_resource_create(client, &wl_test_interface, 1, id);
> + if (!resource) {
> + wl_client_post_no_memory(client);
> + return;
> + }
> +
> wl_resource_set_implementation(resource,
> &test_implementation, test, NULL);
>
An obvious fix is obvious. Doh! ;-D
Thanks,
pq
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston] screen-share: Add screen-share command to weston.ini man page
On Thu, 26 Jun 2014 16:31:43 +0100 Andrew Wedgbury wrote: > This adds a description of the screen-share command configuration key to the > weston.ini man page. > > Signed-off-by: Andrew Wedgbury > --- > man/weston.ini.man | 10 ++ > 1 file changed, 10 insertions(+) > > diff --git a/man/weston.ini.man b/man/weston.ini.man > index 667f70a..86f7e37 100644 > --- a/man/weston.ini.man > +++ b/man/weston.ini.man > @@ -78,6 +78,7 @@ The section headers are: > .BR "keyboard " "Keyboard layouts" > .BR "terminal " "Terminal application options" > .BR "xwayland " "XWayland options" > +.BR "screen-share " "Screen sharing options" > .fi > .RE > .PP > @@ -115,6 +116,7 @@ directory are: > .nf > .BR xwayland.so > .BR cms-colord.so > +.BR screen-share.so > .fi > .RE > .TP 7 > @@ -421,6 +423,14 @@ The terminal shell (string). Sets the $TERM variable. > sets the path to the xserver to run (string). > .RE > .RE > +.SH "SCREEN-SHARE SECTION" > +.TP 7 > +.BI "command=" "/usr/bin/weston --backend=rdp-backend.so \ > +--shell=fullscreen-shell.so --no-clients-resize" > +sets the command to start a fullscreen-shell server for screen sharing > (string). > +The default value starts weston with the RDP backend. > +.RE > +.RE > .SH "SEE ALSO" > .BR weston (1), > .BR weston-launch (1), Hi, pushed, but I removed the sentence about the default value. You have a value set in weston.ini.in which is the example .ini file, yes, but that is not the default. The default is set on the line: weston_config_section_get_string(section, "command", &ss->command, ""); and so the default is the empty string. I also think the default won't run at all. I don't think we even install the example weston.ini anywhere, even though we do generate it. Thanks, pq ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston 3/3] compositor: add an option to set the default numlock value
2014-08-21 10:31 GMT+03:00 Daniel Stone : > Hi, > > > On Wednesday, August 20, 2014, Giulio Camuffo > wrote: >> >> Add a new "numlock-on" option in the [keyboard] section of weston.ini >> which, if set to true, is used to enable the numlock of the keyboards >> attached at startup. > > > I'm fine with this in principle, but would like to see this test if it's > possible (i.e. not nesting / led_update() is valid), and issue a rejection > message to the log otherwise. Uhm, the option is defined as "set the numlock value *if* the backend supports it". Having it print out a warning line every time i start weston in X would be just noise, imho. On the other hand, weston_keyboard_set_leds() returns -1 if it failed, so a user can print a warning where it makes sense. -- Giulio > > Cheers, > Daniel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH wayland] server: Don't expose wl_display as a global
On Thu, 7 Aug 2014 09:55:49 -0400
"Jasper St. Pierre" wrote:
> The idea here was that once upon a time, clients could rebind wl_display
> to a higher version, so we offered the ability to rebind it
> here. However, this is particularly broken. The existing bind
> implementation actually still hardcodes version numbers, and it leaks
> previous resources, overwriting the existing one.
>
> The newly bound resource *also* won't have any listeners attached by the
> client, meaning that the error and delete_id events won't get delivered
> correctly. Unless the client poked into libwayland internals, it also
> can't possibly set up these handlers correctly either, so the client
> will sustain errors and leak all deleted globals.
>
> Since this never worked correctly in the first place, we can feel safe
> removing it.
> ---
> src/wayland-server.c | 28
> 1 file changed, 8 insertions(+), 20 deletions(-)
>
> diff --git a/src/wayland-server.c b/src/wayland-server.c
> index 3c162d4..dc3f502 100644
> --- a/src/wayland-server.c
> +++ b/src/wayland-server.c
> @@ -380,9 +380,8 @@ wl_client_get_display(struct wl_client *client)
> return client->display;
> }
>
> -static void
> -bind_display(struct wl_client *client,
> - void *data, uint32_t version, uint32_t id);
> +static int
> +bind_display(struct wl_client *client, struct wl_display *display);
>
> /** Create a client for the given file descriptor
> *
> @@ -440,9 +439,7 @@ wl_client_create(struct wl_display *display, int fd)
> goto err_map;
>
> wl_signal_init(&client->destroy_signal);
> - bind_display(client, display, 1, 1);
> -
> - if (!client->display_resource)
> + if (bind_display(client, display) < 0)
> goto err_map;
>
> wl_list_insert(display->client_list.prev, &client->link);
> @@ -772,22 +769,20 @@ destroy_client_display_resource(struct wl_resource
> *resource)
> resource->client->display_resource = NULL;
> }
>
> -static void
> -bind_display(struct wl_client *client,
> - void *data, uint32_t version, uint32_t id)
> +static int
> +bind_display(struct wl_client *client, struct wl_display *display)
> {
> - struct wl_display *display = data;
> -
> client->display_resource =
> - wl_resource_create(client, &wl_display_interface, 1, id);
> + wl_resource_create(client, &wl_display_interface, 1, 1);
> if (client->display_resource == NULL) {
> wl_client_post_no_memory(client);
> - return;
> + return -1;
> }
>
> wl_resource_set_implementation(client->display_resource,
> &display_interface, display,
> destroy_client_display_resource);
> + return 0;
> }
>
> /** Create Wayland display object.
> @@ -831,13 +826,6 @@ wl_display_create(void)
>
> wl_array_init(&display->additional_shm_formats);
>
> - if (!wl_global_create(display, &wl_display_interface, 1,
> - display, bind_display)) {
> - wl_event_loop_destroy(display->loop);
> - free(display);
> - return NULL;
> - }
> -
> return display;
> }
>
I asked krh about this in irc, and he eventually agreed that exposing
wl_display as a global is not so useful.
I reviewed the patch, and the alleged problems the current code has,
and I agree they are real.
If we ever end up needing wl_display as a global, we can easily add it
back. Since the implementation was indeed broken, no client should ever
try to bind to wl_display global with version 1.
Therefore, pushed with ack from Jason and R-b from me.
In fact, I think this warrants this patch as a condidate for the stable
branch.
Thanks,
pq
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston 3/3] shell: constrain resize grabs so titlebars don't go under the panel
On Thu, 21 Aug 2014 11:42:03 +0800 Boyan Ding wrote: > Hi, > > On Wed, 2014-08-20 at 17:50 +0300, Pekka Paalanen wrote: > > Hi, > > > > on first glance, it looks like this completely ignores window rotation, > > but when I briefly tested on a rotated window, it seemed work well > > enough! :-o > > > > I can't figure out why. > > > > Now, Boyan's patch at least tries to take rotation into account: > > http://lists.freedesktop.org/archives/wayland-devel/2014-June/015658.html > > but will not work since I merged the two patches from Jonny, and needs > > a rebase if we choose it. > > > > Then there was the patch from Vivek: > > http://lists.freedesktop.org/archives/wayland-devel/2014-June/015528.html > > which I don't think handles rotated windows either? The same reason to > > rebase as for Boyan's patch. > > > > Were there others I missed? > > > > Boyan, can you see if Jonny's patch is right, or did I just not poke > > it the right way to make it break? Just merging that would be > > the easiest. If you don't like it, could you two agree on who makes the > > next one? > > > > Or do we just not care enough about rotated windows? :-) > > (I'd be fine with that, on resizing.) > > After comparing mine and Jonny's patches the more carefully, I found the > patches' idea are different. Jonny's patch puts a "hard requirement" > that when expanding in height on the top edge, titlebar shouldn't go > under top panel or the resizing won't take effect (quite tricky here). > While mine will clamp the y-coordinate of cursor position if it's on the > panel. > > I admit that my approach is somewhat too conservative. But there are > some tricky edge case under which Jonny's approach doesn't work very > well. Rotate a window clockwise 60-75 degrees, move it up until it can't > be moved further. You will find the top edge can't expand (actually > expanding the top edge here does no harm I think). More seriously, > You'll find that if you shrink the top edge a little, it can't resize > back! This behavior, in my opinion, is unacceptable, since too much > constraint is put on rotated windows. > > Also, if you rotate a window 90 degrees and expand the edge that is now > on the top. It will go under the panel. This behavior isn't change by > Jonny's patch but will be changed by mine since I believe the cursor > position in panel shouldn't be used in resizing, regardless of which > edge we are resizing, at least a clamped coordinate should be used. > > I think an ideal solution should surely ensure an unrotated window not > to be expanded under panel, but don't place too much restriction on > rotated windows. > > What's your idea? I think I would prefer the approach, where the pointer location used for move or resize is constrained. We should guarantee, that whatever point you grabbed, was it move or resize, will always stay accessible, i.e. within the work area. We could even have a safety margin around that point. I believe that should be fairly straightforward to implement, and the behaviour would also be obvious to a user. You don't have to constrain the cursor, just constrain the move or resize operation to keep the grab point accessible, if possible. That should also work well together with xdg_shell, as I recall a client must not make a surface bigger than asked for, but in some cases it might be smaller (weston-terminal step-sized resizing). I didn't much like the behaviour, where if you moved the cursor fast, the resizing might stop farther away from the limit compared to if you moved the cursor slowly. With these thoughts, I will be waiting for new patches for this issue, and hopefully also for the below. :-) Thanks, pq > > Btw. it seems that rotated windows already have issues with move > > constraints. Try to open a window fairly big, rotate it 180 degrees, > > grab the window's bottom (which is now facing up), and resize the > > window to very short. Then, try to move the window around the desktop. > > I cannot move the window up past some arbitrary point in the middle of > > the desktop, while there clearly is room to move. > > Yeah, I also noticed that. Maybe we should change that, too. > > Cheers, > Boyan Ding ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH 2/3] tests: remove unnecessary lines from queue-test
Earlier, the wl_display_dispatch_pending were setting number of thread that can dispatch events. This behaviour was removed later, so now these lines are redundant. Related commits: 385fe30e8b144a968aa88c6546c2ef247771b3d7 78cfa967681c965d23f6cbf76e080bbb0b564ff6 3c7e8bfbb4745315b7bcbf69fa746c3d6718c305 Signed-off-by: Marek Chalupa --- tests/queue-test.c | 10 -- 1 file changed, 10 deletions(-) diff --git a/tests/queue-test.c b/tests/queue-test.c index 3de8924..a7b54d9 100644 --- a/tests/queue-test.c +++ b/tests/queue-test.c @@ -130,11 +130,6 @@ client_test_multiple_queues(void) state.display = wl_display_connect(SOCKET_NAME); client_assert(state.display); - /* Make the current thread the display thread. This is because -* wl_display_dispatch_queue() will only read the display fd if -* the main display thread has been set. */ - wl_display_dispatch_pending(state.display); - queue = wl_display_create_queue(state.display); client_assert(queue); @@ -186,11 +181,6 @@ client_test_queue_roundtrip(void) display = wl_display_connect(SOCKET_NAME); client_assert(display); - /* Make the current thread the display thread. This is because -* wl_display_dispatch_queue() will only read the display fd if -* the main display thread has been set. */ - wl_display_dispatch_pending(display); - queue = wl_display_create_queue(display); client_assert(queue); -- 2.0.4 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH 3/3] client: remove unused variable
display_thread variable is unused since
3c7e8bfbb4745315b7bcbf69fa746c3d6718c305
Signed-off-by: Marek Chalupa
---
src/wayland-client.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/src/wayland-client.c b/src/wayland-client.c
index 15a5acc..2252424 100644
--- a/src/wayland-client.c
+++ b/src/wayland-client.c
@@ -97,7 +97,6 @@ struct wl_display {
uint32_t id;
} protocol_error;
int fd;
- pthread_t display_thread;
struct wl_map objects;
struct wl_event_queue display_queue;
struct wl_event_queue default_queue;
--
2.0.4
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH 1/3] tests: remove leaks from queue-test
Destroy all objects that we have created Signed-off-by: Marek Chalupa --- tests/queue-test.c | 5 + 1 file changed, 5 insertions(+) diff --git a/tests/queue-test.c b/tests/queue-test.c index f7ebc2b..3de8924 100644 --- a/tests/queue-test.c +++ b/tests/queue-test.c @@ -86,6 +86,8 @@ client_test_proxy_destroy(void) client_assert(counter == 1); + /* don't destroy the registry, we have already destroyed them +* in the global handler */ wl_display_disconnect(display); return 0; @@ -152,6 +154,7 @@ client_test_multiple_queues(void) while (!state.done && !ret) ret = wl_display_dispatch_queue(state.display, queue); + wl_event_queue_destroy(queue); wl_display_disconnect(state.display); return ret == -1 ? -1 : 0; @@ -221,6 +224,8 @@ client_test_queue_roundtrip(void) wl_callback_destroy(callback1); wl_callback_destroy(callback2); + wl_event_queue_destroy(queue); + wl_display_disconnect(display); return 0; -- 2.0.4 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH wayland v4 2/5] protocol: wl_pointer.set_cursor gives a role
From: Pekka Paalanen Now that we have defined "role", use the term. Signed-off-by: Pekka Paalanen --- protocol/wayland.xml | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/protocol/wayland.xml b/protocol/wayland.xml index 621c64d..89f7c0e 100644 --- a/protocol/wayland.xml +++ b/protocol/wayland.xml @@ -1394,7 +1394,10 @@ Set the pointer surface, i.e., the surface that contains the - pointer image (cursor). This request only takes effect if the pointer + pointer image (cursor). This request gives the surface the role + of a cursor. + + The cursor actually changes only if the pointer focus for this device is one of the requesting client's surfaces or the surface parameter is the current pointer surface. If there was a previous surface set with this request it is -- 1.8.5.5 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH wayland v4 1/5] protocol: define the concept of wl_surface role
From: Pekka Paalanen Define what a role is, and what restrictions there are. A change to existing behaviour is that a role cannot be changed at all once set. However, this is unlikely to cause problems, as there is no reason to re-use wl_surfaces in clients. v2: give more concrete examples of roles, define losing a role, Jasper rewrote the paragraph on how a role is set. v3: make role permanent, there is no such thing as "losing a role". Re-issuing the same role again must be allowed for wl_pointer.set_cursor et al. to work. v4: clarify the semantics of destroying a role object. Signed-off-by: Pekka Paalanen --- protocol/wayland.xml | 33 +++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/protocol/wayland.xml b/protocol/wayland.xml index bb457bc..621c64d 100644 --- a/protocol/wayland.xml +++ b/protocol/wayland.xml @@ -973,8 +973,37 @@ local coordinates of the pixel content, in case a buffer_transform or a buffer_scale is used. - Surfaces are also used for some special purposes, e.g. as - cursor images for pointers, drag icons, etc. + A surface without a "role" is fairly useless, a compositor does + not know where, when or how to present it. The role is the + purpose of a wl_surface. Examples of roles are a cursor for a + pointer (as set by wl_pointer.set_cursor), a drag icon + (wl_data_device.start_drag), a sub-surface + (wl_subcompositor.get_subsurface), and a window as defined by a + shell protocol (e.g. wl_shell.get_shell_surface). + + A surface can have only one role at a time. Initially a + wl_surface does not have a role. Once a wl_surface is given a + role, it is set permanently for the whole lifetime of the + wl_surface object. Giving the current role again is allowed, + unless explicitly forbidden by the relevant interface + specification. + + Surface roles are given by requests in other interfaces such as + wl_pointer.set_cursor. The request should explicitly mention + that this request gives a role to a wl_surface. Often, this + request also creates a new protocol object that represents the + role and adds additional functionality to wl_surface. When a + client wants to destroy a wl_surface, they must destroy this 'role + object' before the wl_surface. + + Destroying the role object does not remove the role from the + wl_surface, but it may stop the wl_surface from "playing the role". + For instance, if a wl_subsurface object is destroyed, the wl_surface + it was created for will be unmapped and forget its position and + z-order. It is allowed to create a wl_subsurface for the same + wl_surface again, but it is not allowed to use the wl_surface as + a cursor (cursor is a different role than sub-surface, and role + switching is not allowed). -- 1.8.5.5 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH wayland v4 4/5] protocol: wl_subcompositor.get_subsurface gives a role
From: Pekka Paalanen Reword the conditions to make use of the definition of "role". It is still forbidden to create more than one wl_subsurface for a wl_surface at a time. Signed-off-by: Pekka Paalanen --- protocol/wayland.xml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/protocol/wayland.xml b/protocol/wayland.xml index 71293dd..ddf94a3 100644 --- a/protocol/wayland.xml +++ b/protocol/wayland.xml @@ -1944,9 +1944,9 @@ associate it with the given parent surface. This turns a plain wl_surface into a sub-surface. - The to-be sub-surface must not already have a dedicated - purpose, like any shell surface type, cursor image, drag icon, - or sub-surface. Otherwise a protocol error is raised. + The to-be sub-surface must not already have another role, and it + must not have an existing wl_subsurface object. Otherwise a protocol + error is raised. http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH wayland v4 3/5] protocol: wl_data_device.start_drag may give a role
From: Pekka Paalanen Now that we have defined "role", use the term. Signed-off-by: Pekka Paalanen --- protocol/wayland.xml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/protocol/wayland.xml b/protocol/wayland.xml index 89f7c0e..71293dd 100644 --- a/protocol/wayland.xml +++ b/protocol/wayland.xml @@ -545,7 +545,8 @@ the top-left corner of the icon surface is placed at the cursor hotspot, but subsequent wl_surface.attach request can move the relative position. Attach requests must be confirmed with - wl_surface.commit as usual. + wl_surface.commit as usual. The icon surface is given the role of + a drag-and-drop icon. The current and pending input regions of the icon wl_surface are cleared, and wl_surface.set_input_region is ignored until the -- 1.8.5.5 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH wayland v4 5/5] protocol: wl_shell.get_shell_surface gives a role
From: Pekka Paalanen Now that we have defined "role", use the term. Signed-off-by: Pekka Paalanen --- protocol/wayland.xml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/protocol/wayland.xml b/protocol/wayland.xml index ddf94a3..3645208 100644 --- a/protocol/wayland.xml +++ b/protocol/wayland.xml @@ -679,7 +679,8 @@ - Create a shell surface for an existing surface. + Create a shell surface for an existing surface. This gives + the wl_surface the role of a shell surface. Only one shell surface can be associated with a given surface. -- 1.8.5.5 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH] option-parser: Don't read off the end of string options without an '='
On Thu, 21 Aug 2014 09:11:22 +1200 Robert Ancell wrote: > Bill's patch looks a lot more comprehensive, I'd use that one. Ok, thank you. - pq > On 21 August 2014 00:37, Pekka Paalanen wrote: > > > On Fri, 20 Jun 2014 15:23:59 +1200 > > Robert Ancell wrote: > > > > > I'm not sure if the expected behaviour is for: > > > $ weston --shell foo.so > > > to work, if so the patch can be modified. > > > > Ah, sorry for not noticing this patch in time. > > What do you think of Bill's series here: > > http://lists.freedesktop.org/archives/wayland-devel/2014-August/016657.html > > > > > > Thanks, > > pq ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH wayland] gitignore: add ./message-test
On Mon, 7 Jul 2014 12:11:08 +0800 Guangyu Zhang wrote: > ./message-test is generated by libtool and should be ignored by git. > --- > .gitignore | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/.gitignore b/.gitignore > index c146bac..d9d26ed 100644 > --- a/.gitignore > +++ b/.gitignore > @@ -47,6 +47,7 @@ fixed-benchmark > fixed-test > list-test > map-test > +message-test > os-wrappers-test > queue-test > resources-test Hi, I happened to pick the identical patch by Harrington before finding this. Sorry. Thanks, pq ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH 2/2] tests: test the wl_display_roundtrip_queue() function
On Thu, 21 Aug 2014 11:05:24 +0200 Marek Chalupa wrote: > The issues I pointed out are not anything big. I'll send a patch that > removes wl_display_dispatch_pending from both test-cases and I noticed that > there are more leaks it the queue-test, so I'll get rid of them all in > another patch if it's OK :) Sounds good to me! Thanks, pq ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH wayland v3] protocol: define the concept of wl_surface role
On Wed, 20 Aug 2014 10:26:48 -0400 "Jasper St. Pierre" wrote: > On Wed, Aug 20, 2014 at 7:09 AM, Pekka Paalanen wrote: > > > From: Pekka Paalanen > > > > Define what a role is, and what restrictions there are. > > > > A change to existing behaviour is that a role cannot be changed at all > > once set. However, this is unlikely to cause problems, as there is no > > reason to re-use wl_surfaces in clients. > > > > v2: give more concrete examples of roles, define losing a role, Jasper > > rewrote the paragraph on how a role is set. > > > > v3: make role permanent, there is no such thing as "losing a role". > > Re-issuing the same role again must be allowed for wl_pointer.set_cursor > > et al. to work. > > > > Signed-off-by: Pekka Paalanen > > --- > > protocol/wayland.xml | 27 +-- > > 1 file changed, 25 insertions(+), 2 deletions(-) > > > > diff --git a/protocol/wayland.xml b/protocol/wayland.xml > > index 2d57f69..7b05338 100644 > > --- a/protocol/wayland.xml > > +++ b/protocol/wayland.xml > > @@ -973,8 +973,31 @@ > >local coordinates of the pixel content, in case a buffer_transform > >or a buffer_scale is used. > > > > - Surfaces are also used for some special purposes, e.g. as > > - cursor images for pointers, drag icons, etc. > > + A surface without a "role" is fairly useless, a compositor does > > + not know where, when or how to present it. The role is the > > + purpose of a wl_surface. Examples of roles are a cursor for a > > + pointer (as set by wl_pointer.set_cursor), a drag icon > > + (wl_data_device.start_drag), a sub-surface > > + (wl_subcompositor.get_subsurface), and a window as defined by a > > + shell protocol (e.g. wl_shell.get_shell_surface). > > + > > + A surface can have only one role at a time. Initially a > > + wl_surface does not have a role. Once a wl_surface is given a > > + role, it is set permanently for the whole lifetime of the > > + wl_surface object. Giving the current role again is allowed, > > + unless explicitly forbidden by the relevant interface > > + specification. > > + > > + Surface roles are given by requests in other interfaces such as > > + wl_pointer.set_cursor. The request should explicitly mention > > + that this request gives a role to a wl_surface. Often, this > > + request also creates a new protocol object that represents the > > + role and adds additional functionality to wl_surface. When a > > + client wants to destroy a wl_surface, they must destroy this 'role > > + object' before the wl_surface. > > + > > + Destroying the role object does not remove the role from the > > + wl_surface, but it may stop the wl_surface from playing the role. > > > > This is an odd expression to use. I'd put it in quotes, and give a few > examples. > > Destroying the role object does not remove the role from the wl_surface, > but it may stop the wl_surface from "playing the role". For instance, > destroying the wl_subsurface object will hide the subsurface, but the > object will still have the subsurface role. To re-show the subsurface, a > new wl_subsurface object may be created on the same wl_surface. Ok, I can add more explanation to it. I am going to avoid phrases like "re-show", because the state is not preserved. I think I'll start a new thread with a whole series including this patch, and fixing the protocol spec for the other things in core wrt. roles. But I will need someone else to write me a weston implementation of this, with a test that checks we actually get an error if role switching is attempted, before this spec change can land. That is the only way to check during the alpha period that we are not breaking apps with this. IOW, if we want this to be included in the alpha, I need to be able to merge it on Friday. Unless there is a good reason to let the schedule slip. Thanks, pq ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH 2/2] tests: test the wl_display_roundtrip_queue() function
The issues I pointed out are not anything big. I'll send a patch that
removes wl_display_dispatch_pending from both test-cases and I noticed that
there are more leaks it the queue-test, so I'll get rid of them all in
another patch if it's OK :)
Thanks,
Marek
On 21 August 2014 10:57, Pekka Paalanen wrote:
> On Thu, 21 Aug 2014 10:46:55 +0200
> Marek Chalupa wrote:
>
> > Hi,
> >
> >
> > On 20 August 2014 18:29, Giulio Camuffo wrote:
> >
> > > ---
> > > tests/queue-test.c | 62
> > > ++
> > > 1 file changed, 62 insertions(+)
> > >
> > > diff --git a/tests/queue-test.c b/tests/queue-test.c
> > > index a4b165d..fc8a920 100644
> > > --- a/tests/queue-test.c
> > > +++ b/tests/queue-test.c
> > > @@ -158,6 +158,63 @@ client_test_multiple_queues(void)
> > > }
> > >
> > > static void
> > > +sync_callback_roundtrip(void *data, struct wl_callback *callback,
> > > uint32_t serial)
> > > +{
> > > + bool *done = data;
> > > + *done = true;
> > > +}
> > > +
> > > +static const struct wl_callback_listener sync_listener_roundtrip = {
> > > + sync_callback_roundtrip
> > > +};
> > > +
> > > +/* Test that doing a roundtrip on a queue only the events on that
> > > + * queue get dispatched. */
> > > +static int
> > > +client_test_queue_roundtrip(void)
> > > +{
> > > + struct wl_event_queue *queue;
> > > + struct wl_callback *callback1;
> > > + struct wl_callback *callback2;
> > > + struct wl_display *display;
> > > +
> > > + display = wl_display_connect(SOCKET_NAME);
> > > + client_assert(display);
> > > +
> > > + /* Make the current thread the display thread. This is because
> > > +* wl_display_dispatch_queue() will only read the display fd if
> > > +* the main display thread has been set. */
> > > + wl_display_dispatch_pending(display);
> > >
> >
> > I don't think this is necessary, this behavior was introduced in
> > 385fe30e8b144a968aa88c6546c2ef247771b3d7 and
> > 78cfa967681c965d23f6cbf76e080bbb0b564ff6
> > and it was removed
> > in 3c7e8bfbb4745315b7bcbf69fa746c3d6718c305 (now there's only unused
> > variable display_thread in wl_display structure, we were discussing it
> back
> > on the list IIRC).
> >
> > >
> > > + queue = wl_display_create_queue(display);
> > > + client_assert(queue);
> > > +
> > > + bool done1 = false;
> > > + callback1 = wl_display_sync(display);
> > > + assert(callback1 != NULL);
> > > + wl_callback_add_listener(callback1, &sync_listener_roundtrip,
> > > &done1);
> > > +
> > > + bool done2 = false;
> > > + callback2 = wl_display_sync(display);
> > > + assert(callback2 != NULL);
> > > + wl_callback_add_listener(callback2, &sync_listener_roundtrip,
> > > &done2);
> > > + wl_proxy_set_queue((struct wl_proxy *) callback2, queue);
> > > +
> > > + wl_display_roundtrip(display);
> > > + client_assert(done1 == true);
> > > + client_assert(done2 == false);
> > > +
> > > + wl_display_roundtrip_queue(display, queue);
> > > + client_assert(done2 == true);
> > > +
> > > + wl_callback_destroy(callback1);
> > > + wl_callback_destroy(callback2);
> > >
> >
> > You're leaking the queue here.
> >
> >
> > > + wl_display_disconnect(display);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static void
> > > client_alarm_handler(int sig)
> > > {
> > > fprintf(stderr, "Received SIGALRM signal, aborting.\n");
> > > @@ -199,6 +256,11 @@ client_main(int fd)
> > > return EXIT_FAILURE;
> > > }
> > >
> > > + if (client_test_queue_roundtrip() != 0) {
> > > + fprintf(stderr, "queue rountrip test failed\n");
> > > + return EXIT_FAILURE;
> > > + }
> > > +
> > > return EXIT_SUCCESS;
> > > }
> > >
> > > --
> > > 2.0.4
> > >
> > > ___
> > > wayland-devel mailing list
> > > wayland-devel@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/wayland-devel
> > >
> >
> > Otherwise:
> >
> > Reviewed-by: Marek Chalupa
>
> Oops, just pushed it already. Anyone care to follow up? :-)
>
>
> Thanks,
> pq
>
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH 2/2] tests: test the wl_display_roundtrip_queue() function
On Thu, 21 Aug 2014 10:46:55 +0200
Marek Chalupa wrote:
> Hi,
>
>
> On 20 August 2014 18:29, Giulio Camuffo wrote:
>
> > ---
> > tests/queue-test.c | 62
> > ++
> > 1 file changed, 62 insertions(+)
> >
> > diff --git a/tests/queue-test.c b/tests/queue-test.c
> > index a4b165d..fc8a920 100644
> > --- a/tests/queue-test.c
> > +++ b/tests/queue-test.c
> > @@ -158,6 +158,63 @@ client_test_multiple_queues(void)
> > }
> >
> > static void
> > +sync_callback_roundtrip(void *data, struct wl_callback *callback,
> > uint32_t serial)
> > +{
> > + bool *done = data;
> > + *done = true;
> > +}
> > +
> > +static const struct wl_callback_listener sync_listener_roundtrip = {
> > + sync_callback_roundtrip
> > +};
> > +
> > +/* Test that doing a roundtrip on a queue only the events on that
> > + * queue get dispatched. */
> > +static int
> > +client_test_queue_roundtrip(void)
> > +{
> > + struct wl_event_queue *queue;
> > + struct wl_callback *callback1;
> > + struct wl_callback *callback2;
> > + struct wl_display *display;
> > +
> > + display = wl_display_connect(SOCKET_NAME);
> > + client_assert(display);
> > +
> > + /* Make the current thread the display thread. This is because
> > +* wl_display_dispatch_queue() will only read the display fd if
> > +* the main display thread has been set. */
> > + wl_display_dispatch_pending(display);
> >
>
> I don't think this is necessary, this behavior was introduced in
> 385fe30e8b144a968aa88c6546c2ef247771b3d7 and
> 78cfa967681c965d23f6cbf76e080bbb0b564ff6
> and it was removed
> in 3c7e8bfbb4745315b7bcbf69fa746c3d6718c305 (now there's only unused
> variable display_thread in wl_display structure, we were discussing it back
> on the list IIRC).
>
> >
> > + queue = wl_display_create_queue(display);
> > + client_assert(queue);
> > +
> > + bool done1 = false;
> > + callback1 = wl_display_sync(display);
> > + assert(callback1 != NULL);
> > + wl_callback_add_listener(callback1, &sync_listener_roundtrip,
> > &done1);
> > +
> > + bool done2 = false;
> > + callback2 = wl_display_sync(display);
> > + assert(callback2 != NULL);
> > + wl_callback_add_listener(callback2, &sync_listener_roundtrip,
> > &done2);
> > + wl_proxy_set_queue((struct wl_proxy *) callback2, queue);
> > +
> > + wl_display_roundtrip(display);
> > + client_assert(done1 == true);
> > + client_assert(done2 == false);
> > +
> > + wl_display_roundtrip_queue(display, queue);
> > + client_assert(done2 == true);
> > +
> > + wl_callback_destroy(callback1);
> > + wl_callback_destroy(callback2);
> >
>
> You're leaking the queue here.
>
>
> > + wl_display_disconnect(display);
> > +
> > + return 0;
> > +}
> > +
> > +static void
> > client_alarm_handler(int sig)
> > {
> > fprintf(stderr, "Received SIGALRM signal, aborting.\n");
> > @@ -199,6 +256,11 @@ client_main(int fd)
> > return EXIT_FAILURE;
> > }
> >
> > + if (client_test_queue_roundtrip() != 0) {
> > + fprintf(stderr, "queue rountrip test failed\n");
> > + return EXIT_FAILURE;
> > + }
> > +
> > return EXIT_SUCCESS;
> > }
> >
> > --
> > 2.0.4
> >
> > ___
> > wayland-devel mailing list
> > wayland-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/wayland-devel
> >
>
> Otherwise:
>
> Reviewed-by: Marek Chalupa
Oops, just pushed it already. Anyone care to follow up? :-)
Thanks,
pq
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH 2/2] tests: test the wl_display_roundtrip_queue() function
On Wed, 20 Aug 2014 19:29:10 +0300
Giulio Camuffo wrote:
> ---
> tests/queue-test.c | 62
> ++
> 1 file changed, 62 insertions(+)
>
> diff --git a/tests/queue-test.c b/tests/queue-test.c
> index a4b165d..fc8a920 100644
> --- a/tests/queue-test.c
> +++ b/tests/queue-test.c
> @@ -158,6 +158,63 @@ client_test_multiple_queues(void)
> }
>
> static void
> +sync_callback_roundtrip(void *data, struct wl_callback *callback, uint32_t
> serial)
> +{
> + bool *done = data;
> + *done = true;
> +}
> +
> +static const struct wl_callback_listener sync_listener_roundtrip = {
> + sync_callback_roundtrip
> +};
> +
> +/* Test that doing a roundtrip on a queue only the events on that
> + * queue get dispatched. */
> +static int
> +client_test_queue_roundtrip(void)
> +{
> + struct wl_event_queue *queue;
> + struct wl_callback *callback1;
> + struct wl_callback *callback2;
> + struct wl_display *display;
> +
> + display = wl_display_connect(SOCKET_NAME);
> + client_assert(display);
> +
> + /* Make the current thread the display thread. This is because
> + * wl_display_dispatch_queue() will only read the display fd if
> + * the main display thread has been set. */
> + wl_display_dispatch_pending(display);
> +
> + queue = wl_display_create_queue(display);
> + client_assert(queue);
> +
> + bool done1 = false;
> + callback1 = wl_display_sync(display);
> + assert(callback1 != NULL);
> + wl_callback_add_listener(callback1, &sync_listener_roundtrip, &done1);
> +
> + bool done2 = false;
> + callback2 = wl_display_sync(display);
> + assert(callback2 != NULL);
> + wl_callback_add_listener(callback2, &sync_listener_roundtrip, &done2);
> + wl_proxy_set_queue((struct wl_proxy *) callback2, queue);
> +
> + wl_display_roundtrip(display);
> + client_assert(done1 == true);
> + client_assert(done2 == false);
> +
> + wl_display_roundtrip_queue(display, queue);
> + client_assert(done2 == true);
> +
> + wl_callback_destroy(callback1);
> + wl_callback_destroy(callback2);
> + wl_display_disconnect(display);
> +
> + return 0;
> +}
> +
> +static void
> client_alarm_handler(int sig)
> {
> fprintf(stderr, "Received SIGALRM signal, aborting.\n");
> @@ -199,6 +256,11 @@ client_main(int fd)
> return EXIT_FAILURE;
> }
>
> + if (client_test_queue_roundtrip() != 0) {
> + fprintf(stderr, "queue rountrip test failed\n");
> + return EXIT_FAILURE;
> + }
> +
> return EXIT_SUCCESS;
> }
>
Looking good, both patches pushed. I squashed some additions to the
test patch:
[Pekka Paalanen: moved variable declarations to before code. Added some
comments, and added the re-arm to additionally test the opposite case.]
which you acked on IRC.
Thanks,
pq
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH] wl_surface: clarify the base of time passed in the callback of frame
On Thu, 21 Aug 2014 00:25:36 +0900 Ryo Munakata wrote: > Signed-off-by: Ryo Munakata > --- > protocol/wayland.xml | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/protocol/wayland.xml b/protocol/wayland.xml > index 2d57f69..bb457bc 100644 > --- a/protocol/wayland.xml > +++ b/protocol/wayland.xml > @@ -1100,7 +1100,7 @@ > attempt to use it after that point. > > The callback_data passed in the callback is the current time, in > - milliseconds. > + milliseconds, with an undefined base. > > > Heh, sure. Pushed. :-) The clock_gettime manual uses the term "some unspecified starting point", but I suppose this works well enough. Thanks, pq ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH 2/2] tests: test the wl_display_roundtrip_queue() function
Hi,
On 20 August 2014 18:29, Giulio Camuffo wrote:
> ---
> tests/queue-test.c | 62
> ++
> 1 file changed, 62 insertions(+)
>
> diff --git a/tests/queue-test.c b/tests/queue-test.c
> index a4b165d..fc8a920 100644
> --- a/tests/queue-test.c
> +++ b/tests/queue-test.c
> @@ -158,6 +158,63 @@ client_test_multiple_queues(void)
> }
>
> static void
> +sync_callback_roundtrip(void *data, struct wl_callback *callback,
> uint32_t serial)
> +{
> + bool *done = data;
> + *done = true;
> +}
> +
> +static const struct wl_callback_listener sync_listener_roundtrip = {
> + sync_callback_roundtrip
> +};
> +
> +/* Test that doing a roundtrip on a queue only the events on that
> + * queue get dispatched. */
> +static int
> +client_test_queue_roundtrip(void)
> +{
> + struct wl_event_queue *queue;
> + struct wl_callback *callback1;
> + struct wl_callback *callback2;
> + struct wl_display *display;
> +
> + display = wl_display_connect(SOCKET_NAME);
> + client_assert(display);
> +
> + /* Make the current thread the display thread. This is because
> +* wl_display_dispatch_queue() will only read the display fd if
> +* the main display thread has been set. */
> + wl_display_dispatch_pending(display);
>
I don't think this is necessary, this behavior was introduced in
385fe30e8b144a968aa88c6546c2ef247771b3d7 and
78cfa967681c965d23f6cbf76e080bbb0b564ff6
and it was removed
in 3c7e8bfbb4745315b7bcbf69fa746c3d6718c305 (now there's only unused
variable display_thread in wl_display structure, we were discussing it back
on the list IIRC).
>
> + queue = wl_display_create_queue(display);
> + client_assert(queue);
> +
> + bool done1 = false;
> + callback1 = wl_display_sync(display);
> + assert(callback1 != NULL);
> + wl_callback_add_listener(callback1, &sync_listener_roundtrip,
> &done1);
> +
> + bool done2 = false;
> + callback2 = wl_display_sync(display);
> + assert(callback2 != NULL);
> + wl_callback_add_listener(callback2, &sync_listener_roundtrip,
> &done2);
> + wl_proxy_set_queue((struct wl_proxy *) callback2, queue);
> +
> + wl_display_roundtrip(display);
> + client_assert(done1 == true);
> + client_assert(done2 == false);
> +
> + wl_display_roundtrip_queue(display, queue);
> + client_assert(done2 == true);
> +
> + wl_callback_destroy(callback1);
> + wl_callback_destroy(callback2);
>
You're leaking the queue here.
> + wl_display_disconnect(display);
> +
> + return 0;
> +}
> +
> +static void
> client_alarm_handler(int sig)
> {
> fprintf(stderr, "Received SIGALRM signal, aborting.\n");
> @@ -199,6 +256,11 @@ client_main(int fd)
> return EXIT_FAILURE;
> }
>
> + if (client_test_queue_roundtrip() != 0) {
> + fprintf(stderr, "queue rountrip test failed\n");
> + return EXIT_FAILURE;
> + }
> +
> return EXIT_SUCCESS;
> }
>
> --
> 2.0.4
>
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
Otherwise:
Reviewed-by: Marek Chalupa
Regards,
Marek
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston 1/3] evdev/libinput: sync the leds of keyboards with the xkb state
2014-08-21 10:34 GMT+03:00 Daniel Stone : > Hi, > > > On Wednesday, August 20, 2014, Giulio Camuffo > wrote: >> >> When a new keyboard is found (including during startup) sync its leds >> with the internal state of the xkb map. >> It appears that by setting them immediately when getting the new device >> we're racing with the kernel or something, which wants to turn all the >> leds off, so we use a timer. > > > Ugh. If you've your own kernel to hand, I'd hack it to WARN_ON(1) on LED > updates, so you can track where the rogue update is coming from. My guess is > that switching VT breaks it, so it might work if you instead trigger an > update of all LED state on every VT enter? Switching VT is another matter, because all the keyboard devices are removed so the xkb state is lost, so when returning to weston's vt we don't know anymore which leds are supposed to be on. This patch just fixes keyboard hotplugging, besides turning the leds off at startup. Besides that, i honestly don't know what WARN_ON is, and i haven't compiled a kernel in a few years. :) -- Giulio > > Cheers, > Daniel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH 2/2] compositor: add a way to change the keyboard leds
Hi, Last nitpick, sorry ... On Wednesday, August 20, 2014, Giulio Camuffo wrote: > > + mods_depressed = > xkb_state_serialize_mods(keyboard->xkb_state.state, > + XKB_STATE_DEPRESSED); > + mods_latched = xkb_state_serialize_mods(keyboard->xkb_state.state, > + XKB_STATE_LATCHED); > + mods_locked = xkb_state_serialize_mods(keyboard->xkb_state.state, > + XKB_STATE_LOCKED); > + group = xkb_state_serialize_group(keyboard->xkb_state.state, > + XKB_STATE_EFFECTIVE); > + > + num = (1 << keyboard->xkb_info->mod2_mod); > + caps = (1 << keyboard->xkb_info->caps_mod); I still don't really believe the non-xkbcommon build should exist, but shouldn't all this be #ifdef'ed? Cheers, Daniel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston 1/3] evdev/libinput: sync the leds of keyboards with the xkb state
Hi, On Wednesday, August 20, 2014, Giulio Camuffo wrote: > When a new keyboard is found (including during startup) sync its leds > with the internal state of the xkb map. > It appears that by setting them immediately when getting the new device > we're racing with the kernel or something, which wants to turn all the > leds off, so we use a timer. Ugh. If you've your own kernel to hand, I'd hack it to WARN_ON(1) on LED updates, so you can track where the rogue update is coming from. My guess is that switching VT breaks it, so it might work if you instead trigger an update of all LED state on every VT enter? Cheers, Daniel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston 3/3] compositor: add an option to set the default numlock value
Hi, On Wednesday, August 20, 2014, Giulio Camuffo wrote: > Add a new "numlock-on" option in the [keyboard] section of weston.ini > which, if set to true, is used to enable the numlock of the keyboards > attached at startup. > I'm fine with this in principle, but would like to see this test if it's possible (i.e. not nesting / led_update() is valid), and issue a rejection message to the log otherwise. Cheers, Daniel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
