<div dir="ltr">Hi Derek,<br><div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Mar 23, 2018 at 11:59 AM, Derek Foreman <span dir="ltr"><<a href="mailto:derekf@osg.samsung.com" target="_blank">derekf@osg.samsung.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="m_5655982582723184870m_-4841399398056735251HOEnZb"><div class="m_5655982582723184870m_-4841399398056735251h5">On 2018-03-22 01:04 AM, Scott Moreau wrote:<br>
> This scales the icon cairo surface for the titlebar if it isn't the<br>
> target size.<br>
><br>
> shared/cairo-util: Add surface resizing function to be used for this<br>
> case and other potential cases.<br>
> ---<br>
><br>
> Changed in v2:<br>
><br>
> - Rebase to [PATCH 1/1 v3] xwm: Choose icon closest to target size<br>
><br>
> shared/cairo-util.c | 61 ++++++++++++++++++++++++++++++<wbr>+++++++++++++++++<br>
> shared/cairo-util.h | 4 ++++<br>
> xwayland/window-manager.c | 9 ++++++-<br>
> 3 files changed, 73 insertions(+), 1 deletion(-)<br>
><br>
> diff --git a/shared/cairo-util.c b/shared/cairo-util.c<br>
> index d71e0ed..b91b5bc 100644<br>
> --- a/shared/cairo-util.c<br>
> +++ b/shared/cairo-util.c<br>
> @@ -142,6 +142,67 @@ blur_surface(cairo_surface_t *surface, int margin)<br>
> return 0;<br>
> }<br>
><br>
> +static cairo_surface_t *<br>
> +scale_surface(cairo_surface_t *source, cairo_filter_t filter,<br>
> + double width, double height)<br>
> +{<br>
> + cairo_surface_t *dest;<br>
> + cairo_t *cr;<br>
> + int old_width, old_height;<br>
> +<br>
> + old_width = cairo_image_surface_get_width(<wbr>source);<br>
> + old_height = cairo_image_surface_get_height<wbr>(source);<br>
> +<br>
> + dest = cairo_surface_create_similar(s<wbr>ource, CAIRO_CONTENT_COLOR_ALPHA,<br>
> + width, height);<br>
> + cr = cairo_create (dest);<br>
> +<br>
> + cairo_scale (cr, width / old_width, height / old_height);<br>
> + cairo_set_source_surface (cr, source, 0, 0);<br>
> +<br>
> + cairo_pattern_set_extend (cairo_get_source(cr), CAIRO_EXTEND_REFLECT);<br>
> +<br>
> + cairo_set_operator (cr, CAIRO_OPERATOR_SOURCE);<br>
> +<br>
> + cairo_paint (cr);<br>
> +<br>
> + cairo_destroy (cr);<br>
> +<br>
> + cairo_surface_destroy(source)<wbr>;<br>
<br>
</div></div>This function looks like it solves the problem without leaking anything<br>
- would love to see more review from others with cairo experience.<br>
<span><br>
> +<br>
> + return dest;<br>
> +}<br>
> +<br>
> +cairo_surface_t *<br>
<br>
</span>I would prefer this function be named something else, so a casual reader<br>
with no knowledge of cairo API (like me!) doesn't think it looks like a<br>
cairo function name.<br></blockquote><div><br></div><div>I agree it could use a rename from cairo_resize_surface to something else.<br>Any suggestions?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="m_5655982582723184870m_-4841399398056735251h5"><br>
> +cairo_resize_surface(cairo_su<wbr>rface_t *source, cairo_filter_t filter,<br>
> + int width, int height)<br>
> +{<br>
> + if (!filter)<br>
> + filter = CAIRO_FILTER_BEST;<br>
> +<br>
> + while((cairo_image_surface_ge<wbr>t_width(source) / 2.0f) > width)<br>
> + source = scale_surface(source, filter,<br>
> + cairo_image_surface_get_width<wbr>(source) / 2.0f,<br>
> + cairo_image_surface_get_heigh<wbr>t(source));<br>
> +<br>
> + while((cairo_image_surface_ge<wbr>t_height(source) / 2.0f) > height)<br>
> + source = scale_surface(source, filter,<br>
> + cairo_image_surface_get_width<wbr>(source),<br>
> + cairo_image_surface_get_heigh<wbr>t(source) / 2.0f);<br>
> +<br>
> + while((cairo_image_surface_ge<wbr>t_width(source) * 2.0f) < width)<br>
> + source = scale_surface(source, filter,<br>
> + cairo_image_surface_get_width<wbr>(source) * 2.0f,<br>
> + cairo_image_surface_get_heigh<wbr>t(source));<br>
> +<br>
> + while((cairo_image_surface_ge<wbr>t_height(source) * 2.0f) < height)<br>
> + source = scale_surface(source, filter,<br>
> + cairo_image_surface_get_width<wbr>(source),<br>
> + cairo_image_surface_get_heigh<wbr>t(source) * 2.0f);<br>
<br>
</div></div>This chain of operations feels complicated - is it not possible to<br>
calculate the optimal target size and just call scale_surface once?<br></blockquote><div><br></div><div>Yes it is possible, but the point of this is to yield a better quality.<br>Scaling directly to the target size can have undesirable results.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="m_5655982582723184870m_-4841399398056735251h5"><br>
> +<br>
> + return scale_surface(source, filter, width, height);<br>
> +}<br>
> +<br>
> void<br>
> render_shadow(cairo_t *cr, cairo_surface_t *surface,<br>
> int x, int y, int width, int height, int margin, int top_margin)<br>
> diff --git a/shared/cairo-util.h b/shared/cairo-util.h<br>
> index 6fd11f6..3bd2b0c 100644<br>
> --- a/shared/cairo-util.h<br>
> +++ b/shared/cairo-util.h<br>
> @@ -49,6 +49,10 @@ rounded_rect(cairo_t *cr, int x0, int y0, int x1, int y1, int radius);<br>
> cairo_surface_t *<br>
> load_cairo_surface(const char *filename);<br>
><br>
> +cairo_surface_t *<br>
> +cairo_resize_surface(cairo_su<wbr>rface_t *source, cairo_filter_t filter,<br>
> + int width, int height);<br>
> +<br>
> struct theme {<br>
> cairo_surface_t *active_frame;<br>
> cairo_surface_t *inactive_frame;<br>
> diff --git a/xwayland/window-manager.c b/xwayland/window-manager.c<br>
> index 30c4b1c..6057490 100644<br>
> --- a/xwayland/window-manager.c<br>
> +++ b/xwayland/window-manager.c<br>
> @@ -1416,6 +1416,7 @@ get_icon_size_from_data(int target_width, int target_height,<br>
> return ret;<br>
> }<br>
><br>
> +#define ICON_SIZE 16<br>
> static void<br>
> weston_wm_handle_icon(struct weston_wm *wm, struct weston_wm_window *window)<br>
> {<br>
> @@ -1437,7 +1438,8 @@ weston_wm_handle_icon(struct weston_wm *wm, struct weston_wm_window *window)<br>
><br>
> data = xcb_get_property_value(reply);<br>
><br>
<br>
</div></div>Just occurred to me...for the last patch I guess. I think something<br>
needs to check data for NULLness here (or get_icon_size_from_data be<br>
made safe against a NULL data...)<br></blockquote><div><br></div><div>I see. I will make get_icon_size_from_data() check for NULL data.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span><br>
> - data = get_icon_size_from_data(16, 16, &width, &height, length, data);<br>
> + data = get_icon_size_from_data(ICON_S<wbr>IZE, ICON_SIZE,<br>
> + &width, &height, length, data);<br>
<br>
</span>Too many tabs (we use 8 wide tabs...)<br>
There are a couple of places like that.<br></blockquote><div><br></div><div>I had my editor set to 4 wide tabs, will fix.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span><br>
><br>
> if (!data)<br>
> return;<br>
> @@ -1453,11 +1455,16 @@ weston_wm_handle_icon(struct weston_wm *wm, struct weston_wm_window *window)<br>
> return;<br>
> }<br>
><br>
> + if (width != ICON_SIZE || height != ICON_SIZE)<br>
> + new_surface =<br>
> + cairo_resize_surface(new_surf<wbr>ace, 0, ICON_SIZE, ICON_SIZE);<br>
> +<br>
> if (window->frame)<br>
> frame_set_icon(window->frame, new_surface);<br>
> else /* We don’t have a frame yet */<br>
> window->icon_surface = new_surface;<br>
> }<br>
<br>
</span>As mentioned on other patch, I think the define for this should be in<br>
the other patch and not get #undef...<br></blockquote><div><br></div><div>Noted.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span><br>
> +#undef ICON_SIZE<br>
><br>
> static void<br>
> weston_wm_handle_property_noti<wbr>fy(struct weston_wm *wm, xcb_generic_event_t *event)<br>
><br>
<br>
</span>Thanks,<br>
Derek<br>
</blockquote></div><br></div><div class="gmail_extra">Thanks,<br></div><div class="gmail_extra">Scott<br></div></div></div>