Yeah, it's on my list of stuff to look into ... the tricky thing is to figure out when to show a shadow and when not to.

last I asked the designers, they wanted that shadow, but maybe that's changed with more experience with the big shadows.

Add me to the list of people who want that shadow gone. It makes it look like the two windows are on different height layers which ruins the whole experience of tiling. I also find it annoying that fullscreen windows throw shadows onto adjacent displays, BTW although that was also present in Gnome 2.

I think the correct behaviour should be to have no shadow when two window sides of equivalent size are exactly level with each other.

So if two windows, one 30x120 and one 560x120, were precisely aligned next to each other - with no gap at all between them, and at the same vertical position - there should be no shadow. If the same two windows were offset (so one was higher than the other), or if there were a gap between them (even a one pixel gap), there should be a shadow.

Same for two windows, 120x30 and 120x560, precisely aligned one above the other.

Special case would be for maximized windows across multiple displays: again no shadow should be cast, even though one window will be shorter than the others - the one which is displayed on the monitor with the panel on it.

A simpler option would be to have no shadow displayed for any two window edges which are right next to each other, no matter the relative sizes, I guess.

Anyway, I'm definitely +1 to changing this, current situation looks pretty icky for some of my windows (I often have Evo maximized on my left-hand monitor and Firefox on my right-hand monitor, and the silly quasi-shadow stuck down the very edge of one monitor or the other just looks odd).

Created attachment 194542 [details] [review]
MetaWindowActor: clip shadows for tiled windows and monitor edges

For tiled windows the shadow in between them ruins the 2 tiles side by side
illusion. Also, if a window is visible only on a single monitor we don't want
its shadow to "bleed" to adjacent monitors.

--
This should fix the most glaring visual oddities with shadows.

Problems:

1) there's a crasher in there which I shall look into tomorrow. Hints on why
it's crashing there highly appreciated though :-)

2) I'm including screen-private.h here to get to all the monitor_infos, maybe
a public getter would be better?

3) there's a funny situation with MetaRectangle and
cairo_rectangle_int_t. Luckily they're the same and thus can just be
casted. We could also do a full mutter port to cairo_rectangle_int_t to be
cleaner. Would that be welcome? Any other idea to not have to make those
casts?

4) I'm a bit worried that all this might be too expensive to do on the paint
handler. Thoughts?

5) I did this without any designer input. This bug has the magic keyword
though so I'm silently wishing that they eventually try and comment on this.

6) depends on the patches in bug 645153. The tiling windows stuff could be
split into a 2nd patch here though.

7) probably lots of other issues and corner cases I didn't think about. That's
why we have reviews ;-)

Why not just reduce the shadow pattern when a window is in tile mode?

> We could also do a full mutter port to cairo_rectangle_int_t to be
> cleaner. Would that be welcome? Any other idea to not have to make those
> casts?

If you try this, you'll realize that cairo_rectangle_int_t isn't introspectable. We could probably just fix that in gjs, though.

Created attachment 195662 [details] [review]
MetaWindowActor: don't draw shadows for tiled windows with a match

The shadow between 2 tiled windows ruins the visual tiles effect.

--
This only addresses the tiled windows shadow. It depends on the patches in bug
645153.

The shadow clipping to a single monitor is totally independent now and I'll
submit it into a new bug report.

Review of attachment 195662 [details] [review]:

A quick comment to the effect of "If we have two snap-tiled windows, we don't want the shadow to obstruct the other window." wouldn't be bad. Otherwise, looks fine (it's only two lines).

The shadow definitely doesn't look right in the tiled case. The two windows are really on the same level. With the unfocused state of the gtk theme we have enough indication of which window receives focus. While wrong, I don't even think the shadow makes it obvious the two windows are at different levels, it's just looking odd.

We already specialcase the titebar decorations, I think it's not outrageous to ask to specialcase the shadow too.

I would really like to see this "no shadow for tiled" in.

Created attachment 208359 [details] [review]
window: Introduce meta_window_find_tile_match()

Finds a matching tiled window. This is the topmost tiled window in a
complementary tile mode that is:

 - on the same monitor;
 - on the same workspace;
 - spanning the remaining monitor width;
 - has no other window overlapping it.

Created attachment 208361 [details] [review]
MetaWindowActor: don't draw shadows for tiled windows with a match

--
Added the comment that Jasper suggested.

