Code review comment for lp:~mzanetti/unity8/right-edge-2

> Questions: there are 2 appSplash instances? Is that if user starts 2 apps
> quickly?

Yeah, it mostly catches the case that the screenshot is not available for some reason, for example the one you mentioned.

>
>
> + property bool attachedToView: true
> a comment explaining this would be nice. In fact, the logic in the
> EdgeDragArea could do with the same.

very valid. added.

>
> + if (oneWayFlick && spreadView.shiftedContentX > units.gu(2) &&
> spreadView.shiftedContentX < spreadView.positionMarker1 * spreadView.width) {
> please wrap that line a bit

wrapped all to 120 chars now.

>
> + property real positionMarker3: 0.6
> + property real positionMarker4: .9
> Consistency please

fixed.

>
> + // Stage of the animation:
> + property int stage: not a fan of the term, as "Stage" has a particular
> meaning for us. Would the Flickable's "States" system be useful here, as
> instead of numbers, can give the animation stages names?

Don't really want to go for strings as I do <, > comparison etc. While that would still work with strings its imo much more fail prone if someone renames or adds a stage. I did, however rename it to "phase" now to avoid the naming collision.

>
> + onShiftedContentXChanged: {
> + case 1:
> /me pedant, but adding "break" here too might spare a future coder some pain

fair enough, fixed.

>
> +// duration: UbuntuAnimation.SleepyDuration
> remove?

done.

>
> + spreadView.stage = 4;
> leftover?

seems so... dropped it.
>
> + if (spreadView.shiftedContentX == spreadView.width *
> spreadView.positionMarker2) {
> what's going on here? floating point equality comparison makes me suspicious

Turns out its not needed any more at all as I later changed the logic to always hit the branch before in this case already.

>
> + id: spreadRow
> + width: Math.max(3, ApplicationManager.count) * spreadView.tileDistance +
> (spreadView.width - spreadView.tileDistance) * 1.5
> fancy maths could do with a comment explaining it :)

done.

> +++ qml/Stages/SpreadDelegate.qml
> + anchors { left: parent.left; bottom: parent.bottom; top: parent.top;
> topMargin: priv.heightDifference * Math.max(0, 1 - root.topMarginProgress) }
> please wrap so topMargin is on its own line, as that's the nontrivial bit.

done.
>
> + scale: 1
> unnecessary, that's the default value.

done