[PATCH weston v3 05/17] compositor-wayland: move configuration parsing to main.c

[Pekka Paalanen]

On Thu,  5 May 2016 22:45:43 +0200
Benoit Gschwind <gschwind at gnu-log.net> wrote:

> Move function load_wayland_backend_config,
> wayland_backend_config_add_new_output, wayland_backend_config_release,
> wayland_output_config_init from compositor-wayland.c to main.c.
> 
> Rename load_wayland_backend_config to load_wayland_backend ad update the
> signature to match other backend load functions. Update the code to
> properly load the backend as others backends.
> 
> Signed-off-by: Benoit Gschwind <gschwind at gnu-log.net>
> ---
>  src/compositor-wayland.c | 215 ---------------------------------------------
>  src/main.c               | 223 ++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 221 insertions(+), 217 deletions(-)

Hi Benoit,

ok, the same diff excercise again:

> --- old-compositor-wayland.c	2016-05-06 16:44:20.817663501 +0300
> +++ new-main.c	2016-05-06 16:45:15.330216061 +0300
> @@ -1,6 +1,10 @@
>  /*
> + * Copyright © 2010-2011 Intel Corporation
> + * Copyright © 2008-2011 Kristian Høgsberg
> + * Copyright © 2012-2015 Collabora, Ltd.
>   * Copyright © 2010-2011 Benjamin Franzke
>   * Copyright © 2013 Jason Ekstrand
> + * Copyright © 2016 Benoit Gschwind
>   *
>   * Permission is hereby granted, free of charge, to any person obtaining
>   * a copy of this software and associated documentation files (the
> @@ -81,8 +85,6 @@
>  
>  }
>  
> -
> -
>  static void
>  wayland_backend_config_release(struct weston_wayland_backend_config *new_config) {
>  	int i;
> @@ -114,9 +116,8 @@
>  }
>  
>  static int
> -load_wayland_backend_config(struct weston_compositor *compositor, int *argc, char *argv[],
> -	     struct weston_config *config,
> -	     struct weston_wayland_backend_config *out_config)
> +load_wayland_backend(struct weston_compositor *compositor, char const * backend,
> +		     int *argc, char *argv[], struct weston_config *config)
>  {
>  	struct weston_wayland_backend_config new_config = {{ 0, }};
>  	struct weston_config_section *section;
> @@ -124,6 +125,7 @@
>  	int count, width, height, scale;
>  	const char *section_name;
>  	char *name;
> +	int ret = 0;
>  
>  	const struct weston_option wayland_options[] = {
>  		{ WESTON_OPTION_INTEGER, "width", 0, &width },
> @@ -160,14 +162,17 @@
>  
>  	if (new_config.sprawl) {
>  		/* do nothing, everything is already set */
> -		*out_config = new_config;
> -		return 0;
> +		ret = load_backend_new(compositor, backend, &new_config.base);
> +		wayland_backend_config_release(&new_config);
> +		return ret;
>  	}
>  
>  	if (new_config.fullscreen) {
>  		oc = wayland_backend_config_add_new_output(&new_config);
> -		if (!oc)
> +		if (!oc) {
> +			ret = -1;
>  			goto err_outputs;
> +		}
>  
>  		oc->width = width;
>  		oc->height = height;
> @@ -175,8 +180,9 @@
>  		oc->transform = WL_OUTPUT_TRANSFORM_NORMAL;
>  		oc->scale = 1;
>  
> -		*out_config = new_config;
> -		return 0;
> +		ret = load_backend_new(compositor, backend, &new_config.base);
> +		wayland_backend_config_release(&new_config);
> +		return ret;
>  	}
>  
>  	section = NULL;
> @@ -195,8 +201,10 @@
>  
>  		oc = wayland_backend_config_add_new_output(&new_config);
>  
> -		if (!oc)
> +		if (!oc) {
> +			ret = -1;
>  			goto err_outputs;
> +		}
>  
>  		wayland_output_config_init(oc, section, width,
>  						height, scale);
> @@ -213,8 +221,10 @@
>  
>  		oc = wayland_backend_config_add_new_output(&new_config);
>  
> -		if (!oc)
> +		if (!oc) {
> +			ret = -1;
>  			goto err_outputs;
> +		}
>  
>  		oc->width = width;
>  		oc->height = height;
> @@ -225,11 +235,12 @@
>  		--count;
>  	}
>  
> -	*out_config = new_config;
> -	return 0;
> +	ret = load_backend_new(compositor, backend, &new_config.base);
> +	wayland_backend_config_release(&new_config);
> +	return ret;
>  
>  err_outputs:
>  	wayland_backend_config_release(&new_config);
> -	return -1;
> +	return ret;
>  }
 

Much smaller diff, much better.

I do still wonder if it is necessary to do the rename from
load_wayland_backend_config to load_wayland_backend in this patch. You
could get an even smaller diff, reducing the chances that anyone needs
to review this patch again during a bug hunt.

You are adding three copies of the same two lines for
load_backend_new() and config_release(). These could be in the caller
just once, that is, a new function load_wayland_backend().


Then the actual patch as you sent it:

> diff --git a/src/compositor-wayland.c b/src/compositor-wayland.c
> index 53f855d..fe8b082 100644
> --- a/src/compositor-wayland.c
> +++ b/src/compositor-wayland.c

> diff --git a/src/main.c b/src/main.c
> index 2a56555..92b7ac2 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -2,6 +2,9 @@
>   * Copyright © 2010-2011 Intel Corporation
>   * Copyright © 2008-2011 Kristian Høgsberg
>   * Copyright © 2012-2015 Collabora, Ltd.
> + * Copyright © 2010-2011 Benjamin Franzke
> + * Copyright © 2013 Jason Ekstrand
> + * Copyright © 2016 Benoit Gschwind

This patch is moving code, so it cannot add new copyrights (your name).
Your name was not present in compositor-wayland.c, it should have been
added there by another patch.

This looks like you are trying to sneak in an additional copyright. ;-)

>   *
>   * Permission is hereby granted, free of charge, to any person obtaining
>   * a copy of this software and associated documentation files (the
> @@ -51,6 +54,9 @@
>  #include "compositor-rdp.h"
>  #include "compositor-fbdev.h"
>  #include "compositor-x11.h"
> +#include "compositor-wayland.h"
> +
> +#define WINDOW_TITLE "Weston Compositor"
>  


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/20160506/91f6d1ff/attachment.sig>