[PATCH weston 06/11] keyboard: Only set toplevel when there is a valid output

[Pekka Paalanen]

On Sun, 19 Jun 2016 11:33:11 +0200
Quentin Glidic <sardemff7+wayland at sardemff7.net> 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ć <krezovic.armin at gmail.com>
> > ---  
> 
> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160620/2c94ab62/attachment.sig>