Re: [RFC] weston: implement inert objects for keyboard/pointer/touch

[Derek Foreman]

On 19/10/15 08:47 AM, David FORT wrote:
> This is the second version. I have restored the ref counting of input devices,
> I think with the name weston_seat_init_pointer is not accurate, perhaps 
> weston_seat_add_pointer_device would be better. I'm really wondering if it's
> the weston core that should do that refcounting, or if the input backend 
> should
> do it itself. I can't see any case where we would have 2 input backends (which
> would be a justification for weston doing it).

I agree that weston_seat_init_* are bad names.  weston_seat_add_*_device
does seem more reasonable to me.

(weston_seat_release_* seems sensible enough as it is.)

> Note that with this patch, we don't save the last position of the pointer. I'm
> wondering why we wanna do this, does that mean that we want the same kind of 
> behaviour for other input devices (saving locks state for keyboard device for 
> example) ?

Michal Suchanek already explained this perfectly, but I'll chime in with
"me too" - we absolutely need to retain that stuff.

(I can almost hear Bryce recommending we test for retained pointer state
in devices_test...  that'd be good at some point but not a pre-requisite
for this work...)

Guess that leaves you in a bad place regarding keyboard/pointer/touch
destruction?  I think it's not insurmountable though - I think when a
*seat* is destroyed that information should be removed.

I think this also answers your question of who should do the
refcounting?  If weston core is storing the retained state, I think it
needs to be responsible for the refcounting?

The RDP back-end has seats that come and go, I don't think the DRM
compositor should ever actually destroy a seat...  Does that make sense
or am I crazy? :)

Oh, I also agree with Bryce that it looks like this is two patches
pushed together - one with a lot of "if (!keyboard) dont_crash()" stuff
that we could review and land as an independent patch.  That stuff will
be an easy review and it'll make the actual "inert object" stuff easier
to look at too.

> David FORT (1):
>   weston: implement inert objects for keyboard/pointer/touch
> 
>  desktop-shell/exposay.c  |  16 ++--
>  desktop-shell/shell.c|  21 ++--
>  src/compositor-wayland.c |  10 +-
>  src/compositor-x11.c |  12 ++-
>  src/input.c  | 242 
> +++
>  src/libinput-device.c|   2 +-
>  src/screen-share.c   |   2 +
>  src/text-backend.c   |  44 +
>  src/zoom.c   |   7 +-
>  tests/weston-test.c  |  18 ++--
>  xwayland/dnd.c   |   3 +-
>  11 files changed, 224 insertions(+), 153 deletions(-)
> 

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [RFC] weston: implement inert objects for keyboard/pointer/touch

[Michal Suchanek]

Hello,

On 19 October 2015 at 15:47, David FORT  wrote:
> This is the second version. I have restored the ref counting of input devices,
> I think with the name weston_seat_init_pointer is not accurate, perhaps
> weston_seat_add_pointer_device would be better. I'm really wondering if it's
> the weston core that should do that refcounting, or if the input backend 
> should
> do it itself. I can't see any case where we would have 2 input backends (which
> would be a justification for weston doing it).
> Note that with this patch, we don't save the last position of the pointer. I'm
> wondering why we wanna do this, does that mean that we want the same kind of
> behaviour for other input devices (saving locks state for keyboard device for
> example) ?
>


On widely used systems that have a pointer the position of relative
pointer is saved across device reconnect. So users will expect that
disconnecting a mouse and connecting another one will not change
pointer position.

Same with keyboard locks. While buttons on a device should be released
on disconnect internal state that is not mechanically represented by
device position should stay. Which includes the pointer position for
devices that drive the pointer with relative axis and state of
keyboard locks that remain locked when the key itself is released.

For devices that drive pointer with absolute axis and have axis that
can be out of proximity and hence can be in state that provides no
pointer position reading the initial pointer position is also needed.
This is like almost all devices, actually.

Since changing the pointer position or keyboard lock state at random
on device reconnect would cause problems for people with USB bus
issues I would think that saving the state is not just something
current systems randomly happen to do. Note that hardware is sometimes
not fixable to work reliably. This is especially true for wireless
buses like Bluetooth.

Thanks

Michal
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [RFC] weston: implement inert objects for keyboard/pointer/touch

[Hardening]

