Split Compositor class in Wayland and X11 child classes
ClosedPublic

Authored by romangg on Jul 1 2019, 6:36 PM.

Details

Summary

This patch is a first take at splitting up of the Compositor class into
Wayland and X11 child classes.

In this first patch we mostly deal with setup and teardown procedures.
A future goal is to further differentiate the compositing part itself too.

Test Plan

Manually X from VT and Wayland nested. Autotests pass.

Diff Detail

Repository
R108 KWin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
romangg created this revision.Jul 1 2019, 6:36 PM
Restricted Application added a project: KWin. · View Herald TranscriptJul 1 2019, 6:36 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
romangg requested review of this revision.Jul 1 2019, 6:36 PM
romangg retitled this revision from Split Compositor class in Wayland and X child classes to WIP: Split Compositor class in Wayland and X child classes.Jul 1 2019, 7:22 PM
romangg updated this revision to Diff 60957.EditedJul 1 2019, 7:22 PM
  • Remove unnecessary Compositor related functions

The isCreated function is only used in a single Q_ASSERT. I'm not in favor of keeping it. We should not need an assert for stuff like that. The currentRefreshRate extern function is there for no apparent reason. Or is there one?

romangg edited the summary of this revision. (Show Details)Jul 1 2019, 7:28 PM
romangg updated this revision to Diff 60964.Jul 1 2019, 8:14 PM
  • Move Suspend reasons into X11Compositor
romangg edited the summary of this revision. (Show Details)Jul 1 2019, 8:15 PM
romangg edited the summary of this revision. (Show Details)
romangg updated this revision to Diff 60965.Jul 1 2019, 8:46 PM
  • Check for overlay window in X11Compositor class
romangg edited the summary of this revision. (Show Details)Jul 1 2019, 8:46 PM
romangg updated this revision to Diff 60967.Jul 1 2019, 9:38 PM
  • Use new slots syntax
romangg edited the summary of this revision. (Show Details)Jul 1 2019, 9:38 PM
zzag added a subscriber: zzag.EditedJul 1 2019, 9:58 PM

options->setCompositingInitialized(false); // TODO: why?

I have a feeling that it has something to do with

https://github.com/KDE/kwin/blob/aa26125635c29755c837ffbacdfcab62e5d05e76/workspace.cpp#L1053-L1064

EDIT: Argh, I've just realized Workspace::slotReinitCompositing and Compositor::slotReinitialize are the same.

In D22195#489295, @zzag wrote:

options->setCompositingInitialized(false); // TODO: why?

I have a feeling that it has something to do with

https://github.com/KDE/kwin/blob/aa26125635c29755c837ffbacdfcab62e5d05e76/workspace.cpp#L1053-L1064

To me it looks like some sort of optimization to not read in the config files more often than necessary. See text at:
https://cgit.kde.org/kwin.git/tree/options.cpp#n959

But I could imagine this is some premature optimization and on Wayland most of the compositing options (or all besides animation speed?) are ignored anyway.

romangg updated this revision to Diff 60971.Jul 1 2019, 10:13 PM
  • Release selection in X11Compositor
romangg edited the summary of this revision. (Show Details)Jul 1 2019, 10:14 PM
anthonyfieroni added inline comments.
composite.cpp
863–864

It looks incorrect.

resume(AllReasonSuspend);

?

romangg added inline comments.Jul 2 2019, 9:27 AM
composite.cpp
863–864

True, what I wanted to do there was calling the parent's method Compositor::reinitialize(). Thanks!

romangg updated this revision to Diff 60998.Jul 2 2019, 10:54 AM
  • Fix call to Compositor reinitialize
  • Remove optimization through compositing initialized check
romangg marked 2 inline comments as done.Jul 2 2019, 10:54 AM
romangg edited the summary of this revision. (Show Details)
romangg updated this revision to Diff 61001.Jul 2 2019, 11:51 AM
  • Setup only in Compositor child classes
  • Rename function to continueSetup
  • Rebase on master
romangg edited the summary of this revision. (Show Details)Jul 2 2019, 11:51 AM
romangg updated this revision to Diff 61003.Jul 2 2019, 12:31 PM
  • Remove reinit dbus method
  • Remove restart function and streamline setup
romangg edited the summary of this revision. (Show Details)Jul 2 2019, 12:32 PM
romangg updated this revision to Diff 61008.Jul 2 2019, 2:18 PM
  • XWayland destroyed signal only in Wayland compositor
  • Move initial unusedSupportPropertyTimer call into X11Compositor
romangg edited the summary of this revision. (Show Details)Jul 2 2019, 2:18 PM
romangg planned changes to this revision.Jul 2 2019, 6:44 PM

The slot syntax changes have been split out to D22218.

romangg updated this revision to Diff 61187.Jul 5 2019, 12:32 AM

Rebase on master (incorporates preliminary cleanup which was split out into
separate commits).

