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

[Pekka Paalanen]

On Wed, 29 Jun 2016 08:07:22 +0000
"Ucan, Emre (ADITG/SW1)" <eucan at de.adit-jv.com> wrote:

> 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.

Alright. And it's just hmi-controller anyway, not the fundamental parts.

> > 
> > 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.

Very good.

Pushed:
   28d240f..37d25bb  master -> master


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160629/9518540a/attachment.sig>