Marco Martin | 1 Nov 2011 20:20

Re: Review Request: Improved Button.qml in PlasmaComponents

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

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 (Diff revision 1) 37


function pressButton() {

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 (Diff revision 1) 50


if (button.checked)

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

plasma/declarativeimports/plasmacomponents/qml/Button.qml (Diff revision 1) 59 50


width: Math.max(50, icon.width + label.paintedWidth + surface.margins.left + surface.margins.right)	width: {

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 (Diff revision 1) 54


	{

coding style: if (icon.valid) { }

plasma/declarativeimports/plasmacomponents/qml/Button.qml (Diff revision 1) 118 132


anchors {	anchors.right: parent.right

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 (Diff revision 1) 121 135


left: icon.valid ? icon.right : parent.left	height: parent.height

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

- Marco

On November 1st, 2011, 7:01 p.m., Mark Gaiser wrote:

Review request for KDE Runtime, Plasma and Marco Martin.

By Mark Gaiser.

Updated Nov. 1, 2011, 7:01 p.m.

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.

Testing

Made some buttons and played with them. It all seems to be working just fine.

Diffs

plasma/declarativeimports/plasmacomponents/qml/Button.qml (d7b62d7)

View Diff

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