On Thu, Aug 13, 2015 at 01:53:06PM +0300, Pekka Paalanen wrote: > From: Giulio Camuffo <giuliocamu...@gmail.com>
So much is being moved around here that it's hard to assure that no bugs got introduced. It feels like there's 2 or 3 refactoring steps being done in one go; if we're dead set on landing this for beta perhaps splitting those would make it easier to be certain it's correct. That said, this looks pretty solid. After reading patch #2, I'm understanding how this hangs together better. I've got a few comments below, mainly about function names, but in general would be comfortable seeing this land post-release. I'd like to see a bit more documentation at least for the new structs being introduced, and I really would like to see some test cases added, at least for the smaller easily-tested functions. > --- > Makefile.am | 3 + > src/compositor-wayland.c | 220 > ++++++++--------------------------------------- > src/compositor-wayland.h | 58 +++++++++++++ > src/main.c | 198 +++++++++++++++++++++++++++++++++++++++++- > 4 files changed, 296 insertions(+), 183 deletions(-) > create mode 100644 src/compositor-wayland.h > > diff --git a/Makefile.am b/Makefile.am > index 2f21930..d05bf88 100644 > --- a/Makefile.am > +++ b/Makefile.am > @@ -73,6 +73,7 @@ weston_SOURCES = \ > src/compositor.c \ > src/compositor.h \ > src/compositor-drm.h \ > + src/compositor-wayland.h \ > src/input.c \ > src/data-device.c \ > src/screenshooter.c \ > @@ -190,6 +191,7 @@ westoninclude_HEADERS = \ > src/version.h \ > src/compositor.h \ > src/compositor-drm.h \ > + src/compositor-wayland.h \ > src/timeline-object.h \ > shared/matrix.h \ > shared/config-parser.h \ > @@ -283,6 +285,7 @@ wayland_backend_la_CFLAGS = \ > $(AM_CFLAGS) > wayland_backend_la_SOURCES = \ > src/compositor-wayland.c \ > + src/compositor-wayland.h \ > shared/helpers.h > nodist_wayland_backend_la_SOURCES = \ > protocol/fullscreen-shell-protocol.c \ > diff --git a/src/compositor-wayland.c b/src/compositor-wayland.c > index 9788714..1f04e65 100644 > --- a/src/compositor-wayland.c > +++ b/src/compositor-wayland.c > @@ -40,6 +40,7 @@ > #include <wayland-cursor.h> > > #include "compositor.h" > +#include "compositor-wayland.h" > #include "gl-renderer.h" > #include "pixman-renderer.h" > #include "shared/helpers.h" > @@ -49,8 +50,6 @@ > #include "fullscreen-shell-client-protocol.h" > #include "presentation_timing-server-protocol.h" > > -#define WINDOW_TITLE "Weston Compositor" > - > struct wayland_backend { > struct weston_backend base; > struct weston_compositor *compositor; > @@ -71,6 +70,7 @@ struct wayland_backend { > > int use_pixman; > int sprawl_across_outputs; > + char *window_title; > > struct theme *theme; > cairo_device_t *frame_device; > @@ -752,21 +752,15 @@ wayland_output_set_windowed(struct wayland_output > *output) > { > struct wayland_backend *b = > (struct wayland_backend *)output->base.compositor->backend; > - int tlen; > char *title; > > if (output->frame) > return 0; > > if (output->name) { > - tlen = strlen(output->name) + strlen(WINDOW_TITLE " - "); > - title = malloc(tlen + 1); > - if (!title) > - return -1; > - > - snprintf(title, tlen + 1, WINDOW_TITLE " - %s", output->name); > + title = strdup(output->name); > } else { > - title = strdup(WINDOW_TITLE); > + title = strdup(b->window_title); > } > > if (!b->theme) { > @@ -1083,65 +1077,6 @@ err_name: > } > > static struct wayland_output * > -wayland_output_create_for_config(struct wayland_backend *b, > - struct weston_config_section *config_section, > - int option_width, int option_height, > - int option_scale, int32_t x, int32_t y) > -{ > - struct wayland_output *output; > - char *mode, *t, *name, *str; > - int width, height, scale; > - uint32_t transform; > - unsigned int slen; > - > - weston_config_section_get_string(config_section, "name", &name, NULL); > - if (name) { > - slen = strlen(name); > - slen += strlen(WINDOW_TITLE " - "); > - str = malloc(slen + 1); > - if (str) > - snprintf(str, slen + 1, WINDOW_TITLE " - %s", name); > - free(name); > - name = str; > - } > - if (!name) > - name = strdup(WINDOW_TITLE); > - > - weston_config_section_get_string(config_section, > - "mode", &mode, "1024x600"); > - if (sscanf(mode, "%dx%d", &width, &height) != 2) { > - weston_log("Invalid mode \"%s\" for output %s\n", > - mode, name); > - width = 1024; > - height = 640; > - } > - free(mode); > - > - if (option_width) > - width = option_width; > - if (option_height) > - height = option_height; > - > - weston_config_section_get_int(config_section, "scale", &scale, 1); > - > - if (option_scale) > - scale = option_scale; > - > - weston_config_section_get_string(config_section, > - "transform", &t, "normal"); > - if (weston_parse_transform(t, &transform) < 0) > - weston_log("Invalid transform \"%s\" for output %s\n", > - t, name); > - free(t); > - > - output = wayland_output_create(b, x, y, width, height, name, 0, > - transform, scale); > - free(name); > - > - return output; > -} > - > -static struct wayland_output * > wayland_output_create_for_parent_output(struct wayland_backend *b, > struct wayland_parent_output *poutput) > { > @@ -1886,6 +1821,7 @@ wayland_destroy(struct weston_compositor *ec) > if (b->parent.shm) > wl_shm_destroy(b->parent.shm); > > + free(b->window_title); > free(b); > } > > @@ -1897,25 +1833,16 @@ static const char *left_ptrs[] = { > }; > > static void > -create_cursor(struct wayland_backend *b, struct weston_config *config) > +create_cursor(struct wayland_backend *b, const char *theme, int size) > { > - struct weston_config_section *s; > - int size; > - char *theme = NULL; > unsigned int i; > > - s = weston_config_get_section(config, "shell", NULL, NULL); > - weston_config_section_get_string(s, "cursor-theme", &theme, NULL); > - weston_config_section_get_int(s, "cursor-size", &size, 32); > - > b->cursor_theme = wl_cursor_theme_load(theme, size, b->parent.shm); > if (!b->cursor_theme) { > fprintf(stderr, "could not load cursor theme\n"); > return; > } > > - free(theme); > - > b->cursor = NULL; > for (i = 0; !b->cursor && i < ARRAY_LENGTH(left_ptrs); ++i) > b->cursor = wl_cursor_theme_get_cursor(b->cursor_theme, > @@ -1950,8 +1877,8 @@ fullscreen_binding(struct weston_keyboard *keyboard, > uint32_t time, > > static struct wayland_backend * > wayland_backend_create(struct weston_compositor *compositor, int use_pixman, > - const char *display_name, int *argc, char *argv[], > - struct weston_config *config) > + const char *display_name, > + const char *cursor_theme, int cursor_size) > { > struct wayland_backend *b; > struct wl_event_loop *loop; > @@ -1977,7 +1904,7 @@ wayland_backend_create(struct weston_compositor > *compositor, int use_pixman, > wl_registry_add_listener(b->parent.registry, ®istry_listener, b); > wl_display_roundtrip(b->parent.wl_display); > > - create_cursor(b, config); > + create_cursor(b, cursor_theme, cursor_size); > > b->use_pixman = use_pixman; > > @@ -2032,62 +1959,48 @@ err_compositor: > return NULL; > } > > -static void > -wayland_backend_destroy(struct wayland_backend *b) > +static struct weston_output * > +wayland_create_output(struct weston_compositor *c, const char *name, > + struct weston_backend_output_config *base) > { > - wl_display_disconnect(b->parent.wl_display); > + struct wayland_backend *b = (struct wayland_backend *)c->backend; > + struct weston_wayland_backend_output_config *config = > + (struct weston_wayland_backend_output_config *)base; > + struct wayland_output *output; > > - if (b->theme) > - theme_destroy(b->theme); > - if (b->frame_device) > - cairo_device_destroy(b->frame_device); > - wl_cursor_theme_destroy(b->cursor_theme); > + output = wayland_output_create(b, 0, 0, config->base.width, > + config->base.height, > + name, config->fullscreen, > + config->base.transform, > + config->base.scale); We have 'wayland_create_output()', which calls 'wayland_output_create()'... Ow my brain > + if (!output) > + return NULL; > > - weston_compositor_shutdown(b->compositor); > - free(b); > + if (config->fullscreen) > + wayland_output_set_fullscreen(output, 0, 0, NULL); > + else > + wayland_output_set_windowed(output); > + > + return &output->base; > } > > WL_EXPORT int > backend_init(struct weston_compositor *compositor, int *argc, char *argv[], > - struct weston_config *config, > - struct weston_backend_config *config_base) > + struct weston_config *wc, > + struct weston_backend_config *config_base) > { > + struct weston_wayland_backend_config *config = > + (struct weston_wayland_backend_config *)config_base; > struct wayland_backend *b; > - struct wayland_output *output; > struct wayland_parent_output *poutput; > - struct weston_config_section *section; > - int x, count, width, height, scale, use_pixman, fullscreen, sprawl; > - const char *section_name, *display_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, &display_name }, > - { WESTON_OPTION_BOOLEAN, "use-pixman", 0, &use_pixman }, > - { WESTON_OPTION_INTEGER, "output-count", 0, &count }, > - { WESTON_OPTION_BOOLEAN, "fullscreen", 0, &fullscreen }, > - { WESTON_OPTION_BOOLEAN, "sprawl", 0, &sprawl }, > - }; > - > - width = 0; > - height = 0; > - scale = 0; > - display_name = NULL; > - use_pixman = 0; > - count = 1; > - fullscreen = 0; > - sprawl = 0; > - parse_options(wayland_options, > - ARRAY_LENGTH(wayland_options), argc, argv); > - > - b = wayland_backend_create(compositor, use_pixman, display_name, > - argc, argv, config); > + b = wayland_backend_create(compositor, config->use_pixman, > config->display, > + config->cursor_theme, config->cursor_size); > if (!b) > return -1; > > - if (sprawl || b->parent.fshell) { > + b->window_title = strdup(config->window_title); > + if (config->sprawl || b->parent.fshell) { > b->sprawl_across_outputs = 1; > wl_display_roundtrip(b->parent.wl_display); > > @@ -2096,68 +2009,11 @@ backend_init(struct weston_compositor *compositor, > int *argc, char *argv[], > > return 0; > } > - > - if (fullscreen) { > - output = wayland_output_create(b, 0, 0, width, height, > - NULL, 1, 0, 1); > - if (!output) > - goto err_outputs; > - > - wayland_output_set_fullscreen(output, 0, 0, NULL); > - return 0; > - } > - > - section = NULL; > - x = 0; > - 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); > - > - output = wayland_output_create_for_config(b, section, width, > - height, scale, x, 0); > - if (!output) > - goto err_outputs; > - if (wayland_output_set_windowed(output)) > - goto err_outputs; > - > - x += output->base.width; > - --count; > - } > - > - if (!width) > - width = 1024; > - if (!height) > - height = 640; > - if (!scale) > - scale = 1; > - while (count > 0) { > - output = wayland_output_create(b, x, 0, width, height, > - NULL, 0, 0, scale); > - if (!output) > - goto err_outputs; > - if (wayland_output_set_windowed(output)) > - goto err_outputs; > - > - x += width; > - --count; > - } > + b->base.create_output = wayland_create_output; > > weston_compositor_add_key_binding(compositor, KEY_F, > MODIFIER_CTRL | MODIFIER_ALT, > fullscreen_binding, b); > > return 0; > - > -err_outputs: > - wayland_backend_destroy(b); > - return -1; > } > diff --git a/src/compositor-wayland.h b/src/compositor-wayland.h > new file mode 100644 > index 0000000..ebbcebb > --- /dev/null > +++ b/src/compositor-wayland.h > @@ -0,0 +1,58 @@ > +/* > + * Copyright © 2010-2011 Benjamin Franzke > + * Copyright © 2013 Jason Ekstrand > + * Copyright © 2015 Giulio Camuffo > + * > + * Permission is hereby granted, free of charge, to any person obtaining > + * a copy of this software and associated documentation files (the > + * "Software"), to deal in the Software without restriction, including > + * without limitation the rights to use, copy, modify, merge, publish, > + * distribute, sublicense, and/or sell copies of the Software, and to > + * permit persons to whom the Software is furnished to do so, subject to > + * the following conditions: > + * > + * The above copyright notice and this permission notice (including the > + * next paragraph) shall be included in all copies or substantial > + * portions of the Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF > + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND > + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS > + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN > + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN > + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE > + * SOFTWARE. > + */ > + > +#ifndef WESTON_COMPOSITOR_WAYLAND_H > +#define WESTON_COMPOSITOR_WAYLAND_H > + > +#ifdef __cplusplus > +extern "C" { > +#endif > + > +#include "compositor.h" > + > +struct weston_wayland_backend_config { > + struct weston_backend_config base; > + > + const char *display; > + bool use_pixman; > + bool sprawl; > + int cursor_size; > + const char *cursor_theme; > + const char *window_title; > +}; > + > +struct weston_wayland_backend_output_config { > + struct weston_backend_output_config base; > + > + int fullscreen; > +}; "weston_wayland_*" appears to be a new name prefixing scheme, and I'm worried this is going to be a bit confusing to people. At least, I'm a bit confused. :-) Can you explain why this style was chosen? Also, "weston_wayland_backend_output_config" is quite a mouthful, and makes for some lengthy lines elsewhere in the patch. > +#ifdef __cplusplus > +} > +#endif > + > +#endif > diff --git a/src/main.c b/src/main.c > index d861c60..09eec42 100644 > --- a/src/main.c > +++ b/src/main.c > @@ -48,6 +48,9 @@ > #include "version.h" > > #include "compositor-drm.h" > +#include "compositor-wayland.h" > + > +#define WINDOW_TITLE "Weston Compositor" > > static struct wl_list child_process_list; > static struct weston_compositor *segv_compositor; > @@ -665,6 +668,199 @@ init_backend_new(struct weston_compositor *compositor, > const char *backend, > return backend_init(compositor, NULL, NULL, NULL, config_base); > } > > +static char * > +create_config_for_output(struct weston_config_section *config_section, > + int option_width, int option_height, > + int option_scale, > + struct weston_backend_output_config *config) > +{ > + char *mode, *t, *name, *str; > + int width, height, scale; > + uint32_t transform; > + unsigned int slen; > + > + weston_config_section_get_string(config_section, "name", &name, NULL); > + if (name) { > + slen = strlen(name); > + slen += strlen(WINDOW_TITLE " - "); > + str = malloc(slen + 1); > + if (str) > + snprintf(str, slen + 1, WINDOW_TITLE " - %s", name); > + free(name); > + name = str; > + } > + if (!name) > + name = strdup(WINDOW_TITLE); > + > + weston_config_section_get_string(config_section, > + "mode", &mode, "1024x600"); > + if (sscanf(mode, "%dx%d", &width, &height) != 2) { > + weston_log("Invalid mode \"%s\" for output %s\n", > + mode, name); > + width = 1024; > + height = 640; > + } > + free(mode); > + > + if (option_width) > + width = option_width; > + if (option_height) > + height = option_height; > + > + weston_config_section_get_int(config_section, "scale", &scale, 1); > + > + if (option_scale) > + scale = option_scale; > + > + weston_config_section_get_string(config_section, > + "transform", &t, "normal"); > + if (weston_parse_transform(t, &transform) < 0) > + weston_log("Invalid transform \"%s\" for output %s\n", > + t, name); > + free(t); > + > + config->width = width; > + config->height = height; > + config->transform = transform; > + config->scale = scale; > + return name; > +} create_config_for_output() would be a bit challenging to write a test for, but it'd pay for itself once people start exposing weston to all the clunky monitors out there. > +static int > +init_wayland_backend(struct weston_compositor *c, const char *backend, > + int *argc, char **argv, struct weston_config *wc) > +{ > + struct weston_wayland_backend_config config = { > + .use_pixman = false, > + .sprawl = false, > + .display = NULL, > + .cursor_size = 32, > + .cursor_theme = NULL, > + .window_title = WINDOW_TITLE, > + }; > + struct weston_config_section *section; > + int count = 1; > + int width = 0, height = 0; > + int scale = 0; > + bool fullscreen = false; > + const char *section_name; > + char *cursor_theme = NULL; > + char *display_name = NULL; > + struct weston_output *output; > + char *name, *output_name; > + int x = 0; > + int ret = 0; > + > + const struct weston_option options[] = { > + { WESTON_OPTION_INTEGER, "width", 0, &width }, > + { WESTON_OPTION_INTEGER, "height", 0, &height }, > + { WESTON_OPTION_INTEGER, "scale", 0, &scale }, > + { WESTON_OPTION_STRING, "display", 0, &display_name }, > + { WESTON_OPTION_BOOLEAN, "use-pixman", 0, &config.use_pixman }, > + { WESTON_OPTION_INTEGER, "output-count", 0, &count }, > + { WESTON_OPTION_BOOLEAN, "fullscreen", 0, &fullscreen }, > + { WESTON_OPTION_BOOLEAN, "sprawl", 0, &config.sprawl }, > + }; > + > + parse_options(options, ARRAY_LENGTH(options), argc, argv); > + > + section = weston_config_get_section(wc, "shell", NULL, NULL); > + weston_config_section_get_string(section, "cursor-theme", > + &cursor_theme, > + cursor_theme); > + weston_config_section_get_int(section, "cursor-size", > + &config.cursor_size, config.cursor_size); > + > + config.cursor_theme = cursor_theme; > + config.display = display_name; > + > + if (init_backend_new(c, backend, &config.base) < 0) { > + ret = -1; > + goto cleanup; > + } > + > + if (!c->backend->create_output) { > + ret = -1; > + goto cleanup; > + } > + > + if (fullscreen) { > + struct weston_wayland_backend_output_config output_config = { > + .base.width = width, > + .base.height = height, > + .base.scale = 1, > + .base.transform = WL_OUTPUT_TRANSFORM_NORMAL, > + .fullscreen = true, > + }; > + output = c->backend->create_output(c, NULL, > &output_config.base); > + goto cleanup; > + } > + > + 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); > + > + struct weston_wayland_backend_output_config output_config; > + output_name = create_config_for_output(section, width, > + height, scale, > + &output_config.base); > + output_config.fullscreen = false; > + output = c->backend->create_output(c, output_name, > + &output_config.base); > + free(output_name); > + > + if (!output) { > + ret = -1; > + goto cleanup; > + } > + > + weston_output_move(output, x, 0); > + x += output->width; > + --count; > + } > + > + if (!width) > + width = 1024; > + if (!height) > + height = 640; > + if (!scale) > + scale = 1; > + > + while (count > 0) { > + struct weston_wayland_backend_output_config output_config = { > + .base.width = width, > + .base.height = height, > + .base.scale = scale, > + .base.transform = WL_OUTPUT_TRANSFORM_NORMAL, > + .fullscreen = false, > + }; > + output = c->backend->create_output(c, NULL, > &output_config.base); > + if (output == NULL) { > + ret = -1; > + goto cleanup; > + } > + weston_output_move(output, x, 0); > + x += width; > + --count; > + } > + > +cleanup: > + free(cursor_theme); > + free(display_name); > + return ret; > +} > + > static int > parse_modeline(const char *s, struct weston_drm_backend_modeline *mode) > { > @@ -794,9 +990,9 @@ init_backend(struct weston_compositor *compositor, const > char *backend, > > if (strstr(backend, "drm-backend.so")) > return init_drm_backend(compositor, backend, argc, argv, > config); > -#if 0 > else if (strstr(backend, "wayland-backend.so")) > return init_wayland_backend(compositor, backend, argc, argv, > config); > +#if 0 > else if (strstr(backend, "x11-backend.so")) > return init_x11_backend(compositor, backend, argc, argv, > config); > else if (strstr(backend, "fbdev-backend.so")) > -- > 2.4.6 > > _______________________________________________ > wayland-devel mailing list > wayland-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/wayland-devel _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel