D7246: Tooltips

[Martin Flöser]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20171227/efd2bdff/attachment.html>