romangg retitled this revision from WIP: Split Compositor class in Wayland and X child classes to Split Compositor class in Wayland and X11 child classes.Jul 5 2019, 12:40 AM
romangg edited the summary of this revision. (Show Details)
romangg edited the test plan for this revision. (Show Details)
romangg added inline comments.Jul 5 2019, 8:03 AM
workspace.cpp
599

Unintended change.

romangg updated this revision to Diff 61210.Jul 5 2019, 11:36 AM

Revert unintended change.

romangg marked an inline comment as done.Jul 5 2019, 11:37 AM
zzag added inline comments.Jul 8 2019, 7:46 AM
composite.cpp
84

Is there a reason for not initializing s_compositor in constructor of Compositor class?

romangg added inline comments.Jul 8 2019, 8:56 AM
composite.cpp
84

Not in particular. Setting the static variable in the static method seems more readable to me. Also since we assert here. And its the same like the kwinglobals factory methods do it.

zzag added inline comments.Jul 9 2019, 9:44 AM
composite.cpp
178

AbstractClient has generic methods that call "platform-specific" methods to do the dirty work, e.g. doResize, doSetKeepAbove, etc. I've been wondering whether we could apply the same pattern here in the Compositor class.

romangg added inline comments.Jul 10 2019, 8:35 AM
composite.cpp
178

Maybe. Is it a blocker or a necessity to this patch here? Otherwise let's tackle one idea after the other, shall we?

zzag added a comment.Jul 10 2019, 9:20 AM

Will support properties stay in the Compositor class?

composite.cpp
178

Well, some parts of KWin(as well Qt) follow this pattern, e.g. EffectsHandlerImpl, so it's a good idea to stick with this pattern in Compositor class. Given that the Compositor class still has X11 bits, I guess we can go with this approach for now.

composite.h
49

"For pointers or references, use a single space before '*' or '&', but not after"

(there are other places in this patch where whitespace for pointers needs to be fixed)

192

I guess we don't need it yet.

dbusinterface.cpp
313

I know that I already mentioned the problem with whitespace before pointers, but this one may not be obvious.

"For pointers or references, use a single space before '*' or '&', but not after"

applies to templates as well, so the static_cast will look like

static_cast<X11Compositor *>(m_compositor)->....

(I hope that one day we will be able to run clang-format over KWin's code bbase)

In D22195#493439, @zzag wrote:

Will support properties stay in the Compositor class?

Yea, it's needed for the single case of a graphics reset through Gl error. I don't know if we could recover at all from that on Wayland, but I let it in for now.

composite.h
49

On function return values I prefer to put the asterisks without space next to the return type since it specifies the return type and not the function. If there are no counter arguments to that and you want to make a rule out of it we can discuss that at KWin sprint.

192

It's either way fine. I like to put the destructor in since most people expect it to be somewhere in a class and it's only a single line. Please don't let us discuss every little shit while ignoring the big picture.

dbusinterface.cpp
313

It does not make sense to put a whitespace in this case, since there is no identifier to the right of the asterisks. Sticking to rules is fine, as long as it makes sense.

zzag added inline comments.Jul 10 2019, 10:24 AM
composite.h
49

As a "WebKit coding style"-head, I agree that putting a whitespace before a pointer doesn't make sense because it's part of the type, but we follow Kdelibs/Frameworks coding style, which dictates that a whitespace has to be put before pointers or references. It doesn't matter whether it's a function or a template argument, you have to put a whitespace. Similar story with clang-format. If we run it, the asterisk will be aligned to right.

Please notice that I didn't "invent" or "make out" this rule.

dbusinterface.cpp
313

Why not add matching virtual function in base, doing nothing in Wayland, every time i see exclusive check on polymorphic type is just weak design.

romangg added inline comments.Jul 10 2019, 11:10 AM
composite.h
49

KWin doesn't follow Kdelibs/Frameworks coding style per se since it's not part of frameworks. Let's discuss this at KWin sprint.

zzag added inline comments.Jul 15 2019, 12:38 PM
composite.h
49

Nope, it does, see HACKING.md.

romangg added inline comments.Jul 15 2019, 1:00 PM
composite.h
49

We should change what this file says then. As said, let's discuss this at KWin sprint.

zzag added inline comments.Jul 15 2019, 1:19 PM
composite.h
49

I don't see any reason why we should switch the coding style. KWin's been following the kdelibs coding style since 2009 or since when Martin reformatted the code.

As said, let's discuss this at KWin sprint.

Okay, but as I said, I don't see any reason to switch. Pointer alignment based on context is pretty weak because from time to time one should ask himself how a pointer/reference has to be aligned. People will also get it wrong. Heck, we have issues even with aligning pointers and references always to right.

Another disadvantage of the proposed pointer alignment scheme is that it's quite exotic and rare. Think of it as our current
Doxygen comment style. /** **/ looks balanced and neat, but most people don't use it. They instead prefer /** */, more common style.

romangg updated this revision to Diff 63298.Wed, Aug 7, 4:36 PM

Rebase on master.

This revision was not accepted when it landed; it landed in state Needs Review.Wed, Aug 7, 7:07 PM
This revision was automatically updated to reflect the committed changes.