graesslin requested changes to this revision. graesslin added inline comments. This revision now requires changes to proceed.
INLINE COMMENTS > CMakeLists.txt:27 > Test > + Widgets > ) I don't see Widgets used anywhere. > CMakeLists.txt:1 > +add_definitions(-DTRANSLATION_DOMAIN="kcmkwindecoration") > + This is not kcmkwin, but kdecoration > CMakeLists.txt:22 > Qt5::Gui > + Qt5::Widgets > PRIVATE I don't see Widgets used anywhere > decorationbutton.cpp:302 > connect(this, &DecorationButton::hoveredChanged, this, updateSlot); > + connect(this, &DecorationButton::hoveredChanged, this, > &DecorationButton::showTooltip); > connect(this, &DecorationButton::pressedChanged, this, updateSlot); If I see correctly this could become a lambda function which would not require to add a showTooltip method > decorationbutton.cpp:512-513 > + > + //TODO: change offset to something valuable > + hovered ? emit showtooltip(type) : hidetooltip(); > + Instead of emitting the signal please invoke the method requestShowTooltip directly (I don't think we need to queue it) > decorationbutton.h:155-156 > > + //* show tooltip > + void showTooltip(bool); > + I think this method is not needed (see comment about the lambda connection) > decorationbutton.h:158 > + > + QString typeToString( DecorationButtonType type ); > + This should not be in the public header. Please move to private > decorationbutton.h:167-168 > void doubleClicked(); > + void showtooltip(QString); > + void hidetooltip(); > I don't think we need those signals. > decoratedclientprivate.h:79-80 > > + virtual void requestShowToolTip(QString &text) = 0; > + virtual void requestHideToolTip() = 0; > virtual void requestClose() = 0; If we add pure virtuals we need to increase the so version of the private libraru. REPOSITORY R129 Window Decoration Library REVISION DETAIL https://phabricator.kde.org/D7246 To: McPain, #breeze, #plasma, graesslin Cc: ngraham, broulik, plasma-devel, #breeze, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
