Code review comment for lp:~unity-team/unity8/mirCompositor

> === added file 'qml/Stages/TabletStage.qml'
> + objectName: "stages"
> that a good name? Also PhoneStage has no objectName set at all, should we add
> one?

Well, there's tests for PhoneStage but not TabletStage yet. I'll add them when the split mode will be implemented.

>
> "overlay" = sidestage effectively, right? Just the term threw me for a bit.
> Rename isn't needed, but perhaps a comment saying so would be nice.

It is sidestage in overlay mode. Parts of the code are prepared for split mode already. I've added some more comments.

>
> "invalid" state is when no app is open, so spread should not be shown? That
> really an "invalid" state, or just empty?

fair enough. renamed.

>
>
> + property int phase
> You were kind enough to add a nice comment in PhoneStage explaining the
> different stages, could you do the same here please?

done

>
> + property bool locked: spreadView.phase == 2
> + property string focusedAppId: ApplicationManager.focusedApplicationId
> readonly? there's a bunch of properties in the Flickable that could also be
> readonly.

made some more readonly.

>
> + // Add 1 pixel to make sure we definitely hit positionMarker4 even with
> rounding errors of the animation.
> + snapAnimation.targetContentX = spreadView.width * spreadView.positionMarker4
> + 1;
> Math.ceil?

I really want it to step over the marker, not just reach it.

>
> "ApplicationInfoInterface.MainStage" <- bit of a mouthful, let's try to
> shorten that in future :)

Better than guessing what AII.MS might be, no? The "Interface" is a bit odd in deed" but well... That's how our architecture works.

>
> + function indexToZIndex(index) {
> a few more comments in here would help, I'm struggling to figure it out.

done

>
> + anchors.leftMargin: spreadView.width - spreadView.sideStageWidth +
> spreadView.sideStageWidth * sideStageDragHandle.progress
> can compact the maths a tiny bit

I tried for a bunch of hours to boot my N10. No chance, not even with the released images. Don't want to change that blindly without testing it... I'll try to do so when the N10 image comes back.

>
> + } else if (progress < spreadView.positionMarker1 + snappingCurve.period){
> nitpick: space before opening brace

done.