Re: Review Request 129663: Don't break accelerators in KToolBar

2016-12-17 Thread David Faure 

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



I agree that doing this in Show is far too late - and that KAcceleratorManager 
needs to be told, to avoid the infinite loop.

However the reason for this code still holds I think, so it seems to me that it 
needs to be improved instead of removed.

=> this should be done in Polish (the event, not the language :-) ) and 
KAcceleratorManager::setNoAccel(tb) should prevent the recursion.

(... and the no-op case (nothing to remove) shouldn't trigger anything... How 
come tb->setText(tb->text()) does anything? Or are you seeing this bug with CJK 
languages only?)

- David Faure


On Dec. 17, 2016, 12:23 p.m., Martin Tobias Holmedahl Sandsmark wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129663/
> ---
> 
> (Updated Dec. 17, 2016, 12:23 p.m.)
> 
> 
> Review request for KDE Frameworks, David Faure and Chusslove Illich.
> 
> 
> Repository: kxmlgui
> 
> 
> Description
> ---
> 
> Don't try to strip out accelerators in the KToolBar event handler. It makes 
> no sense to me, potentially creates an endless repaint loop and fights with 
> KAcceleratorManager which will constantly re-add accelerators.
> 
> 
> Diffs
> -
> 
>   src/ktoolbar.cpp 31be9b0 
> 
> Diff: https://git.reviewboard.kde.org/r/129663/diff/
> 
> 
> Testing
> ---
> 
> With this patch not only the control button in Dolphin has an accelerator.
> 
> 
> Thanks,
> 
> Martin Tobias Holmedahl Sandsmark
> 
>

Re: Review Request 129666: [KNotificationRestrictions] Let user can specify restriction reason string

2016-12-17 Thread Martin Klapetek 


> On Dec. 17, 2016, 4:53 p.m., Martin Klapetek wrote:
> > Just because it "looks annoying" doesn't mean it doesn't have its use. 
> > 
> > I'll do the git-blame-search for you: 
> > https://cgit.kde.org/knotifications.git/commit/?id=3f080d44af41d0158d7b5c51269449e78d5b128f
> 
> Anthony Fieroni wrote:
> I will correct GwenView too, about me this patch is at wrong place

This is not about Gwenview, this is about safe API that won't break with any 
other app actually not providing the reason.

Please leave this part alone.


> On Dec. 17, 2016, 4:53 p.m., Martin Klapetek wrote:
> > src/knotificationrestrictions.h, line 97
> > 
> >
> > Why?
> 
> Anthony Fieroni wrote:
> If new contructor is accepted, old one can be omitted.

This is a useful constructor, it doesn't need to be removed. Not all 
inhibitions *must* include a reason, iirc anyway.


- Martin


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


On Dec. 17, 2016, 4:48 p.m., Anthony Fieroni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129666/
> ---
> 
> (Updated Dec. 17, 2016, 4:48 p.m.)
> 
> 
> Review request for KDE Frameworks and Martin Klapetek.
> 
> 
> Repository: knotifications
> 
> 
> Description
> ---
> 
> "no_reason_specified" looks annoying reason
> 
> 
> Diffs
> -
> 
>   src/knotificationrestrictions.h e9179ae 
>   src/knotificationrestrictions.cpp 5f5d908 
> 
> Diff: https://git.reviewboard.kde.org/r/129666/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anthony Fieroni
> 
>

Re: Review Request 129666: [KNotificationRestrictions] Let user can specify restriction reason string

2016-12-17 Thread Anthony Fieroni 


> On Дек. 17, 2016, 5:53 след обяд, Martin Klapetek wrote:
> > Just because it "looks annoying" doesn't mean it doesn't have its use. 
> > 
> > I'll do the git-blame-search for you: 
> > https://cgit.kde.org/knotifications.git/commit/?id=3f080d44af41d0158d7b5c51269449e78d5b128f

I will correct GwenView too, about me this patch is at wrong place


> On Дек. 17, 2016, 5:53 след обяд, Martin Klapetek wrote:
> > src/knotificationrestrictions.h, line 97
> > 
> >
> > Why?

If new contructor is accepted, old one can be omitted.


- Anthony


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


