Re: [PATCH weston 1/2] input: Update keyboard serial on press and release
Hi, On 2 September 2016 at 09:30, Giulio Camuffo wrote: > 2016-09-02 10:27 GMT+02:00 Olivier Fourdan : >>> 2016-09-01 11:52 GMT+02:00 Olivier Fourdan : >>> >> I’m ok with that, but why not updating all three "grab_*" members? >>> > >>> > While I think the key itself would be OK, I suspect updating the grab_time >>> > on all states (ie like key release) might have additional side effects. >>> > >>> > So I took the most conservative approach which was to update the serial >>> > only, as this one is compared against the popup serial. >>> >>> I can't tell if it's right or not, but so maybe it would be good to >>> add a comment about it, unless someone (daniels?) can confirm it's >>> right. >> >> Well, this is needed when traversing submenus using the keyboard. >> >> Once you navigate to a submenu with the keyboard, the corresponding popup is >> created by the toolkit using the serial provided by the compositor, which >> will compare against the last known serial. If the grab serial is updated on >> key press only, the serial won't match and the submenu will be dismissed. > > Yes, i meant if it's right or not to leave the grab_time and grab_key > as they are. Yes, leaving those as-is makes things work as intended. What's unpleasant about this patch isn't that though, but that clients which attempt to use the key _press_ to trigger something are now going to fail. I don't like that, but only triggering on release makes much much more sense (hence why GTK+ does it), and maintaining multiple possible serials is a lot more work. So, pushed with review, thanks! To ssh://git.freedesktop.org/git/wayland/weston be8a6d3..df84dbe push -> master Cheers, Daniel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston 1/2] input: Update keyboard serial on press and release
2016-09-02 10:27 GMT+02:00 Olivier Fourdan :
> Hi,
>
> - Original Message -
>> 2016-09-01 11:52 GMT+02:00 Olivier Fourdan :
>> > Hi
>> >
>> >> > + keyboard->grab_serial =
>> >> > wl_display_get_serial(compositor->wl_display);
>> >> > if (state == WL_KEYBOARD_KEY_STATE_PRESSED) {
>> >> > - keyboard->grab_serial =
>> >> > - wl_display_get_serial(compositor->wl_display);
>> >> > keyboard->grab_time = time;
>> >> > keyboard->grab_key = key;
>> >> > }
>> >> >
>> >>
>> >> I’m ok with that, but why not updating all three "grab_*" members?
>> >
>> > While I think the key itself would be OK, I suspect updating the grab_time
>> > on all states (ie like key release) might have additional side effects.
>> >
>> > So I took the most conservative approach which was to update the serial
>> > only, as this one is compared against the popup serial.
>>
>> I can't tell if it's right or not, but so maybe it would be good to
>> add a comment about it, unless someone (daniels?) can confirm it's
>> right.
>
> Well, this is needed when traversing submenus using the keyboard.
>
> Once you navigate to a submenu with the keyboard, the corresponding popup is
> created by the toolkit using the serial provided by the compositor, which
> will compare against the last known serial. If the grab serial is updated on
> key press only, the serial won't match and the submenu will be dismissed.
Yes, i meant if it's right or not to leave the grab_time and grab_key
as they are.
>
> This can easily be reproduced using an up-to-date gtk+ from git master.
>
> Cheers,
> Olivier
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston 1/2] input: Update keyboard serial on press and release
Hi,
- Original Message -
> 2016-09-01 11:52 GMT+02:00 Olivier Fourdan :
> > Hi
> >
> >> > + keyboard->grab_serial =
> >> > wl_display_get_serial(compositor->wl_display);
> >> > if (state == WL_KEYBOARD_KEY_STATE_PRESSED) {
> >> > - keyboard->grab_serial =
> >> > - wl_display_get_serial(compositor->wl_display);
> >> > keyboard->grab_time = time;
> >> > keyboard->grab_key = key;
> >> > }
> >> >
> >>
> >> I’m ok with that, but why not updating all three "grab_*" members?
> >
> > While I think the key itself would be OK, I suspect updating the grab_time
> > on all states (ie like key release) might have additional side effects.
> >
> > So I took the most conservative approach which was to update the serial
> > only, as this one is compared against the popup serial.
>
> I can't tell if it's right or not, but so maybe it would be good to
> add a comment about it, unless someone (daniels?) can confirm it's
> right.
Well, this is needed when traversing submenus using the keyboard.
Once you navigate to a submenu with the keyboard, the corresponding popup is
created by the toolkit using the serial provided by the compositor, which will
compare against the last known serial. If the grab serial is updated on key
press only, the serial won't match and the submenu will be dismissed.
This can easily be reproduced using an up-to-date gtk+ from git master.
Cheers,
Olivier
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston 1/2] input: Update keyboard serial on press and release
2016-09-01 11:52 GMT+02:00 Olivier Fourdan :
> Hi
>
>> > + keyboard->grab_serial = wl_display_get_serial(compositor->wl_display);
>> > if (state == WL_KEYBOARD_KEY_STATE_PRESSED) {
>> > - keyboard->grab_serial =
>> > - wl_display_get_serial(compositor->wl_display);
>> > keyboard->grab_time = time;
>> > keyboard->grab_key = key;
>> > }
>> >
>>
>> I’m ok with that, but why not updating all three "grab_*" members?
>
> While I think the key itself would be OK, I suspect updating the grab_time on
> all states (ie like key release) might have additional side effects.
>
> So I took the most conservative approach which was to update the serial only,
> as this one is compared against the popup serial.
I can't tell if it's right or not, but so maybe it would be good to
add a comment about it, unless someone (daniels?) can confirm it's
right.
Cheers,
Giulio
>
> Cheers,
> Olivier
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston 1/2] input: Update keyboard serial on press and release
Hi
> > + keyboard->grab_serial = wl_display_get_serial(compositor->wl_display);
> > if (state == WL_KEYBOARD_KEY_STATE_PRESSED) {
> > - keyboard->grab_serial =
> > - wl_display_get_serial(compositor->wl_display);
> > keyboard->grab_time = time;
> > keyboard->grab_key = key;
> > }
> >
>
> I’m ok with that, but why not updating all three "grab_*" members?
While I think the key itself would be OK, I suspect updating the grab_time on
all states (ie like key release) might have additional side effects.
So I took the most conservative approach which was to update the serial only,
as this one is compared against the popup serial.
Cheers,
Olivier
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston 1/2] input: Update keyboard serial on press and release
On 30/06/2016 16:01, Olivier Fourdan wrote:
Other compositors such as mutter update the keyboard serial for both key
press and key release, unlike weston which updates it only on key press.
When dealing with popup windows which require a match in serials, if the
event that caused the popup to be shown is a key release, then the popup
would be dismissed.
This occurs when navigating gtk+ sub-menus using the keyboard.
Signed-off-by: Olivier Fourdan
Bugzilla: https://bugzilla.gnome.org/show_bug.cgi?id=768017
---
libweston/input.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/libweston/input.c b/libweston/input.c
index 8f46698..18c4c1f 100644
--- a/libweston/input.c
+++ b/libweston/input.c
@@ -1661,9 +1661,8 @@ notify_key(struct weston_seat *seat, uint32_t time,
uint32_t key,
state);
}
+ keyboard->grab_serial = wl_display_get_serial(compositor->wl_display);
if (state == WL_KEYBOARD_KEY_STATE_PRESSED) {
- keyboard->grab_serial =
- wl_display_get_serial(compositor->wl_display);
keyboard->grab_time = time;
keyboard->grab_key = key;
}
I’m ok with that, but why not updating all three "grab_*" members?
Cheers,
--
Quentin “Sardem FF7” Glidic
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston 1/2] input: Update keyboard serial on press and release
Hi all,
Small reminder, I don't think I got any review/reply of these two patches...
Cheers,
Olivier
- Original Message -
> Other compositors such as mutter update the keyboard serial for both key
> press and key release, unlike weston which updates it only on key press.
>
> When dealing with popup windows which require a match in serials, if the
> event that caused the popup to be shown is a key release, then the popup
> would be dismissed.
>
> This occurs when navigating gtk+ sub-menus using the keyboard.
>
> Signed-off-by: Olivier Fourdan
> Bugzilla: https://bugzilla.gnome.org/show_bug.cgi?id=768017
> ---
> libweston/input.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/libweston/input.c b/libweston/input.c
> index 8f46698..18c4c1f 100644
> --- a/libweston/input.c
> +++ b/libweston/input.c
> @@ -1661,9 +1661,8 @@ notify_key(struct weston_seat *seat, uint32_t time,
> uint32_t key,
> state);
> }
>
> + keyboard->grab_serial = wl_display_get_serial(compositor->wl_display);
> if (state == WL_KEYBOARD_KEY_STATE_PRESSED) {
> - keyboard->grab_serial =
> - wl_display_get_serial(compositor->wl_display);
> keyboard->grab_time = time;
> keyboard->grab_key = key;
> }
> --
> 2.7.4
>
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
