Review Request 129648: New widget: tooltip that contains another widget

[Christoph Feck]

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



Looks good so far, but see issues below. Please do not use C++11 features in library code yet (it's okey for tests).

Regarding the 500 ms timeout, does it need to be configurable using a property? I am unsure how the interaction with the widget interferes with the 'hide' timeout.


src/ktooltipwidget.h (line 35)
<https://git.reviewboard.kde.org/r/129648/#comment67973>

    missing 'you'



src/ktooltipwidget.h (line 37)
<https://git.reviewboard.kde.org/r/129648/#comment67974>

    tooltips



src/ktooltipwidget.h (line 46)
<https://git.reviewboard.kde.org/r/129648/#comment67975>

    Please use Q_NULLPTR as is used elsewhere in KWidgetsAddons.



src/ktooltipwidget.h (line 72)
<https://git.reviewboard.kde.org/r/129648/#comment67976>

    I am not a native english speaker, but I think 'showBelow' sounds better.
    
    Needs a comment from a native speaker maybe.
    
    Additionally, how about adding a 'showAbove' too?



src/ktooltipwidget.h (line 89)
<https://git.reviewboard.kde.org/r/129648/#comment67977>

    Please use Q_DECL_OVERRIDE as used elsewhere in KWidgetsAddons.



src/ktooltipwidget.cpp (line 176)
<https://git.reviewboard.kde.org/r/129648/#comment67978>

    No lambda, please.


- Christoph Feck


On Dec. 20, 2016, 4:40 p.m., Elvis Angelaccio wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129648/
> -----------------------------------------------------------
> 
> (Updated Dec. 20, 2016, 4:40 p.m.)
> 
> 
> Review request for KDE Frameworks, Ben Cooksley, Christoph Feck, and Martin Gräßlin.
> 
> 
> Repository: kwidgetsaddons
> 
> 
> Description
> -------
> 
> This new widget is based on the KToolTip code that is duplicated across multiple products: at least Dolphin, systemsettings, kinfocenter, ktp-contact-list.
> 
> Rationale: with a single class in frameworks, it will be possible to apply features/fixes only once. See for example the comments in https://phabricator.kde.org/D3112
> 
> A new feature that the old code doesn't have is the delayed hide: this makes it possible to actually use the widget shown in the tooltip.
> 
> 
> Diffs
> -----
> 
>   autotests/CMakeLists.txt ffcce3a046ec98b07c4677578f6bc997de1ef16b 
>   autotests/ktooltipwidgettest.h PRE-CREATION 
>   autotests/ktooltipwidgettest.cpp PRE-CREATION 
>   src/CMakeLists.txt de0a8d965f1541d5ffeec93d1aa06600b0b9c138 
>   src/ktooltipwidget.h PRE-CREATION 
>   src/ktooltipwidget.cpp PRE-CREATION 
>   tests/CMakeLists.txt fcb348b1ae9d4270468c3f9003c5ba5f3903db84 
>   tests/ktooltipwidget_test.h PRE-CREATION 
>   tests/ktooltipwidget_test.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/129648/diff/
> 
> 
> Testing
> -------
> 
> Manual test works both in X11 and Wayland. Unit tests pass.
> 
> Ported Dolphin locally to this new class, everything seems to work (and this will fix bug [#352276](https://bugs.kde.org/show_bug.cgi?id=352276) and [#371223](https://bugs.kde.org/show_bug.cgi?id=371223)).
> 
> 
> Thanks,
> 
> Elvis Angelaccio
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20161221/dcf28d66/attachment.html>