[weston] cairo-util: fix shadows for small clients

Submitted by Marek Chalupa on Oct. 15, 2014, 8:16 a.m.

Details

Message ID 1413360984-5347-1-git-send-email-mchqwerty@gmail.com
State Superseded
Headers show

Not browsing as part of any series.

Commit Message

Marek Chalupa Oct. 15, 2014, 8:16 a.m.
If the client is small (< 128 pixels in any ward),
then the shadows overlap and create dark lines behind clients.
This is a problem mosly with pop-up menues. The lines become observable
when the menu has less than three items. The other case is when
the client doesn't restrict its size when resizing.

This fixes a part of the bug:
https://bugs.freedesktop.org/show_bug.cgi?id=78511

Signed-off-by: Marek Chalupa <mchqwerty@gmail.com>
---
 shared/cairo-util.c | 130 ++++++++++++++++++++++++++++++++++------------------
 1 file changed, 85 insertions(+), 45 deletions(-)

Patch hide | download patch | download mbox

diff --git a/shared/cairo-util.c b/shared/cairo-util.c
index 26286c5..e8a749e 100644
--- a/shared/cairo-util.c
+++ b/shared/cairo-util.c
@@ -142,7 +142,7 @@  tile_mask(cairo_t *cr, cairo_surface_t *surface,
 {
 	cairo_pattern_t *pattern;
 	cairo_matrix_t matrix;
-	int i, fx, fy, vmargin;
+	int i, fx, fy, shadow_height, shadow_width;
 
 	cairo_set_operator(cr, CAIRO_OPERATOR_OVER);
 	pattern = cairo_pattern_create_for_surface (surface);
@@ -157,63 +157,103 @@  tile_mask(cairo_t *cr, cairo_surface_t *surface,
 					    -y + fy * (128 - height));
 		cairo_pattern_set_matrix(pattern, &matrix);
 
-		if (fy)
-			vmargin = margin;
-		else
-			vmargin = top_margin;
+		if (fy) {
+			/* lower corner's margin */
+			shadow_height = margin;
+		} else {
+			/* upper corner's margin */
+			shadow_height = top_margin;
+		}
+
+		if (height < 2 * shadow_height) {
+			shadow_height = height / 2;
+
+			/* when the height is odd, we need some part of the
+			 * shadow to be one pixel higher. Let's choose the
+			 * upper one */
+			if (height % 2 != 0 && !fy)
+				++shadow_height;
+		}
+
+		shadow_width = margin;
+		if (width < 2 * shadow_width) {
+			shadow_width = width / 2;
+
+			/* the odd width case again */
+			if (width % 2 != 0 && !fx)
+				++shadow_width;
+		}
 
 		cairo_reset_clip(cr);
 		cairo_rectangle(cr,
-				x + fx * (width - margin),
-				y + fy * (height - vmargin),
-				margin, vmargin);
+				x + fx * (width - shadow_width),
+				y + fy * (height - shadow_height),
+				shadow_width, shadow_height);
 		cairo_clip (cr);
 		cairo_mask(cr, pattern);
 	}
 
+
+	shadow_width = width - 2 * margin;
+	shadow_height = top_margin;
+	if (height < 2 * shadow_height)
+		shadow_height = height / 2;
+
 	/* Top stretch */
-	cairo_matrix_init_translate(&matrix, 60, 0);
-	cairo_matrix_scale(&matrix, 8.0 / width, 1);
-	cairo_matrix_translate(&matrix, -x - width / 2, -y);
-	cairo_pattern_set_matrix(pattern, &matrix);
-	cairo_rectangle(cr, x + margin, y, width - 2 * margin, margin);
+	if (shadow_width > 0 && shadow_height) {
+		cairo_matrix_init_translate(&matrix, 60, 0);
+		cairo_matrix_scale(&matrix, 8.0 / width, 1);
+		cairo_matrix_translate(&matrix, -x - width / 2, -y);
+		cairo_pattern_set_matrix(pattern, &matrix);
+		cairo_rectangle(cr, x + margin, y, shadow_width, shadow_height);
 
-	cairo_reset_clip(cr);
-	cairo_rectangle(cr,
-			x + margin,
-			y,
-			width - 2 * margin, margin);
-	cairo_clip (cr);
-	cairo_mask(cr, pattern);
+		cairo_reset_clip(cr);
+		cairo_rectangle(cr,
+				x + margin, y,
+				shadow_width, shadow_height);
+		cairo_clip (cr);
+		cairo_mask(cr, pattern);
 
-	/* Bottom stretch */
-	cairo_matrix_translate(&matrix, 0, -height + 128);
-	cairo_pattern_set_matrix(pattern, &matrix);
+		/* Bottom stretch */
+		cairo_matrix_translate(&matrix, 0, -height + 128);
+		cairo_pattern_set_matrix(pattern, &matrix);
 
-	cairo_reset_clip(cr);
-	cairo_rectangle(cr, x + margin, y + height - margin,
-			width - 2 * margin, margin);
-	cairo_clip (cr);
-	cairo_mask(cr, pattern);
+		cairo_reset_clip(cr);
+		cairo_rectangle(cr, x + margin, y + height - margin,
+				shadow_width, margin);
+		cairo_clip (cr);
+		cairo_mask(cr, pattern);
+	}
 
