Skip to content
This repository was archived by the owner on Nov 1, 2021. It is now read-only.

tinywl: fix crash if there is no keyboard#3152

Open
ifreund wants to merge 1 commit into
swaywm:masterfrom
ifreund:tinywl-fix
Open

tinywl: fix crash if there is no keyboard#3152
ifreund wants to merge 1 commit into
swaywm:masterfrom
ifreund:tinywl-fix

Conversation

@ifreund

@ifreund ifreund commented Aug 31, 2021

Copy link
Copy Markdown
Member

To reproduce, run

WLR_BACKENDS=headles tinywl -s foot

@emersion

Copy link
Copy Markdown
Member

Do we need to give keyboard focus and activate the toplevel in any particular order? Do clients care about it?

@ifreund

ifreund commented Aug 31, 2021

Copy link
Copy Markdown
Member Author

Do we need to give keyboard focus and activate the toplevel in any particular order? Do clients care about it?

I don't think it matters:

      <entry name="activated" value="4" summary="the surface is now activated">
	<description summary="the surface is now activated">
	  Client window decorations should be painted as if the window is
	  active. Do not assume this means that the window actually has
	  keyboard or pointer focus.
	</description>
      </entry>

@ifreund

ifreund commented Aug 31, 2021

Copy link
Copy Markdown
Member Author

Though I guess it would make more sense to do absolutely nothing in this function if there is no keyboard.

@emersion

Copy link
Copy Markdown
Member

Hm, but the user could still want to focus a toplevel with the pointer, even if there is no keyboard?

@ifreund

ifreund commented Aug 31, 2021

Copy link
Copy Markdown
Member Author

Hm, but the user could still want to focus a toplevel with the pointer, even if there is no keyboard?

Yeah, that better matches what sway and river at least actually do as well. Reverted the latest force push.

To reproduce, run

WLR_BACKENDS=headles tinywl -s foot
@ifreund

ifreund commented Aug 31, 2021

Copy link
Copy Markdown
Member Author

Now we still send the enter even if there is no actual keyboard, but with no keys or modifiers pressed.

Comment thread tinywl/tinywl.c
Comment on lines +127 to +133
struct wlr_keyboard *keyboard = wlr_seat_get_keyboard(seat);
if (keyboard != NULL) {
wlr_seat_keyboard_notify_enter(seat, view->xdg_surface->surface,
keyboard->keycodes, keyboard->num_keycodes, &keyboard->modifiers);
} else {
wlr_seat_keyboard_notify_enter(seat, surface, NULL, 0, NULL);
}

@ifreund ifreund Aug 31, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This kinda makes me wonder why wlroots makes compositors do this check themselves, instead wlr_seat_keyboard_notify_enter() could just take a possibly NULL pointer to a wlr_keyboard. Or even just call wlr_seat_get_keyboard() itself internally.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, yes, that's a good point. At some point we wanted to get rid of wlr_seat_set_keyboard and de-couple wlr_keyboard from wlr_seat (just like wlr_cursor/wlr_pointer is decoupled from wlr_seat), but stopped halfway through. Ref #803

Comment thread tinywl/tinywl.c
wlr_seat_keyboard_notify_enter(seat, view->xdg_surface->surface,
keyboard->keycodes, keyboard->num_keycodes, &keyboard->modifiers);
} else {
wlr_seat_keyboard_notify_enter(seat, surface, NULL, 0, NULL);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I wonder if it really makes sense to do this…?

Sway may do it because of data device related things (copy/paste relies on keyboard focus).

@emersion

emersion commented Nov 1, 2021

Copy link
Copy Markdown
Member

wlroots has migrated to gitlab.freedesktop.org. This pull request has been moved to:

https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/3152

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants