----------------------------------------------------------- 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 > >
