Bug 379637

Unfortunately, it's not really so simple. We can write shadows appropriately for WMs which interact well with the nonstandard hints GTK CSDs are using. However, from my basic understanding, they conflict with KWin's current architecture and fail to conform to a stable implementation. This is part of why ARGB doesn't function as desired to round off window corners, for instance. In the case of shadows, that would result in a thick, fully opaque border around every CSD window.

In a brief discussion I had with Martin, he made it fairly clear that the amount of work required to use unsupported hints like these isn't trivial and would potentially be wasted as minor changes could break such functionality.

Unless he changes his mind about that, or I've misunderstood him, it's unlikely we'll see a resolution to this without some extra work on GTK itself.

I think that it is related to the same topic: menus on gtk apps like libreoffice or firefox have no drop shadow.
I'm running plasma 5.14.2 on Archlinux and my graphic card is Intel Iris. I use mesa and intel drivers and compositor uses opengl 3.1
Anyway to solve the issue?
Thanks
Gianluca

>Can we just have breeze-gtk hint to KWin that window shadows need to be drawn?

No.

You can't use Kwin's weird custom protocol for client's providing shadows that's used in the unmanaged windows as you're entirely in CSS so can't just modify atoms. 

From the CSS you'd have to send the _GTK_FRAME_EXTENTS 
At which point a pre-requisite is having that exist in kwin.

From a breeze-gtk POV there is nothing sane you can do.

So what do we do?

GTK folks say it's our fault. We say it's their fault. It can't be nobody's fault. And if it's primarily one party's fault but they won't budge, then users suffer and we get blamed regardless. There must be *something* we can do here. This is pretty significant papercut for our users.

Long term kwin needs to change. 

There are 3 current implementation for shadows:

1) Server draws them based on what the deco theme says (what classic toplevels and KDE toplevels do)
2) The client sends the shadow via an entirely custom KDE protocol and attaches it to the window (what KDE breeze popups do) 
3) The client draws a shadow and sends metadata over a brand new protocol about where the real window is (modern GTK + mutter)

(2) is overly complex. It's even less of a standard than the one used in 3 expecting a 3rd party to support it (especially when they're also feature frozen!) is completely unreasonable.

The CSD vs SSD argument is one discussion; but if you think only about popups we need to support (3) in kwin regardless.

For wayland at least (3) is documented core protocol and we'll need to support xdg_surface.set_window_geometry as otherwise even with fully working xdg_decoration, popups will be offset.

I will be adding support for it.
However we're feature frozen on X so we're not doing anything there.

-----

The only other option available is redoing the way shadows work entirely and ignore what clients want. Fredrick Hoglund once did a mockup with real 3D shadows.   Might have issues for non-square windows though.

Is it possible to implement 1) Server side shadows on both top level windows and to popups and menus? It would give more consistency to the applications.
If we give each client the freedom to draw shadows as they want then we could end up with a program having red shadows and another program having blue shadows.
Consistency is key for me.

You could hack something together, but it wouldn't work great.

If you drew a square border round the plasma dialog popups it'd look weird in the corners.

My estimate for option 3 on X11 was about half a year of development effort with huge potential to break. Basically adding support for this breaks a core assumption of the compositor: x pixmap size equals window size. We support adding to the window, just subtracting was never thought of. Window placement, resizing, etc. all is based on the x window geometry. We use the variable in thousands of places - every usage meds to be checked for whether it's meant visually or as geometry.

The reason that KWin doesn't support it is not that I don't like csd, but that we didn't have the time to develop it and that I am afraid of the potential breakage.

The assumption is deep inside KWin - due to that we also don't support this on Wayland. In the standardization of xdg shell I raised several times the concerns that our code base doesn't support this concept. My concerns were ignored. I tried to write ShellClient in a way that the assumption doesn't hurt that much, but currently it's still there.

Personally I would prefer if we would not try to implement option 3 for X11. By the time we have it the switch to Wayland might be finished and what's left is a broken X11 window manager. I will not review any change as the experience brought me close to burn out. Our design philosophy is form follows function. I think a working window manager is more important than good looking csd windows.

I can appreciate the technical problems associated with fixing this bug. 

But this issue is significant from a user perspective. An even worse one is Bug 390550, which results in an impairment to function, not just aesthetics (you can't resize CSD windows at all). Taken together the experience of using GTK3 apps with CSD windows is just really painful and bad, and represents a strategic weakness for our platform.

If the codebase is so fragile that it leaves us unable to fix bugs, that's a major problem. Function doesn't matter if the form turns people off so they don't want to use it. In the commercial world, projects get canceled because their UX is bad even when the tech and code are good, because users buy competing offerings instead. In the FOSS world, the effect is not as dramatic, but what happens is that people vote with their feet and just stop using it over time. We can't afford to say, "this is too risky to fix, sorry." CSDs are proliferating in the GTK world and we can't ignore that and let a larger and larger share of Linux software look and feel bad on our platform.

Bug 390550 is definitely a lot more serious but it's not necessarily solved by the same thing.

You can have a larger CSD resize area without relying on frame extents to signify your window content. I would expect that to be do-able within the current frameworks of both sides.

Kwin also does have (though apparently contentious) a concept of the "resize only border" extents outside the active frame.

Our code base is neither fragile nor is this a bug. Gtk requires a new not standardized functionality which is not in line with ICCCM and EWMH. We have a window manager implemented against the standard. Claiming not supporting this feature is a bug or claiming that our code base is too fragile to support it is an insult against the code base and it's developers (I did not contribute to this old part of KWin). And this is something I find really demotivating.

Martin, I interpreted your fear of regressions and breakage to suggest a fragile codebase, since I'm pretty sure that the problem isn't a lack of skill on your part. :) If my interpretation was incorrect, I apologize.

But this is a real bug. The bug is that GTK3 CSD windows don't get shadows like the user expects.

For years this bug has gone unsolved with everyone saying it's someone else's problem: the GTK folks say it's up to KWin to support this, and KWin folks say it's up to GTK to use a more standardized implementation. In the meantime, nothing gets done, the user experience suffers, and our platform becomes less attractive over time as more GTK apps adopt CSDs. Firefox just did by default, for example.

I am hoping we can find a way to move forward somehow. David mentioned that this is all fixable on Wayland at least, right?

let's get the wording right: gtk changing X in an incompatible way is not a bug on our side. It's a new feature or requirement. Please let's not call it a bug.

The code is not fragile, the code base is huge. We are talking about several thousand lines of code which need to be reviewed and changed. Yes I am afraid of regressions - not because of the code base but due to the wrirdness of X. This is exactly the reason why we feature froze X: we are not able to touch without regressions because of X. We don't have the expertise anymore.

Btw. I'm completely not available for working on it. I got burned out over the experience  and right now my heart is beating like mad. This was extremely frustrating when gtk broke our code and they did not fix due to "it's just rambling Martin"


Also I think there are more important areas like making Wayland work and thus resolving the issue. As I wrote my estimate is half a person year of work - my available time is about a hour a week.

If you want it fixed: takk to gtk or make no-csd the default. KWin just cannot be changed - the amount of effort is unreasonable large for a visual issue.

I think discussion is getting too much emotional, probably because there is a lot of past efforts and frustration due to gtk always changing cards on the table.
Is it not better to force server side decorations for CSD windows and pop up menus.
By the way Libreoffice has menus without shadows, same Firefox.
It is difficult to say they can be discarded to have a non gtk environment.

Martin am I wrong or are you the master and Lord of kwin? So why you say we don't have the competency any more?

(In reply to Gianluca Pettinello from comment #15)
> Martin am I wrong or are you the master and Lord of kwin? 

I'm no longer maintainer, but probably still the KWin dev with most knowledge in the code base.

> So why you say we
> don't have the competency any more?

X11 geometry handling was implemented before I started to contribute to KWin. In all the years I didn't have to touch it. We only added new concepts such as quick tiling. But the core handling is a code base I basically do not know. Which shows how good it is: I never had to dig into it. 

None of the developers of that code is active any more. We are talking here about changing requirements 15 to 20 years after the code was written. We don't have the hardcore X developers any more. I'm probably the most experienced but I don't want to work on X any more.

I understand the point. My suggestions coming from my experience in a chemical industry:
1) Be the teacher to the others since you have deep knowledge. Write a document explaining the structure of kwin classes and what each method does, what is the flow of actions once a window is created etc.

2) are you still in contact with the old developers to push them to do the same?
And in the future with Wayland do the same otherwise the project will die

I intend to add support for xdg_surface.set_window_geometry on wayland, supporting it in popups first then we'll see about top levels.

I don't intend to make the equivalent change on X. 

I think we should explore other solutions for 390550 - and I will keep that open to do so.

Ok, I think I need to explain more on the situation on X11. The problem is not that the code is fragile, undocumented or umaintained. The code is good, the quality of the code in question is superb and one can easily understand which area of X11 it reflects. No increase of documentation would make it possible to implement this change request.

Let's look at the root problem. On X11 the window geometry is retrieved through the get_geometry call: https://www.x.org/releases/X11R7.7/doc/xproto/x11protocol.html#requests:GetGeometry

This specifies the geometry of a drawable - either a pixmap or a window. That's what KWin's geometry handling code is based on. Now what GTK did is use this window geometry and added the shadow into it. Additional there is a property which indicates what to subtract from the x geometry to get to the window geometry. If we wouldn't subtract it, we would snap to shadow, quick tiling would include shadow, maximize would include shadow, etc. etc.

So basically everywhere where we use geometry we would have to remove that this maps to x window geometry and replace it by an abstract geometry concept which is either the geometry or the geometry subtracted by shadow. And this is the fundamental problem for us. When KWin was developed nobody thought that the geometry of a window would not match the geometry. Now you might wonder how things like window decorations work? Well it's still a window. The actual client window gets reparented to the window decoration and then we just use the geometry of the decoration window again.

The geometry handling in KWin is deep involved and the assumption is carried everywhere. It goes into the effect system (e.g. Present Windows uses it to layout), it's deep in the compositor (we actually have a check to verify the geometry matches the pixmap size, if it doesn't we don't render the window, see https://phabricator.kde.org/source/kwin/browse/master/scene.cpp$1042). We have multiple level of convenient api for it: Toplevel::width, Toplevel::height, Toplevel::x, Toplevel::y, Toplevel::size, Toplevel::pos, etc. So it's not just one method we have to check. Then there are methods like isOnScreen intersecting the screen geometry with the window geometry.

So all of that is a huge effort to restructure the code base to support this new requirement. And then there are multiple things which are completely unknown to me as this is a not standardized protocol: when resizing do we have to add the shadow or not? A maximized window does it include the shadow or not? How does gravity interact with shadow? What if a base increment is set? What about vertical only or horizontal only maximize state? Gtk doesn't like those, so do they support it? Is it possible to have shadows only on some sides or is it always all? Can we expect that Gtk won't change that? After all they broke us here several times.

So to reiterate some important points:
 * this is not a bug on our side, our code is written against the X protocol, ICCCM and EWMH
 * GTK changed basic understanding of what a window geometry is, this understanding is completely incompatible to the assumptions KWin was implemented on
 * The code base is in a superb state, geometry handling has very seldom bugs, it's a done and working code base
 * The effort is comparable to adding Wayland support - the difficult part was getting the geometry handling done
 * implementing the change is a huge effort as the code base is large and it touches complex areas of X where the expected behavior is undocumented by GTK (I just tried google for _GTK_FRAME_EXTENTS and found nothing)

----
Comparing to the state on Wayland: on Wayland the geometry and visual geometry are separated. The visual geometry is defined by the size of the attached buffer. The geometry of the window is not directly mapped from the buffer as we have concepts such as scale built into the protocol. While KWin still is largely based on the idea that visual geometry matches window geometry, it's not as bad as on X11. We already weakened the assumption a lot by adding window decorations ;-). Implementing the required request should be relatively straight forward and doable without larger problems. We have a well tested code base on Wayland and I am sure it can be implemented without regressions. The main reason why the request is not implemented is that there was a little bit of version mess when that area of code got written. That's now resolved and it's possible to test with real world applications.

Overall I think the way forward is supporting Wayland. On X11 we need to find different solutions which don't involve changing KWin or convince GTK to not build a GUI based on fundamental changes to X11. I understand that they are not able to understand KWin's problems - after all Mutter supports it. Maybe it's possible to talk to the devs again if someone other than me approaches them. I got from the responsible dev once the reply that he ignored all my bug reports because it was me. Looking around it seems to me that Emmanuale Bassi is more open to us and more open to changes to support us. Maybe Nate could try to approach them - he is not a dev involved and focused on the quality of the whole system. Explaining that KWin cannot implement it for the reasons given here might help. Explaining that you are afraid of breakage might help. After all if one is responsible for quality the change in KWin is something one needs to take a step away from if quality is supposed to matter.

