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

Armin Krezović krezovic.armin at gmail.com
Wed Jun 22 09:46:55 UTC 2016


On 22.06.2016 11:39, Armin Krezović wrote:
> 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).
> 

Ah, now I see. display_output_handler will be ran at startup too when
it receives the first output.

> 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/65995d6f/attachment.sig>


More information about the wayland-devel mailing list