As a preparation for further changes clean up the Compositor
class. First step is to use the new slot syntax and rename
some of the slots.
Includes some other minor code style improvements to the class
as well.
zzag |
KWin |
As a preparation for further changes clean up the Compositor
class. First step is to use the new slot syntax and rename
some of the slots.
Includes some other minor code style improvements to the class
as well.
Manually in X and Wayland nested session.
Lint Errors | Excuse: Untouched code. |
Severity | Location | Code | Message |
---|---|---|---|
Error | kwinbindings.cpp:137 | Cppcheck | syntaxError |
No Unit Test Coverage |
Buildable 13583 | |
Build 13601: arc lint + arc unit |
TimerEvent shouldn't really be made private.
We typically don't change accessibility of superclass methods.
Now I get lots of
QObject::connect: No such slot KWin::Compositor::scheduleRepaint() in /home/vlad/Workspace/KDE/src/kde/workspace/kwin/workspace.cpp:740 QObject::connect: No such slot KWin::Compositor::scheduleRepaint() in /home/vlad/Workspace/KDE/src/kde/workspace/kwin/workspace.cpp:740 QObject::connect: No such slot KWin::Compositor::scheduleRepaint() in /home/vlad/Workspace/KDE/src/kde/workspace/kwin/workspace.cpp:740
We don't? It's not really a make-or-break thing for me, but what's the rationale for that? It seems to me more sensible to have the most narrow accessibility used when possible and if there are no child classes to begin with don't declare something protected.
composite.h | ||
---|---|---|
86–87 | There is a invokeMethod call to it. |
I have another warning
QObject::connect: No such slot KWin::Compositor::scheduleRepaint() in /home/vlad/Workspace/KDE/src/kde/workspace/kwin/workspace.cpp:608
Thanks again. Did you trigger getting these warnings somehow? It didn't show for me. Will change.
Can you re-iterate on why Q_SLOTS section has been removed?
Besides timerEvent and Q_INVOKABLE, I don't see any issues.
We don't usually use it nowadays anymore since any function can be the receiver of a signal with the new connect syntax. Is this incorrect?
Okay, I see, let's keep suspend and setup as Q_INVOKABLE. Though I do agree with David that timerEvent should stay protected.
Kepp the timerEvent protected. Discussed on irc and in query: argument for
keeping it is that it is done this way in Qt internals and that for deciding
about the accessibility of a method the semantic meaning is more important
than the actual usage by child classes.
dbusinterface.h | ||
---|---|---|
172 | Wrong terminator (not cyborg). Use **/ instead. |
dbusinterface.h | ||
---|---|---|
172 | Wrong by what arbitrary rule? Two stars is wrong when looking at upstream recommendation and other projects. I told you this already half a year ago after you went through reformatting all the code base. Still you insist on that being the "right" way to do it. We should use one star at the end of Doxygen comments consistently for new comments. |
dbusinterface.h | ||
---|---|---|
172 | And this reformatting was done without checking back on my opinion on that first although quite a lot of that code was written by me and I'm an active core contributor to KWin longer than you. Similarly just a few days ago you reformatted my complete XWayland drag-and-drop code to your personal code rules without review or even saying anything beforehand. I hope there won't be again an excuse for you to not come to a meetup so we can discuss your behavior in regards to code styling and review etiquette later this month at the KWin sprint. |
dbusinterface.h | ||
---|---|---|
172 | Listen, put yourself in my shoes. I see a code base with some certain coding style, and your patch follows slightly different style. Do you even realize that such small disagreements contribute to coding style masquerade? If you don't like the current doxygen style, then submit a patch to change it to a more main stream style. Running git grep + sed with s/ \*\*\// \*\// should be enough. I'll accept that patch, and I think any fellas from Plasma do the same. Also, I find it a bit frustrating that a lot of time goes into disputing coding style. If you don't like something, stick with it for a moment, but don't invent "new style." Address the issue in another patch.
OK. |
dbusinterface.h | ||
---|---|---|
172 | Yea, let's talk about this at the sprint. Just so we understand each other correctly: I don't have anything against you personally and I value your expertise and hard work on KWin. Different people just have different styles of solving problems and if that's not organized correctly this might clash. We need to organize our project such that it suits the project, us and other part-time contributors and the KWin sprint will give us opportunity to do that. |