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.
Details
Details
- Reviewers
zzag - Group Reviewers
KWin - Maniphest Tasks
- T11071: Rework compositing pipeline
- Commits
- R108:ead1b3456313: KSelectionOwner usage in ApplicationX11 and Compositor classes
Manually in X session and Wayland windowed.
Diff Detail
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.
Comment Actions
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.
Comment Actions
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.
| 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. | |
| 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. | |
Comment Actions
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.
| composite.cpp | ||
|---|---|---|
| 72–92 |
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. | |
Comment Actions
I still don't see why KWinSelectionOwner and CompositorSelectionOwner have to be defined in cpp files. They can stay in header files.
Comment Actions
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.