On Thu, 5 May 2016 22:45:43 +0200 Benoit Gschwind <gschw...@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 <gschw...@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
pgpuWzH60CZ7S.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
