[PATCH weston 1/2] text backend should handle existing seats on init.

[Pekka Paalanen]

On Tue,  9 Jun 2015 20:24:26 +0000
Murray Calavera <murray.calavera at gmail.com> wrote:

> Currently the text backend will crash the compositor if
> seats have already been created.

Hi,

crash? I didn't realize I should've been looking for causes of crashes,
I only thought the input method simply didn't work on the initial
seats. Crashing sounds like there might be more to fix.

What was the crash exactly?

A more to the point patch summary would be "text: handle existing seats
on init". Prefix to show the component(s) affected, and a short
statement on what is done here.

The commit message body could explain better why this is needed: a
following patch will be moving text init call into shell modules, which
will be called much later than in current code.

> Signed-off-by: Murray Calavera <murray.calavera at gmail.com>
> ---
>  src/text-backend.c | 29 +++++++++++++++++++----------
>  1 file changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/src/text-backend.c b/src/text-backend.c
> index daae03c..01cb70a 100644
> --- a/src/text-backend.c
> +++ b/src/text-backend.c
> @@ -933,13 +933,9 @@ launch_input_method(struct text_backend *text_backend)
>  }
>  
>  static void
> -handle_seat_created(struct wl_listener *listener,
> -		    void *data)
> +handle_seat_created(struct weston_seat *seat,
> +		    struct text_backend *text_backend)
>  {

I think a better name for this would now be
	text_backend_seat_created(struct text_backend *text_backend,
				  struct weston_seat *seat)

It would go more in line with other text_backend methods (OO-style a
bit), and handle_*() functions are usually directly plugged into
callbacks.

> -	struct weston_seat *seat = data;
> -	struct text_backend *text_backend =
> -		container_of(listener, struct text_backend,
> -			     seat_created_listener);
>  	struct input_method *input_method;
>  	struct weston_compositor *ec = seat->compositor;
>  
> @@ -966,6 +962,17 @@ handle_seat_created(struct wl_listener *listener,
>  }
>  
>  static void
> +notify_seat_created(struct wl_listener *listener, void *data)
> +{

Then this would be called handle_seat_created.

> +	struct weston_seat *seat = data;
> +	struct text_backend *text_backend =
> +		container_of(listener, struct text_backend,
> +			     seat_created_listener);
> +
> +	handle_seat_created(seat, text_backend);
> +}
> +
> +static void
>  text_backend_configuration(struct text_backend *text_backend)
>  {
>  	struct weston_config_section *section;
> @@ -998,11 +1005,11 @@ text_backend_notifier_destroy(struct wl_listener *listener, void *data)
>  	free(text_backend);
>  }
>  
> -
>  WL_EXPORT int
>  text_backend_init(struct weston_compositor *ec)
>  {
>  	struct text_backend *text_backend;
> +	struct weston_seat *seat;
>  
>  	text_backend = zalloc(sizeof(*text_backend));
>  	if (text_backend == NULL)
> @@ -1010,15 +1017,17 @@ text_backend_init(struct weston_compositor *ec)
>  
>  	text_backend->compositor = ec;
>  
> -	text_backend->seat_created_listener.notify = handle_seat_created;
> +	text_backend_configuration(text_backend);
> +
> +	wl_list_for_each(seat, &ec->seat_list, link)
> +		handle_seat_created(seat, text_backend);
> +	text_backend->seat_created_listener.notify = notify_seat_created;
>  	wl_signal_add(&ec->seat_created_signal,
>  		      &text_backend->seat_created_listener);
>  
>  	text_backend->destroy_listener.notify = text_backend_notifier_destroy;
>  	wl_signal_add(&ec->destroy_signal, &text_backend->destroy_listener);
>  
> -	text_backend_configuration(text_backend);
> -
>  	text_input_manager_create(ec);
>  
>  	return 0;

The patch looks good to me. As is, it is:
Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>

in any case, though I think it would be nice to do the minor
adjustments I suggested.


Thanks,
pq