(In reply to Martin Flöser from comment #19)
> Ok, I think I need to explain more on the situation on X11. The problem is
> not that the code is fragile, undocumented or umaintained. The code is good,
> the quality of the code in question is superb and one can easily understand
> which area of X11 it reflects. No increase of documentation would make it
> possible to implement this change request.
> 
> Let's look at the root problem. On X11 the window geometry is retrieved
> through the get_geometry call:
> https://www.x.org/releases/X11R7.7/doc/xproto/x11protocol.html#requests:
> GetGeometry
> 
> This specifies the geometry of a drawable - either a pixmap or a window.
> That's what KWin's geometry handling code is based on. Now what GTK did is
> use this window geometry and added the shadow into it. Additional there is a
> property which indicates what to subtract from the x geometry to get to the
> window geometry. If we wouldn't subtract it, we would snap to shadow, quick
> tiling would include shadow, maximize would include shadow, etc. etc.
> 
> So basically everywhere where we use geometry we would have to remove that
> this maps to x window geometry and replace it by an abstract geometry
> concept which is either the geometry or the geometry subtracted by shadow.
> And this is the fundamental problem for us. When KWin was developed nobody
> thought that the geometry of a window would not match the geometry. Now you
> might wonder how things like window decorations work? Well it's still a
> window. The actual client window gets reparented to the window decoration
> and then we just use the geometry of the decoration window again.
> 
> The geometry handling in KWin is deep involved and the assumption is carried
> everywhere. It goes into the effect system (e.g. Present Windows uses it to
> layout), it's deep in the compositor (we actually have a check to verify the
> geometry matches the pixmap size, if it doesn't we don't render the window,
> see https://phabricator.kde.org/source/kwin/browse/master/scene.cpp$1042).
> We have multiple level of convenient api for it: Toplevel::width,
> Toplevel::height, Toplevel::x, Toplevel::y, Toplevel::size, Toplevel::pos,
> etc. So it's not just one method we have to check. Then there are methods
> like isOnScreen intersecting the screen geometry with the window geometry.
> 
> So all of that is a huge effort to restructure the code base to support this
> new requirement. And then there are multiple things which are completely
> unknown to me as this is a not standardized protocol: when resizing do we
> have to add the shadow or not? A maximized window does it include the shadow
> or not? How does gravity interact with shadow? What if a base increment is
> set? What about vertical only or horizontal only maximize state? Gtk doesn't
> like those, so do they support it? Is it possible to have shadows only on
> some sides or is it always all? Can we expect that Gtk won't change that?
> After all they broke us here several times.
> 
> So to reiterate some important points:
>  * this is not a bug on our side, our code is written against the X
> protocol, ICCCM and EWMH
>  * GTK changed basic understanding of what a window geometry is, this
> understanding is completely incompatible to the assumptions KWin was
> implemented on
>  * The code base is in a superb state, geometry handling has very seldom
> bugs, it's a done and working code base
>  * The effort is comparable to adding Wayland support - the difficult part
> was getting the geometry handling done
>  * implementing the change is a huge effort as the code base is large and it
> touches complex areas of X where the expected behavior is undocumented by
> GTK (I just tried google for _GTK_FRAME_EXTENTS and found nothing)
> 
> ----
> Comparing to the state on Wayland: on Wayland the geometry and visual
> geometry are separated. The visual geometry is defined by the size of the
> attached buffer. The geometry of the window is not directly mapped from the
> buffer as we have concepts such as scale built into the protocol. While KWin
> still is largely based on the idea that visual geometry matches window
> geometry, it's not as bad as on X11. We already weakened the assumption a
> lot by adding window decorations ;-). Implementing the required request
> should be relatively straight forward and doable without larger problems. We
> have a well tested code base on Wayland and I am sure it can be implemented
> without regressions. The main reason why the request is not implemented is
> that there was a little bit of version mess when that area of code got
> written. That's now resolved and it's possible to test with real world
> applications.
> 
> Overall I think the way forward is supporting Wayland. On X11 we need to
> find different solutions which don't involve changing KWin or convince GTK
> to not build a GUI based on fundamental changes to X11. I understand that
> they are not able to understand KWin's problems - after all Mutter supports
> it. Maybe it's possible to talk to the devs again if someone other than me
> approaches them. I got from the responsible dev once the reply that he
> ignored all my bug reports because it was me. Looking around it seems to me
> that Emmanuale Bassi is more open to us and more open to changes to support
> us. Maybe Nate could try to approach them - he is not a dev involved and
> focused on the quality of the whole system. Explaining that KWin cannot
> implement it for the reasons given here might help. Explaining that you are
> afraid of breakage might help. After all if one is responsible for quality
> the change in KWin is something one needs to take a step away from if
> quality is supposed to matter.

Thanks Martin for all the explanations. I'm not an expert programmer, indeed I'm a chemical engineer and my comment about documentation comes from personal experience: I saw several very competent people leaving the company and experienced loss of knowledge because the company was not keen enough to get their knowledge documented.

