Review Request: Improved Button.qml in PlasmaComponents

[Marco Martin]

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


i like the effect of having an animated press effect.
this comes with a cost memory wise but could be worth it.
ToolButton should be adapted to this as well, wouldn't bother with other elements for now, since those widgets will be instantiated so many times that the memory impact is actually present, we'll see for the future *if* some profiling gets done.

this can go in after a couple issues in the code have been solved:


plasma/declarativeimports/plasmacomponents/qml/Button.qml
<http://git.reviewboard.kde.org/r/103020/#comment6761>

    why this is removed? it still has to check for it to be enabled, it has to be ported to the new logic, not removed



plasma/declarativeimports/plasmacomponents/qml/Button.qml
<http://git.reviewboard.kde.org/r/103020/#comment6770>

    i don't see any porting of this check of checked state to the new two elements model



plasma/declarativeimports/plasmacomponents/qml/Button.qml
<http://git.reviewboard.kde.org/r/103020/#comment6767>

    width of the buttons should always be the same unless explicitly resized by the client scripts.
    reason: cost in terms of amount of pixmap data.
    
    also, functions to compute size should be as tiny as possible, so -1 on this change



plasma/declarativeimports/plasmacomponents/qml/Button.qml
<http://git.reviewboard.kde.org/r/103020/#comment6769>

    coding style:
    
    if (icon.valid) {
    
    }



plasma/declarativeimports/plasmacomponents/qml/Button.qml
<http://git.reviewboard.kde.org/r/103020/#comment6760>

    this syntax for anchors is quite hideous
    
    (yes, some formal code style guidelines like the c++ ones have to be written ;))



plasma/declarativeimports/plasmacomponents/qml/Button.qml
<http://git.reviewboard.kde.org/r/103020/#comment6768>

    anchoring top and bottom is faster in the runtime than binding the height property.
    
    the leftmargin binding is correct tough


- Marco Martin


On Nov. 1, 2011, 7:01 p.m., Mark Gaiser wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103020/
> -----------------------------------------------------------
> 
> (Updated Nov. 1, 2011, 7:01 p.m.)
> 
> 
> Review request for KDE Runtime, Plasma and Marco Martin.
> 
> 
> Description
> -------
> 
> I - quite heavily - modified the Button.qml to just look better. The list of changes:
> - Added myself to the copyright :p
> - Added a second leftMargin to the text if an icon was used. The icon and text where a little to close.
> - Added surfacePressed and renamed surface to surfaceNormal. This is done for a cross fade between 2 images. Works really nice!
> - Added a parallel animation for the cross fade. You just have to test this out! To do so, make a button and press/release it. You will see the fade effect but look carefully since it only lasts 100 milliseconds ;)
> - Removed some - now obsolete - javascript code
> - Fixed the Text{} to use the additional margin space when an icon is used
> 
> For the added animation. This is a crossfade and it looks really nice when you press/release a button.
> 
> 
> Diffs
> -----
> 
>   plasma/declarativeimports/plasmacomponents/qml/Button.qml d7b62d7 
> 
> Diff: http://git.reviewboard.kde.org/r/103020/diff/diff
> 
> 
> Testing
> -------
> 
> Made some buttons and played with them. It all seems to be working just fine.
> 
> 
> Thanks,
> 
> Mark Gaiser
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20111101/3e0a793e/attachment-0001.html>