Hi Kristian, Sorry it took so long to get back to you on this but I had to deal with some other more pressing things. I wanted to reply while I am rebasing this series to master, in case there are any other concerns. Please see responses below. <div class="gmail_quote">On Wed, Nov 28, 2012 at 7:48 PM, Kristian Høgsberg <<a href="mailto:hoegsberg@gmail.com" target="_blank">hoegsberg@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>On Thu, Nov 15, 2012 at 12:37:01AM -0700, Scott Moreau wrote: > This patch uses the special surface_data interface to send information about > the surface to the shell. The shell then uses this information to render a > window list in the panel. </div>The previous XML patch was good and the overall approach here is fine. There's just a few more things below we need to fix and then we can commit this. </blockquote><div> Cool. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div> > --- > clients/desktop-shell.c | 526 ++++++++++++++++++++++++++++++++++++++++++++++-- > data/Makefile.am | 1 + > data/list_item_icon.png | Bin 0 -> 176 bytes > src/compositor.c | 2 + > src/compositor.h | 1 + > src/shell.c | 170 ++++++++++++++++ > 6 files changed, 683 insertions(+), 17 deletions(-) > create mode 100644 data/list_item_icon.png > > diff --git a/clients/desktop-shell.c b/clients/desktop-shell.c > index 1cae789..ea42c66 100644 > --- a/clients/desktop-shell.c > +++ b/clients/desktop-shell.c > @@ -44,6 +44,8 @@ > > #include "desktop-shell-client-protocol.h" > > +#define MIN(x, y) (((x) < (y)) ? (x) : (y)) > + > extern char **environ; /* defined by libc */ > > struct desktop { > @@ -51,37 +53,59 @@ struct desktop { > struct desktop_shell *shell; > struct unlock_dialog *unlock_dialog; > struct task unlock_task; > + struct wl_list surfaces; > struct wl_list outputs; > + uint32_t output_count; > > struct window *grab_window; > struct widget *grab_widget; > > enum cursor_type grab_cursor; > + > + struct surface_data_manager *surface_data_manager; > }; > > struct surface { > + struct wl_list item_list; > + struct surface_data *surface_data; > + uint32_t output_mask; > + char *title; > + > + struct wl_list link; > +}; > + > +struct resize { > void (*configure)(void *data, > struct desktop_shell *desktop_shell, > uint32_t edges, struct window *window, > int32_t width, int32_t height); > }; > > +struct rgba { > + float r, g, b, a; > +}; > + > struct panel { > - struct surface base; > + struct resize base; > struct window *window; > struct widget *widget; > struct wl_list launcher_list; > + struct wl_list window_list; > + struct rectangle window_list_rect; > + uint32_t surface_count; > + struct rgba focused_item; > struct panel_clock *clock; > }; > > struct background { > - struct surface base; > + struct resize base; > struct window *window; > struct widget *widget; > }; > > struct output { > struct wl_output *output; > + uint32_t id; > struct wl_list link; > > struct panel *panel; > @@ -99,6 +123,16 @@ struct panel_launcher { > struct wl_array argv; > }; > > +struct list_item { > + struct surface *surface; > + struct widget *widget; > + struct panel *panel; > + cairo_surface_t *icon; > + int focused, highlight; > + struct wl_list link; > + struct wl_list surface_link; > +}; > + > struct panel_clock { > struct widget *widget; > struct panel *panel; > @@ -249,6 +283,15 @@ set_hex_color(cairo_t *cr, uint32_t color) > } > > static void > +get_hex_color_rgba(uint32_t color, float *r, float *g, float *b, float *a) > +{ > + *r = ((color >> 16) & 0xff) / 255.0; > + *g = ((color >> 8) & 0xff) / 255.0; > + *b = ((color >> 0) & 0xff) / 255.0; > + *a = ((color >> 24) & 0xff) / 255.0; > +} > + > +static void > panel_redraw_handler(struct widget *widget, void *data) > { > cairo_surface_t *surface; > @@ -421,15 +464,49 @@ panel_button_handler(struct widget *widget, > } > > static void > +panel_window_list_schedule_redraw(struct panel *panel) > +{ > + struct list_item *item; > + float x, y, w, h; > + float item_width, padding; > + > + /* If there are no window list items, redraw the panel to clear it */ > + if (wl_list_empty(&panel->window_list)) { > + widget_schedule_redraw(panel->widget); > + return; > + } > + > + item_width = ((float) panel->window_list_rect.width / > + panel->surface_count); > + padding = MIN(item_width * 0.1f, 2.0f); </div></div>We don't want floats here. We're trying to compute an integer number of pixels that will be the item width. Also, item padding is always a constant number of pixels (2 is fine), not proportional to the width. </blockquote><div> Yes, you are right. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div> > + x = panel->window_list_rect.x + padding; > + y = 16; > + w = MIN(item_width - padding, 200); > + h = 24; > + > + wl_list_for_each(item, &panel->window_list, link) { > + widget_set_allocation(item->widget, x, y - h / 2, w + 1, h + 1); </div>It'd be more straightforward to just use x, 4, w, 24 here. I know it's done that way elsewhere in desktop-shell.c but it doesn't make sense there either. </blockquote><div> Done. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div> > + x += w + padding; > + widget_schedule_redraw(item->widget); > + } > +} > + > +static void > panel_resize_handler(struct widget *widget, > int32_t width, int32_t height, void *data) > { > struct panel_launcher *launcher; > + struct rectangle launcher_rect; > + struct rectangle clock_rect; > struct panel *panel = data; > int x, y, w, h; > - > + > x = 10; > y = 16; > + > + launcher_rect.x = x; > + > wl_list_for_each(launcher, &panel->launcher_list, link) { > w = cairo_image_surface_get_width(launcher->icon); > h = cairo_image_surface_get_height(launcher->icon); > @@ -437,12 +514,25 @@ panel_resize_handler(struct widget *widget, > x, y - h / 2, w + 1, h + 1); > x += w + 10; </div></div>We could fix the weird y - h / 2 here too. </blockquote><div> No problem. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div> > } > - h=20; > + > + launcher_rect.width = x - launcher_rect.x; > + > w=170; > + h=20; > > if (panel->clock) > widget_set_allocation(panel->clock->widget, > width - w - 8, y - h / 2, w + 1, h + 1); > + > + widget_get_allocation(panel->clock->widget, &clock_rect); > + > + panel->window_list_rect.x = launcher_rect.x + launcher_rect.width; > + panel->window_list_rect.y = 2; > + panel->window_list_rect.width = width - > + panel->window_list_rect.x - > + (clock_rect.width + 20); > + panel->window_list_rect.height = 28; > + panel_window_list_schedule_redraw(panel); > } > > static void > @@ -451,7 +541,7 @@ panel_configure(void *data, > uint32_t edges, struct window *window, > int32_t width, int32_t height) > { > - struct surface *surface = window_get_user_data(window); > + struct resize *surface = window_get_user_data(window); > struct panel *panel = container_of(surface, struct panel, base); > > window_schedule_resize(panel->window, width, 32); > @@ -490,6 +580,25 @@ panel_destroy(struct panel *panel) > free(panel); > } > > +static void > +panel_set_list_item_focus_color(struct panel *panel) > +{ > + float r, g, b, a; > + > + /* Consider panel color when choosing item highlight color */ > + get_hex_color_rgba(key_panel_color, &r, &b, &g, &a); > + if (r += 0.2, g += 0.2, b += 0.2, r > 1.0 || g > 1.0 || b > 1.0) { </div></div>Can we avoid all this stuff inside the if? Also, just saturating to white might be better. </blockquote><div> Yes we can avoid the statements in the conditional but I don't think fading to white is a good idea. This code attempts to detect lighter color panels near white and sets the highlight color of the list items to a darker color to offset. I did restructure it a bit however. This is what I have currently: /* Consider panel color when choosing item highlight color */ get_hex_color_rgba(key_panel_color, &r, &b, &g, &a); r += 0.2; g += 0.2; b += 0.2; panel->focused_item.r = r > 1.0 ? 0.6 : r; panel->focused_item.g = g > 1.0 ? 0.6 : g; panel->focused_item.b = b > 1.0 ? 0.6 : b; panel->focused_item.a = 0.75; </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div> > + panel->focused_item.r = 0.6; > + panel->focused_item.g = 0.6; > + panel->focused_item.b = 0.6; > + } else { > + panel->focused_item.r = r; > + panel->focused_item.g = g; > + panel->focused_item.b = b; > + } > + panel->focused_item.a = 0.75; > +} > + > static struct panel * > panel_create(struct display *display) > { > @@ -502,6 +611,7 @@ panel_create(struct display *display) > panel->window = window_create_custom(display); > panel->widget = window_add_widget(panel->window, panel); > wl_list_init(&panel->launcher_list); > + wl_list_init(&panel->window_list); > > window_set_title(panel->window, "panel"); > window_set_user_data(panel->window, panel); > @@ -509,7 +619,9 @@ panel_create(struct display *display) > widget_set_redraw_handler(panel->widget, panel_redraw_handler); > widget_set_resize_handler(panel->widget, panel_resize_handler); > widget_set_button_handler(panel->widget, panel_button_handler); > - > + > + panel->surface_count = 0; > + panel_set_list_item_focus_color(panel); > panel_add_clock(panel); > > return panel; > @@ -518,18 +630,21 @@ panel_create(struct display *display) > static cairo_surface_t * > load_icon_or_fallback(const char *icon) > { > - cairo_surface_t *surface = cairo_image_surface_create_from_png(icon); > + cairo_surface_t *surface; > cairo_t *cr; > - > - if (cairo_surface_status(surface) == CAIRO_STATUS_SUCCESS) > - return surface; > - > - cairo_surface_destroy(surface); > - fprintf(stderr, "ERROR loading icon from file '%s'\n", icon); > +#ifdef CAIRO_HAS_PNG_FUNCTIONS > + if (icon) { > + surface = cairo_image_surface_create_from_png(icon); </div></div>We should just use load_cairo_surface() here instead of using the cairo png functions. That lets us load other formats and we don't need the #ifdef. </blockquote><div> Yes, this is much better since it adds support for other image types as well. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div> > + if (cairo_surface_status(surface) == CAIRO_STATUS_SUCCESS) > + return surface; > + > + cairo_surface_destroy(surface); > + fprintf(stderr, "ERROR loading icon from file '%s'\n", icon); > + } > +#endif > > /* draw fallback icon */ > - surface = cairo_image_surface_create(CAIRO_FORMAT_ARGB32, > - 20, 20); > + surface = cairo_image_surface_create(CAIRO_FORMAT_ARGB32, 20, 20); > cr = cairo_create(surface); > > cairo_set_source_rgba(cr, 0.8, 0.8, 0.8, 1); > @@ -862,7 +977,7 @@ desktop_shell_configure(void *data, > int32_t width, int32_t height) > { > struct window *window = wl_surface_get_user_data(surface); > - struct surface *s = window_get_user_data(window); > + struct resize *s = window_get_user_data(window); > > s->configure(data, desktop_shell, edges, window, width, height); > } > @@ -946,6 +1061,362 @@ background_destroy(struct background *background) > free(background); > } > > +static void > +panel_list_item_redraw_handler(struct widget *widget, void *data) > +{ > + cairo_t *cr; > + cairo_surface_t *surface; > + struct list_item *item = data; > + struct rectangle rect; > + cairo_text_extents_t extents; > + cairo_font_extents_t font_extents; > + int icon_width, icon_height; > + unsigned int dots; > + double padding; > + char title[128]; > + > + widget_get_allocation(widget, &rect); > + if (rect.width == 0) > + return; > + > + surface = window_get_surface(item->panel->window); > + cr = cairo_create(surface); > + > + if (item->focused) { > + cairo_set_source_rgba(cr, item->panel->focused_item.r, > + item->panel->focused_item.g, > + item->panel->focused_item.b, > + item->panel->focused_item.a); > + cairo_move_to(cr, rect.x, rect.y); > + cairo_line_to(cr, rect.x + rect.width, rect.y); > + cairo_line_to(cr, rect.x + rect.width, rect.y + rect.height); > + cairo_line_to(cr, rect.x, rect.y + rect.height); > + cairo_line_to(cr, rect.x, rect.y); </div></div>Use cairo_rectangle() instead? </blockquote><div> Yep. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div> > + cairo_fill(cr); > + } > + > + icon_width = cairo_image_surface_get_width(item->icon); > + icon_height = cairo_image_surface_get_height(item->icon); > + padding = (rect.height / 2.f) - (icon_height / 2.f); > + if (rect.width > icon_width * 2) { > + cairo_set_source_surface(cr, item->icon, > + rect.x + padding, > + rect.y + padding); > + cairo_paint(cr); > + } else { > + icon_width = 0; > + icon_height = 0; > + padding = 1; > + } > + > + strcpy(title, item->surface->title); > + cairo_select_font_face(cr, "sans", > + CAIRO_FONT_SLANT_NORMAL, > + CAIRO_FONT_WEIGHT_NORMAL); > + cairo_set_font_size(cr, 14); > + cairo_text_extents(cr, title, &extents); > + > + /* If the string is too long, clip text to button width */ > + while (extents.width > (rect.width - (icon_width + padding * 3))) { > + title[strlen(title) - 1] = '\0'; > + cairo_text_extents(cr, title, &extents); > + if (extents.width <= 0) { > + title[0] = '\0'; > + break; > + } > + } > + > + /* If the text is clipped, add an ellipsis */ > + dots = 3; > + if (strlen(title) < dots) > + dots = strlen(title) + 1; > + if (strlen(title) != strlen(item->surface->title)) > + while (dots-- > 0) > + title[strlen(title) - dots] = '.'; </div></div>I suppose this is fine... if we want to do better, we'll have to use something like pango or harfbuzz to ellipsize the text. For our toy desktop, this is par for the course. <div><div> > + cairo_font_extents (cr, &font_extents); > + cairo_move_to(cr, rect.x + icon_width + padding * 3 + 1, > + rect.y + 3 * (rect.height >> 2) + 1); > + cairo_set_source_rgb(cr, 0, 0, 0); > + cairo_show_text(cr, title); > + cairo_move_to(cr, rect.x + icon_width + padding * 3, > + rect.y + 3 * (rect.height >> 2)); > + if (item->focused) > + cairo_set_source_rgb(cr, 1, 1, 1); > + else > + cairo_set_source_rgb(cr, 0.85, 0.85, 0.85); > + cairo_show_text(cr, title); > + cairo_destroy(cr); > +} > + > +static int > +panel_list_item_motion_handler(struct widget *widget, struct input *input, > + uint32_t time, float x, float y, void *data) > +{ > + struct list_item *item = data; > + > + widget_set_tooltip(widget, basename((char *)item->surface->title), x, y); > + > + return CURSOR_LEFT_PTR; > +} > + > +static int > +panel_list_item_enter_handler(struct widget *widget, struct input *input, > + float x, float y, void *data) > +{ > + struct list_item *item = data; > + > + item->focused = 1; > + widget_schedule_redraw(widget); > + > + return CURSOR_LEFT_PTR; > +} > + > +static void > +panel_list_item_leave_handler(struct widget *widget, > + struct input *input, void *data) > +{ > + struct list_item *item = data; > + > + item->focused = 0; > + widget_destroy_tooltip(widget); > + widget_schedule_redraw(widget); > +} > + > +static void > +panel_list_item_button_handler(struct widget *widget, > + struct input *input, uint32_t time, > + uint32_t button, > + enum wl_pointer_button_state state, void *data) > +{ > + widget_schedule_redraw(widget); > + /* TODO: Toggle minimize */ > +} > + > +static struct list_item * > +panel_list_item_add(struct panel *panel, const char *icon, const char *text) > +{ > + struct list_item *item; > + item = malloc(sizeof *item); > + memset(item, 0, sizeof *item); > + > + item->icon = load_icon_or_fallback(icon); > + > + item->panel = panel; > + wl_list_insert(panel->window_list.prev, &item->link); > + panel->surface_count++; > + > + item->widget = widget_add_widget(panel->widget, item); > + widget_set_enter_handler(item->widget, panel_list_item_enter_handler); > + widget_set_leave_handler(item->widget, panel_list_item_leave_handler); > + widget_set_button_handler(item->widget, panel_list_item_button_handler); > + widget_set_redraw_handler(item->widget, panel_list_item_redraw_handler); > + widget_set_motion_handler(item->widget, panel_list_item_motion_handler); > + > + return item; > +} > + > +static void > +panel_list_item_remove(struct list_item *item) > +{ > + item->panel->surface_count--; > + wl_list_remove(&item->link); > + wl_list_remove(&item->surface_link); > + widget_destroy(item->widget); > + panel_window_list_schedule_redraw(item->panel); > + free(item); > +} > + > +static int > +panel_list_item_exists(struct panel *panel, struct surface *surface) > +{ > + struct list_item *p_item, *s_item; > + > + wl_list_for_each(p_item, &panel->window_list, link) { > + wl_list_for_each(s_item, &surface->item_list, surface_link) { > + if (p_item == s_item) > + return 1; > + } > + } > + > + return 0; > +} > + > +static void > +output_update_window_list(struct output *output, struct surface *surface) > +{ > + struct list_item *item, *next; > + struct panel *panel; > + > + panel = output->panel; > + > + /* Make a list item for each panel of the surfaces output mask */ > + if ((1 << output->id) & surface->output_mask) { > + if (!panel_list_item_exists(panel, surface)) { > + item = panel_list_item_add(panel, > + DATADIR "/weston/list_item_icon.png", > + surface->title); > + wl_list_insert(surface->item_list.prev, > + &item->surface_link); > + item->surface = surface; > + } > + } else { > + /* Remove item from panel if surface > + * is no longer on the output */ > + wl_list_for_each_safe(item, next, &surface->item_list, > + surface_link) { > + if (item->panel == panel) > + panel_list_item_remove(item); > + } > + } > + > + panel_window_list_schedule_redraw(panel); > +} > + > +static struct surface* > +desktop_create_surface(struct desktop *desktop, > + struct surface_data *surface_data) > +{ > + struct surface *surface; > + > + surface = calloc(1, sizeof *surface); > + > + if (!surface) { > + fprintf(stderr, "ERROR: Failed to allocate memory!\n"); > + exit(EXIT_FAILURE); > + } > + > + surface->surface_data = surface_data; > + surface->title = strdup("unknown"); > + surface->output_mask = 1; > + wl_list_init(&surface->item_list); > + wl_list_insert(&desktop->surfaces, &surface->link); > + > + return surface; > +} > + > +static void > +desktop_destroy_surface(struct surface *surface) > +{ > + struct list_item *item, *next; > + > + wl_list_for_each_safe(item, next, &surface->item_list, surface_link) > + panel_list_item_remove(item); > + > + wl_list_remove(&surface->link); > + free(surface->title); > + free(surface); > +} > + > +static struct surface * > +desktop_get_surface(struct desktop *desktop, struct surface_data *surface_data) > +{ > + struct output *output; > + struct list_item *item; > + > + wl_list_for_each(output, &desktop->outputs, link) { > + wl_list_for_each(item, &output->panel->window_list, link) { > + if (surface_data == item->surface->surface_data) > + return item->surface; > + } > + } </div></div>Just store struct surface as user_data for struct surface_data using surface_data_set_user_data(). Then this function becomes just surface_data_get_user_data(). </blockquote><div> This seems like it will work. </div><div></div><div></div><div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div> > + return NULL; > +} > + > +static void > +desktop_update_list_items(struct desktop *desktop, struct surface *surface) > +{ > + struct output *output; > + > + wl_list_for_each(output, &desktop->outputs, link) > + output_update_window_list(output, surface); > +} > + > +static void > +surface_data_set_output_mask(void *data, > + struct surface_data *surface_data, > + uint32_t output_mask) > +{ > + struct desktop *desktop; > + struct surface *surface; > + > + desktop = data; > + > + surface = desktop_get_surface(desktop, surface_data); > + > + if (!surface) > + surface = desktop_create_surface(desktop, surface_data); > + > + surface->output_mask = output_mask; > + > + desktop_update_list_items(desktop, surface); > +} > + > +static void > +surface_data_set_title(void *data, > + struct surface_data *surface_data, > + const char *title) > +{ > + struct desktop *desktop; > + struct surface *surface; > + > + desktop = data; > + > + surface = desktop_get_surface(desktop, surface_data); > + > + if (!surface) > + surface = desktop_create_surface(desktop, surface_data); > + > + if (surface->title) > + free(surface->title); > + surface->title = strdup(title); > + > + desktop_update_list_items(desktop, surface); > +} > + > +static void > +surface_data_destroy_handler(void *data, struct surface_data *surface_data) > +{ > + struct list_item *item, *next; > + struct desktop *desktop; > + struct output *output; > + struct panel *panel; > + > + desktop = data; > + > + surface_data_destroy(surface_data); > + > + wl_list_for_each(output, &desktop->outputs, link) { > + panel = output->panel; > + wl_list_for_each_safe(item, next, &panel->window_list, link) { > + if (surface_data == item->surface->surface_data) { > + desktop_destroy_surface(item->surface); > + return; > + } > + } > + } > +} > + > +static const struct surface_data_listener surface_data_listener = { > + surface_data_set_output_mask, > + surface_data_set_title, > + surface_data_destroy_handler > +}; > + > +static void > +surface_data_receive_surface_object(void *data, > + struct surface_data_manager *manager, > + struct surface_data *surface_data) > +{ > + surface_data_add_listener(surface_data, > + &surface_data_listener, data); </div></div>Just always create struct surface here (fold desktop_create_surface() into this function and set the surface_data user data. struct surface will need a pointer back the struct desktop. </blockquote><div> Yep, sounds good. </div><div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div> > +} > + > +static const struct surface_data_manager_listener surface_data_manager_listener = { > + surface_data_receive_surface_object > +}; > + > static struct background * > background_create(struct desktop *desktop) > { > @@ -1022,6 +1493,15 @@ desktop_destroy_outputs(struct desktop *desktop) > } > > static void > +desktop_destroy_surfaces(struct desktop *desktop) > +{ > + struct surface *surface, *next; > + > + wl_list_for_each_safe(surface, next, &desktop->surfaces, link) > + desktop_destroy_surface(surface); > +} > + > +static void > create_output(struct desktop *desktop, uint32_t id) > { > struct output *output; > @@ -1033,7 +1513,9 @@ create_output(struct desktop *desktop, uint32_t id) > output->output = > display_bind(desktop->display, id, &wl_output_interface, 1); > > - wl_list_insert(&desktop->outputs, &output->link); > + output->id = desktop->output_count++; > + > + wl_list_insert(desktop->outputs.prev, &output->link); > } > > static void > @@ -1046,6 +1528,12 @@ global_handler(struct display *display, uint32_t id, > desktop->shell = display_bind(desktop->display, > id, &desktop_shell_interface, 1); > desktop_shell_add_listener(desktop->shell, &listener, desktop); > + } else if (strcmp(interface, "surface_data_manager") == 0) { > + desktop->surface_data_manager = > + display_bind(display, id, > + &surface_data_manager_interface, 1); > + surface_data_manager_add_listener(desktop->surface_data_manager, > + &surface_data_manager_listener, desktop); > } else if (!strcmp(interface, "wl_output")) { > create_output(desktop, id); > } > @@ -1100,6 +1588,9 @@ int main(int argc, char *argv[]) > return -1; > } > > + wl_list_init(&desktop.surfaces); > + desktop.output_count = 0; > + > display_set_user_data(desktop.display, &desktop); > display_set_global_handler(desktop.display, global_handler); > > @@ -1133,6 +1624,7 @@ int main(int argc, char *argv[]) > > /* Cleanup */ > grab_surface_destroy(&desktop); > + desktop_destroy_surfaces(&desktop); > desktop_destroy_outputs(&desktop); > if (desktop.unlock_dialog) > unlock_dialog_destroy(desktop.unlock_dialog); > diff --git a/data/Makefile.am b/data/Makefile.am > index a7cc944..293d2bc 100644 > --- a/data/Makefile.am > +++ b/data/Makefile.am > @@ -7,6 +7,7 @@ dist_westondata_DATA = \ > terminal.png \ > border.png \ > icon_window.png \ > + list_item_icon.png \ > sign_close.png \ > sign_maximize.png \ > sign_minimize.png > diff --git a/data/list_item_icon.png b/data/list_item_icon.png > new file mode 100644 > index 0000000000000000000000000000000000000000..ac987e997313910ec3e02a11be8ccda0dfa581c7 > GIT binary patch > literal 176 > zcmeAS@N?(olHy`uVBq!ia0vp^0wB!61|;P_|4#%`Y)RhkE)4%caKYZ?lYt_f1s;*b > z3=G`DAk4@xYmNj^kiEpy*OmPaH$RJ!-UgFp7lA^yo-U3d7N_4%cI0AE;5i(0^Z))B > z|EBQpgWZ)55eu3H82wmS6TZt#lE`$@S@Cy{aM-usE&4llHh-IQIlXHpO8~cw3TFo| > P&>#j+S3j3^P6<r_g#|R0 > > literal 0 > HcmV?d00001 > > diff --git a/src/compositor.c b/src/compositor.c > index 587fded..686a11b 100644 > --- a/src/compositor.c > +++ b/src/compositor.c > @@ -374,6 +374,8 @@ weston_surface_update_output_mask(struct weston_surface *es, uint32_t mask) > if (1 << output->id & left) > wl_surface_send_leave(&es->surface.resource, resource); > } > + > + es->compositor->shell_interface.send_output_mask(es); > } > > static void > diff --git a/src/compositor.h b/src/compositor.h > index e5c579b..a60563a 100644 > --- a/src/compositor.h > +++ b/src/compositor.h > @@ -88,6 +88,7 @@ struct weston_shell_interface { > int (*move)(struct shell_surface *shsurf, struct weston_seat *ws); > int (*resize)(struct shell_surface *shsurf, > struct weston_seat *ws, uint32_t edges); > + void (*send_output_mask)(struct weston_surface *surface); </div></div>This doesn't feel right. The weston_shell_interface are for higher level modules (right now, only the xwm) to call into the shell module, not for the core to call into. This creates a core dependency on the shell module, which we don't want. A better approach would be a weston_surface wl_signal that we fire from the end of weston_surface_update_transform. </blockquote><div> Yes, a wl_signal fits much more nicely. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div> > }; > > diff --git a/src/shell.c b/src/shell.c > index cccfe66..86f2f6f 100644 > --- a/src/shell.c > +++ b/src/shell.c > @@ -97,6 +97,8 @@ struct desktop_shell { > uint32_t deathstamp; > } child; > > + struct wl_resource *surface_data_manager; > + > bool locked; > bool showing_input_panels; > bool prepare_event_sent; > @@ -201,6 +203,7 @@ struct shell_surface { > struct wl_list link; > > const struct weston_shell_client *client; > + struct wl_resource *surface_data; > }; > > struct shell_grab { > @@ -1396,6 +1399,118 @@ shell_surface_pong(struct wl_client *client, struct wl_resource *resource, > } > > static void > +surface_data_object_destroy(struct wl_resource *resource) > +{ > + struct shell_surface *shsurf = resource->data; > + > + free(resource); > + > + if (!shsurf) > + return; > + > + shsurf->surface_data = NULL; > +} > + > +static void > +surface_data_destroy_handler(struct wl_client *client, > + struct wl_resource *resource) > +{ > + wl_resource_destroy(resource); > +} > + > +static const struct surface_data_interface > + surface_data_implementation = { > + surface_data_destroy_handler > +}; > + > +static int > +create_surface_data(struct desktop_shell *shell, struct shell_surface *shsurf) > +{ > + struct wl_resource *surface_data; > + > + surface_data = malloc(sizeof *surface_data); > + if (surface_data == NULL) > + return -1; > + > + surface_data->data = shsurf; > + surface_data-><a href="http://object.id" target="_blank">object.id</a> = 0; > + surface_data->object.interface = &surface_data_interface; > + surface_data->destroy = surface_data_object_destroy; > + surface_data->object.implementation = > + (void (**)(void)) &surface_data_implementation; > + wl_signal_init(&surface_data->destroy_signal); > + > + wl_client_add_resource(shell->surface_data_manager->client, surface_data); > + > + shsurf->surface_data = surface_data; > + > + return 0; > +} > + > +static void > +send_surface_data_object(struct desktop_shell *shell, struct shell_surface *shsurf) > +{ > + surface_data_manager_send_surface_object(shell->surface_data_manager, > + shsurf->surface_data); > +} </div></div>This function doesn't add much, just call surface_data_manager_send_surface_object directly below? Or call it from create_surface_data? </blockquote><div> Yes, that's better. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div> > +static bool > +surface_is_window_list_candidate(struct weston_surface *surface, > + struct shell_surface *out) > +{ > + struct desktop_shell *shell; > + struct shell_surface *shsurf; > + > + shsurf = get_shell_surface(surface); > + if (!shsurf) > + return false; > + > + shell = shsurf->shell; > + > + if (!shell->surface_data_manager) > + return false; > + > + switch (shsurf->type) { > + default: > + case SHELL_SURFACE_TRANSIENT: > + case SHELL_SURFACE_POPUP: > + case SHELL_SURFACE_NONE: > + return false; > + case SHELL_SURFACE_FULLSCREEN: > + case SHELL_SURFACE_MAXIMIZED: > + case SHELL_SURFACE_TOPLEVEL: > + if (!shsurf->surface_data) { > + if (create_surface_data(shell, shsurf)) > + return false; > + send_surface_data_object(shell, shsurf); > + } > + *out = *shsurf; </div></div>This is copying the entire shsurf by value... you could return NULL for "not a candidate" or the shsurf pointer for "yes", instead of the bool, but don't copy the entire struct. But I'd rather just do all this up front when we create the shell_surface. </blockquote><div> Right, so I've restructured this significantly so that this is fixed in a cleaner way. We have to wait until shsurf->type is set to create the surface_data object since we need to know the type to decide if it is a window list candidate. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div> > + return true; > + } > +} > + > +static void > +send_surface_data_output_mask(struct weston_surface *surface) > +{ > + struct shell_surface shsurf; > + > + if (surface_is_window_list_candidate(surface, &shsurf)) > + surface_data_send_output_mask(shsurf.surface_data, > + surface->output_mask); > +} > + > +static void > +send_surface_data_title(struct weston_surface *surface) > +{ > + struct shell_surface shsurf; > + > + if (surface_is_window_list_candidate(surface, &shsurf)) </div>If we create the surface_data up front, we just need to call get_shell_surface(surface) and then check if shsurf->surface_data is not NULL. </blockquote><div> Yes, this works. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div> > + surface_data_send_title(shsurf.surface_data, > + shsurf.title == NULL ? > + "Surface" : shsurf.title); > +} > + > +static void > shell_surface_set_title(struct wl_client *client, > struct wl_resource *resource, const char *title) > { > @@ -1403,6 +1518,7 @@ shell_surface_set_title(struct wl_client *client, > > free(shsurf->title); > shsurf->title = strdup(title); > + send_surface_data_title(shsurf->surface); > } > > static void > @@ -1519,6 +1635,8 @@ set_surface_type(struct shell_surface *shsurf) > default: > break; > } > + > + send_surface_data_title(surface); > } > > static void > @@ -1930,6 +2048,8 @@ static const struct wl_shell_surface_interface shell_surface_implementation = { > static void > destroy_shell_surface(struct shell_surface *shsurf) > { > + if (shsurf->surface_data) > + surface_data_send_gone(shsurf->surface_data); > if (shsurf->popup.grab.pointer) > wl_pointer_end_grab(shsurf->popup.grab.pointer); > > @@ -2314,6 +2434,20 @@ static const struct desktop_shell_interface desktop_shell_implementation = { > desktop_shell_set_grab_surface > }; > > +static void > +surface_data_send_all_info(struct desktop_shell *shell) > +{ > + struct weston_surface *surface; > + struct workspace *ws; > + > + ws = get_current_workspace(shell); > + > + wl_list_for_each(surface, &ws->layer.surface_list, layer_link) { </div></div>And here we'd have to create the surface_data before sending out these events. </blockquote><div> Yep. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div> > + send_surface_data_output_mask(surface); > + send_surface_data_title(surface); > + } > +} > + > static enum shell_surface_type > get_shell_surface_type(struct weston_surface *surface) > { > @@ -3143,6 +3277,37 @@ bind_desktop_shell(struct wl_client *client, > } > > static void > +unbind_surface_data_manager(struct wl_resource *resource) > +{ > + struct desktop_shell *shell = resource->data; > + > + shell->surface_data_manager = NULL; > + free(resource); > +} > + > +static void > +bind_surface_data_manager(struct wl_client *client, > + void *data, uint32_t version, uint32_t id) > +{ > + struct desktop_shell *shell = data; > + struct wl_resource *resource; > + > + resource = wl_client_add_object(client, &surface_data_manager_interface, > + NULL, id, shell); > + > + if (client == shell->child.client) { > + resource->destroy = unbind_surface_data_manager; > + shell->surface_data_manager = resource; > + surface_data_send_all_info(shell); > + return; > + } > + > + wl_resource_post_error(resource, WL_DISPLAY_ERROR_INVALID_OBJECT, > + "permission to bind desktop_shell denied"); > + wl_resource_destroy(resource); > +} > + > +static void > screensaver_configure(struct weston_surface *surface, int32_t sx, int32_t sy) > { > struct desktop_shell *shell = surface->private; > @@ -3836,6 +4001,7 @@ module_init(struct weston_compositor *ec) > ec->shell_interface.set_transient = set_transient; > ec->shell_interface.move = surface_move; > ec->shell_interface.resize = surface_resize; > + ec->shell_interface.send_output_mask = send_surface_data_output_mask; > > wl_list_init(&shell->screensaver.surfaces); > wl_list_init(&shell->input_panel.surfaces); > @@ -3887,6 +4053,10 @@ module_init(struct weston_compositor *ec) > shell, bind_workspace_manager) == NULL) > return -1; > > + if (wl_display_add_global(ec->wl_display, &surface_data_manager_interface, > + shell, bind_surface_data_manager) == NULL) > + return -1; > + > shell->child.deathstamp = weston_compositor_get_time(); > > loop = wl_display_get_event_loop(ec->wl_display); > -- > 1.7.11.7 </div></div></blockquote><div> Thanks for the review Kristian, I will probably have this set updated and ready soon. - Scott </div></div>