On Thu, 28 Apr 2016 20:33:16 +0200 Benoit Gschwind <gschw...@gnu-log.net> wrote:
> Move all parsing functions from the wayland backend and put
> them into the weston code and add versionning to configuration
> structure.
Hi Benoit,
the commit message already reveals that this should be at least two
separate patches: "move" and "add versioning". It also forgets to
mention that the loading of wayland backend migrates to the new style
for libweston, which is the whole point of the series.
Since this patch is moving a lot of code from one file to another, it
is more interesting to look at the diff between the old and the new
files:
> --- old-compositor-wayland.c 2016-05-04 16:12:25.853977842 +0300
> +++ new-main.c 2016-05-04 16:12:19.702030189 +0300
> @@ -1,6 +1,7 @@
> /*
> - * Copyright © 2010-2011 Benjamin Franzke
> - * Copyright © 2013 Jason Ekstrand
> + * Copyright © 2010-2011 Intel Corporation
> + * Copyright © 2008-2011 Kristian Høgsberg
> + * Copyright © 2012-2015 Collabora, Ltd.
Hmm, there is no overlap in copyrights. To be completely sure, we'd
need to copy them all, if we don't dig the git history to find out who
wrote what.
Btw. you should probably add your own copyright in main.c, there is
quite some code written by you, right? That can be done on a patch
(preferably it would have happened on the first such patch) where you
add a non-trivial amount of code.
> *
> * Permission is hereby granted, free of charge, to any person obtaining
> * a copy of this software and associated documentation files (the
> @@ -28,15 +29,40 @@
>
>
>
> -#define WINDOW_TITLE "Weston Compositor"
>
> +static void
> +weston_wayland_backend_config_release(struct weston_wayland_backend_config
> *new_config) {
> + int i;
> + for (i = 0; i < new_config->num_outputs; ++i) {
> + free(new_config->outputs[i].name);
> + }
> + free(new_config->cursor_theme);
> + free(new_config->display_name);
> + free(new_config->outputs);
> +}
Ok, this function is just misplaced. It would be much better to not
reorder code when moving, so that the difference is easier to review.
There is a hunk below removing an identical piece of code.
>
> +/*
> + * Append a new output struct at the end of new_config.outputs and return a
> + * pointer to the newly allocated structure or NULL if fail. The allocated
> + * structure is NOT cleared nor set to default values.
> + */
> +static struct weston_wayland_backend_output_config *
> +weston_wayland_backend_config_add_new_output(struct
> weston_wayland_backend_config *new_config) {
> + struct weston_wayland_backend_output_config *outputs;
> + outputs = realloc(new_config->outputs,
> + (new_config->num_outputs+1)*sizeof(struct
> weston_wayland_backend_output_config));
> + if (!outputs)
> + return NULL;
> + new_config->num_outputs += 1;
> + new_config->outputs = outputs;
> + return &(new_config->outputs[new_config->num_outputs-1]);
> +}
For this one, too. A separate patch should fix all the whitespace and
long line issues here.
>
> static void
> -wayland_output_init_from_config(struct weston_wayland_backend_output_config
> *output,
> - struct weston_config_section *config_section,
> - int option_width, int option_height,
> - int option_scale)
> +weston_wayland_backend_output_init(struct
> weston_wayland_backend_output_config *output,
> + struct weston_config_section *config_section,
> + int option_width, int option_height,
> + int option_scale)
I asked for renaming this better in earlier review comments, the rename
should be a separate patch.
> {
> char *mode, *t, *str;
> unsigned int slen;
> @@ -85,87 +111,60 @@
>
> }
>
> -
> -
> -static void
> -wayland_backend_config_release(struct weston_wayland_backend_config
> *new_config) {
> - int i;
> - for (i = 0; i < new_config->num_outputs; ++i) {
> - free(new_config->outputs[i].name);
> - }
> - free(new_config->cursor_theme);
> - free(new_config->display_name);
> - free(new_config->outputs);
> -}
> -
> -/*
> - * Append a new output struct at the end of new_config.outputs and return a
> - * pointer to the newly allocated structure or NULL if fail. The allocated
> - * structure is NOT cleared nor set to default values.
> - */
> -static struct weston_wayland_backend_output_config *
> -wayland_backend_config_add_new_output(struct weston_wayland_backend_config
> *new_config) {
> - struct weston_wayland_backend_output_config *outputs;
> - outputs = realloc(new_config->outputs,
> - (new_config->num_outputs+1)*sizeof(struct
> weston_wayland_backend_output_config));
> - if (!outputs)
> - return NULL;
> - new_config->num_outputs += 1;
> - new_config->outputs = outputs;
> - return &(new_config->outputs[new_config->num_outputs-1]);
> -}
The two functions above were just misplaced.
> -
> 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 *c, char const *backend,
> + int *argc, char *argv[], struct weston_config *wc)
> {
> - struct weston_wayland_backend_config new_config = { 0, };
> + struct weston_wayland_backend_config config = {{ 0, }};
Variable renames would be good to separate into another patch, so that
they are easier to review. Even in this diff they are just mixed with
all kinds of changes, and in the actual patch it was practically
impossible to see this change.
> struct weston_config_section *section;
> struct weston_wayland_backend_output_config *oc;
> 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 },
> { WESTON_OPTION_INTEGER, "height", 0, &height },
> { WESTON_OPTION_INTEGER, "scale", 0, &scale },
> - { WESTON_OPTION_STRING, "display", 0, &new_config.display_name
> },
> - { WESTON_OPTION_BOOLEAN, "use-pixman", 0,
> &new_config.use_pixman },
> + { WESTON_OPTION_STRING, "display", 0, &config.display_name },
> + { WESTON_OPTION_BOOLEAN, "use-pixman", 0, &config.use_pixman },
> { WESTON_OPTION_INTEGER, "output-count", 0, &count },
> - { WESTON_OPTION_BOOLEAN, "fullscreen", 0,
> &new_config.fullscreen },
> - { WESTON_OPTION_BOOLEAN, "sprawl", 0, &new_config.sprawl },
> + { WESTON_OPTION_BOOLEAN, "fullscreen", 0, &config.fullscreen },
> + { WESTON_OPTION_BOOLEAN, "sprawl", 0, &config.sprawl },
> };
>
> width = 0;
> height = 0;
> scale = 0;
> - new_config.display_name = NULL;
> - new_config.use_pixman = 0;
> + config.display_name = NULL;
> + config.use_pixman = 0;
> count = 1;
> - new_config.fullscreen = 0;
> - new_config.sprawl = 0;
> + config.fullscreen = 0;
> + config.sprawl = 0;
> parse_options(wayland_options,
> ARRAY_LENGTH(wayland_options), argc, argv);
>
> - new_config.cursor_size = 32;
> - new_config.cursor_theme = NULL;
> + config.base.struct_version = WESTON_WAYLAND_BACKEND_CONFIG_VERSION;
> + config.base.struct_size = sizeof(struct weston_wayland_backend_config);
Adding these does not belong to a patch that just moves code.
> + config.cursor_size = 32;
> + config.cursor_theme = NULL;
>
> - section = weston_config_get_section(config, "shell", NULL, NULL);
> + section = weston_config_get_section(wc, "shell", NULL, NULL);
> weston_config_section_get_string(section, "cursor-theme",
> - &new_config.cursor_theme, NULL);
> + &config.cursor_theme, NULL);
> weston_config_section_get_int(section, "cursor-size",
> - &new_config.cursor_size, 32);
> + &config.cursor_size, 32);
>
> - if (new_config.sprawl) {
> + if (config.sprawl) {
> /* do nothing, everything is already set */
> - *out_config = new_config;
> - return 0;
> + ret = load_backend_new(c, backend, &config.base);
> + weston_wayland_backend_config_release(&config);
> + return ret;
That is quite unusual to have several alternative places calling
load_backend_new(). Maybe this function should not actually load the
backend, but just create the config.
This is not simple movement of code, either.
> }
>
> - if (new_config.fullscreen) {
> - oc = wayland_backend_config_add_new_output(&new_config);
> + if (config.fullscreen) {
> + oc = weston_wayland_backend_config_add_new_output(&config);
Function renames should be done before or after, not during the move.
> if (!oc)
> goto err_outputs;
>
> @@ -175,12 +174,13 @@
> oc->transform = WL_OUTPUT_TRANSFORM_NORMAL;
> oc->scale = 1;
>
> - *out_config = new_config;
> - return 0;
> + ret = load_backend_new(c, backend, &config.base);
> + weston_wayland_backend_config_release(&config);
> + return ret;
The same comments as for the load_backend_new() above.
> }
>
> section = NULL;
> - while (weston_config_next_section(config, §ion, §ion_name)) {
> + while (weston_config_next_section(wc, §ion, §ion_name)) {
Variable renames may drown other subtle changes and hide interesting
things from a reviewer.
> if (!section_name || strcmp(section_name, "output") != 0)
> continue;
> weston_config_section_get_string(section, "name", &name, NULL);
> @@ -193,12 +193,12 @@
> }
> free(name);
>
> - oc = wayland_backend_config_add_new_output(&new_config);
> + oc = weston_wayland_backend_config_add_new_output(&config);
>
> if (!oc)
> goto err_outputs;
>
> - wayland_output_init_from_config(oc, section, width,
> + weston_wayland_backend_output_init(oc, section, width,
> height, scale);
> --count;
> }
> @@ -211,7 +211,7 @@
> scale = 1;
> while (count > 0) {
>
> - oc = wayland_backend_config_add_new_output(&new_config);
> + oc = weston_wayland_backend_config_add_new_output(&config);
>
> if (!oc)
> goto err_outputs;
> @@ -225,11 +225,12 @@
> --count;
> }
>
> - *out_config = new_config;
> - return 0;
> + ret = load_backend_new(c, backend, &config.base);
> + weston_wayland_backend_config_release(&config);
> + return ret;
>
> err_outputs:
> - wayland_backend_config_release(&new_config);
> - return -1;
> + weston_wayland_backend_config_release(&config);
> + return ret;
> }
>
Ok, that was fairly easy to check through, but I did have to do extra
work to extract that diff. Compare that to the below:
>
> Signed-off-by: Benoit Gschwind <gschw...@gnu-log.net>
> ---
> src/compositor-wayland.c | 232
> ++++-------------------------------------------
> src/compositor-wayland.h | 5 +
> src/main.c | 211 +++++++++++++++++++++++++++++++++++++++++-
> 3 files changed, 232 insertions(+), 216 deletions(-)
>
> diff --git a/src/compositor-wayland.c b/src/compositor-wayland.c
> index 142cb3a..6083bf3 100644
> --- a/src/compositor-wayland.c
> +++ b/src/compositor-wayland.c
> @@ -51,8 +51,6 @@
> #include "presentation-time-server-protocol.h"
> #include "linux-dmabuf.h"
>
> -#define WINDOW_TITLE "Weston Compositor"
> -
> struct wayland_backend {
> struct weston_backend base;
> struct weston_compositor *compositor;
> @@ -1091,59 +1089,6 @@ err_name:
> return NULL;
> }
>
> -static void
> -wayland_output_init_from_config(struct weston_wayland_backend_output_config
> *output,
> - struct weston_config_section *config_section,
> - int option_width, int option_height,
> - int option_scale)
> -{
> - char *mode, *t, *str;
> - unsigned int slen;
> -
> - weston_config_section_get_string(config_section, "name", &output->name,
> - NULL);
> - if (output->name) {
> - slen = strlen(output->name);
> - slen += strlen(WINDOW_TITLE " - ");
> - str = malloc(slen + 1);
> - if (str)
> - snprintf(str, slen + 1, WINDOW_TITLE " - %s",
> - output->name);
> - free(output->name);
> - output->name = str;
> - }
> - if (!output->name)
> - output->name = strdup(WINDOW_TITLE);
> -
> - weston_config_section_get_string(config_section,
> - "mode", &mode, "1024x600");
> - if (sscanf(mode, "%dx%d", &output->width, &output->height) != 2) {
> - weston_log("Invalid mode \"%s\" for output %s\n",
> - mode, output->name);
> - output->width = 1024;
> - output->height = 640;
> - }
> - free(mode);
> -
> - if (option_width)
> - output->width = option_width;
> - if (option_height)
> - output->height = option_height;
> -
> - weston_config_section_get_int(config_section, "scale", &output->scale,
> 1);
> -
> - if (option_scale)
> - output->scale = option_scale;
> -
> - weston_config_section_get_string(config_section,
> - "transform", &t, "normal");
> - if (weston_parse_transform(t, &output->transform) < 0)
> - weston_log("Invalid transform \"%s\" for output %s\n",
> - t, output->name);
> - free(t);
> -
> -}
> -
> static struct wayland_output *
> wayland_output_create_for_config(struct wayland_backend *b,
> struct weston_wayland_backend_output_config
> *oc,
> @@ -2326,176 +2271,37 @@ wayland_backend_destroy(struct wayland_backend *b)
> }
>
> static void
> -wayland_backend_config_release(struct weston_wayland_backend_config
> *new_config) {
> - int i;
> - for (i = 0; i < new_config->num_outputs; ++i) {
> - free(new_config->outputs[i].name);
> - }
> - free(new_config->cursor_theme);
> - free(new_config->display_name);
> - free(new_config->outputs);
> -}
> -
> -/*
> - * Append a new output struct at the end of new_config.outputs and return a
> - * pointer to the newly allocated structure or NULL if fail. The allocated
> - * structure is NOT cleared nor set to default values.
> - */
> -static struct weston_wayland_backend_output_config *
> -wayland_backend_config_add_new_output(struct weston_wayland_backend_config
> *new_config) {
> - struct weston_wayland_backend_output_config *outputs;
> - outputs = realloc(new_config->outputs,
> - (new_config->num_outputs+1)*sizeof(struct
> weston_wayland_backend_output_config));
> - if (!outputs)
> - return NULL;
> - new_config->num_outputs += 1;
> - new_config->outputs = outputs;
> - return &(new_config->outputs[new_config->num_outputs-1]);
> -}
> -
> -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)
> -{
> - struct weston_wayland_backend_config new_config = { 0, };
> - struct weston_config_section *section;
> - struct weston_wayland_backend_output_config *oc;
> - int count, width, height, scale;
> - const char *section_name;
> - char *name;
> -
> - const struct weston_option wayland_options[] = {
> - { WESTON_OPTION_INTEGER, "width", 0, &width },
> - { WESTON_OPTION_INTEGER, "height", 0, &height },
> - { WESTON_OPTION_INTEGER, "scale", 0, &scale },
> - { WESTON_OPTION_STRING, "display", 0, &new_config.display_name
> },
> - { WESTON_OPTION_BOOLEAN, "use-pixman", 0,
> &new_config.use_pixman },
> - { WESTON_OPTION_INTEGER, "output-count", 0, &count },
> - { WESTON_OPTION_BOOLEAN, "fullscreen", 0,
> &new_config.fullscreen },
> - { WESTON_OPTION_BOOLEAN, "sprawl", 0, &new_config.sprawl },
> - };
> -
> - width = 0;
> - height = 0;
> - scale = 0;
> - new_config.display_name = NULL;
> - new_config.use_pixman = 0;
> - count = 1;
> - new_config.fullscreen = 0;
> - new_config.sprawl = 0;
> - parse_options(wayland_options,
> - ARRAY_LENGTH(wayland_options), argc, argv);
> -
> - new_config.cursor_size = 32;
> - new_config.cursor_theme = NULL;
> -
> - section = weston_config_get_section(config, "shell", NULL, NULL);
> - weston_config_section_get_string(section, "cursor-theme",
> - &new_config.cursor_theme, NULL);
> - weston_config_section_get_int(section, "cursor-size",
> - &new_config.cursor_size, 32);
> -
> - if (new_config.sprawl) {
> - /* do nothing, everything is already set */
> - *out_config = new_config;
> - return 0;
> - }
> -
> - if (new_config.fullscreen) {
> -
> - oc = wayland_backend_config_add_new_output(&new_config);
> -
> - if (!oc)
> - goto err_outputs;
> -
> - oc->width = width;
> - oc->height = height;
> - oc->name = NULL;
> - oc->transform = WL_OUTPUT_TRANSFORM_NORMAL;
> - oc->scale = 1;
> -
> - *out_config = new_config;
> - return 0;
> - }
> -
> - section = NULL;
> - while (weston_config_next_section(config, §ion, §ion_name)) {
> - if (!section_name || strcmp(section_name, "output") != 0)
> - continue;
> - weston_config_section_get_string(section, "name", &name, NULL);
> - if (name == NULL)
> - continue;
> -
> - if (name[0] != 'W' || name[1] != 'L') {
> - free(name);
> - continue;
> - }
> - free(name);
> -
> - oc = wayland_backend_config_add_new_output(&new_config);
> -
> - if (!oc)
> - goto err_outputs;
> -
> - wayland_output_init_from_config(oc, section, width,
> - height, scale);
> - --count;
> - }
> -
> - if (!width)
> - width = 1024;
> - if (!height)
> - height = 640;
> - if (!scale)
> - scale = 1;
> - while (count > 0) {
> -
> - oc = wayland_backend_config_add_new_output(&new_config);
> -
> - if (!oc)
> - goto err_outputs;
> -
> - oc->width = width;
> - oc->height = height;
> - oc->name = NULL;
> - oc->transform = WL_OUTPUT_TRANSFORM_NORMAL;
> - oc->scale = scale;
> -
> - --count;
> - }
> -
> - *out_config = new_config;
> - return 0;
> -
> -err_outputs:
> - wayland_backend_config_release(&new_config);
> - return -1;
> +config_init_to_defaults(struct weston_wayland_backend_config *config)
> +{
> }
>
> WL_EXPORT int
> backend_init(struct weston_compositor *compositor, int *argc, char *argv[],
> - struct weston_config *config,
> + struct weston_config *wc,
> struct weston_backend_config *config_base)
> {
> struct wayland_backend *b;
> struct wayland_output *output;
> struct wayland_parent_output *poutput;
> - struct weston_wayland_backend_config new_config;
> + struct weston_wayland_backend_config config = {{ 0, }};
> int x, count;
>
> - if(load_wayland_backend_config(compositor, argc, argv, config,
> - &new_config) < 0) {
> - wayland_backend_config_release(&new_config);
> + if (config_base == NULL ||
> + config_base->struct_version !=
> WESTON_WAYLAND_BACKEND_CONFIG_VERSION ||
> + config_base->struct_size > sizeof(struct
> weston_wayland_backend_config)) {
> + weston_log("wayland backend config structure is invalid\n");
> return -1;
> }
>
> - b = wayland_backend_create(compositor, &new_config, argc, argv, config);
> + config_init_to_defaults(&config);
> + memcpy(&config, config_base, config_base->struct_size);
> +
> + b = wayland_backend_create(compositor, &config, argc, argv, wc);
>
> if (!b)
> return -1;
>
> - if (new_config.sprawl || b->parent.fshell) {
> + if (config.sprawl || b->parent.fshell) {
> b->sprawl_across_outputs = 1;
> wl_display_roundtrip(b->parent.wl_display);
>
> @@ -2505,12 +2311,12 @@ backend_init(struct weston_compositor *compositor,
> int *argc, char *argv[],
> return 0;
> }
>
> - if (new_config.fullscreen) {
> - if (new_config.num_outputs != 1 || !new_config.outputs)
> + if (config.fullscreen) {
> + if (config.num_outputs != 1 || !config.outputs)
> goto err_outputs;
>
> output = wayland_output_create_for_config(b,
> -
> &new_config.outputs[0],
> + &config.outputs[0],
> 1, 0, 0);
> if (!output)
> goto err_outputs;
> @@ -2520,8 +2326,8 @@ backend_init(struct weston_compositor *compositor, int
> *argc, char *argv[],
> }
>
> x = 0;
> - for (count = 0; count < new_config.num_outputs; ++count) {
> - output = wayland_output_create_for_config(b,
> &new_config.outputs[count],
> + for (count = 0; count < config.num_outputs; ++count) {
> + output = wayland_output_create_for_config(b,
> &config.outputs[count],
> 0, x, 0);
> if (!output)
> goto err_outputs;
> @@ -2534,11 +2340,9 @@ backend_init(struct weston_compositor *compositor, int
> *argc, char *argv[],
> weston_compositor_add_key_binding(compositor, KEY_F,
> MODIFIER_CTRL | MODIFIER_ALT,
> fullscreen_binding, b);
> - wayland_backend_config_release(&new_config);
> return 0;
>
> err_outputs:
> wayland_backend_destroy(b);
> - wayland_backend_config_release(&new_config);
> return -1;
> }
> diff --git a/src/compositor-wayland.h b/src/compositor-wayland.h
> index ab216eb..7032f08 100644
> --- a/src/compositor-wayland.h
> +++ b/src/compositor-wayland.h
> @@ -32,6 +32,10 @@ extern "C" {
>
> #include "compositor.h"
>
> +#define WESTON_WAYLAND_BACKEND_CONFIG_VERSION 1
> +
> +#define WINDOW_TITLE "Weston Compositor"
> +
> struct weston_wayland_backend_output_config {
> int width;
> int height;
> @@ -41,6 +45,7 @@ struct weston_wayland_backend_output_config {
> };
>
> struct weston_wayland_backend_config {
> + struct weston_backend_config base;
> int use_pixman;
> int sprawl;
> char *display_name;
> diff --git a/src/main.c b/src/main.c
> index f034dda..f90bd7b 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -49,6 +49,7 @@
>
> #include "compositor-headless.h"
> #include "compositor-rdp.h"
> +#include "compositor-wayland.h"
>
> static struct wl_list child_process_list;
> static struct weston_compositor *segv_compositor;
> @@ -767,6 +768,212 @@ load_rdp_backend(struct weston_compositor *c, char
> const * backend,
> return ret;
> }
>
> +static void
> +weston_wayland_backend_config_release(struct weston_wayland_backend_config
> *new_config) {
> + int i;
> + for (i = 0; i < new_config->num_outputs; ++i) {
> + free(new_config->outputs[i].name);
> + }
> + free(new_config->cursor_theme);
> + free(new_config->display_name);
> + free(new_config->outputs);
> +}
> +
> +/*
> + * Append a new output struct at the end of new_config.outputs and return a
> + * pointer to the newly allocated structure or NULL if fail. The allocated
> + * structure is NOT cleared nor set to default values.
> + */
> +static struct weston_wayland_backend_output_config *
> +weston_wayland_backend_config_add_new_output(struct
> weston_wayland_backend_config *new_config) {
> + struct weston_wayland_backend_output_config *outputs;
> + outputs = realloc(new_config->outputs,
> + (new_config->num_outputs+1)*sizeof(struct
> weston_wayland_backend_output_config));
> + if (!outputs)
> + return NULL;
> + new_config->num_outputs += 1;
> + new_config->outputs = outputs;
> + return &(new_config->outputs[new_config->num_outputs-1]);
> +}
> +
> +static void
> +weston_wayland_backend_output_init(struct
> weston_wayland_backend_output_config *output,
> + struct weston_config_section *config_section,
> + int option_width, int option_height,
> + int option_scale)
> +{
> + char *mode, *t, *str;
> + unsigned int slen;
> +
> + weston_config_section_get_string(config_section, "name", &output->name,
> + NULL);
> + if (output->name) {
> + slen = strlen(output->name);
> + slen += strlen(WINDOW_TITLE " - ");
> + str = malloc(slen + 1);
> + if (str)
> + snprintf(str, slen + 1, WINDOW_TITLE " - %s",
> + output->name);
> + free(output->name);
> + output->name = str;
> + }
> + if (!output->name)
> + output->name = strdup(WINDOW_TITLE);
> +
> + weston_config_section_get_string(config_section,
> + "mode", &mode, "1024x600");
> + if (sscanf(mode, "%dx%d", &output->width, &output->height) != 2) {
> + weston_log("Invalid mode \"%s\" for output %s\n",
> + mode, output->name);
> + output->width = 1024;
> + output->height = 640;
> + }
> + free(mode);
> +
> + if (option_width)
> + output->width = option_width;
> + if (option_height)
> + output->height = option_height;
> +
> + weston_config_section_get_int(config_section, "scale", &output->scale,
> 1);
> +
> + if (option_scale)
> + output->scale = option_scale;
> +
> + weston_config_section_get_string(config_section,
> + "transform", &t, "normal");
> + if (weston_parse_transform(t, &output->transform) < 0)
> + weston_log("Invalid transform \"%s\" for output %s\n",
> + t, output->name);
> + free(t);
> +
> +}
> +
> +static int
> +load_wayland_backend(struct weston_compositor *c, char const *backend,
> + int *argc, char *argv[], struct weston_config *wc)
> +{
> + struct weston_wayland_backend_config config = {{ 0, }};
> + struct weston_config_section *section;
> + struct weston_wayland_backend_output_config *oc;
> + 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 },
> + { WESTON_OPTION_INTEGER, "height", 0, &height },
> + { WESTON_OPTION_INTEGER, "scale", 0, &scale },
> + { WESTON_OPTION_STRING, "display", 0, &config.display_name },
> + { WESTON_OPTION_BOOLEAN, "use-pixman", 0, &config.use_pixman },
> + { WESTON_OPTION_INTEGER, "output-count", 0, &count },
> + { WESTON_OPTION_BOOLEAN, "fullscreen", 0, &config.fullscreen },
> + { WESTON_OPTION_BOOLEAN, "sprawl", 0, &config.sprawl },
> + };
> +
> + width = 0;
> + height = 0;
> + scale = 0;
> + config.display_name = NULL;
> + config.use_pixman = 0;
> + count = 1;
> + config.fullscreen = 0;
> + config.sprawl = 0;
> + parse_options(wayland_options,
> + ARRAY_LENGTH(wayland_options), argc, argv);
> +
> + config.base.struct_version = WESTON_WAYLAND_BACKEND_CONFIG_VERSION;
> + config.base.struct_size = sizeof(struct weston_wayland_backend_config);
> + config.cursor_size = 32;
> + config.cursor_theme = NULL;
> +
> + section = weston_config_get_section(wc, "shell", NULL, NULL);
> + weston_config_section_get_string(section, "cursor-theme",
> + &config.cursor_theme, NULL);
> + weston_config_section_get_int(section, "cursor-size",
> + &config.cursor_size, 32);
> +
> + if (config.sprawl) {
> + /* do nothing, everything is already set */
> + ret = load_backend_new(c, backend, &config.base);
> + weston_wayland_backend_config_release(&config);
> + return ret;
> + }
> +
> + if (config.fullscreen) {
> +
> + oc = weston_wayland_backend_config_add_new_output(&config);
> +
> + if (!oc)
> + goto err_outputs;
> +
> + oc->width = width;
> + oc->height = height;
> + oc->name = NULL;
> + oc->transform = WL_OUTPUT_TRANSFORM_NORMAL;
> + oc->scale = 1;
> +
> + ret = load_backend_new(c, backend, &config.base);
> + weston_wayland_backend_config_release(&config);
> + return ret;
> + }
> +
> + section = NULL;
> + while (weston_config_next_section(wc, §ion, §ion_name)) {
> + if (!section_name || strcmp(section_name, "output") != 0)
> + continue;
> + weston_config_section_get_string(section, "name", &name, NULL);
> + if (name == NULL)
> + continue;
> +
> + if (name[0] != 'W' || name[1] != 'L') {
> + free(name);
> + continue;
> + }
> + free(name);
> +
> + oc = weston_wayland_backend_config_add_new_output(&config);
> +
> + if (!oc)
> + goto err_outputs;
> +
> + weston_wayland_backend_output_init(oc, section, width,
> + height, scale);
> + --count;
> + }
> +
> + if (!width)
> + width = 1024;
> + if (!height)
> + height = 640;
> + if (!scale)
> + scale = 1;
> + while (count > 0) {
> +
> + oc = weston_wayland_backend_config_add_new_output(&config);
> +
> + if (!oc)
> + goto err_outputs;
> +
> + oc->width = width;
> + oc->height = height;
> + oc->name = NULL;
> + oc->transform = WL_OUTPUT_TRANSFORM_NORMAL;
> + oc->scale = scale;
> +
> + --count;
> + }
> +
> + ret = load_backend_new(c, backend, &config.base);
> + weston_wayland_backend_config_release(&config);
> + return ret;
> +
> +err_outputs:
> + weston_wayland_backend_config_release(&config);
> + return ret;
> +}
> +
> static int
> load_backend(struct weston_compositor *compositor, const char *backend,
> int *argc, char **argv, struct weston_config *config)
> @@ -775,11 +982,11 @@ load_backend(struct weston_compositor *compositor,
> const char *backend,
> return load_headless_backend(compositor, backend, argc, argv,
> config);
> else if (strstr(backend, "rdp-backend.so"))
> return load_rdp_backend(compositor, backend, argc, argv,
> config);
> + else if (strstr(backend, "wayland-backend.so"))
> + return load_wayland_backend(compositor, backend, argc, argv,
> config);
> #if 0
> else if (strstr(backend, "drm-backend.so"))
> return load_drm_backend(compositor, backend, argc, argv,
> config);
> - else if (strstr(backend, "wayland-backend.so"))
> - return load_wayland_backend(compositor, backend, argc, argv,
> config);
> else if (strstr(backend, "x11-backend.so"))
> return load_x11_backend(compositor, backend, argc, argv,
> config);
> else if (strstr(backend, "fbdev-backend.so"))
It is very difficult to see what is going on straight from that.
If someone later finds a regression and bisects to this commit, they
need to go through the same excercise just to see what this commit
changed.
All these more or less cosmetic comments aside, I have very little to
complain about the changes themselves, they seem mostly good. It is
just a matter of serving them in an easily digestable format - not just
for review today, but also for git archeologists of the future trying
to figure out why a piece of code is like it is. :-)
Which reminds me, most of your commit messages were practically empty.
A patch has to be very obvious to avoid the need to explain "why this
change" in the commit message.
Thanks,
pq
pgptgMReH7g3G.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
