GNOME Bugzilla – Bug 731353
stop putting shadows on menus
Last modified: 2014-06-10 11:48:05 UTC
Bug 731187 has patches to make GTK+ use csd shadows for menus (as part of getting rid of the adwaita engine). That works best if mutter doesn't put its own shadows on menus.
Created attachment 278080 [details] [review] shadow-factor: Make sure to never use a negative spread The smallest possible spread corresponds to an unblurred shadow, which neither grows nor shrinks - thus the spread should be zero not negative as returned by our current calculation.
Created attachment 278081 [details] [review] shadow-factory: Remove shadows from menus GTK+ will add its own shadows client-side, so we don't need to anymore.
(In reply to comment #2) > Created an attachment (id=278081) [details] [review] > shadow-factory: Remove shadows from menus > > GTK+ will add its own shadows client-side, so we don't need to anymore. What about non gtk applications? Having shadows or not based on which toolkit the application uses sounds wrong ...
Agree with Adel - this needs to be coordinated with a property.
Review of attachment 278081 [details] [review]: See comment 3 and 4
Created attachment 278163 [details] [review] meta-window-actor: Don't add shadows to popups if the toolkit already does GTK+ will add its own shadows client-side, so we don't need to anymore. I haven't tested this at all with the GTK+ patches; I'm assuming that GTK+ will call gdk_window_set_shadow_width() for CSD override-redirect windows here - Matthias, is this asumption correct (or if not - sane enough to make it so)?
Yes, my patch treats csd o-r windows just the same as any other csd window. They should have _GTK_FRAME_EXTENTS set.
Review of attachment 278080 [details] [review]: The only possible input that will make the return value negative is 0, right? Can we just explicitly test for 0 at the top? Also, does this fix anything, or is it just a drive-by you noticed while debugging?
Review of attachment 278163 [details] [review]: ::: src/compositor/meta-window-actor.c @@ +751,3 @@ */ + if (priv->window->override_redirect && + !priv->window->has_custom_frame_extents) Can you use meta_window_is_client_decorated() so it works for Wayland clients as well?
Review of attachment 278163 [details] [review]: Also, I should point out, this will break shadows on clients that use GTK+ 3.10 or 3.12 with mutter 3.14, but maybe that just isn't that big of an issue to care about.
(In reply to comment #8) > Review of attachment 278080 [details] [review]: > > The only possible input that will make the return value negative is 0, right? Yes. > Can we just explicitly test for 0 at the top? Sure. > Also, does this fix anything, or is it just a drive-by you noticed while > debugging? It prevents hitting an assertion failure when using a blur radius of 0, so not just cosmetics. (In reply to comment #9) > Can you use meta_window_is_client_decorated() so it works for Wayland clients > as well? I'm reluctant to do that - first, popups are not really decorated, so this is very much relying on the actual implementation of the method. Secondly, I'm not sure that the assumption that *all* toolkits will add shadows client-side to menus on wayland is correct, so maybe we need something else here? Also from a quick skimming, override_redirect should never be set for wayland surfaces? (In reply to comment #10) > Also, I should point out, this will break shadows on clients that use GTK+ 3.10 > or 3.12 with mutter 3.14 Why? As far as I see, GTK+ has not been setting frame extents for override-redirect windows before, so this should work just fine?
Created attachment 278165 [details] [review] shadow-factory: Make sure to never use a negative spread
Review of attachment 278165 [details] [review]: I'm a bit curious why we've never hit this before, but OK.
(In reply to comment #11) > I'm reluctant to do that - first, popups are not really decorated, so this is > very much relying on the actual implementation of the method. Yeah, this is why I'm not too fond of it. I think it would be better if the client explicitly marked it was drawing the shadow (_NET_WM_CLIENT_DRAWS=shadow or similar), but that opens up a whole can of worms about negotiation. > Secondly, I'm not > sure that the assumption that *all* toolkits will add shadows client-side to > menus on wayland is correct, so maybe we need something else here? Also from a > quick skimming, override_redirect should never be set for wayland surfaces? Hm, yeah. Really, using override-redirect here is wrong, we should be paying attention to the window type and checking for WINDOW_TYPE_MENU, which meta-wayland-surface.c sets correctly. It's just luck that we're not hitting this on Wayland :) I think we've decided by that we want all decorations to be done by the client, not by the client, and yes, that includes shadows, even for menus. Wayland surfaces are almost always ARGB32, which means we can't really construct a good shadow shape for them, so the client always has a better idea about what it should draw into its buffer. Yes, we know it makes Owen angry because it's philosophically incorrect. :) > (In reply to comment #10) > Why? As far as I see, GTK+ has not been setting frame extents for > override-redirect windows before, so this should work just fine? Oh, I missed that.
(In reply to comment #13) > I'm a bit curious why we've never hit this before, but OK. Apparently nobody tried to set a 0 blur radius before. (In reply to comment #14) > Hm, yeah. Really, using override-redirect here is wrong, we should be paying > attention to the window type and checking for WINDOW_TYPE_MENU WINDOW_DROPDOWN_MENU and WINDOW_POPUP_MENU. I'm not opposed to that, but note that this *is* a change with regard to the current behavior - we currently do add shadows to other OR windows like tooltips (they just don't have a dedicated shadow class like menus/popups/dropdowns).
Yeah, I wasn't suggesting changing it, just saying that we shouldn't really ever be reading the "override-redirect" flag for things like this, only for making sure we don't ever raise / focus / move / resize such windows.
Attachment 278163 [details] pushed as af3aae7 - meta-window-actor: Don't add shadows to popups if the toolkit already does Attachment 278165 [details] pushed as 98e219d - shadow-factory: Make sure to never use a negative spread I updated the comment to explicitly mention X11, and pushed - the wayland case already works as expected, as the condition covers a case where we *do* want to show server-side shadows: we never want to hit this in the wayland case, so the FALSE fallthrough at the end of the function is correct.