After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 635268 - Mutter git breaks transparency in non-maximized windows
Mutter git breaks transparency in non-maximized windows
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: general
git master
Other Linux
: High normal
: ---
Assigned To: Owen Taylor
mutter-maint
: 643085 644555 646264 647169 647472 647550 649129 649397 652896 653427 654303 665714 (view as bug list)
Depends on:
Blocks: 627880
 
 
Reported: 2010-11-19 14:01 UTC by Andreas Proschofsky
Modified: 2011-12-07 11:59 UTC
See Also:
GNOME target: ---
GNOME version: 2.91/3.0


Attachments
Convert frame region handling to cairo regions (8.05 KB, patch)
2011-03-22 19:44 UTC, Owen Taylor
committed Details | Review
Only shadow ARGB windows with a frame outside the frame (28.10 KB, patch)
2011-03-22 19:44 UTC, Owen Taylor
reviewed Details | Review
Only shadow ARGB windows with a frame outside the frame (28.18 KB, patch)
2011-03-26 04:45 UTC, Owen Taylor
committed Details | Review
Fixes for ARGB window shadows patch (squash) (4.14 KB, patch)
2011-04-26 17:46 UTC, Owen Taylor
committed Details | Review
Fix XShape (915 bytes, patch)
2011-05-26 00:50 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review

Description Andreas Proschofsky 2010-11-19 14:01:52 UTC
Title says it all, since the recent commits regarding mutter shadow drawing transparency in gnome-terminal does not work anymore, background is solid black instead.
Comment 1 Florian Müllner 2011-02-23 16:54:06 UTC
*** Bug 643085 has been marked as a duplicate of this bug. ***
Comment 2 Christian Persch 2011-03-12 18:00:10 UTC
*** Bug 644555 has been marked as a duplicate of this bug. ***
Comment 3 Jeremy Nickurak 2011-03-20 16:39:41 UTC
Transparency DOES work when the window is maximized, just not when it's normal.