(In reply to comment #10)
> Created an attachment (id=208359) [details] [review]
> window: Introduce meta_window_find_tile_match()
> 
> Finds a matching tiled window. This is the topmost tiled window in a
> complementary tile mode that is:
> 
>  - on the same monitor;
>  - on the same workspace;
>  - spanning the remaining monitor width;
>  - has no other window overlapping it.

I don't quite understand the logic behind those rules - if I have two windows tiled side-by-side, why is there a shadow if I have a 3rd window floating above the others?

Uhm, I think it used to look odd, with the different style for the unfocused window having the drop shadow seems logic to me, since tiled windows are two different entities and they should look and behave like all other windows.
Another option would be to somehow melt the two windows making them behave like a single object, but this would require some research and some design iteration to make it right. To summarize I think we should leave the current behaviour for now.

Created attachment 208367 [details] [review]
window: Introduce meta_window_find_tile_match()

Finds a matching tiled window. This is the topmost tiled window in a
complementary tile mode that is:

 - on the same monitor;
 - on the same workspace;
 - spanning the remaining monitor width;
 - not partially overlaped by a 3rd window in between.

(In reply to comment #12)
> I don't quite understand the logic behind those rules - if I have two windows
> tiled side-by-side, why is there a shadow if I have a 3rd window floating above
> the others?

Indeed. I refined the behavior now.

Review of attachment 208367 [details] [review]:

Definitively better - still unsure about some of the edge cases, see comments below

::: src/core/window.c
@@ +10699,3 @@
+  match = tmp = window;
+  while ((tmp = meta_stack_get_above (window->screen->stack, match, TRUE)))
+    match = tmp;

Are you sure you cannot use

  match = meta_stack_get_top (window->screen->stack); ?

@@ +10708,3 @@
+        break;
+    }
+  while ((match = meta_stack_get_below (window->screen->stack, match, TRUE)));

What exactly is the reasoning behind restricting the match to one layer? It's a bit odd that with two tiled windows (and nothing else on the workspace), I can get a shadow by middle clicking the titlebar or setting one of the windows as "keep-above" ...

To avoid assignment in the condition, I'd probably write that as

for (match = meta_stack_get_top (stack);
     match;
     match = meta_stack_get_below (stack))

@@ +10730,3 @@
+
+      if ((above = meta_stack_get_above (window->screen->stack, bottommost, TRUE)))
+        do

Ugh - if ((foo = something())) do { /* stuff */ } while ((foo = something()))

for (above = meta_stack_get_above (stack, bottommost, TRUE /* FALSE? */);
     above != topmost;
     above = meta_stack_get_above (stack, above, TRUE /* FALSE? */))

@@ +10738,3 @@
+
+            if (meta_rectangle_contains_rect (&bottommost_rect, &above_rect))
+              continue;

This still misses the case where the parts of above_rect that are not within bottommost_rect are off-screen (same for topmost_rect below).

(On a side note - I'm slightly worried about the amount of calculations we do here, you later use this function in window_actor_has_shadow(), which is called every frame for each window)

(In reply to comment #15)
> Ugh - if ((foo = something())) do { /* stuff */ } while ((foo = something()))
> 
> for (above = meta_stack_get_above (stack, bottommost, TRUE /* FALSE? */);
>      above != topmost;

above && above != topmost ...

Created attachment 208463 [details] [review]
window: Introduce meta_window_get_tile_match()

--
Thanks for the review Florian.

The layer restricting was just me trying to be more careful than necessary
which ended up being worse... Fixed now.

The ugly looking loops were just... ugly, should be ok now.

I'm now handling the 3rd window in between as I intended initially (the logic
in the code was wrong though).

Finally, calling this from MetaWindowActor's drawing code was arguably the
ugliest thing here :-) . I've now reworked it to cache the value in MetaWindow
and compute it when there are stacking changes or a window is moved/resized
which, I think, covers everything relevant but I might very well be missing
some cases.

Created attachment 208464 [details] [review]
MetaWindowActor: don't draw shadows for tiled windows with a match

--
Rebased.

Review of attachment 208463 [details] [review]:

Looks mostly good, what's left is basically style comments.

::: src/core/stack.c
@@ +1312,3 @@
   stack->last_root_children_stacked = root_children_stacked;
 
+  meta_stack_compute_tile_matches (stack, NULL);

"update tile matches whenever we call stack_sync_to_server()" sounds about right; having the method call _in_ stack_sync_to_server() does not (we are making each window update some internal state, nothing at all server related).

I'm not entirely convinced by the window->stack->foreach-window dance, but can't really think of something better (on a side note: not too sure about the name stack_compute_tile_matches() either, as the stack itself does not really know about tile matches - maybe meta_stack_update_window_tile_matches()?)

::: src/core/window.c
@@ +10683,3 @@
+ *  - on the same workspace;
+ *  - spanning the remaining monitor width;
+ *  - not partially overlaped by a 3rd window in between.

overlaped => overlapped, but the condition does not match the code anyway.

@@ +10706,3 @@
+      window->minimized ||
+      ! META_WINDOW_TILED_SIDE_BY_SIDE (window))
+    goto set_and_exit;

goto is acceptable if it allows you to avoid duplicating lots of clean up code or to break out of a nested loop - neither is the case here. Just replace

match = NULL;

if (foo || bar || baz)
  goto set_and_exit;

with

window->tile_match = NULL;

if (foo || bar || baz)
  return;


(the goto below is completely equivalent to 'break')

@@ +10719,3 @@
+       match = meta_stack_get_below (stack, match, FALSE))
+    {
+      if (!match->shaded && !match->minimized &&

My preference would be one line per condition, but not a strong opinion.

@@ +10760,3 @@
+
+          if (meta_rectangle_overlap (&above_rect, &bottommost_rect) &&
+              meta_rectangle_overlap (&above_rect, &topmost_rect))

I can still construct a slightly odd case :-)

Window A: tiled left
Window B: tiled right, keep-above
Window C: normal, narrow (< 1/2 monitor)

Moving window C from left to right, there's no shadow when it is entirely above A or below B, but the shadow appears while crossing. It kinda looks like windows A and B open up to let window C through, which is actually not completely crazy, so it's probably fine to leave it at that.

Review of attachment 208464 [details] [review]:

Code looks good, it's up to the designers if we want this:

  Jakub, Lapo ...   F I G H T !

Created attachment 208651 [details] [review]
window: Introduce meta_window_get_tile_match()

--
(In reply to comment #19)
> ::: src/core/stack.c
> @@ +1312,3 @@
>    stack->last_root_children_stacked = root_children_stacked;
>
> +  meta_stack_compute_tile_matches (stack, NULL);
>
> "update tile matches whenever we call stack_sync_to_server()" sounds about
> right; having the method call _in_ stack_sync_to_server() does not (we are
> making each window update some internal state, nothing at all server related).

I agree but doing either of a) creating a new wrapper function just to call
stack_sync_to_server() and meta_stack_compute_tile_matches() or b) adding a
meta_stack_compute_tile_matches() call after every stack_sync_to_server(),
sound like a lot of code churn for little gain.

