[PATCH weston v2 9/9] wayland-backend: move all parsing functions to weston
Benoit Gschwind
gschwind at gnu-log.net
Thu May 5 21:02:53 UTC 2016
Hello Pekka,
I submitted a patches set (v3) to address all comments.
Best regards.
On 04/05/2016 15:41, Pekka Paalanen wrote:
> On Thu, 28 Apr 2016 20:33:16 +0200
> Benoit Gschwind <gschwind at 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 <gschwind at 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
>
>
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
More information about the wayland-devel
mailing list