*** Bug 643085 has been marked as a duplicate of this bug. ***

*** Bug 644555 has been marked as a duplicate of this bug. ***

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

(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.

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).

(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.

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.

could we just clip the shadow to the region outside the window if the window has a 32-bit visual?

or... hm... "if the window has a 32-bit visual and is not shaped and does not suppress the window decorations"

(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.

Is there any reason to special-case this for gnome-terminal? What about other ARGB apps?

(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

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.

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 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 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... :-)

(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.

(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. :-)

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.

(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?)

*** Bug 646264 has been marked as a duplicate of this bug. ***

*** Bug 647169 has been marked as a duplicate of this bug. ***

*** Bug 647472 has been marked as a duplicate of this bug. ***

*** Bug 647550 has been marked as a duplicate of this bug. ***

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 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".

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

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.

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

*** Bug 649129 has been marked as a duplicate of this bug. ***

*** Bug 649397 has been marked as a duplicate of this bug. ***

(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.

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.

*** Bug 652896 has been marked as a duplicate of this bug. ***

Review of attachment 188630 [details] [review]:

Good to commit

Attachment 188630 [details] pushed as d752096 - Fix XShape

Applied these into 3.0.2 locally, and it's worked well. Thanks!

*** Bug 653427 has been marked as a duplicate of this bug. ***

*** Bug 654303 has been marked as a duplicate of this bug. ***

*** Bug 665714 has been marked as a duplicate of this bug. ***