> I'm not entirely convinced by the window->stack->foreach-window dance, but
> can't really think of something better

I have the same gut feel and that's why for the move/resize case I restrict
the dance to windows of that single workspace. But here, on stacking changes,
I didn't see a way to get to the relevant workspace.

Generally you really have to recompute this stuff for all windows of a
workspace because any single movement either in x, y or z coordinates can
result in a match being available or one going away. We could maybe optimize
it by skipping totally obscured windows etc. but that's just moving
computations from one place to another...

> (on a side note: not too sure about the
> name stack_compute_tile_matches() either, as the stack itself does not really
> know about tile matches - maybe meta_stack_update_window_tile_matches()?)

Agree and renamed it accordingly.

> ::: src/core/window.c
> @@ +10683,3 @@
> + *  - on the same workspace;
> + *  - spanning the remaining monitor width;
> + *  - not partially overlaped by a 3rd window in between.
>
> overlaped => overlapped, but the condition does not match the code anyway.

flyspell-mode failed me! But see below.

> @@ +10706,3 @@
> +      window->minimized ||
> +      ! META_WINDOW_TILED_SIDE_BY_SIDE (window))
> +    goto set_and_exit;
>
> goto is acceptable if it allows you to avoid duplicating lots of clean up code
> or to break out of a nested loop - neither is the case here. Just replace

Yeah, I didn't like it either but sometimes when I'm looking too much at some
code I just can't find a more elegant way. Getting to it after a break usually
results in a better looking solution.

> @@ +10719,3 @@
> +       match = meta_stack_get_below (stack, match, FALSE))
> +    {
> +      if (!match->shaded && !match->minimized &&
>
> My preference would be one line per condition, but not a strong opinion.

Yep.

> @@ +10760,3 @@
> +
> +          if (meta_rectangle_overlap (&above_rect, &bottommost_rect) &&
> +              meta_rectangle_overlap (&above_rect, &topmost_rect))
>
> I can still construct a slightly odd case :-)
>
> Window A: tiled left
> Window B: tiled right, keep-above
> Window C: normal, narrow (< 1/2 monitor)
>
> Moving window C from left to right, there's no shadow when it is entirely above
> A or below B, but the shadow appears while crossing. It kinda looks like
> windows A and B open up to let window C through, which is actually not
> completely crazy, so it's probably fine to leave it at that.

I actually tested that ;-) but you don't even need such an odd case. Having
any window stacked between 2 tiles without the shadow of the topmost tile
visible looks really odd to me. Handling that is the whole point of the 2nd
part of the function and why it looks so long for a seemingly simple task...

