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 <[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. >> > > 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
>>
>
signature.asc
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
