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

_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Reply via email to