KSelectionOwner usage in ApplicationX11 and Compositor classes
ClosedPublic

Authored by romangg on Jun 7 2019, 8:00 PM.

Details

Summary

Declare it in the source file for internal usage of ApplicationX11 and
Compositor classes. Additionally use modern style connect and do other
code style updates.

Test Plan

Manually in X session and Wayland windowed.

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.Jun 7 2019, 8:00 PM
Restricted Application added a project: KWin. · View Herald TranscriptJun 7 2019, 8:00 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
romangg requested review of this revision.Jun 7 2019, 8:00 PM
romangg updated this revision to Diff 59370.Jun 7 2019, 8:27 PM
  • Also improve the KScelectionOwner of ApplicationX11
romangg retitled this revision from Rework CompositorSelectionOwner code to KSelectionOwner usage in ApplicationX11 and Compositor classes.Jun 7 2019, 8:29 PM
romangg edited the summary of this revision. (Show Details)
zzag added a subscriber: zzag.Jun 11 2019, 10:41 AM

Hmm, I wonder whether Application or Platform could provide/create desired Compositor instance. So, there's would be generic Compositor class and X11 specific CompositorX11 class with all that manager selection headache.

In D21655#478044, @zzag wrote:

Hmm, I wonder whether Application or Platform could provide/create desired Compositor instance. So, there's would be generic Compositor class and X11 specific CompositorX11 class with all that manager selection headache.

Do you mean the same thing as I did in the first "Structure" idea in T11071? As in:

Split up Compositor class into X and Wayland subclasses.

zzag added a comment.Jun 11 2019, 11:00 AM

Seems like it is. I haven't read the task description yet, sorry.

zzag requested changes to this revision.Tue, Jun 18, 11:01 AM
zzag added inline comments.
composite.cpp
72–92

It's perfectly fine to keep declaration of CompositorSelectionOwner class in the header file. It's also worth to point out that KSelectionOwner is a QObject, which means CompositorSelectionOwner needs to have a Q_OBJECT macro.

main_x11.h
27–40

I don't see why we need to move this class to the cpp file. It can stay here.

This revision now requires changes to proceed.Tue, Jun 18, 11:01 AM
romangg added inline comments.Tue, Jun 18, 3:23 PM
composite.cpp
72–92

It's a scope question. CompositorSelectionOwner is only used internally by Compositor class while the header is included in many other source files. When CompositorSelectionOwner in the header this is not obvious, when it's in the source file it is.

In regards to the macro: according to docs it is only needed when the class uses slots and signals. Well it does so why does it compile? Would it just fail at runtime?

Alternatives are to put the CompositorSelectionOwner class in a separate file or include the moc in the composite.cpp file.

Well it does so why does it compile? Would it just fail at runtime?

Adding and emitting a signal in this subclass will fail at compile time.
Anything that uses metaObject will fail at runtime. Old style connects to new slots won't work silently and it used to be a common infuriating trap to debug.

zzag added inline comments.Tue, Jun 18, 8:34 PM
composite.cpp
72–92

it is only needed when the class uses slots and signals

Not really, you need Q_OBJECT for other things too. In general, it's much easier to add Q_OBJECT and not worry about it.

romangg updated this revision to Diff 60308.Sat, Jun 22, 10:36 AM

Rebase on master.

romangg updated this revision to Diff 60309.Sat, Jun 22, 10:37 AM
  • Include moc to add back Q_OBJECT macro
romangg updated this revision to Diff 60311.Sat, Jun 22, 10:40 AM
  • Include moc to add back Q_OBJECT macro for main_x11.cpp too
zzag requested changes to this revision.Sat, Jun 22, 3:03 PM

I still don't see why KWinSelectionOwner and CompositorSelectionOwner have to be defined in cpp files. They can stay in header files.

This revision now requires changes to proceed.Sat, Jun 22, 3:03 PM
In D21655#484351, @zzag wrote:

I still don't see why KWinSelectionOwner and CompositorSelectionOwner have to be defined in cpp files. They can stay in header files.

I reasoned for this in a reply to your inline comment already. If you don't agree with the reason brought for you can argument against it.

Pushing the change now since no arguments have been given against it.

This revision was not accepted when it landed; it landed in state Needs Revision.Thu, Jun 27, 4:08 PM
This revision was automatically updated to reflect the committed changes.