On Дек. 17, 2016, 5:48 след обяд, Anthony Fieroni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129666/
> ---
> 
> (Updated Дек. 17, 2016, 5:48 след обяд)
> 
> 
> Review request for KDE Frameworks and Martin Klapetek.
> 
> 
> Repository: knotifications
> 
> 
> Description
> ---
> 
> "no_reason_specified" looks annoying reason
> 
> 
> Diffs
> -
> 
>   src/knotificationrestrictions.h e9179ae 
>   src/knotificationrestrictions.cpp 5f5d908 
> 
> Diff: https://git.reviewboard.kde.org/r/129666/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anthony Fieroni
> 
>

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 129666: [KNotificationRestrictions] Let user can specify restriction reason string

2016-12-17 Thread Martin Klapetek 

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



Just because it "looks annoying" doesn't mean it doesn't have its use. 

I'll do the git-blame-search for you: 
https://cgit.kde.org/knotifications.git/commit/?id=3f080d44af41d0158d7b5c51269449e78d5b128f


src/knotificationrestrictions.h (line 97)


Why?


- Martin Klapetek


On Dec. 17, 2016, 4:48 p.m., Anthony Fieroni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129666/
> ---
> 
> (Updated Dec. 17, 2016, 4:48 p.m.)
> 
> 
> Review request for KDE Frameworks and Martin Klapetek.
> 
> 
> Repository: knotifications
> 
> 
> Description
> ---
> 
> "no_reason_specified" looks annoying reason
> 
> 
> Diffs
> -
> 
>   src/knotificationrestrictions.h e9179ae 
>   src/knotificationrestrictions.cpp 5f5d908 
> 
> Diff: https://git.reviewboard.kde.org/r/129666/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anthony Fieroni
> 
>

Review Request 129666: [KNotificationRestrictions] Let user can specify restriction reason string

2016-12-17 Thread Anthony Fieroni 

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

Review request for KDE Frameworks and Martin Klapetek.


Repository: knotifications


Description
---

"no_reason_specified" looks annoying reason


Diffs
-

  src/knotificationrestrictions.h e9179ae 
  src/knotificationrestrictions.cpp 5f5d908 

Diff: https://git.reviewboard.kde.org/r/129666/diff/


Testing
---


Thanks,

Anthony Fieroni

Re: Review Request 129665: [KStatusNotifierItem] Restore mnimized window as normal

2016-12-17 Thread Anthony Fieroni 

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

(Updated Дек. 17, 2016, 5:39 след обяд)


Review request for KDE Frameworks and Martin Gräßlin.


Repository: knotifications


Description
---

I think, we want minized window to be shown as normal when it's closed in tray. 
Why we could want a window to be shown as minimized ?


Diffs (updated)
-

  src/kstatusnotifieritem.cpp 1cd5e00 

Diff: https://git.reviewboard.kde.org/r/129665/diff/


Testing
---

+ Remove deprecated warnings


Thanks,

Anthony Fieroni

Review Request 129665: [KStatusNotifierItem] Restore mnimized window as normal

2016-12-17 Thread Anthony Fieroni 

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

Review request for KDE Frameworks and Martin Gräßlin.


Repository: knotifications


Description (updated)
---

I think, we want minized window to be shown as normal when it's closed in tray. 
Why we could want a window to be shown as minimized ?


Diffs
-


Diff: https://git.reviewboard.kde.org/r/129665/diff/


Testing (updated)
---

+ Remove deprecated warnings


Thanks,

Anthony Fieroni

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

Review Request 129663: Don't break accelerators in KToolBar

2016-12-17 Thread Martin Tobias Holmedahl Sandsmark 

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

Review request for KDE Frameworks, David Faure and Chusslove Illich.


Repository: kxmlgui


Description
---

Don't try to strip out accelerators in the KToolBar event handler. It makes no 
sense to me, potentially creates an endless repaint loop and fights with 
KAcceleratorManager which will constantly re-add accelerators.


Diffs
-

  src/ktoolbar.cpp 31be9b0 

Diff: https://git.reviewboard.kde.org/r/129663/diff/


Testing
---

With this patch not only the control button in Dolphin has an accelerator.


Thanks,

Martin Tobias Holmedahl Sandsmark

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

[Differential] [Closed] D3636: [kconfig_compiler] Improve documentation about Inherits

2016-12-17 Thread Martin Gräßlin 
This revision was automatically updated to reflect the committed changes.
Closed by commit R237:b4206d635444: [kconfig_compiler] Improve documentation 
about Inherits (authored by graesslin).

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D3636?vs=9049=9112

REVISION DETAIL
  https://phabricator.kde.org/D3636

AFFECTED FILES
  src/kconfig_compiler/README.dox

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: graesslin, #frameworks, mdawson, dfaure
Cc: nalvarez

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