[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