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 <[email protected]> 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 <[email protected]> >> --- >> 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 > [email protected] > https://lists.freedesktop.org/mailman/listinfo/wayland-devel > _______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
