[wayland] Syncronise pending geometry with acked configure requests
ClosedPublic

Authored by davidedmundson on Aug 28 2018, 11:01 PM.

Details

Summary

When we want to change a client's size and position together we have to
request the client becomes a new size and only then move the window to
the new location.

Currently we process the new position the next time the buffer updates,
but with no guarantee that it has actually tried to resize/whatever yet.
The client could be providing a new buffer just because the contents
have changed.

XDGShell has an acked serial designed to keep everything precisely in
sync. A surface represents the last configure that was acked.

This patch tracks the pending position for each configure and applies it
accordingly.

WL_shell does not have this mechanism, so behaviour is kept the same as
before.


This is a pre-requisite to syncing maximisedState/isFullScreen with the
configure request.

Potentially we could remove the isWaitingForResizeSync checks when
resizing and it will still resize smoothly.

Test Plan

Relevant unit test still passes with the client responding
Resized a window from the left edge with WLShell and XDGShellV6

Diff Detail

Repository
R108 KWin
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 2314
Build 2332: arc lint + arc unit
Restricted Application added a project: KWin. · View Herald TranscriptAug 28 2018, 11:01 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
davidedmundson requested review of this revision.Aug 28 2018, 11:01 PM

Clarify FIFO stack behaviour by just using QVector rather than a weird mix

romangg requested changes to this revision.Sep 13 2018, 2:53 PM
romangg added a subscriber: romangg.

A surface represents the last configure that was acked.

What does this mean in the summary? What surface?

shell_client.cpp
1210

Instead of taking always the first one and by that looping through ignore (and remove) all configure events before the one associated with m_lastAckedConfigureRequest and only add a layer repaint and so on for m_lastAckedConfigureRequest.

shell_client.h
215

minor: to better understand the comment replace "it" with "serialId"

217

quint32

This revision now requires changes to proceed.Sep 13 2018, 2:53 PM
romangg added inline comments.Sep 13 2018, 3:15 PM
shell_client.cpp
1216

If several configure events have been sent the one last acked might not respond to the last configure event when updatePendingGeometry is called. Setting then the geometry with the current client size could still lead to not synchronized painting.

davidedmundson added inline comments.Sep 13 2018, 3:23 PM
shell_client.cpp
1216

If several configure events have been sent the one last acked might not respond to the last configure event when updatePendingGeometry is called.

m_clientSize is the size of the last committed buffer (see ShellClient::addDamage).
At that point we are in sync with the last acked configure.

davidedmundson added inline comments.Sep 13 2018, 3:52 PM
shell_client.cpp
1210

Do you mean something like

QPoint position = geom.topLeft();
it = m_pendingConfigureRequests.begin();
while (it != m_pendingConfigureRequests.end()) {
    if (it->serialId == m_lastAckedConfigureRequest) {
        if (position != it->positionAfterResize) {
             addLayerRepaint(geometry());
        }
        position = it->positionAfterResize;

        m_pendingConfigureRequests.erase(m_pendingConfigureRequests.begin(), it);
        break;
    }
    it++;
}
doSetGeometry(QRect(position, m_clientSize + QSize(borderLeft() + borderRight(), borderTop() + borderBottom())));

?

romangg added inline comments.Sep 13 2018, 4:04 PM
shell_client.cpp
1210

Yes. Use it++ in the erase statement to also erase the last acked one. Also there should still be an early loop exit in case the last acked configure request is lower and a cleanup in case the client does never send acks or acks without correct serials. Otherwise the vector can grow quickly.

Rewrite the loop in a slightly different way. Behaviour should be the same.

romangg accepted this revision.Sep 13 2018, 5:35 PM

It's missing still a cleanup in case the client is broken as in not responding to any configure events or with the wrong serials and the vector would grow limitless. But we can do this in a second patch.

shell_client.cpp
1206

Instead of checking on every iteration, just check against the first element of the vector before looping since the serials are monotonically increasing.

This revision is now accepted and ready to land.Sep 13 2018, 5:35 PM

It's missing still a cleanup in case the client is broken as in not responding to any configure events or with the wrong serials and the vector would grow limitless. But we can do this in a second patch.>

That's over complicating something that really doesn't need it.
XdgShellInterface in frameworks will raise an error if serials are super wrong.

That's over complicating something that really doesn't need it.
XdgShellInterface in frameworks will raise an error if serials are super wrong.

Wasn't aware of that. But `XdgTopLevelV6Interface::Private::ackConfigure says:

if (!configureSerials.contains(serial)) {
    // TODO: send error?
    return;
}

So at the moment it's not sending any error. But since configure events are triggered by user or compositor actions and the geometry of a non-reacting client would just freeze and not generate more configure events we can still ignore misbehaving clients.

This revision was automatically updated to reflect the committed changes.