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

Wed Jun 22 09:39:51 UTC 2016

On 20.06.2016 16:03, Pekka Paalanen wrote:
> 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.
> 

But, if I remove the toplevel setting here when there is an actual
output, there's a chance set_toplevel will not get ran in at startup
(or at all).

Your suggestion below is valid, but shouldn't this call remain here
too just to make sure that set_toplevel is actually ran at startup
when there's an output?

Current implementation only sets the toplevel once, at startup, on
the output it gets from display_get_output. This patch doesn't change
that behavior - if there's an output at startup, run set_toplevel,
otherwise run it only when an output gets attached, and only on the
first attached output (hence the flag).

This may need some reworking when all outputs get unplugged at runtime,
but this patch is only meant to fix "start with zero outputs" case
at this time.

Thanks both for the input,

Armin.

>>> +}
>>>
>>> -	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: signature.asc
Type: application/pgp-signature
Size: 855 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160622/4c4d6685/attachment.sig>