[wayland] Implement window geometry more properly
AcceptedPublic

Authored by zzag on Oct 7 2019, 10:41 AM.

Details

Reviewers
davidedmundson
Group Reviewers
KWin
Maniphest Tasks
T10867: XDG WindowGeometry
Summary

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.

Test Plan

New tests pass.

Diff Detail

Repository
R108 KWin
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 17383
Build 17401: arc lint + arc unit
zzag created this revision.Oct 7 2019, 10:41 AM
Restricted Application added a project: KWin. · View Herald TranscriptOct 7 2019, 10:41 AM
Restricted Application added a subscriber: kwin. · View Herald Transcript
zzag requested review of this revision.Oct 7 2019, 10:41 AM
zzag added a comment.EditedOct 7 2019, 10:42 AM

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

zzag added a comment.Tue, Oct 29, 5:37 PM

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.

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.

davidedmundson accepted this revision.Wed, Nov 13, 12:43 PM
davidedmundson added a subscriber: davidedmundson.

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
1311

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
549

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
m_windowGeometryExplicitlySet or something it's fine.

This revision is now accepted and ready to land.Wed, Nov 13, 12:43 PM