[PATCH weston] ivi-shell: add surface_created listener after launchers

[Ucan, Emre (ADITG/SW1)]

Hi Pekka,

My comments are below

Best regards

Emre Ucan
Software Group I (ADITG/SW1)

Tel. +49 5121 49 6937

> -----Original Message-----
> From: Pekka Paalanen [mailto:ppaalanen at gmail.com]
> Sent: Mittwoch, 29. Juni 2016 09:48
> To: Ucan, Emre (ADITG/SW1)
> Cc: wayland-devel at lists.freedesktop.org; Natsume, Wataru (ADITJ/SWG)
> Subject: Re: [PATCH weston] ivi-shell: add surface_created listener after
> launchers
> 
> On Fri, 17 Jun 2016 13:50:16 +0000
> "Ucan, Emre (ADITG/SW1)" <eucan at de.adit-jv.com> wrote:
> 
> > Add surface_created listener after the initialization of launchers.
> > Otherwise, surfaces of the launchers will be added to the application
> > layer too.
> >
> > Signed-off-by: Emre Ucan <eucan at de.adit-jv.com>
> > ---
> >  ivi-shell/hmi-controller.c |   10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/ivi-shell/hmi-controller.c b/ivi-shell/hmi-controller.c
> > index 77278ee..df99183 100644
> > --- a/ivi-shell/hmi-controller.c
> > +++ b/ivi-shell/hmi-controller.c
> > @@ -845,9 +845,6 @@ hmi_controller_create(struct weston_compositor
> *ec)
> >  	wl_list_insert(&hmi_ctrl->workspace_fade.layer_list,
> >  		       &tmp_link_layer->link);
> >
> > -	hmi_ctrl->surface_created.notify = set_notification_create_surface;
> > -	ivi_layout_interface->add_listener_create_surface(&hmi_ctrl-
> >surface_created);
> > -
> >  	hmi_ctrl->surface_removed.notify =
> set_notification_remove_surface;
> >  	ivi_layout_interface->add_listener_remove_surface(&hmi_ctrl-
> >surface_removed);
> >
> > @@ -1277,6 +1274,13 @@ ivi_hmi_controller_UI_ready(struct wl_client
> *client,
> >  	ivi_layout_interface->commit_changes();
> >
> >  	ivi_hmi_controller_add_launchers(hmi_ctrl, 256);
> > +
> > +	/* Add surface_created listener after the initialization of launchers.
> > +	 * Otherwise, surfaces of the launchers will be added to application
> > +	 * layer too.*/
> > +	hmi_ctrl->surface_created.notify = set_notification_create_surface;
> > +	ivi_layout_interface->add_listener_create_surface(&hmi_ctrl-
> >surface_created);
> > +
> >  	hmi_ctrl->is_initialized = 1;
> >  }
> >
> 
> Hi Emre,
> 
> this does fix the issue, but I'm not sure it is appropriate.
> 
> Would this not cause clients that set up surfaces before the UI client
> signals ready to be missed? That would be a race in start-up that could
> be rarely lost, so also hard to debug if it actually happened.

I am not sure if such a race-condition is possible. Because weston-ivi-shell-user-interface
is launched always as weston startup. It is a helper client. The helper client then create all UI surfaces
and sends the UI ready request to the compositor. It is not possible to start another wayland application with launchers, before UI is ready.
If someone tries to start a wayland application on parallel with weston startup. It can always end up badly,
if some kind of a synchronization mechanism is not used, e.g: sd-notify. Therefore, I think it is ok.

> 
> Is is_surf_in_ui_widget() not supposed to detect those surfaces so that
> set_notification_create_surface() and others will skip them?
> 
> Particularly it looks like set_notification_configure_surface() might be
> able to mess up launchers also later.
> 
> Actually, I suspect set_notification_create_surface() triggers before
> the ui widget surfaces get recorded as ui widgets in e.g.
> ivi_hmi_controller_add_launchers() et al. so you'd need to create the
> ui widget list before any clients connect. That should be possible,
> since you get all the ivi_ids from the config already. But it's also
> quite some work, I suppose.
> 
> If you are ok with the caveats, I can add some notes in the commit
> message and just land this patch and the two pending ones. Would that
> be good?

I am ok with that.

> 
> 
> Thanks,
> pq