[PATCH weston 1/6] ivi-shell: register ivi_layout_interface

[Pekka Paalanen]

On Thu, 25 Jan 2018 12:56:37 +0000
"Ucan, Emre (ADITG/ESB)" <eucan at de.adit-jv.com> wrote:

> > -----Original Message-----
> > From: Pekka Paalanen [mailto:ppaalanen at gmail.com]
> > Sent: Montag, 22. Januar 2018 13:28
> > To: Ucan, Emre (ADITG/ESB)
> > Cc: wayland-devel at lists.freedesktop.org
> > Subject: Re: [PATCH weston 1/6] ivi-shell: register ivi_layout_interface
> > 
> > On Wed, 3 Jan 2018 16:09:16 +0100
> > Emre Ucan <eucan at de.adit-jv.com> wrote:
> >   
> > > controller modules can be loaded from the main function of weston.
> > > They will get the ivi_layout_interface via weston plugin registry.
> > >
> > > Signed-off-by: Emre Ucan <eucan at de.adit-jv.com>
> > > ---
> > >  ivi-shell/ivi-layout-export.h | 13 +++++++++++++
> > >  ivi-shell/ivi-layout.c        |  6 ++++++
> > >  ivi-shell/ivi-shell.c         |  2 --
> > >  3 files changed, 19 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/ivi-shell/ivi-layout-export.h b/ivi-shell/ivi-layout-export.h
> > > index 277ac59..c15c7f8 100644
> > > --- a/ivi-shell/ivi-layout-export.h
> > > +++ b/ivi-shell/ivi-layout-export.h
> > > @@ -59,6 +59,7 @@ extern "C" {
> > >
> > >  #include "stdbool.h"
> > >  #include "compositor.h"
> > > +#include "plugin-registry.h"
> > >
> > >  #define IVI_SUCCEEDED (0)
> > >  #define IVI_FAILED (-1)
> > > @@ -140,6 +141,8 @@ enum ivi_layout_transition_type{
> > >  	IVI_LAYOUT_TRANSITION_MAX,
> > >  };
> > >
> > > +#define IVI_LAYOUT_API_NAME "ivi_layout_api_v1"
> > > +
> > >  struct ivi_layout_interface {
> > >
> > >  	/**
> > > @@ -572,6 +575,16 @@ struct ivi_layout_interface {
> > >  				       struct ivi_layout_layer *removelayer);
> > >  };
> > >
> > > +static inline const struct ivi_layout_interface *
> > > +ivi_layout_get_api(struct weston_compositor *compositor)
> > > +{
> > > +	const void *api;
> > > +	api = weston_plugin_api_get(compositor, IVI_LAYOUT_API_NAME,
> > > +				    sizeof(struct ivi_layout_interface));
> > > +
> > > +	return (const struct ivi_layout_interface *)api;
> > > +}
> > > +
> > >  #ifdef __cplusplus
> > >  }
> > >  #endif /* __cplusplus */
> > > diff --git a/ivi-shell/ivi-layout.c b/ivi-shell/ivi-layout.c
> > > index 394179b..a11b658 100644
> > > --- a/ivi-shell/ivi-layout.c
> > > +++ b/ivi-shell/ivi-layout.c
> > > @@ -1906,6 +1906,8 @@ ivi_layout_surface_create(struct weston_surface  
> > *wl_surface,  
> > >  	return ivisurf;
> > >  }
> > >
> > > +static struct ivi_layout_interface ivi_layout_interface;
> > > +
> > >  void
> > >  ivi_layout_init_with_compositor(struct weston_compositor *ec)
> > >  {
> > > @@ -1934,6 +1936,10 @@ ivi_layout_init_with_compositor(struct  
> > weston_compositor *ec)  
> > >
> > >  	layout->transitions = ivi_layout_transition_set_create(ec);
> > >  	wl_list_init(&layout->pending_transition_list);
> > > +
> > > +	weston_plugin_api_register(ec, IVI_LAYOUT_API_NAME,
> > > +				   &ivi_layout_interface,
> > > +				   sizeof(struct ivi_layout_interface));
> > >  }
> > >
> > >  static struct ivi_layout_interface ivi_layout_interface = {
> > > diff --git a/ivi-shell/ivi-shell.c b/ivi-shell/ivi-shell.c
> > > index 766a1fd..c096954 100644
> > > --- a/ivi-shell/ivi-shell.c
> > > +++ b/ivi-shell/ivi-shell.c
> > > @@ -425,8 +425,6 @@ ivi_shell_setting_create(struct ivi_shell_setting  
> > *dest,  
> > >  	if (!dest->ivi_module &&
> > >  	    weston_config_section_get_string(section, "ivi-module",
> > >  					     &dest->ivi_module, NULL) < 0) {
> > > -		weston_log("Error: ivi-shell: No ivi-module set\n");
> > > -		result = -1;  
> > 
> > I see a later patch removes this if-statement completely, but here the
> > resulting code looks really strange, like some mistake in patch
> > handling.
> > 
> > I wonder if this change even belongs in this patch, otherwise it's a
> > nice single logical step.
> > 
> > Acked-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>  
> 
> This change has to be here. Otherwise it is not possible to start
> ivi-shell without setting ivi-module. The third patch requires this
> change.

Sure, the third patch requires it. The question is, which patch this
hunk belongs in? If it needs to be here, why do you leave a useless
if-statement around?

	if (...) {
	}

looks like a mistake in the code. If you delete everything inside the
block, might as well delete the whole block to not leave dead code
around.


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