Coming back to your explanation I see that gtk windows don't have shadows, so kwin someway clips the shadow that gtk (sigh!) adds to window geometry to make the geometry.
So my thought (probably it is naif) is that if we trap the type of window (_gtk_frame_extents) then we can apply the shadow as if it were a normal window / menu. There are some occurrences of gtk_frame_extents in kwin code but I'm not able to understand what happens. There are two methods that retrieve xwindow properties.

No, gtk nowadays doesn't add shadows if the window manager does not announce support for the gtk specific property.

We really need to stop thinking KWin could do anything here. I analyzed this whole mess years ago. If there were any easy solution it would have been added.

RESOLVED FIXED isn't an appropriate status if the issue of GTK3 headerbar windows not getting shadows hasn't actually been fixed.

RESOLVED INTENTIONAL would be appropriate if you don't plan to fix it, but it sounds like David has a plan to eventually fix it on Wayland. Therefore the original RESOLVED LATER status was appropriate. Changing back.

Sorry, I didn't want to change the status at all.

(In reply to Nate Graham from comment #22)
> RESOLVED FIXED isn't an appropriate status if the issue of GTK3 headerbar
> windows not getting shadows hasn't actually been fixed.
> 
> RESOLVED INTENTIONAL would be appropriate if you don't plan to fix it, but
> it sounds like David has a plan to eventually fix it on Wayland. Therefore
> the original RESOLVED LATER status was appropriate. Changing back.

I'm very happy to know that David will fix the issue in Wayland

My DE/WM before KDE was xfce with compiz. And with that wm shadows were rendered in gtk apps and also in apps which are neither gtk nor kde, for instance gmsh. Since compiz is a very old piece of software but still working along many version of gtk, it means that the shadow rendering was designed agnostic of the toolkit. In the case of kde I see this piece of code in shadow.cpp

<code>
QVector< uint32_t > Shadow::readX11ShadowProperty(xcb_window_t id)
{
    QVector<uint32_t> ret;
    if (id != XCB_WINDOW) {
        Xcb::Property property(false, id, atoms->kde_net_wm_shadow, XCB_ATOM_CARDINAL, 0, 12);
        uint32_t *shadow = property.value<uint32_t*>();
        if (shadow) {
            ret.reserve(12);
            for (int i=0; i<12; ++i) {
                ret << shadow[i];
            }
        }
    }
    return ret;
}

</code>

where I see that only windows supporting kde_net_wm_shadow will be rendered correctly (at least I hope to have interpreted the code well).

Anyway I understand that nothing will be done for X11 and I will temporarily switch to my old DE waiting for Wayland to support shadows also for non KDE apps.
Regards
Gianluca

BTW Martin, on a more personal note, I understand completely what you mean when you say that the topic makes your heart beat quickly. I have the same experience. It's a stressful subject for sure. :(

Coming back to the topic as I'm hard to give up if I think the goal is reachable.
The function:
Xcb::Property property(false, id, atoms->kde_net_wm_shadow, XCB_ATOM_CARDINAL, 0, 12);
as it is called in function:
readX11ShadowProperty
is the key to get the shadow properly rendered in pop up menus.
As already described in:
https://community.kde.org/KWin/Shadow
the last four uint32_t contain the padding of the shadow (I deliberately injected a piece of code to manipulate them)
The first eight uint32_t contain the id of the pixmaps.
Who creates the pixmaps. In which point of the code could I find it?
Thanks
Gianluca

the pixmap is created by the client.

(In reply to Martin Flöser from comment #27)
> the pixmap is created by the client.

Could you tell me in which point of the code?
Thanks :)

For an example see DialogShadows.cpp in plasma-framework.

You'd have to get that done by the GTK process.

(In reply to David Edmundson from comment #29)
> For an example see DialogShadows.cpp in plasma-framework.
> 
> You'd have to get that done by the GTK process.

And if we trap it and force shadows done as KDE model?

How?

(In reply to David Edmundson from comment #31)
> How?
If we use the DialogShadows.cpp code and apply it to each Window that has _GTK_FRAME_EXTENTS enabled (we can trap it in kwin events.cpp)?

Too naif?

yes, as soon as gtk adds that property window management breaks

(In reply to Martin Flöser from comment #33)
> yes, as soon as gtk adds that property window management breaks

But if we added _KDE_NETWM_SHADOW property with shadw pixmaps, how can gtk destroy them? And then if kde_netwm_shadow exists then kwin renders the shadows, isn't it?

_GTK_FRAME_EXTENTS support is now planned for KWin--in a way that doesn't break other things, of course. :) I'm hoping this lands in Plasma 5.18. Marking as a duplicate of the bug tracking that work.

*** This bug has been marked as a duplicate of bug 390550 ***