Re: Review Request 112880: Added KColorSchemeToken class.

2013-09-22 Thread Denis Kuplyakov 

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

(Updated Sept. 22, 2013, 4:09 p.m.)


Review request for kdelibs.


Changes
---

Have rewriten fully token class to use Q_INVOKABLE methods.
It is better as
1) we don't need to store parameters
2) we don't need to create many KColorSchemeTokens for each color
3) it is more comfortable to use
4) we don't bother about nonsense NOTIFY, READ for parameters and nonsense 
NOTIFY for result.


Description
---

It is wrapper to access KColorScheme's methods from QML code.
Also added Q_GADGET to KColorScheme to enable Q_ENUMS using, to make them 
accessible from QML code.

As it will be accepted, QML-clone of KgPopupItem will be posted for review to 
libkdegames, as it uses it to access KDE's color theme.

More info:
* search for "KDE theme colors API for QML" thread at kdelibs and kdegames 
mailinglists *

NEED TO FIX:
I can't include it like #include  at KReversi's code, only 
"kcolorschemetoken.h". Maybe I've missed something?


Diffs (updated)
-

  kdeui/colors/kcolorschemetoken.cpp PRE-CREATION 
  kdeui/colors/kcolorschemetoken.h PRE-CREATION 
  kdeui/colors/kcolorscheme.cpp a6650ac 
  kdeui/colors/kcolorscheme.h 17570fd 
  kdeui/CMakeLists.txt b439e04 

Diff: http://git.reviewboard.kde.org/r/112880/diff/


Testing
---

I've tested it with KReversi's deniskup/gsoc2013/newdesign branch.


Thanks,

Denis Kuplyakov

Re: Review Request 112880: Added KColorSchemeToken class.

2013-09-22 Thread Denis Kuplyakov 


> On Sept. 22, 2013, 1:55 p.m., Kevin Krammer wrote:
> >

The main question is still opened: can we make needed enums make usable within 
Q_INVOKABLE function? It will be much simplier in implementation for enduser.


- Denis


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112880/#review40458
---


