Review Request: Added iconName property to KPushButton

[Michael Leupold]

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


I generally like the idea and there's also a related bugreport on bugs: https://bugs.kde.org/show_bug.cgi?id=201793

I tried to work on it at some point but stumbled upon a behaviour of designer I did not expect:
While typing the icon name, designer will set the property for each letter you type. This will lead to the widget permanently trying to find an appropriate icon. When I tried it this lead to a little slowdown on my machine. I guess that might not matter that much (but I'd prefer if someone with a slower rig could try as well).

One other issue is that reading the property is impossible if the icon is set programmatically (iconName() will just return an empty string). For that reason I think you should properly document the methods and introduce a warning that says something along the lines of:
"This is only there for designer integration. Do not rely on those methods and especially don't expect iconName() to return anything along the lines of the actual name of the current icon."

... that is unless you find a way to get the icon's name in all cases :-)


/trunk/KDE/kdelibs/kdeui/widgets/kpushbutton.h
<http://reviewboard.kde.org/r/1484/#comment1500>

    Calling this setIconName would be better for readability. Add @since 4.4.



/trunk/KDE/kdelibs/kdeui/widgets/kpushbutton.h
<http://reviewboard.kde.org/r/1484/#comment1501>

    This should be const. Also add @since 4.4.



/trunk/KDE/kdelibs/kdeui/widgets/kpushbutton.cpp
<http://reviewboard.kde.org/r/1484/#comment1502>

    Check coding style in this method, eg. there should be a space in front of this line's {.


- Michael


On 2009-08-30 23:02:45, Davide Bettio wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1484/
> -----------------------------------------------------------
> 
> (Updated 2009-08-30 23:02:45)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> This patch allows to set KPushButton icons using designer.
> It also allows to set an icon using a string. 
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdelibs/kdeui/widgets/kpushbutton.h 1017571 
>   /trunk/KDE/kdelibs/kdeui/widgets/kpushbutton.cpp 1016374 
> 
> Diff: http://reviewboard.kde.org/r/1484/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Davide
> 
>