| davidedmundson |
| KWin |
So far the window geometry from xdg-shell wasn't implemented as it should
be. A toplevel must have two geometries assigned to it - frame and buffer.
The frame geometry describes bounds of the client excluding server-side
and client-side drop-shadows. The buffer geometry specifies rectangle on
the screen occupied by the main surface.
State and geometry handling in XdgShellClient is still a bit broken. This
change doesn't intend to fix that, it must be done in another patch asap.
New tests pass.
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
I have to admit that this patch doesn't look nice, especially doMove method. I'm going to rewrite state and geoemtry handling in XdgShellClient. There is also a lot of issues related to input handling... :/
I've tried to address some of those issues before, but each attempt to fix some aspect of our xdg-shell implementation grew into a monstereous "re-write foo in kwin." So, I decided to tackle one thing at a time.
As I said, this patch is an ugly piece of trash, but I do hope that the whole xdg-shell situation will be addressed with follow-up patches (in the future, not this one).
Please give this diff a more telling name what it is about. You introduce a new cached buffer size value and redefine geom via window geometry. What this does and how it relates to all the other sizes we have is not clear to me.
What would help: Create some images where the different areas of a window and what our sizes relate to are described. I mean it's clear from the specs what window and buffer geometry are supposed to be but we have/had geometry, client size, requested client size and so on.
| xdgshellclient.h | ||
|---|---|---|
| 197 | Reducing complexity: can we just check m_xdgShellSurface->windowGeometry().isValid()? | |
So, as said, the buffer geometry specifies a rectangle on the screen occupied by the main surface. The frame geometry is the geometry of the client excluding things like drop-shadows. Both the buffer geometry and the frame geometry don't have strict order between each other, so to speak. The buffer geometry can be completely inside the frame geometry, and vice versa.
About redefining geom in terms of the window geometry... Well, we don't other sensible choices. If the client is decorated by kwin, the server-side decoration must be put around the window geometry and thus the frame geometry is actually window geometry + frame margins. If the client is not decorated or client-side decorated, the frame geometry corresponds to the window geometry. Such design doesn't require dramatic changes in things like snapping, or placement code. Last but not least, such geometry choice will help us fix all corner cases related to restoring from maximized or fullscreen mode. Of course we could send a configure event with width and height set to 0, but we don't do that.
About the client geometry. I think it's worth to make the client geometry more explicit. We have two heavy users of it - X11Client and decorations, though decorations only need the size. In long term, I think we have to aim for the introduction of the client geometry. So, just to re-iterate, we would have four fundamental geometries: input, frame, client, and buffer (a zoo full of different geometries).
requestedClientSize is the last requested window geometry size.
I know that this patch does a couple of things at the same time, but there's no any other way around. In order to implement the window geometry, we need to have the buffer geometry to some extent.
This patch is nasty as f but I do hope that this whole geometry hell will be resolved by 5.18.
| xdgshellclient.h | ||
|---|---|---|
| 197 | Will change. I added this bool to avoid a few unnecessary comparisons in QRect::isValid. However, I guess it's a premature optimization. | |
Technically there's one potentially interesting behavioural change.
If a client is server side decorated and then draws a subsurface outside the main surface, previously I believe we would have drawn our frame over the main surface, whereas now I think we'll draw our frame over the union. Probably not a real world issue though.
| autotests/integration/xdgshellclient_test.cpp | ||
|---|---|---|
| 1338 | Generally it's a lot easier to read something like QCOMPARE(..., QSize(100 + 20, 50 + 10)); and then it's apparent where the new numbers come from (don't bother changing it though) | |
| xdgshellclient.cpp | ||
| 553 | the if (!m_unampped) has gone Arguably it's a non issue as we'll just trigger a repaint that will in turn not do anything, but I'm wondering if it was there deliberately. | |
| xdgshellclient.h | ||
| 197 | It makes sense to keep a boolean to see if it's externally set or implicitly set. I think with a rename to | |