On Sept. 22, 2013, 10:17 a.m., Denis Kuplyakov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112880/
> ---
> 
> (Updated Sept. 22, 2013, 10:17 a.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Description
> ---
> 
> It is wrapper to access KColorScheme's methods from QML code.
> Also added Q_GADGET to KColorScheme to enable Q_ENUMS using, to make them 
> accessible from QML code.
> 
> As it will be accepted, QML-clone of KgPopupItem will be posted for review to 
> libkdegames, as it uses it to access KDE's color theme.
> 
> More info:
> * search for "KDE theme colors API for QML" thread at kdelibs and kdegames 
> mailinglists *
> 
> NEED TO FIX:
> I can't include it like #include  at KReversi's code, only 
> "kcolorschemetoken.h". Maybe I've missed something?
> 
> 
> Diffs
> -
> 
>   kdeui/CMakeLists.txt b439e04 
>   kdeui/colors/kcolorscheme.h 17570fd 
>   kdeui/colors/kcolorscheme.cpp a6650ac 
>   kdeui/colors/kcolorschemetoken.h PRE-CREATION 
>   kdeui/colors/kcolorschemetoken.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/112880/diff/
> 
> 
> Testing
> ---
> 
> I've tested it with KReversi's deniskup/gsoc2013/newdesign branch.
> 
> 
> Thanks,
> 
> Denis Kuplyakov
> 
>

Re: Review Request 112880: Added KColorSchemeToken class.

2013-09-22 Thread Denis Kuplyakov 


> On Sept. 22, 2013, 1:55 p.m., Kevin Krammer wrote:
> > kdeui/colors/kcolorschemetoken.h, line 55
> > 
> >
> > a property with a setter but no NOTIFYlooks wrong to me for a QML 
> > wrapper

All of the first seven properties are used only to set arguments to call 
apropriate KColorScheme method. It will be good to make them to be set once as 
KColorSchemeToken instance is created (like const in class). Is there a way to 
achive this?


- Denis


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112880/#review40458
---


On Sept. 22, 2013, 10:17 a.m., Denis Kuplyakov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112880/
> ---
> 
> (Updated Sept. 22, 2013, 10:17 a.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Description
> ---
> 
> It is wrapper to access KColorScheme's methods from QML code.
> Also added Q_GADGET to KColorScheme to enable Q_ENUMS using, to make them 
> accessible from QML code.
> 
> As it will be accepted, QML-clone of KgPopupItem will be posted for review to 
> libkdegames, as it uses it to access KDE's color theme.
> 
> More info:
> * search for "KDE theme colors API for QML" thread at kdelibs and kdegames 
> mailinglists *
> 
> NEED TO FIX:
> I can't include it like #include  at KReversi's code, only 
> "kcolorschemetoken.h". Maybe I've missed something?
> 
> 
> Diffs
> -
> 
>   kdeui/CMakeLists.txt b439e04 
>   kdeui/colors/kcolorscheme.h 17570fd 
>   kdeui/colors/kcolorscheme.cpp a6650ac 
>   kdeui/colors/kcolorschemetoken.h PRE-CREATION 
>   kdeui/colors/kcolorschemetoken.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/112880/diff/
> 
> 
> Testing
> ---
> 
> I've tested it with KReversi's deniskup/gsoc2013/newdesign branch.
> 
> 
> Thanks,
> 
> Denis Kuplyakov
> 
>

Re: Review Request 112880: Added KColorSchemeToken class.

2013-09-22 Thread Kevin Krammer 

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112880/#review40458
---



kdeui/colors/kcolorschemetoken.h


a property with a setter but no NOTIFYlooks wrong to me for a QML wrapper



kdeui/colors/kcolorschemetoken.h


not "get" for property getters in Qt or KDE, just the propert name.
see e.g. QLabel's "text" property



kdeui/colors/kcolorschemetoken.h


very uncommmon signal name for a QML wrapper.
the resulting QML signal handler would be "onOnBackgroundChange".
Change signals are usually the property name followed by "Changed"



kdeui/colors/kcolorschemetoken.h


Public class without d-pointer?



kdeui/colors/kcolorschemetoken.cpp


is it guaranteed that the background changes when a color set is set?
Couldn't the argument be the same as the current value or couldn't the 
background brush be the same?


- Kevin Krammer


On Sept. 22, 2013, 10:17 a.m., Denis Kuplyakov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112880/
> ---
> 
> (Updated Sept. 22, 2013, 10:17 a.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Description
> ---
> 
> It is wrapper to access KColorScheme's methods from QML code.
> Also added Q_GADGET to KColorScheme to enable Q_ENUMS using, to make them 
> accessible from QML code.
> 
> As it will be accepted, QML-clone of KgPopupItem will be posted for review to 
> libkdegames, as it uses it to access KDE's color theme.
> 
> More info:
> * search for "KDE theme colors API for QML" thread at kdelibs and kdegames 
> mailinglists *
> 
> NEED TO FIX:
> I can't include it like #include  at KReversi's code, only 
> "kcolorschemetoken.h". Maybe I've missed something?
> 
> 
> Diffs
> -
> 
>   kdeui/CMakeLists.txt b439e04 
>   kdeui/colors/kcolorscheme.h 17570fd 
>   kdeui/colors/kcolorscheme.cpp a6650ac 
>   kdeui/colors/kcolorschemetoken.h PRE-CREATION 
>   kdeui/colors/kcolorschemetoken.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/112880/diff/
> 
> 
> Testing
> ---
> 
> I've tested it with KReversi's deniskup/gsoc2013/newdesign branch.
> 
> 
> Thanks,
> 
> Denis Kuplyakov
> 
>

Re: Review Request 112869: Do not leak sockets in kdelibs

2013-09-22 Thread Lamarque Souza 

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

(Updated Sept. 22, 2013, 12:21 p.m.)


Review request for kdelibs and Solid.


Changes
---

Add missing include and update "Testing Done" field.


Description
---

NetworkInterface::isWireless() in 
kdelibs/solid/solid/backends/udev/udevnetworkinterface.cpp does not close the 
socket it opens to call an ioctl leading to problem to kded4 that can affect 
plasma-desktop as well (temporary freezing plasma-desktop as described in the 
bug entry).


This addresses bug 324954.
http://bugs.kde.org/show_bug.cgi?id=324954


Diffs (updated)
-

  solid/solid/backends/udev/udevnetworkinterface.cpp 06dc907 

Diff: http://git.reviewboard.kde.org/r/112869/diff/


Testing (updated)
---

Tested by the bug reporter and it fixes the problem for him.


Thanks,

Lamarque Souza

Review Request 112880: Added KColorSchemeToken class.

2013-09-22 Thread Denis Kuplyakov 

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

Review request for kdelibs.


Description
---

It is wrapper to access KColorScheme's methods from QML code.
Also added Q_GADGET to KColorScheme to enable Q_ENUMS using, to make them 
accessible from QML code.

As it will be accepted, QML-clone of KgPopupItem will be posted for review to 
libkdegames, as it uses it to access KDE's color theme.

More info:
* search for "KDE theme colors API for QML" thread at kdelibs and kdegames 
mailinglists *

NEED TO FIX:
I can't include it like #include  at KReversi's code, only 
"kcolorschemetoken.h". Maybe I've missed something?


Diffs
-

  kdeui/CMakeLists.txt b439e04 
  kdeui/colors/kcolorscheme.h 17570fd 
  kdeui/colors/kcolorscheme.cpp a6650ac 
  kdeui/colors/kcolorschemetoken.h PRE-CREATION 
  kdeui/colors/kcolorschemetoken.cpp PRE-CREATION 

Diff: http://git.reviewboard.kde.org/r/112880/diff/


Testing
---


Thanks,

Denis Kuplyakov

Re: Review Request 112880: Added KColorSchemeToken class.

2013-09-22 Thread Denis Kuplyakov 

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

(Updated Sept. 22, 2013, 10:17 a.m.)


Review request for kdelibs.


Changes
---

I don't know why but "Testing done" field was empty.


Description
---

It is wrapper to access KColorScheme's methods from QML code.
Also added Q_GADGET to KColorScheme to enable Q_ENUMS using, to make them 
accessible from QML code.

As it will be accepted, QML-clone of KgPopupItem will be posted for review to 
libkdegames, as it uses it to access KDE's color theme.

More info:
* search for "KDE theme colors API for QML" thread at kdelibs and kdegames 
mailinglists *

NEED TO FIX:
I can't include it like #include  at KReversi's code, only 
"kcolorschemetoken.h". Maybe I've missed something?


Diffs
-

  kdeui/CMakeLists.txt b439e04 
  kdeui/colors/kcolorscheme.h 17570fd 
  kdeui/colors/kcolorscheme.cpp a6650ac 
  kdeui/colors/kcolorschemetoken.h PRE-CREATION 
  kdeui/colors/kcolorschemetoken.cpp PRE-CREATION 

Diff: http://git.reviewboard.kde.org/r/112880/diff/


Testing (updated)
---

I've tested it with KReversi's deniskup/gsoc2013/newdesign branch.


Thanks,

Denis Kuplyakov

Re: Review Request 111093: PolKit Actions KCM: do not crash if policies could not be read

2013-09-22 Thread Jaime Torres Amate 


> On Sept. 21, 2013, 6:40 p.m., Jaime Torres Amate wrote:
> > I've tried to apply it to today master head and it does not apply clean. 
> > Some previous patch has done something similar to your patch. (sorry, not 
> > much time to check it all).
> 
> Jonathan Marten wrote:
> Is the definitive source base and master branch the one at 
> git://anongit.kde.org/polkit-kde-kcmodules-1?  Just tried updating to the 
> latest from there and the diff produced is identical.
> 
>

yes, the last commit is 6fe05fe98a3f33d72b66c4a55200e6c88e050168.
Ops, I'm sorry. I still had your previous patch for the bug applied (3 months 
without time for KDE things).

I've tried again, now applied right. And it works very nicely without crashes.
You have my ship it!.


- Jaime Torres


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111093/#review40425
---


On Sept. 18, 2013, 8:37 a.m., Jonathan Marten wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111093/
> ---
> 
> (Updated Sept. 18, 2013, 8:37 a.m.)
> 
> 
> Review request for kde-workspace and Polkit KDE Configuration.
> 
> 
> Description
> ---
> 
> The referenced bug describes a crash in the System Settings - System 
> Administration - Actions Policy module.  The first time that an action is 
> clicked on, an attempt is made to read the current system policy settings via 
> ActionWidget::reloadPKLAs().  Internally this checks the authorisation for 
> the org.kde.polkitkde1.readauthorizations action.  If this action is not 
> allowed, or the authorization fails, then a DBus error is returned - but 
> never checked.  There is then a crash (via qFatal) when an attempt is made to 
> read from the invalid returned QDBusArgument.
> 
> Unless the user made a mistake (e.g. typing the root password incorrectly), 
> this indicates a system configuration problem.  However, even if the fix 
> needs to be elsewhere, systemsettings should not just crash with no 
> indication of where the problem lies.
> 
> This change checks and reports the DBus error if one is returned.  Nothing 
> can be done within this module if this is the case, but at least there is a 
> diagnostic message.  The widget is left disabled, but will try the 
> authorization again if another action is selected.
> 
> 
> This addresses bug 300050.
> http://bugs.kde.org/show_bug.cgi?id=300050
> 
> 
> Diffs
> -
> 
>   polkitactions/ActionWidget.h ca83bf5 
>   polkitactions/ActionWidget.cpp c5785c0 
> 
> Diff: http://git.reviewboard.kde.org/r/111093/diff/
> 
> 
> Testing
> ---
> 
> With the default policy in place for org.kde.polkit1.readauthorizations 
> (active session = auth_admin, inactive session = no), run 'kcmshell4 
> kcm_polkitactions'.  Expand the tree and click on any action.  In the 
> password dialogue, either cancel or enter an incorrect password.  Check that 
> there is no crash and that the message box is displayed.
> 
> Click on another action, correctly enter the password and observe that there 
> is no message and policies are displayed as expected.
> 
> 
> Thanks,
> 
> Jonathan Marten
> 
>

Re: Review Request 111093: PolKit Actions KCM: do not crash if policies could not be read

2013-09-22 Thread Jonathan Marten 


> On Sept. 21, 2013, 6:40 p.m., Jaime Torres Amate wrote:
> > I've tried to apply it to today master head and it does not apply clean. 
> > Some previous patch has done something similar to your patch. (sorry, not 
> > much time to check it all).

Is the definitive source base and master branch the one at 
git://anongit.kde.org/polkit-kde-kcmodules-1?  Just tried updating to the 
latest from there and the diff produced is identical.


- Jonathan


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111093/#review40425
---


On Sept. 18, 2013, 8:37 a.m., Jonathan Marten wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111093/
> ---
> 
> (Updated Sept. 18, 2013, 8:37 a.m.)
> 
> 
> Review request for kde-workspace and Polkit KDE Configuration.
> 
> 
> Description
> ---
> 
> The referenced bug describes a crash in the System Settings - System 
> Administration - Actions Policy module.  The first time that an action is 
> clicked on, an attempt is made to read the current system policy settings via 
> ActionWidget::reloadPKLAs().  Internally this checks the authorisation for 
> the org.kde.polkitkde1.readauthorizations action.  If this action is not 
> allowed, or the authorization fails, then a DBus error is returned - but 
> never checked.  There is then a crash (via qFatal) when an attempt is made to 
> read from the invalid returned QDBusArgument.
> 
> Unless the user made a mistake (e.g. typing the root password incorrectly), 
> this indicates a system configuration problem.  However, even if the fix 
> needs to be elsewhere, systemsettings should not just crash with no 
> indication of where the problem lies.
> 
> This change checks and reports the DBus error if one is returned.  Nothing 
> can be done within this module if this is the case, but at least there is a 
> diagnostic message.  The widget is left disabled, but will try the 
> authorization again if another action is selected.
> 
> 
> This addresses bug 300050.
> http://bugs.kde.org/show_bug.cgi?id=300050
> 
> 
> Diffs
> -
> 
>   polkitactions/ActionWidget.h ca83bf5 
>   polkitactions/ActionWidget.cpp c5785c0 
> 
> Diff: http://git.reviewboard.kde.org/r/111093/diff/
> 
> 
> Testing
> ---
> 
> With the default policy in place for org.kde.polkit1.readauthorizations 
> (active session = auth_admin, inactive session = no), run 'kcmshell4 
> kcm_polkitactions'.  Expand the tree and click on any action.  In the 
> password dialogue, either cancel or enter an incorrect password.  Check that 
> there is no crash and that the message box is displayed.
> 
> Click on another action, correctly enter the password and observe that there 
> is no message and policies are displayed as expected.
> 
> 
> Thanks,
> 
> Jonathan Marten
> 
>