Le 16/10/2015 11:53, David FORT a écrit :
> This patch implements inert objects for wl_keyboard, wl_pointer and wl_touch.
> The target case is when the server has just send a capability event about a
> disappearing object, and the client binds the corresponding object. We bind an
> inert object: an object does nothing when it is requested. If the client 
> behave
> correctly, this object should be released when the capability event is 
> received
> and treated (calling the corresponding release request).
> As a consequence, we can rely on seat->[keyboard|pointer|touch]_state to know
> if the seat has the corresponding object.
> Weston doesn't really handle multiple mice for one wl_pointer, so I have 
> removed
> the corresponding code and tests (it was quite a weston_test hack BTW).
> Finally I have fixed a wrong behaviour: the capabilities event's 
> documentation states
> that the capabilities should be sent when a new capability is set on the 
> seat. So
> attaching a second mouse to an existing wl_pointer should not broadcast seat
> capabilities (and the same for keyboard and touch).
> ---
>  desktop-shell/exposay.c  |  16 ++--
>  desktop-shell/shell.c|  21 +++--
>  src/compositor-wayland.c |  10 +-
>  src/compositor-x11.c |  12 ++-
>  src/compositor.h |   3 -
>  src/input.c  | 235 
> ++-
>  src/libinput-device.c|   2 +-
>  src/screen-share.c   |   2 +
>  src/text-backend.c   |  44 +
>  src/zoom.c   |   7 +-
>  tests/devices-test.c |  20 
>  tests/weston-test.c  |  18 ++--
>  xwayland/dnd.c   |   3 +-
>  13 files changed, 216 insertions(+), 177 deletions(-)
> 
> diff --git a/desktop-shell/exposay.c b/desktop-shell/exposay.c
> index 08b86a3..d6919b3 100644
> --- a/desktop-shell/exposay.c
> +++ b/desktop-shell/exposay.c
> @@ -574,21 +574,23 @@ exposay_transition_active(struct desktop_shell *shell)
>   bool animate = false;
>  
>   shell->exposay.workspace = get_current_workspace(shell);
> - shell->exposay.focus_prev = get_default_view(keyboard->focus);
> - shell->exposay.focus_current = get_default_view(keyboard->focus);
> + if (keyboard) {
> + shell->exposay.focus_prev = get_default_view(keyboard->focus);
> + shell->exposay.focus_current = 
> get_default_view(keyboard->focus);
> + }
>   shell->exposay.clicked = NULL;
>   wl_list_init(>exposay.surface_list);
>  
>   lower_fullscreen_layer(shell, NULL);
>   shell->exposay.grab_kbd.interface = _kbd_grab;
> - weston_keyboard_start_grab(keyboard,
> ->exposay.grab_kbd);
> - weston_keyboard_set_focus(keyboard, NULL);
> + if (keyboard) {
> + weston_keyboard_start_grab(keyboard, >exposay.grab_kbd);
> + weston_keyboard_set_focus(keyboard, NULL);
> + }
>  
>   shell->exposay.grab_ptr.interface = _ptr_grab;
>   if (pointer) {
> - weston_pointer_start_grab(pointer,
> -   >exposay.grab_ptr);
> + weston_pointer_start_grab(pointer, >exposay.grab_ptr);
>   weston_pointer_clear_focus(pointer);
>   }
>   wl_list_for_each(shell_output, >output_list, link) {
> diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
> index 3eb3f5c..cdddf09 100644
> --- a/desktop-shell/shell.c
> +++ b/desktop-shell/shell.c
> @@ -380,7 +380,8 @@ shell_grab_start(struct shell_grab *grab,
>   struct desktop_shell *shell = shsurf->shell;
>   struct weston_touch *touch = weston_seat_get_touch(pointer->seat);
>  
> - popup_grab_end(pointer);
> + if (pointer)
> + popup_grab_end(pointer);
>   if (touch)
>   touch_popup_grab_end(touch);
>  
> @@ -391,11 +392,13 @@ shell_grab_start(struct shell_grab *grab,
> >shsurf_destroy_listener);
>  
>   shsurf->grabbed = 1;
> - weston_pointer_start_grab(pointer, >grab);
> + if (pointer)
> + weston_pointer_start_grab(pointer, >grab);
>   if (shell->child.desktop_shell) {
>   desktop_shell_send_grab_cursor(shell->child.desktop_shell,
>  cursor);
> - weston_pointer_set_focus(pointer,
> + if (pointer)
> + weston_pointer_set_focus(pointer,
>get_default_view(shell->grab_surface),
>wl_fixed_from_int(0),
>wl_fixed_from_int(0));
> @@ -925,12 +928,10 @@ restore_focus_state(struct desktop_shell *shell, struct 
> workspace *ws)
>   wl_list_insert(>compositor->seat_list,
>  >seat->link);
>  
> - if (!keyboard)
> - continue;
> -
>   surface = state->keyboard_focus;
>  
> - weston_keyboard_set_focus(keyboard, surface);
> +  

Re: [RFC] weston: implement inert objects for keyboard/pointer/touch

[Hardening]

Le 16/10/2015 11:53, David FORT a écrit :
> This patch implements inert objects for wl_keyboard, wl_pointer and wl_touch.
> The target case is when the server has just send a capability event about a
> disappearing object, and the client binds the corresponding object. We bind an
> inert object: an object does nothing when it is requested. If the client 
> behave
> correctly, this object should be released when the capability event is 
> received
> and treated (calling the corresponding release request).
> As a consequence, we can rely on seat->[keyboard|pointer|touch]_state to know
> if the seat has the corresponding object.
> Weston doesn't really handle multiple mice for one wl_pointer, so I have 
> removed
> the corresponding code and tests (it was quite a weston_test hack BTW).
> Finally I have fixed a wrong behaviour: the capabilities event's 
> documentation states
> that the capabilities should be sent when a new capability is set on the 
> seat. So
> attaching a second mouse to an existing wl_pointer should not broadcast seat
> capabilities (and the same for keyboard and touch).
> ---
>  desktop-shell/exposay.c  |  16 ++--
>  desktop-shell/shell.c|  21 +++--
>  src/compositor-wayland.c |  10 +-
>  src/compositor-x11.c |  12 ++-
>  src/compositor.h |   3 -
>  src/input.c  | 235 
> ++-
>  src/libinput-device.c|   2 +-
>  src/screen-share.c   |   2 +
>  src/text-backend.c   |  44 +
>  src/zoom.c   |   7 +-
>  tests/devices-test.c |  20 
>  tests/weston-test.c  |  18 ++--
>  xwayland/dnd.c   |   3 +-
>  13 files changed, 216 insertions(+), 177 deletions(-)
> 
> diff --git a/desktop-shell/exposay.c b/desktop-shell/exposay.c
> index 08b86a3..d6919b3 100644
> --- a/desktop-shell/exposay.c
> +++ b/desktop-shell/exposay.c
> @@ -574,21 +574,23 @@ exposay_transition_active(struct desktop_shell *shell)
>   bool animate = false;
>  
>   shell->exposay.workspace = get_current_workspace(shell);
> - shell->exposay.focus_prev = get_default_view(keyboard->focus);
> - shell->exposay.focus_current = get_default_view(keyboard->focus);
> + if (keyboard) {
> + shell->exposay.focus_prev = get_default_view(keyboard->focus);
> + shell->exposay.focus_current = 
> get_default_view(keyboard->focus);
> + }
>   shell->exposay.clicked = NULL;
>   wl_list_init(>exposay.surface_list);
>  
>   lower_fullscreen_layer(shell, NULL);
>   shell->exposay.grab_kbd.interface = _kbd_grab;
> - weston_keyboard_start_grab(keyboard,
> ->exposay.grab_kbd);
> - weston_keyboard_set_focus(keyboard, NULL);
> + if (keyboard) {
> + weston_keyboard_start_grab(keyboard, >exposay.grab_kbd);
> + weston_keyboard_set_focus(keyboard, NULL);
> + }
>  
>   shell->exposay.grab_ptr.interface = _ptr_grab;
>   if (pointer) {
> - weston_pointer_start_grab(pointer,
> -   >exposay.grab_ptr);
> + weston_pointer_start_grab(pointer, >exposay.grab_ptr);
>   weston_pointer_clear_focus(pointer);
>   }
>   wl_list_for_each(shell_output, >output_list, link) {
> diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
> index 3eb3f5c..cdddf09 100644
> --- a/desktop-shell/shell.c
> +++ b/desktop-shell/shell.c
> @@ -380,7 +380,8 @@ shell_grab_start(struct shell_grab *grab,
>   struct desktop_shell *shell = shsurf->shell;
>   struct weston_touch *touch = weston_seat_get_touch(pointer->seat);
>  
> - popup_grab_end(pointer);
> + if (pointer)
> + popup_grab_end(pointer);
>   if (touch)
>   touch_popup_grab_end(touch);
>  
> @@ -391,11 +392,13 @@ shell_grab_start(struct shell_grab *grab,
> >shsurf_destroy_listener);
>  
>   shsurf->grabbed = 1;
> - weston_pointer_start_grab(pointer, >grab);
> + if (pointer)
> + weston_pointer_start_grab(pointer, >grab);
>   if (shell->child.desktop_shell) {
>   desktop_shell_send_grab_cursor(shell->child.desktop_shell,
>  cursor);
> - weston_pointer_set_focus(pointer,
> + if (pointer)
> + weston_pointer_set_focus(pointer,
>get_default_view(shell->grab_surface),
>wl_fixed_from_int(0),
>wl_fixed_from_int(0));
> @@ -925,12 +928,10 @@ restore_focus_state(struct desktop_shell *shell, struct 
> workspace *ws)
>   wl_list_insert(>compositor->seat_list,
>  >seat->link);
>  
> - if (!keyboard)
> - continue;
> -
>   surface = state->keyboard_focus;
>  
> - weston_keyboard_set_focus(keyboard, surface);
> +