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

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

Hmm... Not sure... I also use itemOpacity when it is unfolded. Would require to be very careful with distinguishing opacity and itemOpacity (which intentionally are 2 different things). The gain would be very minimal I think.

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

Sure. But somewhat out of the scope of this merge.

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

renamed LauncherListDelegate to FoldingLauncherDelegate.

>
> =====
>
> 604 + Item {
>
> Do we really need this?

Yes. This does the clipping when unfolded. I need to have the MouseArea to be a child of the ListView otherwise weird mouse event stealing things will happen. Also, the dragTarget (the dragFakeItem) should be in a parent of the same size as the MouseArea or it indtroduces long calculations and nasty jumping at transitions. I tried that before.

>
> =====
>
> 658 + layoutDirection: root.inverted ? Qt.RightToLeft :
> Qt.LeftToRight
>
> Hum?

I got fed up by the rotating thing and tried if we couldn't abuse the layoutDirection for the inversion. Doesn't work unfortunately. Removed this leftover.

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

fixed.

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

Very good point. I've limited the drag area and haven't been able to reproduce this any more.

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

Done.

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

The behavior you describe is pretty much what I want. When leaving one of those special states, just go back to whatever it was before.

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

Moved it over to the states. Yes, we should never use Easing on opacity changes.

>
> =====
>
> 729 + }
> 730 +
> 731 + ]
>
> Drop newline.

done.

>
> =====
>
> 767 + anchors.fill: parent
> 768 + anchors.topMargin: launcherListView.topMargin
> 769 + anchors.bottomMargin:
> launcherListView.bottomMargin
>
> Try to remember and group those.

done.

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

changed them to be properties of the ListView.

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

fixed.

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

Yes, I always set inverted to false while developing and only set it back to true before changing the merge request to "Needs Review". Makes development less brainfucking and tests both directions :)

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

It works. I just removed the UI because the Popover is really nowhere near ready for this. But functionality works ok already. I have a quicklist here that lets me pin items and remove items.

If you don't mind I'd prefer to leave this code as it's a bit wired up with the automatic pinning when starting a drag. I just removed all the quicklist UI code for this review.