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

2017-01-01 Thread Elvis Angelaccio 

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

(Updated Jan. 1, 2017, 4:56 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks, Ben Cooksley, Christoph Feck, and Martin 
Gräßlin.


Changes
---

Submitted with commit d0b80687769d0b33d4ec0f3f747383f5c28bc1d6 by Elvis 
Angelaccio to branch master.


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

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

2017-01-01 Thread Elvis Angelaccio 


> On Jan. 1, 2017, 3:25 p.m., Martin Gräßlin wrote:
> > Looks good to me. Using the transientParent's qscreen was what I meant.

Thanks, I'm going to commit then. :)


- Elvis


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


On Dec. 30, 2016, 10:27 a.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129648/
> ---
> 
> (Updated Dec. 30, 2016, 10:27 a.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
> 
>

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

2017-01-01 Thread Martin Gräßlin 

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



Looks good to me. Using the transientParent's qscreen was what I meant.

- Martin Gräßlin


On Dec. 30, 2016, 11:27 a.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129648/
> ---
> 
> (Updated Dec. 30, 2016, 11:27 a.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
> 
>

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

2016-12-30 Thread Elvis Angelaccio 

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

(Updated Dec. 30, 2016, 10:27 a.m.)


Review request for KDE Frameworks, Ben Cooksley, Christoph Feck, and Martin 
Gräßlin.


Changes
---

Use the QScreen of the transientParent QWindow


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 (updated)
-

  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

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

2016-12-30 Thread Elvis Angelaccio 


> On Dec. 29, 2016, 4:01 p.m., Martin Gräßlin wrote:
> > src/ktooltipwidget.cpp, line 101
> > 
> >
> > this won't work on Wayland, there is no global cursor pos.
> 
> Elvis Angelaccio wrote:
> hmm, not sure what to use instead.
> `rect`? Or maybe `rect.topLeft()` is enough?
> 
> Martin Gräßlin wrote:
> Cant you just use the QScreen of the window you place the tooltip on?

The problem is that these size checks are done before the tooltip gets placed. 
Maybe I can try to use the transientParent's QScreen...


- Elvis


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


On Dec. 29, 2016, 10:11 a.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129648/
> ---
> 
> (Updated Dec. 29, 2016, 10:11 a.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
> 
>

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

2016-12-29 Thread Martin Gräßlin 


> On Dec. 29, 2016, 5:01 p.m., Martin Gräßlin wrote:
> > src/ktooltipwidget.cpp, line 101
> > 
> >
> > this won't work on Wayland, there is no global cursor pos.
> 
> Elvis Angelaccio wrote:
> hmm, not sure what to use instead.
> `rect`? Or maybe `rect.topLeft()` is enough?

Cant you just use the QScreen of the window you place the tooltip on?


- Martin


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


On Dec. 29, 2016, 11:11 a.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129648/
> ---
> 
> (Updated Dec. 29, 2016, 11:11 a.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
> 
>

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

2016-12-29 Thread Elvis Angelaccio 


> On Dec. 29, 2016, 4:01 p.m., Martin Gräßlin wrote:
> > src/ktooltipwidget.cpp, line 101
> > 
> >
> > this won't work on Wayland, there is no global cursor pos.

hmm, not sure what to use instead.
`rect`? Or maybe `rect.topLeft()` is enough?


- Elvis


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


On Dec. 29, 2016, 10:11 a.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129648/
> ---
> 
> (Updated Dec. 29, 2016, 10:11 a.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
> 
>

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

2016-12-29 Thread Martin Gräßlin 

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




src/ktooltipwidget.cpp (line 101)


this won't work on Wayland, there is no global cursor pos.


- Martin Gräßlin


On Dec. 29, 2016, 11:11 a.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129648/
> ---
> 
> (Updated Dec. 29, 2016, 11:11 a.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
> 
>

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

2016-12-29 Thread Elvis Angelaccio 

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

(Updated Dec. 29, 2016, 10:11 a.m.)


Review request for KDE Frameworks, Ben Cooksley, Christoph Feck, and Martin 
Gräßlin.


Changes
---

* QScopedPointer for the d pointer
* QDesktopWidget -> QScreen (loop over available geometries and check which one 
contains the mouse cursor)


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 (updated)
-

  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

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

2016-12-28 Thread Martin Gräßlin 

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




src/ktooltipwidget.cpp (line 98)


Do not use QDesktopWidget. Most likely the information is wrong. Use 
QScreen instead.



src/ktooltipwidget.cpp (line 146)


use a QScopedPointer for the d pointer and don't care about manually 
deleting it ;-)


- Martin Gräßlin


On Dec. 21, 2016, 7:43 p.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129648/
> ---
> 
> (Updated Dec. 21, 2016, 7:43 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
> 
>

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

2016-12-28 Thread Elvis Angelaccio 

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




src/ktooltipwidget.cpp (lines 139 - 141)


@Martin: is everything ok here from your side?


- Elvis Angelaccio


On Dec. 21, 2016, 6:43 p.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129648/
> ---
> 
> (Updated Dec. 21, 2016, 6:43 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
> 
>

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

2016-12-21 Thread Elvis Angelaccio 

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

(Updated Dec. 21, 2016, 6:43 p.m.)


Review request for KDE Frameworks, Ben Cooksley, Christoph Feck, and Martin 
Gräßlin.


Changes
---

Removed `d->hideDelay` member, it's enough to use `interval()` and 
`setInterval()` on the timer. Expanded manual test.


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 (updated)
-

  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

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

2016-12-21 Thread Christoph Feck 

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


Fix it, then Ship it!




That's it from my side.


src/ktooltipwidget.cpp (line 180)


Need to set the configured value for the timer here, otherwise it will 
always be 500.


- Christoph Feck


On Dec. 21, 2016, 6:57 p.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129648/
> ---
> 
> (Updated Dec. 21, 2016, 6:57 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
> 
>

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

2016-12-21 Thread Elvis Angelaccio 

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

(Updated Dec. 21, 2016, 5:57 p.m.)


Review request for KDE Frameworks, Ben Cooksley, Christoph Feck, and Martin 
Gräßlin.


Changes
---

* Fixed initializations and renamed to 'showBelow'
* If `hideDelay()` is 0, `hideLater()` should be equivalent to `hide()`. Add 
test case for this.


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 (updated)
-

  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

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

2016-12-21 Thread Christoph Feck 

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




src/ktooltipwidget.cpp (line 48)


Please move initializations to the constructor as member initialization.



src/ktooltipwidget.cpp (line 48)


Please move initializations to the constructor as member initialization.


- Christoph Feck


On Dec. 21, 2016, 6:31 p.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129648/
> ---
> 
> (Updated Dec. 21, 2016, 6:31 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
> 
>

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

2016-12-21 Thread Elvis Angelaccio 


> On Dec. 21, 2016, 5:23 p.m., Christoph Feck wrote:
> > src/ktooltipwidget.cpp, line 94
> > 
> >
> > 'auto' is C++11, too (if used as a type). Please use explicit types 
> > here.
> > 
> > Especially with 'screen' I was thinking this is the screen number. 
> > Suggest to rename it to 'screenRect' or 'screenGeometry' to make it clearer.
> > 
> > I hope the mouse is actually at the screen this function assumes it is 
> > :)

I think so, seems to work if I move the test app to another screen


- Elvis


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


On Dec. 21, 2016, 5:31 p.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129648/
> ---
> 
> (Updated Dec. 21, 2016, 5:31 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
> 
>

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

2016-12-21 Thread Elvis Angelaccio 

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

(Updated Dec. 21, 2016, 5:29 p.m.)


Review request for KDE Frameworks and Christoph Feck.


Changes
---

Drop `auto` and rename `screen` variable


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 (updated)
-

  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

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

2016-12-21 Thread Christoph Feck 


> On Dec. 21, 2016, 5 p.m., Christoph Feck wrote:
> > src/ktooltipwidget.h, line 72
> > 
> >
> > 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?
> 
> Elvis Angelaccio wrote:
> Pheraps we need a different naming approach here: `showBelow` is actually 
> "show below if there is enough space on screen", otherwise it will be shown 
> above the rectangle. That's why I think a `showAbove` wouldn't make much 
> sense.
> 
> What about something like `showNear`?

Well, I was thinking about making it possible for the application to choose the 
default position (top or bottom). But you are right, the default should really 
be below, because the content starts at the top, and I think the name should 
reflect that. So unless someone corrects me, go for 'showBelow'.


- Christoph


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


On Dec. 21, 2016, 6:19 p.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129648/
> ---
> 
> (Updated Dec. 21, 2016, 6:19 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
> 
>

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

2016-12-21 Thread Christoph Feck 

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




src/ktooltipwidget.cpp (line 94)


'auto' is C++11, too (if used as a type). Please use explicit types here.

Especially with 'screen' I was thinking this is the screen number. Suggest 
to rename it to 'screenRect' or 'screenGeometry' to make it clearer.

I hope the mouse is actually at the screen this function assumes it is :)


- Christoph Feck


On Dec. 21, 2016, 6:19 p.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129648/
> ---
> 
> (Updated Dec. 21, 2016, 6:19 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
> 
>

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

2016-12-21 Thread Elvis Angelaccio 

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

(Updated Dec. 21, 2016, 5:19 p.m.)


Review request for KDE Frameworks, Ben Cooksley, Christoph Feck, and Martin 
Gräßlin.


Changes
---

Add `hideDelay` properties, defaults to 500 ms.


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 (updated)
-

  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

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

2016-12-21 Thread Elvis Angelaccio 


> On Dec. 21, 2016, 4 p.m., Christoph Feck wrote:
> > src/ktooltipwidget.h, line 72
> > 
> >
> > 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?

Pheraps we need a different naming approach here: `showBelow` is actually "show 
below if there is enough space on screen", otherwise it will be shown above the 
rectangle. That's why I think a `showAbove` wouldn't make much sense.

What about something like `showNear`?


- Elvis


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


On Dec. 21, 2016, 5:01 p.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129648/
> ---
> 
> (Updated Dec. 21, 2016, 5:01 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
> 
>

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

2016-12-21 Thread Elvis Angelaccio 

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

(Updated Dec. 21, 2016, 5:01 p.m.)


Review request for KDE Frameworks, Ben Cooksley, Christoph Feck, and Martin 
Gräßlin.


Changes
---

Fixed typos/c++11 issues


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 (updated)
-

  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

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

2016-12-21 Thread Elvis Angelaccio 


> On Dec. 21, 2016, 4 p.m., Christoph Feck wrote:
> > 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.

If the mouse hovers the tooltip the hide timeout is stopped and the tooltip 
will stay visible. This allows interaction with the inner widget. The tooltip 
will be hidden as soon as the mouse leaves the window.

About the timeout itself, yes I think that a property may be desiderable, after 
all 500ms is just a random value that I picked.


- Elvis


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


On Dec. 20, 2016, 3: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, 3: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
> 
>

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

2016-12-21 Thread 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)


missing 'you'



src/ktooltipwidget.h (line 37)


tooltips



src/ktooltipwidget.h (line 46)


Please use Q_NULLPTR as is used elsewhere in KWidgetsAddons.



src/ktooltipwidget.h (line 72)


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)


Please use Q_DECL_OVERRIDE as used elsewhere in KWidgetsAddons.



src/ktooltipwidget.cpp (line 176)


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

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

2016-12-20 Thread Elvis Angelaccio 

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

(Updated Dec. 20, 2016, 3:40 p.m.)


Review request for KDE Frameworks, Ben Cooksley, Christoph Feck, and Martin 
Gräßlin.


Changes
---

Improve apidox


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 (updated)
-

  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

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

2016-12-17 Thread Elvis Angelaccio 

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

(Updated Dec. 17, 2016, 4:24 p.m.)


Review request for KDE Frameworks, Ben Cooksley, Christoph Feck, and Martin 
Gräßlin.


Changes
---

Rename to KToolTipWidget


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 (updated)
-

  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

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

2016-12-17 Thread Christoph Feck 

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



Minor nitpick: it's "ToolTip", not "Tooltip", see QToolTip class.

I will review the rest in the coming days.

- Christoph Feck


On Dec. 17, 2016, 1:35 p.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129648/
> ---
> 
> (Updated Dec. 17, 2016, 1:35 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
> 
>

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

2016-12-17 Thread Elvis Angelaccio 

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

(Updated Dec. 17, 2016, 12:35 p.m.)


Review request for KDE Frameworks, Ben Cooksley, Christoph Feck, and Martin 
Gräßlin.


Changes
---

Fixed crash (forgot to move the createWinId() call)


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 (updated)
-

  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

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

2016-12-17 Thread Elvis Angelaccio 

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

(Updated Dec. 17, 2016, 11:25 a.m.)


Review request for KDE Frameworks and Christoph Feck.


Changes
---

* Fixed license
* Set property on the native window to hint for the blur-behind effect (thanks 
Martin)


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 (updated)
-

  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

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

2016-12-17 Thread Martin Gräßlin 


> On Dec. 14, 2016, 8:21 p.m., Martin Gräßlin wrote:
> > If I see correctly we are losing a feature here: blur behind.
> 
> Elvis Angelaccio wrote:
> Right, I had to drop that because we cannot use KWindowSystem in tier 1. 
> Is there a way to achieve the same feature with Qt only?
> 
> Martin Gräßlin wrote:
> Well there are multiple solutions:
> 
> * don't put it in this framework
> * make this framework a tier 2
> * add a property and let Plasma's platform plugin do the blurring
> 
> Elvis Angelaccio wrote:
> > add a property and let Plasma's platform plugin do the blurring
> 
> Can you expand on this? How would it work? Sounds like the best 
> solution...

sure, we have an example for it. KColorSchemeManager installs a property on the 
app, which is then read by the plasma-integration. In plasma-integration we 
have an event filter which monitors for all windows which get created and then 
install the X11/Wayland property.

In your case it will be even easier. Just set a dynamic property on the QWindow 
(not the QWidget!) to hint a fullscreen blur. Then we can do the respective 
call in plasma-integration.


- Martin


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


On Dec. 13, 2016, 4:45 p.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129648/
> ---
> 
> (Updated Dec. 13, 2016, 4:45 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
> 
>

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

2016-12-15 Thread Martin Gräßlin 


> On Dec. 14, 2016, 8:21 p.m., Martin Gräßlin wrote:
> > If I see correctly we are losing a feature here: blur behind.
> 
> Elvis Angelaccio wrote:
> Right, I had to drop that because we cannot use KWindowSystem in tier 1. 
> Is there a way to achieve the same feature with Qt only?

Well there are multiple solutions:

* don't put it in this framework
* make this framework a tier 2
* add a property and let Plasma's platform plugin do the blurring


- Martin


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


On Dec. 13, 2016, 4:45 p.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129648/
> ---
> 
> (Updated Dec. 13, 2016, 4:45 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
> 
>

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

2016-12-14 Thread Elvis Angelaccio 


> On Dec. 14, 2016, 7:21 p.m., Martin Gräßlin wrote:
> > If I see correctly we are losing a feature here: blur behind.

Right, I had to drop that because we cannot use KWindowSystem in tier 1. Is 
there a way to achieve the same feature with Qt only?


- Elvis


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


On Dec. 13, 2016, 3:45 p.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129648/
> ---
> 
> (Updated Dec. 13, 2016, 3:45 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
> 
>

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

2016-12-14 Thread Martin Gräßlin 

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



If I see correctly we are losing a feature here: blur behind.


src/ktooltipwidget.cpp (lines 8 - 11)


Should we update the license header to the LGPLv2.1+ as in 
https://community.kde.org/Policies/Licensing_Policy#LGPL_Header

Obviously applies for other files as well.


- Martin Gräßlin


On Dec. 13, 2016, 4:45 p.m., Elvis Angelaccio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129648/
> ---
> 
> (Updated Dec. 13, 2016, 4:45 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
> 
>

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

2016-12-13 Thread Elvis Angelaccio 

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

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