Code review comment for lp://staging/~mzanetti/unity/phablet-folding-launcher

> Icons are not getting highlighted when pressed. The only way I could get an
> icon highlighted was by long-pressing it.

fixed.

> =====================================================
>
> The branch alone works fine, but if I merge it with latest trunk it get the
> following wrong behavior:
> I have an application on foreground. Right from the beginning of a left-edge
> drag the application window slides rightwards instead of only after the
> launcher is fully revealed.

Hmm... Can't reproduce. After merging it still seems to be the same for me.

> ======================================================
>
> + MouseArea {
> + id: closeMouseArea
> + enabled: root.state == "visible"
> + anchors.fill: panel
> + anchors.rightMargin: -units.gu(2)
> + drag {
> + axis: Drag.XAxis
> + maximumX: 0
> + target: panel
> + }
> +
> + onReleased: {
> + if (panel.x < -panel.width/3) {
> + root.switchToNextState("")
> + } else {
> + root.switchToNextState("visible")
> }
> }
> - }
>
> - function hide() {
> - launcher.state = ""
> + }
> + MouseArea {
> + anchors {
> + left: closeMouseArea.right
> + top: parent.top
> + right: parent.right
> + bottom: parent.bottom
> + }
> + enabled: root.state == "visible"
> + onPressed: {
> + root.state = ""
> + }
> }
>
> I think second MouseArea should be the one named "closeMouseArea" (as it
> closes the launcher when clicked), not the first one.

Fixed.

> ============================================================
>
> + if (distance > root.width / 2){
>
> Coding style: spacing between ')' and '{'

Fixed.

> ============================================================
>
> - default: // Rejected
> + case Rejected:
> + Q_EMIT draggingChanged(false);
> + break;
> + default:
> // no-op
> break;
>
> Since the default case now deals specifically with invalid states, we could
> make it issue a qFatal().

How I read it, the default case is still valid for the Recognized state.

> ============================================================
>
> You can remove this line from Launcher.qml:
>
> import "../Applications/applications.js" as ApplicationsModel
>
> And this one from tst_Launcher.qml:
>
> import "../../../Applications/applications.js" as ApplicationsModel

fixed.

> ============================================================
>
> In Shell.qml:
>
> """
> + InputFilterArea {
> + blockInput: launcher.shown
> + anchors {
> + top: parent.top
> + bottom: parent.bottom
> + left: parent.left
> + }
> + width: launcher.shown ? launcher.width : launcher.panelWidth
> + }
>
> """
>
> Why not make width just:
>
> + width: launcher.width
>
> Afterall it only works when launcher.shown is true anyway.

Fixed.

> ============================================================
>
> tst_Launcher fails to run due to missing symbols, related to "import Unity
> 0.1".

Hmm... it passes on jenkins and after checking quite often I relatively sure the CMakeLists.txt is correct. Will keep on trying. Maybe Saviq has a hint?