Code review comment for lp:~mzanetti/unity8/dnd-and-quicklists

Revision history for this message
Michał Sawicz (saviq) wrote :

I wonder if it'd make sense to "enable: angle != 0" the ShaderEffect?

=====

Ultimately I'd like to see a shader-based replacement for the three rotations...

=====

Ideas of better names for LauncherDelegate and LauncherListDelegate? Like LauncherItem and LauncherDelegate, maybe?

=====

 604 + Item {

Do we really need this?

=====

 658 + layoutDirection: root.inverted ? Qt.RightToLeft : Qt.LeftToRight

Hum?

=====

 662 + // destructed. Note that if the move() operation when dragging switches

destructed → destroyed

=====

Can we protect ourselves somehow from the "breaks badly"? Like limit the dragging distance?

=====

 674 + NumberAnimation { properties: "x,y"; duration: UbuntuAnimation.FastDuration; easing: UbuntuAnimation.StandardEasing }

 743 + NumberAnimation { properties: "angle,offset"; duration: UbuntuAnimation.FastDuration; easing: UbuntuAnimation.StandardEasing }

 749 + NumberAnimation { properties: "angle,offset"; duration: UbuntuAnimation.BriskDuration; easing: UbuntuAnimation.StandardEasing }

 754 + NumberAnimation { properties: "height"; duration: UbuntuAnimation.FastDuration; easing: UbuntuAnimation.StandardEasing }

 760 + NumberAnimation { properties: "height"; duration: UbuntuAnimation.BriskDuration; easing: UbuntuAnimation.StandardEasing }

Use UbuntuNumberAnimation {} here.

=====

You need a State { name: "" } with the default values, otherwise the *current* values (e.g. in the middle of a transition) will be assumed as the default ones.

=====
 695 + opacity: parent.dragging ? 1 : 0
 696 + Behavior on opacity {
 697 + NumberAnimation { duration: UbuntuAnimation.FastDuration }
 698 + }

Since there are states already, maybe move that there? UbuntuNumberAnimation could be used here, too, although I think someone said we shouldn't use the easing for opacity?

=====

 729 + }
 730 +
 731 + ]

Drop newline.

=====

 767 + anchors.fill: parent
 768 + anchors.topMargin: launcherListView.topMargin
 769 + anchors.bottomMargin: launcherListView.bottomMargin

Try to remember and group those.

=====

 778 + var realContentY = launcherListView.contentY + launcherListView.topMargin

 785 + var realContentY = launcherListView.contentY + launcherListView.topMargin

 873 + var realContentY = launcherListView.contentY + launcherListView.topMargin

Should take originY into account.

Maybe make it into a function to reuse? Same for realItemHeight?

=====

 790 + if (index == 0 || index == launcherListView.count -1) {

Space before -.

=====

Will dragging work fine with the launcher (not) inverted?

=====

There's quicklist-related stuff in launcheritem, care to move it to when it will actually work?

=====

Working good!

[1] http://developer.ubuntu.com/api/ubuntu-12.10/qml/mobile/qml-ubuntu-components0-ubuntunumberanimation.html

review: Needs Fixing

« Back to merge proposal