On Sun, 19 Jun 2016 11:33:11 +0200 Quentin Glidic <[email protected]> wrote:
> On 18/06/2016 19:15, Armin Krezović wrote: > > Currently, the keyboard client is created and the input > > panel surface is set as toplevel on the first output it > > finds. This does not work in a scenario when there are > > no outputs, resulting in weston-keyboard to crash at > > startup due to operating on an invalid output pointer. > > > > This makes input panel toplevel setting depend on a > > valid output, and if there was no output present at > > startup, it will be set toplevel as soon as an output > > gets plugged in. > > > > Signed-off-by: Armin Krezović <[email protected]> > > --- > > The overall idea is good, let’s see the details. > > > > clients/keyboard.c | 33 +++++++++++++++++++++++++++------ > > 1 file changed, 27 insertions(+), 6 deletions(-) > > > > diff --git a/clients/keyboard.c b/clients/keyboard.c > > index d719764..4fb4200 100644 > > --- a/clients/keyboard.c > > +++ b/clients/keyboard.c > > @@ -24,6 +24,7 @@ > > > > #include "config.h" > > > > +#include <stdbool.h> > > #include <stdio.h> > > #include <stdlib.h> > > #include <string.h> > > @@ -56,6 +57,7 @@ struct virtual_keyboard { > > char *surrounding_text; > > uint32_t surrounding_cursor; > > struct keyboard *keyboard; > > + bool toplevel; > > }; > > > > enum key_type { > > @@ -953,11 +955,26 @@ global_handler(struct display *display, uint32_t name, > > } > > > > static void > > +set_toplevel(struct output *output, struct virtual_keyboard > > *virtual_keyboard) > > +{ > > + struct zwp_input_panel_surface_v1 *ips; > > + struct keyboard *keyboard = virtual_keyboard->keyboard; > > + > > + ips = > > zwp_input_panel_v1_get_input_panel_surface(virtual_keyboard->input_panel, > > + > > window_get_wl_surface(keyboard->window)); > > + > > + zwp_input_panel_surface_v1_set_toplevel(ips, > > + output_get_wl_output(output), > > + > > ZWP_INPUT_PANEL_SURFACE_V1_POSITION_CENTER_BOTTOM); > > + > > + virtual_keyboard->toplevel = true; > > +} > > + > > +static void > > keyboard_create(struct output *output, struct virtual_keyboard > > *virtual_keyboard) > > { > > struct keyboard *keyboard; > > const struct layout *layout; > > - struct zwp_input_panel_surface_v1 *ips; > > > > layout = get_current_layout(virtual_keyboard); > > > > @@ -981,13 +998,16 @@ keyboard_create(struct output *output, struct > > virtual_keyboard *virtual_keyboard > > layout->columns * key_width, > > layout->rows * key_height); > > > > + if (output) > > + set_toplevel(output, virtual_keyboard); > > I would drop it completely from here, see below. Hi, actually I think rather than just removing this here, this should be replaced with the call to display_set_output_configure_handler(). Explanation below. > > +} > > > > - ips = > > zwp_input_panel_v1_get_input_panel_surface(virtual_keyboard->input_panel, > > - > > window_get_wl_surface(keyboard->window)); > > +static > > +void display_output_handler(struct output *output, void *data) { > > + struct virtual_keyboard *keyboard = data; > > > > - zwp_input_panel_surface_v1_set_toplevel(ips, > > - output_get_wl_output(output), > > - > > ZWP_INPUT_PANEL_SURFACE_V1_POSITION_CENTER_BOTTOM); > > + if (!keyboard->toplevel) > > + set_toplevel(output, keyboard); > > } > > > > int > > @@ -1006,6 +1026,7 @@ main(int argc, char *argv[]) > > > > display_set_user_data(virtual_keyboard.display, &virtual_keyboard); > > display_set_global_handler(virtual_keyboard.display, global_handler); > > + display_set_output_configure_handler(virtual_keyboard.display, > > display_output_handler); > > Is that handler called only on hotplug or is it called at startup too? > IOW, could we rely on it to be called at least one? We can rely on it being called if there is an output. It will also get called whenever any output sends a 'mode' event with WL_OUTPUT_MODE_CURRENT, which means it will be called for permanent mode switches too (which don't usually happen with Weston). However, it may even get called straight from inside display_set_output_configure_handler() if we already have an output with complete information. If that happens, set_toplevel() will crash because keyboard_create() has not ran yet. > > > > if (virtual_keyboard.input_panel == NULL) { > > fprintf(stderr, "No input panel global\n"); > > > > > Later in this file, there is: > output = display_get_output(virtual_keyboard.display); > keyboard_create(output, &virtual_keyboard); > > Could be replaced by: > keyboard_create(&virtual_keyboard); > output = display_get_output(virtual_keyboard.display); > if (output) > set_toplevel(output, keyboard); > > Or keyboard_create only if the handler is assured to be called at least > once. > > Anyone has comments? :-) Referring to my suggestion above, removing the output argument from keyboard_create() would be good, and we don't even need the set_toplevel() call here at all. Anyway, this patch needs more work because of the possibility for the crash. This patch does not handle the last output being hot-unplugged, but it's not meant to either. I suspect handling that case - or more precisely, an output coming back after that - may need some more. weston-keyboard would probably have to re-set the output designation for the input panel surface, and the shells need to cope with that too. Thanks, pq
pgpeIJpVitv5Y.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
