Review Request 129373: Plasma Framework changes for #129372

[Kai Uwe Broulik]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129373/#review100763
-----------------------------------------------------------



Thanks for your patch!

I'm not fully sure about this, but let's wait for other Plasma developer feedback :) Especially, since I couldn't yet look at your patch that actually makes use of this API in icontasks.


src/declarativeimports/core/tooltip.h (line 106)
<https://git.reviewboard.kde.org/r/129373/#comment67630>

    This won't work when tooltip is "interactive: false" (the default) because then mouse events pass right through it



src/declarativeimports/core/tooltip.h (line 128)
<https://git.reviewboard.kde.org/r/129373/#comment67631>

    I know it's been wrong in the other property already but "if a tooltip will show".
    
    Also, clarify when, "automatically show when the mouse hovers" or sth like that



src/declarativeimports/core/tooltip.h (line 147)
<https://git.reviewboard.kde.org/r/129373/#comment67632>

    Regardless of the rest of this patch, I like this. For example, in Application Dashboard we could use a single ToolTip instance and set the title and manually show it, just like we only have one global MouseArea in there as well.



src/declarativeimports/core/tooltip.cpp (line 162)
<https://git.reviewboard.kde.org/r/129373/#comment67633>

    You're connecting to this signal every time you do showToolTip(), use Qt::UniqueConnection



src/declarativeimports/core/tooltip.cpp (line 332)
<https://git.reviewboard.kde.org/r/129373/#comment67634>

    Coding style, brace on next line for functions:
    
    void Foo()
    {
    ...
    }



src/declarativeimports/core/tooltip.cpp (line 350)
<https://git.reviewboard.kde.org/r/129373/#comment67635>

    Coding style: Space after if:
    
    if (foo) {



src/declarativeimports/core/tooltip.cpp (line 358)
<https://git.reviewboard.kde.org/r/129373/#comment67639>

    I just wanted to complain about it not being keepAlive but that's been already there before :)



src/declarativeimports/core/tooltipdialog.cpp (line 153)
<https://git.reviewboard.kde.org/r/129373/#comment67636>

    can probably be const QObject *



src/declarativeimports/core/tooltipdialog.cpp (line 155)
<https://git.reviewboard.kde.org/r/129373/#comment67637>

    Coding style



src/declarativeimports/core/tooltipdialog.cpp (line 162)
<https://git.reviewboard.kde.org/r/129373/#comment67638>

    Coding style


- Kai Uwe Broulik


On Nov. 10, 2016, 8:47 nachm., René Fürst wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129373/
> -----------------------------------------------------------
> 
> (Updated Nov. 10, 2016, 8:47 nachm.)
> 
> 
> Review request for Plasma and Eike Hein.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> -------
> 
> Plasma Framework changes for #129372
> 
> 
> Diffs
> -----
> 
>   src/declarativeimports/core/tooltip.h d38c49b 
>   src/declarativeimports/core/tooltip.cpp ffe1064 
>   src/declarativeimports/core/tooltipdialog.h d4e0ff0 
>   src/declarativeimports/core/tooltipdialog.cpp 28ba9be 
> 
> Diff: https://git.reviewboard.kde.org/r/129373/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> René Fürst
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20161110/66982203/attachment.html>