(In reply to comment #21)
> Created an attachment (id=208651) [details] [review]
> window: Introduce meta_window_get_tile_match()
> 
> --
> (In reply to comment #19)
> > ::: src/core/stack.c
> > @@ +1312,3 @@
> >    stack->last_root_children_stacked = root_children_stacked;
> >
> > +  meta_stack_compute_tile_matches (stack, NULL);
> >
> > "update tile matches whenever we call stack_sync_to_server()" sounds about
> > right; having the method call _in_ stack_sync_to_server() does not (we are
> > making each window update some internal state, nothing at all server related).
> 
> I agree but doing either of a) creating a new wrapper function just to call
> stack_sync_to_server() and meta_stack_compute_tile_matches() or b) adding a
> meta_stack_compute_tile_matches() call after every stack_sync_to_server(),
> sound like a lot of code churn for little gain.

I agree about a), but I don't think b) is that bad. Alternatively c) rename stack_sync_to_server() to stack_sync_to_server_and_update_tile_matches would work as well.



> > Moving window C from left to right, there's no shadow when it is entirely above
> > A or below B, but the shadow appears while crossing. It kinda looks like
> > windows A and B open up to let window C through, which is actually not
> > completely crazy, so it's probably fine to leave it at that.
> 
> I actually tested that ;-) but you don't even need such an odd case. Having
> any window stacked between 2 tiles without the shadow of the topmost tile
> visible looks really odd to me. Handling that is the whole point of the 2nd
> part of the function and why it looks so long for a seemingly simple task...

Well, the (easier) alternative would be to always show the shadows when a window is stacked between the tiled ones (e.g. ignore position/overlap/whatever) :)
(Not saying you should do that; your code is trickier, but covers more cases where it is nice to hide the shadows)

Created attachment 209640 [details] [review]
window: Introduce meta_window_get_tile_match()

--
(In reply to comment #22)
> (In reply to comment #21)
> > I agree but doing either of a) creating a new wrapper function just to call
> > stack_sync_to_server() and meta_stack_compute_tile_matches() or b) adding a
> > meta_stack_compute_tile_matches() call after every stack_sync_to_server(),
> > sound like a lot of code churn for little gain.
>
> I agree about a), but I don't think b) is that bad. Alternatively c) rename
> stack_sync_to_server() to stack_sync_to_server_and_update_tile_matches would
> work as well.

OK, I did b) and it actually has a nice side-effect: in most of these
calls I now have a window in scope so I can restrict the computations
to windows only on that window's workspace.

Review of attachment 209640 [details] [review]:

Oh, I really didn't think about the window->screen->active_workspace side effect, which is pretty cool!

::: src/core/stack.c
@@ +216,3 @@
   stack->freeze_count -= 1;
   stack_sync_to_server (stack);
+  meta_stack_update_window_tile_matches (stack, NULL);

Is this really useful unless meta_stack_update_window_tile_matches() respects freeze_count?
(which I guess wouldn't be a bad idea anyway, even if it's not about server roundtrips)

::: src/core/window.c
@@ +10702,3 @@
+  MetaStack *stack;
+
+  window->tile_match = match = NULL;

Nit: initializing match is pointless (the first time it is used is the "match = meta_stack_get_top()" initialization in the for() loop)

@@ +10706,3 @@
+  if (window->shaded ||
+      window->minimized ||
+      ! META_WINDOW_TILED_SIDE_BY_SIDE (window))

No space after !

Created attachment 209935 [details] [review]
window: Introduce meta_window_get_tile_match()

--
Addressed all your three points Florian. Thanks

Review of attachment 209935 [details] [review]:

Looks good - do you know what's the state of the other patch? Did Jakub and Lapo fight it out yet?

This is was finally approved on #gnome-design.

Attachment 208464 [details] pushed as a8ead4d - MetaWindowActor: don't draw shadows for tiled windows with a match
Attachment 209935 [details] pushed as 2926323 - window: Introduce meta_window_get_tile_match()

This seems to have made a comeback. When tiling a CSD window, shadows are cast every time, even when two half-fullscreen windows are side by side. When doing the same with regular mutter decorated windows, there is no shadow even when only one window is half-maxed.

We should definitely be consistent with the behavior. 

I do believe half-maximized and maximized windows do need special casing, because shadows cast on other physical displays or onto each other when tiling trigger an OSD response from me, but we definitely shouldn't have two behaviors.

Heh the use of acronyms backfired on me. OCD response, not OSD reponse ;)

(In reply to comment #28)
> This seems to have made a comeback. When tiling a CSD window, shadows are cast
> every time, even when two half-fullscreen windows are side by side.

For CSD windows, shadows are drawn client-side by GTK+. So unless we want to revert the patches from this bug to make mutter consistent with GTK+, a new bug against GTK+ would be better than reassigning an old and fixed bug report.

Putting this one to rest then.

Turns out I'm getting different behavior for Nautilus and Polari, so perhaps the issue is even more complicated.