Applies to other windows that use transparency as well as gnome-terminal
Comment 4 drago01 2011-03-20 17:05:18 UTC
(In reply to comment #3)
> Transparency DOES work when the window is maximized, just not when it's normal.
> 
> Applies to other windows that use transparency as well as gnome-terminal

This is just a side effect of maximized windows not having shadows, we draw the shadow texture behind the window, thus when the window isn't fully opaque the shadow texture is visible.
Comment 5 Jeremy Nickurak 2011-03-22 05:10:55 UTC
So, the bug is that shadows are drawn behind the windows, instead of just on their edges?

This would seem to be a regression vs previous gnome releases (if not exactly mutter), breaking a feature i found very useful (for reading information from windows behind the current window).
Comment 6 John Keller 2011-03-22 09:15:04 UTC
(In reply to comment #5)
> So, the bug is that shadows are drawn behind the windows, instead of just on
> their edges?

Yes, it's exactly that. From bug 643085 (mistakenly filed against gnome-shell, since that's where it manifested for me):

In Metacity, gnome-terminal's shadow effect is projected from the edges of the window, leaving the region behind window's client area unaffected. This means that a theoretical 100% transparency in gnome-terminal's background would display the windows, etc. behind it as-is.

But in gnome-shell, the shadow effect is for the entire window: a rectangle casting a solid shadow - regardless of the transparency of the window's client area. This means that the shadow effect darkens all the items behind gnome-terminal, and so the background of the window's client area is much darker than expected.

Metacity's shadow effect is certainly less "accurate", geometrically speaking, but it leaves the area behind the window untouched. It also means that black text on a semi-transparent white background is readable, whereas the same combination in gnome-shell is hard to read because the background is that much darker, reducing contrast with the text.
Comment 7 Owen Taylor 2011-03-22 12:56:57 UTC
I have patches that allow gnome-terminal to set the region it wants to be shadowed as an X property, and to make mutter pick up this region and draw the correct shadow, but they are quite large, still have a bug or two, and in my opinion not 3.0 material. Simple alternative might be to draw no shadow for gnome-terminal, but assuming that most terminals aren't transparent, that hurts the users ability to tell if a terminal is focused and will make terminals look strange compared to other windows.
Comment 8 Dan Winship 2011-03-22 13:19:44 UTC
could we just clip the shadow to the region outside the window if the window has a 32-bit visual?
Comment 9 Dan Winship 2011-03-22 13:23:56 UTC
or... hm... "if the window has a 32-bit visual and is not shaped and does not suppress the window decorations"
Comment 10 Owen Taylor 2011-03-22 14:36:44 UTC
(In reply to comment #8)
> could we just clip the shadow to the region outside the window if the window
> has a 32-bit visual?

Trouble is it's hard to distinguish ARGB used for a antialiased shape of a round window vs. ARGB used for gnome-terminal. If the window has an antialiased outline, clipping to that will look ugly. Maybe if the window is ARGB and has a frame from Mutter (never antialiased) we could clip the shadow by the frame shape? I can look (if I get time) if there's a subset of my patches that could be used to implement that, since one of the things they do is add getting the frame shape as a cairo_region_t.
Comment 11 Jeremy Nickurak 2011-03-22 17:33:18 UTC
Is there any reason to special-case this for gnome-terminal? What about other ARGB apps?
Comment 12 Owen Taylor 2011-03-22 17:49:52 UTC
(In reply to comment #11)
> Is there any reason to special-case this for gnome-terminal? What about other
> ARGB apps?

See what I said in comment 10
Comment 13 Owen Taylor 2011-03-22 19:44:24 UTC
Created attachment 184118 [details] [review]
Convert frame region handling to cairo regions

It's useful to get frame shapes and manipulate them within Mutter, for
example so that the compositor can use them to clip drawing.
For this, we'll need the regions as cairo regions not X regions, so
convert frame shaping code to work in terms of cairo_region_t.
Comment 14 Owen Taylor 2011-03-22 19:44:27 UTC
Created attachment 184119 [details] [review]
Only shadow ARGB windows with a frame outside the frame

An ARGB window with a frame is likely something like a transparent
terminal. It looks awful (and breaks transparency) to draw a big
opaque black shadow under the window, so clip out the region under
the terminal from the shadow we draw.

Add meta_window_get_frame_bounds() to get a cairo region for the
outer bounds of the frame of a window, and modify the frame handling
code to notice changes to the frame shape and discard a cached
region. meta_frames_apply_shapes() is refactored so we can extract
meta_frames_get_frame_bounds() from it.
Comment 15 Dan Winship 2011-03-23 16:55:35 UTC
Comment on attachment 184118 [details] [review]
Convert frame region handling to cairo regions

this all seems plausible. The shape extension methods don't have very good man pages, so it's hard to say for sure that there isn't some subtle mixup, but I suppose it would show up pretty quickly.
Comment 16 Dan Winship 2011-03-23 17:58:19 UTC
Comment on attachment 184119 [details] [review]
Only shadow ARGB windows with a frame outside the frame

Everything looks like it should work. Some nitpicking:

>+ * @clip_strict: (allow-none): if %TRUE, drawing will be clipped

It's @clip_strictly, and (allow-none) is meaningless/invalid for a gboolean

>+                  src_x1 = (src_x[i] * (dest_rect.x + dest_rect.width - rect.x) +
>+                            src_x[i + 1] * (rect.x - dest_rect.x)) / dest_rect.width;

Looking at the code before and after this, I can figure out what this must be doing, but it really doesn't look like the math is right (even though it apparently is). Not sure what can be done there.

>@@ -706,17 +706,52 @@ meta_window_actor_paint (ClutterActor *actor)

>+      if (priv->argb32 && !clip)
>+        {
>+          cairo_region_t *frame_bounds = meta_window_get_frame_bounds (priv->window);
>+          if (frame_bounds)
...
>+                         priv->argb32 && priv->window->frame);

In the former case, you're relying on the fact that meta_window_get_frame_bounds() will return NULL for an undecorated window, while in the latter you're checking priv->window->frame explicitly. Weirdish. (You also do it the former way in meta_window_actor_set_visible_region_beneath().)

>+cairo_region_t *
>+meta_ui_get_frame_bounds (MetaUI  *ui,

oh good, there weren't enough other meta_*_get_frame_bounds methods yet... :-)
Comment 17 Owen Taylor 2011-03-23 20:10:51 UTC
(In reply to comment #15)
> (From update of attachment 184118 [details] [review])
> this all seems plausible. The shape extension methods don't have very good man
> pages, so it's hard to say for sure that there isn't some subtle mixup, but I
> suppose it would show up pretty quickly.

One thing to make you feel better is that ordering argument to XShapeCombineRectangles() is actually ignored. At some point keithp mentiond to m me that idea of passing it was that the server could save work if it was passed a set of rectangles that already matched the internal representation of an X region. Unfortunately, the effect of just stuffing a set of rectangles into an X region without validation is a server crash if the person passing them lied, so they had drop that from the server code and make no assumptions about the rectangle passed in.
Comment 18 Owen Taylor 2011-03-26 04:45:06 UTC
(In reply to comment #16)
> (From update of attachment 184119 [details] [review])
> Everything looks like it should work. Some nitpicking:
> 
> >+ * @clip_strict: (allow-none): if %TRUE, drawing will be clipped
> 
> It's @clip_strictly, and (allow-none) is meaningless/invalid for a gboolean
> 
> >+                  src_x1 = (src_x[i] * (dest_rect.x + dest_rect.width - rect.x) +
> >+                            src_x[i + 1] * (rect.x - dest_rect.x)) / dest_rect.width;
> 
> Looking at the code before and after this, I can figure out what this must be
> doing, but it really doesn't look like the math is right (even though it
> apparently is). Not sure what can be done there.

I'm going to claim that it reads as normal 1-d linear interpolation

 f(p) = (F(p_0) * dist(p, p_1) + F(p_1) * dist(p, p_0)) / dist(p_0, p_1)

Since for 1 dimension  dist(p, p_0) + dist(p, p_1) = dist(p_0, p_1), you can verify that:

 f(p_0) = F(p_0)
 f(p_1) = F(p_1)
 if F(p_0) = F(p_1) then f(p) == F(p_0) == F(p_1) for all p

If it's interpolation and it's linear, and the boundaries are correct, then it must be linear interpolation :-)

I'll add the comment:

                  /* Separately linear interpolate X and Y coordinates in the source                                     
                   * based on the destination X and Y coordinates */

But not sure it really adds anything.

> >@@ -706,17 +706,52 @@ meta_window_actor_paint (ClutterActor *actor)
> 
> >+      if (priv->argb32 && !clip)
> >+        {
> >+          cairo_region_t *frame_bounds = meta_window_get_frame_bounds (priv->window);
> >+          if (frame_bounds)
> ...
> >+                         priv->argb32 && priv->window->frame);
> 
> In the former case, you're relying on the fact that
> meta_window_get_frame_bounds() will return NULL for an undecorated window,
> while in the latter you're checking priv->window->frame explicitly. Weirdish.
> (You also do it the former way in
> meta_window_actor_set_visible_region_beneath().)

Hmm, at some point I was trying to code this on the idea that "priv->window->frame" isn't or shouldn't be accessible here outside src/core", but somehow lost track of the fact that it accessible and I left a leftover using it.

I'll convert it be consistently priv->argb32 && priv->window->frame, since I think that's the clearest represetnation of "ARGB window with a frame"
 
> >+cairo_region_t *
> >+meta_ui_get_frame_bounds (MetaUI  *ui,
> 
> oh good, there weren't enough other meta_*_get_frame_bounds methods yet... :-)

I had it in half the places as meta_*_get_frame_bounds_region() earlier. That wasn't better.

I'll attach a new version with these fixes, but since I just rejected Alex's big patch and Florian's big patch set as too risky for 3.0.0, I think I have to put off this one as well for consistency. :-)
Comment 19 Owen Taylor 2011-03-26 04:45:41 UTC
Created attachment 184266 [details] [review]
Only shadow ARGB windows with a frame outside the frame

An ARGB window with a frame is likely something like a transparent
terminal. It looks awful (and breaks transparency) to draw a big
opaque black shadow under the window, so clip out the region under
the terminal from the shadow we draw.

Add meta_window_get_frame_bounds() to get a cairo region for the
outer bounds of the frame of a window, and modify the frame handling
code to notice changes to the frame shape and discard a cached
region. meta_frames_apply_shapes() is refactored so we can extract
meta_frames_get_frame_bounds() from it.
Comment 20 Dan Winship 2011-03-26 11:50:51 UTC
(In reply to comment #18)
> I'll add the comment:
> 
>                   /* Separately linear interpolate X and Y coordinates in the
> source                                     
>                    * based on the destination X and Y coordinates */
> 
> But not sure it really adds anything.

If you think the existing code is obvious to people who have been doing graphics-related math recently (ie, not me), then feel free to leave it uncommented.

> I'll attach a new version with these fixes, but since I just rejected Alex's
> big patch and Florian's big patch set as too risky for 3.0.0, I think I have to
> put off this one as well for consistency. :-)

If we want *some* fix for 3.0.0, you could swap the has-frame and is-argb checks in meta_window_actor_has_shadow(), so that transparent terminals would end up with no shadow at all, which might be better than the current situation.

(Hm... and it uses (priv->argb32 || priv->opacity != 0xff) there, which is probably what you want in the other cases as well?)
Comment 21 Dan Winship 2011-03-30 19:25:20 UTC
*** Bug 646264 has been marked as a duplicate of this bug. ***
Comment 22 Christian Persch 2011-04-08 14:43:20 UTC
*** Bug 647169 has been marked as a duplicate of this bug. ***
Comment 23 Christian Persch 2011-04-11 18:02:53 UTC
*** Bug 647472 has been marked as a duplicate of this bug. ***
Comment 24 Christian Persch 2011-04-12 12:00:38 UTC
*** Bug 647550 has been marked as a duplicate of this bug. ***
Comment 25 Tilo Prütz 2011-04-15 12:42:27 UTC
Hello, I stumbled about this annoying bug, too.

I have no knowledge about mutter or metacity but the latter has got this case right (as mentioned in Bug 646264).
Is it impossible to apply a similar solution as used in metacity? Or do they all this stuff totally different, so that the problem didn't even occur in metacity?
Comment 26 Dan Winship 2011-04-26 14:15:17 UTC
Comment on attachment 184266 [details] [review]
Only shadow ARGB windows with a frame outside the frame

>+       * transparency rather than a shape.  We don't want to show
>+       * the shadow through the translucent areas since the shadow
>+       * is wrong for translucent windows (it should be
>+       * translucent itself and colored), and not only that will
>+       * look horribly wrong - a misplaced big black blob. As a

There's some missing or extra text near "not only that".
Comment 27 Owen Taylor 2011-04-26 16:35:17 UTC
Actually, no missing or extra text - but some missing punctuation and emphasis

 and not only that will look horribly wrong 
 and not only that, will /look/ horribly wrong
Comment 28 Owen Taylor 2011-04-26 17:46:13 UTC
Created attachment 186674 [details] [review]
Fixes for ARGB window shadows patch (squash)

- Fix punctuation in comment
- Handle _NET_WM_WINDOW_OPACITY windows like ARGB ones.
Comment 29 Owen Taylor 2011-04-27 14:32:26 UTC
Patches pushed. While this approach is sort of simple and hacky, the
much more complicated approach I had mostly finished patches for where
gnome-terminal exported a region to be shadowed didn't actually look
nearly as good. So I'm going to go ahead and close the bug.

This is going to need revisiting, however, if and when
we get antialiased border shapes since we can't just clip the shadow
if the border shape isn't pixel-exact. (It probably looks fine if
the shadow runs in under the border-AA pixels, but it won't look fine
if the shadow stops short of them)

Attachment 184118 [details] pushed as c3a04bf - Convert frame region handling to cairo regions
Attachment 184266 [details] pushed as 67c3c93 - Only shadow ARGB windows with a frame outside the frame
Comment 30 Christian Persch 2011-05-02 09:31:10 UTC
*** Bug 649129 has been marked as a duplicate of this bug. ***
Comment 31 Christian Persch 2011-05-04 17:43:32 UTC
*** Bug 649397 has been marked as a duplicate of this bug. ***
Comment 32 Jasper St. Pierre (not reading bugmail) 2011-05-26 00:50:15 UTC
(In reply to comment #15)
> (From update of attachment 184118 [details] [review])
> this all seems plausible. The shape extension methods don't have very good man
> pages, so it's hard to say for sure that there isn't some subtle mixup, but I
> suppose it would show up pretty quickly.

How right you were. This broke the XShape extension, but thankfully it's a relatively easy fix. Fix attached.
Comment 33 Jasper St. Pierre (not reading bugmail) 2011-05-26 00:50:47 UTC
Created attachment 188630 [details] [review]
Fix XShape

Commit c3a04bf unintentionally broke XShape handling. By studying the code
extremely carefully, I found this inconsistency with the code that was
there before.
Comment 34 Christian Persch 2011-06-18 14:13:23 UTC
*** Bug 652896 has been marked as a duplicate of this bug. ***
Comment 35 Owen Taylor 2011-06-29 18:45:18 UTC
Review of attachment 188630 [details] [review]:

Good to commit
Comment 36 Jasper St. Pierre (not reading bugmail) 2011-06-29 20:08:08 UTC
Attachment 188630 [details] pushed as d752096 - Fix XShape
Comment 37 Jeremy Nickurak 2011-06-30 21:14:31 UTC
Applied these into 3.0.2 locally, and it's worked well. Thanks!
Comment 38 Dan Winship 2011-07-05 19:25:50 UTC
*** Bug 653427 has been marked as a duplicate of this bug. ***
Comment 39 Christian Persch 2011-07-26 12:00:16 UTC
*** Bug 654303 has been marked as a duplicate of this bug. ***
Comment 40 Christian Persch 2011-12-07 11:59:35 UTC
*** Bug 665714 has been marked as a duplicate of this bug. ***