[WIP] Init ShellClient only when commited to the buffer
Needs ReviewPublic

Authored by davidedmundson on Mon, Jan 28, 2:59 PM.

Details

Reviewers
None
Group Reviewers
KWin
Summary

Everything on the wl_surface is double buffered.

When we create an XdgShell toplevel or popup we shouldn't treat it as
attached until it's committed to the surface.

A client should commit the surface after it's sent it's initial state of
the Xdg topLevel; minimumSize, title, app_id, etc.

By delaying init we will have the intial state correct to apply rules
and more correct geometry and everything is more atomic.

It's WIP because it breaks a lot of (arguably incorrect) unit tests -
and because I need to go through init() and make sure we load the
initial state of everything as well as connecting the signals.

Uploading now because zzag has a patch pending for applying rules
which relies on correct initial state.

Arguably this applies to WlShellSurface too, but I've left it unchanged
as it's deprecated and hard to verify real client behaviour.

Test Plan

WIP

Diff Detail

Repository
R108 KWin
Branch
popupplacement2
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 8257
Build 8275: arc lint + arc unit
davidedmundson created this revision.Mon, Jan 28, 2:59 PM
Restricted Application added a project: KWin. · View Herald TranscriptMon, Jan 28, 2:59 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
davidedmundson requested review of this revision.Mon, Jan 28, 2:59 PM

Add hooks for unit tests to easily block for configure events from the compositor
before attaching buffers

new connect syntax

zzag added a subscriber: zzag.Mon, Jan 28, 8:49 PM

What's the purpose of Test::renderShellClientAndWaitForShown?

zzag added a comment.Mon, Jan 28, 8:53 PM

I guess the reason why createShellSurface doesn't have CreationSetup parameter is because of wl_shell, but without it we would need custom "createShellSurface" helpers in some tests, e.g. TestShellClient::testUserSetFullscreenXdgShell.

Merge patch from zzag fixing the expected configure count in testQuickTilingPointerMove

I guess the reason why createShellSurface doesn't have CreationSetup parameter is because of wl_shell,

Exactly, having low level control doesn't make sense if you're also trying to abstract.
You have to handle the configure yourself, and you can't do that with a QObject.

I'd be up for adding an XdgShellSurface *createXdgShellSurface(ShellSurfaceType, CreationSetup) which doesn't include WlShell

zzag added a comment.Mon, Jan 28, 9:41 PM

I'd be up for adding an XdgShellSurface *createXdgShellSurface(ShellSurfaceType, CreationSetup) which doesn't include WlShell

Yep, makes sense.

zzag added inline comments.Tue, Jan 29, 3:58 PM
autotests/integration/quick_tiling_test.cpp
474

We don't want frame callback, sorry for this mistake.

autotests/integration/test_helpers.cpp
529

We don't want the frame callback.

davidedmundson updated this revision to Diff 51243.EditedSat, Feb 9, 12:20 PM

New simpler approach.

We init on startup, but put a geometry blocker over everything until the shell is
committed.

All signals/slots work without changes, client still only gets one correct configure.

We might need to still re-evaluate some rules in finishInit.
And we can maybe speed things up by adding some early returns if
m_initialSetupBlocker is set.

Still WIP as I have some cleanup to do and 2 more unit tests to fix and fix XdgPopups initial size being reported properly

Rebase and cleanup

davidedmundson marked an inline comment as done.Sat, Feb 9, 12:26 PM
zzag added a comment.EditedSat, Feb 9, 5:57 PM

I have several comments, feel free to ignore them until this revision is good enough for landing:

  • it would be better to just call blockRequestGeometry and unblockRequestGeometry instead of using RAII because we don't actually benefit from it. Also, we have a field that is used only once, during initialization;
  • some of my frame callback comments are still not addressed, e.g. initXdgShellSurface and testQuickTiling. This can fire back at us in some tests;
  • would it make sense to add testXdgInitiallyMinimized as well?
  • given what RequestGeometryBlocker is doing, could you please add a todo item to rename it to ConfigureBlocker? We would address it after wl-shell support is dropped.

As far as I understand, we'll stick with the current approach. Emitting surfaceCreated after the surface is committed is out of scope, right?

I don't want to use AbstractClient::blockGeometryUpdates as it doesn't just call configure when it's done, it goes into setGeometry which is subtly different, especially for the code path where toplevels should choose their own size.

I could refactor ShellClient::RequestGeometryBlocker to call into some new method on ShellClient when it destructs, which finishInit could then call directly.
I'm not really convinced it's better, as we're probably still going to want some field to know we're in this state.

As far as I understand, we'll stick with the current approach. Emitting surfaceCreated after the surface is committed is out of scope, right?

Yeah, I think that would introduce more problems than it solves and we'd probably still end up needing a geometryblocker to process all the different initial states happening in different orders.

One compromise maybe worth considering is putting a signal in ShellClient emitted after the first surface commit.

Update

With the other related patches, popups work, all unit tests pass \o/
Been using it on my machine for a day without issue.

Patch doesn't include all zzag requests, happy to discuss and changing, just
updating what I have on here now with everything working well.

Popups are positioned twice, which isn't ideal, but I think unavoidable.
Hypotheticaly situations could have changed between creating the shell and
the first map - or the popup could attach a buffer at different size.

zzag added a comment.Wed, Feb 13, 4:55 PM

I'm not really convinced it's better, as we're probably still going to want some field to know we're in this state.

In which state?