-
Notifications
You must be signed in to change notification settings - Fork 341
Conversation
The test client is at https://code.puri.sm/Librem5/weston/src/virtual_keyboard - please use the
|
rootston/config.c
Outdated
@@ -19,14 +19,15 @@ | |||
|
|||
static void usage(const char *name, int ret) { | |||
fprintf(stderr, | |||
"usage: %s [-C <FILE>] [-E <COMMAND>]\n" | |||
"usage: %s [-C <FILE>] [-E <COMMAND>] [-E <EXEC>]\n" | |||
"\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicat -E, should be -K ?
include/rootston/input_method.h
Outdated
@@ -0,0 +1,11 @@ | |||
//#include <wlr/types/wlr_input_method.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dead code?
rootston/input_method.c
Outdated
static void spawn_im_process() { | ||
*im_pid = fork(); | ||
if (*im_pid == 0) { | ||
sleep(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sleep for what? If a sleep is needed for any reason it shoud likely be polling for something to be not racy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially, I added the sleep there because when spawn_im_process
is first called, the server is not completely initialized, causing an immediate crash. Later on, I realized this would be useful to rate-limit the spawns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-1
Rate-limiting shouldn't be done via sleep
, and rootston shouldn't rate-limit. Child processes should only be spawned when the server is completely initialized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How to wait for full initialization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I say rate-limit, I mean the situation where a faulty process will crash very soon after spawning and get restarted. I think it the infinite spawning loop should be prevented from excessively consuming resources, although not necessarily using sleep.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can either split your setup and call the spawn part directly before going into the main loop, or add an idle event at setup time to start the client from the mainloop.
void *credentials_data; | ||
struct { | ||
struct wl_signal new_virtual_keyboard; // struct wlr_virtual_keyboard* | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other places use
struct {
} events;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I knew something was off here.
struct wlr_seat *seat; | ||
struct { | ||
struct wl_signal destroy; // struct wlr_virtual_keyboard* | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
29ed980
to
e9e537d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments. Looks good to me otherwise.
rootston/virtual_keyboard.c
Outdated
roots_seat_add_device(seat, &keyboard->input_device); | ||
wl_signal_add(&keyboard->destroy, &desktop->virtual_keyboard_destroy); | ||
desktop->virtual_keyboard_destroy.notify = handle_virtual_keyboard_destroy; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why register it when handle_virtual_keyboard_destroy
is a noop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's vestigial from an earlier version - removing.
types/wlr_virtual_keyboard.c
Outdated
return wl_resource_get_user_data(resource); | ||
} | ||
|
||
static void virtual_keyboard_keymap(struct wl_client *client, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style nitpick: Extra space
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was actually a tab :)
e9e537d
to
510a264
Compare
I don't like rootston having special support for spawing the IM and keeping up with its process. I also don't like that the wlroots implementation has a special function to check for authoriziation to the protocol. This isn't how we're going to secure protocols. For now, just let any client use this protocol and yank out all of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks great, just gotta yank out the process management stuff
rootston/input_method.c
Outdated
gid_t gid; | ||
wl_client_get_credentials(client, &pid, &uid, &gid); | ||
return pid == allowed_pid; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically this whole file should go away
rootston/config.c
Outdated
switch (c) { | ||
case 'C': | ||
config->config_path = strdup(optarg); | ||
break; | ||
case 'E': | ||
config->startup_cmd = strdup(optarg); | ||
break; | ||
case 'K': | ||
config->im_cmd = strdup(optarg); | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-1
struct wlr_virtual_keyboard_manager { | ||
struct wl_global *global; | ||
struct wl_resource *resource; | ||
wlr_virtual_keyboard_check_credentials_func_t check_credentials; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-1
I think that's fine, we can carry this as a patch on top of rootston for the librem5 and rather work on the general logic to authorize clients since layer-shell, etc. are also affected. |
510a264
to
a36c234
Compare
types/wlr_virtual_keyboard.c
Outdated
xkb_context_unref(context); | ||
context_fail: | ||
wl_client_post_no_memory(client); | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This return is not needed.
types/wlr_virtual_keyboard.c
Outdated
} | ||
|
||
struct wlr_keyboard* keyboard = calloc(1, sizeof(struct wlr_keyboard)); | ||
if (keyboard == NULL) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be
if (!keyboard) {
for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah, both are fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The style is mixed in the same function though... your call.
a36c234
to
8c64f16
Compare
Yanked and fixed. |
@@ -0,0 +1,3 @@ | |||
#include <wayland-server-core.h> |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This is looking really good. Just had one minor issue, and I still need to test it on my touchscreen device. |
types/wlr_virtual_keyboard.c
Outdated
} | ||
|
||
wl_resource_set_implementation(resource, &manager_impl, manager, NULL); | ||
manager->resource = resource; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If two clients bind to the global, manager->resource
will be overwritten. You should use a list and wl_resource_get_link
instead.
types/wlr_virtual_keyboard.c
Outdated
|
||
static void virtual_keyboard_manager_bind(struct wl_client *client, void *data, | ||
uint32_t version, uint32_t id) { | ||
assert(client); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assert is unnecessary.
types/wlr_virtual_keyboard.c
Outdated
virtual_keyboard->resource = keyboard_resource; | ||
virtual_keyboard->seat = seat_client->seat; | ||
wl_signal_init(&virtual_keyboard->events.destroy); | ||
wlr_signal_emit_safe(&manager->events.new_virtual_keyboard, virtual_keyboard); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have a list of all virtual keyboards in the manager.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let the users iterate on the list and properly cleanup when destroying the manager.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the advantage of letting manager do the cleanup over letting keyboards clean up themselves?
As I understand, there's not much utility in destroying the manager while objects are bound. In addition to that, the manager is acting as a factory rather than a collection, so there's no harm in leaving keyboards alive after destroying it.
In addition to that, having a list of active keyboards in the manager will need introducing another object to remember which keyboards are held by which user, making the whole thing a doubly-nested structure:
- manager global
- manager binds
- keyboards
The way it is now seems just as functional and can stay simple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Giving it a bit more thought, a binds list is not implied by a keyboard list, but I stand by my comment that it's not necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for complicated list. The global, a list of resources and a list of keyboards in the manager is enough.
The list is useful to let compositors inspect it without having to maintain their own list.
types/wlr_virtual_keyboard.c
Outdated
|
||
wlr_input_device_init(&virtual_keyboard->input_device, | ||
WLR_INPUT_DEVICE_KEYBOARD, &input_device_impl, "virtual keyboard", | ||
0xabcd, 0x1234); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, better to use 0 instead of these constants :)
types/wlr_virtual_keyboard.c
Outdated
|
||
static void virtual_keyboard_destroy(struct wl_client *client, | ||
struct wl_resource *resource) { | ||
virtual_keyboard_destroy_resource(resource); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will leak the resource. Also, this allows the client to call destroy
twice and make the compositor crash.
Should just call wl_resource_destroy(resource)
.
How does this work relate to #892? |
}; | ||
|
||
struct wlr_virtual_keyboard_manager *wlr_virtual_keyboard_manager_create( | ||
struct wl_display *display); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The compositor should have a way to destroy the manager here
@@ -0,0 +1,28 @@ | |||
#ifndef WLR_TYPES_WLR_VIRTUAL_KEYBOARD_H | |||
#define WLR_TYPES_WLR_VIRTUAL_KEYBOARD_H |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of the symbols should be include the unstable version too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This protocol is virtual_keyboard_unstable_v1
. All symbols should be chosen so that there's no conflict when a new unstable version is published (e.g. virtual_keyboard_unstable_v2
) or when the stable version is adopted (virtual_keyboard
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the file name need to change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good!
Tested on touch screen 👍 |
8c64f16
to
9196d22
Compare
9196d22
to
220da12
Compare
Added list of manager resources and keyboards, renamed public interface to include |
if (!context) { | ||
goto context_fail; | ||
} | ||
void *data = mmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case of success, when is this unmapped?
}; | ||
|
||
struct wlr_virtual_keyboard_manager_unstable_v1* | ||
wlr_virtual_keyboard_manager_unstable_v1_create( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We currently name unstable protocols without "unstable" in the symbols, just with _v1
.
We should probably change that to add a z
prefix or something, but that's unrelated to this PR.
220da12
to
5334ee8
Compare
Removed "unstable" and fixed unmapping. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks! |
This change is to support the recently proposed
virtual-keyboard-v1
protocol.Because virtual keyboards are privileged, the commits setting up an authorization system - meant to be general for all input methods - are separate.
The main commit provides support in wlroots as well as rootston. The keyboard does not attempt to synchronize its modifier status with the rest of the system, and code for having multiple instances running at the same time has not been extensively tested, however it's rather simple and wasn't failing either.