Init ShellClient only when commited to the buffer
ClosedPublic

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

Details

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.

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

Ran all unit tests

Diff Detail

Repository
R108 KWin
Branch
master
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 8059
Build 8077: arc lint + arc unit
davidedmundson created this revision.Jan 28 2019, 2:59 PM
Restricted Application added a project: KWin. · View Herald TranscriptJan 28 2019, 2:59 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
davidedmundson requested review of this revision.Jan 28 2019, 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.Jan 28 2019, 8:49 PM

What's the purpose of Test::renderShellClientAndWaitForShown?

zzag added a comment.Jan 28 2019, 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.Jan 28 2019, 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.Jan 29 2019, 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.EditedFeb 9 2019, 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.Feb 9 2019, 12:26 PM
zzag added a comment.EditedFeb 9 2019, 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.Feb 13 2019, 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?

Hopefully address all review comments

I got rid of the RAII blocker, and mod the value manually

testMinimised is added but uncovers an existing bug! (so that was a good suggestion)

testing fullscreen configure events was merged into the newly added test about fullscreen restore

davidedmundson retitled this revision from [WIP] Init ShellClient only when commited to the buffer to Init ShellClient only when commited to the buffer.
davidedmundson edited the summary of this revision. (Show Details)
davidedmundson edited the test plan for this revision. (Show Details)
zzag accepted this revision.Feb 26 2019, 12:00 PM

Looks good to me, though I'd prefer to have blockRequestGeometry method, which would be called by RequestGeoemtryBlocker and initialization routines. :-)

shell_client.h
192–194

I'm not sure whether /* */ is the right choice here. Maybe use single line comments or stick with /** **/?

272

Also it would be great to tell "future folks" when they could rename it to ConfigureBlocker. :-)

This revision is now accepted and ready to land.Feb 26 2019, 12:00 PM

Looks good to me, though I'd prefer to have blockRequestGeometry method

I don't think it adds a lot unless it's as a matching block/unblock pair of methods, and I deliberately refactored requestGeometry for the sole reason of having it easily re-usable for finishInit and the Blocker destructor.

I've made the other two changes. Lets revisit this one when we do that rename.

This revision was automatically updated to reflect the committed changes.