-	/* Left stretch */
-	cairo_matrix_init_translate(&matrix, 0, 60);
-	cairo_matrix_scale(&matrix, 1, 8.0 / height);
-	cairo_matrix_translate(&matrix, -x, -y - height / 2);
-	cairo_pattern_set_matrix(pattern, &matrix);
-	cairo_reset_clip(cr);
-	cairo_rectangle(cr, x, y + margin, margin, height - 2 * margin);
-	cairo_clip (cr);
-	cairo_mask(cr, pattern);
+	shadow_width = margin;
+	if (width < 2 * shadow_width)
+		shadow_width = width / 2;
 
-	/* Right stretch */
-	cairo_matrix_translate(&matrix, -width + 128, 0);
-	cairo_pattern_set_matrix(pattern, &matrix);
-	cairo_rectangle(cr, x + width - margin, y + margin,
-			margin, height - 2 * margin);
-	cairo_reset_clip(cr);
-	cairo_clip (cr);
-	cairo_mask(cr, pattern);
+	shadow_height = height - margin - top_margin;
+
+	/* if height is smaller than sum of margins,
+	 * then the shadow is already done by the corners */
+	if (shadow_height > 0 && shadow_width) {
+		/* Left stretch */
+		cairo_matrix_init_translate(&matrix, 0, 60);
+		cairo_matrix_scale(&matrix, 1, 8.0 / height);
+		cairo_matrix_translate(&matrix, -x, -y - height / 2);
+		cairo_pattern_set_matrix(pattern, &matrix);
+		cairo_reset_clip(cr);
+		cairo_rectangle(cr, x, y + top_margin,
+				shadow_width, shadow_height);
+		cairo_clip (cr);
+		cairo_mask(cr, pattern);
+
+		/* Right stretch */
+		cairo_matrix_translate(&matrix, -width + 128, 0);
+		cairo_pattern_set_matrix(pattern, &matrix);
+		cairo_rectangle(cr, x + width - shadow_width, y + top_margin,
+				shadow_width, shadow_height);
+		cairo_reset_clip(cr);
+		cairo_clip (cr);
+		cairo_mask(cr, pattern);
+	}
 
 	cairo_pattern_destroy(pattern);
 	cairo_reset_clip(cr);

Comments

On 10/15/2014 01:16 AM, Marek Chalupa wrote:
>  +			shadow_height = height / 2;
> +
> +			/* when the height is odd, we need some part of the
> +			 * shadow to be one pixel higher. Let's choose the
> +			 * upper one */
> +			if (height % 2 != 0 && !fy)
> +				++shadow_height;

This does the same thing:
			shadow_height = (height + (fy ? 0 : 1)) / 2;

If there is any chance height is negative you want to use >>1 rather 
than /2. This is because it always rounds in the same direction (down 
rather than toward zero). I don't think negative height happens here, 
however.
Hi,

thanks for review!

On 27 October 2014 18:50, Bill Spitzak <spitzak@gmail.com> wrote:

>
>
> On 10/15/2014 01:16 AM, Marek Chalupa wrote:
>
>>  +                      shadow_height = height / 2;
>> +
>> +                       /* when the height is odd, we need some part of
>> the
>> +                        * shadow to be one pixel higher. Let's choose the
>> +                        * upper one */
>> +                       if (height % 2 != 0 && !fy)
>> +                               ++shadow_height;
>>
>
> This does the same thing:
>                         shadow_height = (height + (fy ? 0 : 1)) / 2;
>

Thinking about that, fy is always 0 or 1, so it will work even without the
ternary operator. And if we want to be really sure that fy is 0 or 1 (even
when
somebody will change its values range) we can do:

    shadow_height = (height + !fy) / 2;

It definitely looks better than the former code.


> If there is any chance height is negative you want to use >>1 rather than
> /2. This is because it always rounds in the same direction (down rather
> than toward zero). I don't think negative height happens here, however.
>

Good point. Hypothetically, the height can be negative, because its given
from the client - but then the mistake is on the client's side and it would
break much more things than one-pixel error in shadow. Maybe we should add
a check for negative values, but that belongs in another patch. I gonna go
for /2 because it does the same thing (assuming correct input) and it is
more readable.


> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>

Thanks!
Marek