Merge lp:~mterry/unity8/unlock-sim-after-wizard into lp:unity8

[Michael Terry]

Only call unlockAllModems once the wizard is done. (LP: #1425161)

Only call unlockAllModems once the wizard is done. (LP: #1425161)

This is only a problem in vivid because in RTM, the shell isn't running when the wizard is.

I never noticed this when testing because I'm a silly American.

== Checklist ==

 * Are there any related MPs required for this MP to build/function as expected? Please list.
 No

 * Did you perform an exploratory manual test run of your code change and any related functionality?
 Yes

 * Did you make sure that your branch does not contain spurious tags?
 Yes

 * If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
 NA

 * If you changed the UI, has there been a design review?
 NA

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1636
http://jenkins.qa.ubuntu.com/job/unity8-ci/5362/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/1540
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-vivid/526
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-amd64-ci/527
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-i386-ci/527
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/1370
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/1538
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/1538/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/18358

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/5362/rebuild

review: Needs Fixing (continuous-integration)

[Daniel d'Andrada (dandrader)]

Would you mind rebasing it on top of lp:~dandrader/unity8/unifyShellTests?

With unifyShellTests, tests functions have to load the shell explicitly using the loadShell(form-factor) helper function, which will be useful for your new test.

Besides, the two conflict anyway, so either of the MPs would have to do that in any case.

[Michael Terry (mterry)]

OK, rebased on unifyShellTests.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1637
http://jenkins.qa.ubuntu.com/job/unity8-ci/5384/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/1627
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-vivid/548
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-amd64-ci/549
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-i386-ci/549
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/1442
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/1625
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/1625/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/18510

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/5384/rebuild

review: Needs Fixing (continuous-integration)

[Daniel d'Andrada (dandrader)]

"""
62 - readonly property alias active: loader.active
63 + readonly property bool active: loader.active
"""

Why is that needed?

review: Needs Information

[Michael Terry (mterry)]

> Why is that needed?

Because the loader's active property was emitting a changed signal during object creation as an alias. Having it cached as a property made it only emit a changed signal when it actually did change.

I figured it was some quirk of qml that I didn't understand. But it seemed like a harmless enough change.

[lp:~mterry/unity8/unlock-sim-after-wizard]

1638. By Michael Terry on 2015-03-10

Merge from unifyShellTests

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1638
http://jenkins.qa.ubuntu.com/job/unity8-ci/5407/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/1705
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-vivid/571
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-amd64-ci/572
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-i386-ci/572
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/1512
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/1703
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/1703/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/18711

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/5407/rebuild

review: Needs Fixing (continuous-integration)

[Daniel d'Andrada (dandrader)]

> > Why is that needed?
>
> Because the loader's active property was emitting a changed signal during
> object creation as an alias. Having it cached as a property made it only emit
> a changed signal when it actually did change.
>
> I figured it was some quirk of qml that I didn't understand. But it seemed
> like a harmless enough change.

That's really odd. Certainly deserves a comment in the code. Specially if your code will misbehave in case it's changed to an alias back again.

[lp:~mterry/unity8/unlock-sim-after-wizard]

1639. By Michael Terry on 2015-03-11

Add some comments on why we cache the 'active' bool instead of using an alias

[Michael Terry (mterry)]

Added comment to explain

[Daniel d'Andrada (dandrader)]

Code looks ok, but I can't test if it fixes the bug as I don't have a spare SIM card (particularly one that has a PIN code) to try it.

review: Approve (code)

[Michał Sawicz (saviq)]

review: Approve (functional)

=== modified file 'qml/Greeter/Greeter.qml'
--- qml/Greeter/Greeter.qml	2015-02-23 15:43:41 +0000
+++ qml/Greeter/Greeter.qml	2015-03-11 14:48:55 +0000
@@ -19,7 +19,6 @@
 import LightDM 0.1 as LightDM
 import Ubuntu.Components 1.1
 import Ubuntu.SystemImage 0.1
-import Unity.Connectivity 0.1
 import Unity.Launcher 0.1
 import "../Components"
 
@@ -179,10 +178,6 @@
         }
     }
 
-    Component.onCompleted: {
-        Connectivity.unlockAllModems();
-    }
-
     Timer {
         id: forcedDelayTimer
         interval: 1000 * 60

=== modified file 'qml/Shell.qml'
--- qml/Shell.qml	2015-03-02 12:10:22 +0000
+++ qml/Shell.qml	2015-03-11 14:48:55 +0000
@@ -23,6 +23,7 @@
 import Ubuntu.Components.Popups 1.0
 import Ubuntu.Gestures 0.1
 import Ubuntu.Telephony 0.1 as Telephony
+import Unity.Connectivity 0.1
 import Unity.Launcher 0.1
 import Utils 0.1
 import LightDM 0.1 as LightDM
@@ -508,8 +509,18 @@
 
         Wizard {
             id: wizard
+            objectName: "wizard"
             anchors.fill: parent
             background: shell.background
+
+            function unlockWhenDoneWithWizard() {
+                if (!active) {
+                    Connectivity.unlockAllModems();
+                }
+            }
+
+            Component.onCompleted: unlockWhenDoneWithWizard()
+            onActiveChanged: unlockWhenDoneWithWizard()
         }
 
         Rectangle {

=== modified file 'qml/Wizard/Wizard.qml'
--- qml/Wizard/Wizard.qml	2014-11-24 17:56:44 +0000
+++ qml/Wizard/Wizard.qml	2015-03-11 14:48:55 +0000
@@ -25,7 +25,12 @@
     // The background wallpaper to use
     property string background
 
-    readonly property alias active: loader.active
+    // This is a bool instead of an alias because Loader classes like to emit
+    // changed signals for 'active' during startup even if they aren't actually
+    // changing values. Having it cached as a proper Qml bool property prevents
+    // unnecessary 'changed' emissions and provides consuming classes the
+    // expected behavior of no emission on startup.
+    readonly property bool active: loader.active
 
     hideAnimation: StandardAnimation { property: "opacity"; to: 0 }
 

=== modified file 'tests/qmltests/tst_Shell.qml'
--- tests/qmltests/tst_Shell.qml	2015-03-11 14:48:55 +0000
+++ tests/qmltests/tst_Shell.qml	2015-03-11 14:48:55 +0000
@@ -31,6 +31,7 @@
 import Unity.Notifications 1.0
 import Unity.Test 0.1 as UT
 import Powerd 0.1
+import Wizard 0.1 as Wizard
 
 import "../../qml"
 
@@ -288,6 +289,7 @@
             setLightDMMockMode("single"); // back to the default value
 
             AccountsService.demoEdges = false;
+            Wizard.System.wizardEnabled = false;
 
             // kill all (fake) running apps
             killApps(ApplicationManager);
@@ -940,6 +942,21 @@
             tryCompare(unlockAllModemsSpy, "count", 1)
         }
 
+        function test_unlockAllModemsAfterWizard() {
+            Wizard.System.wizardEnabled = true;
+            loadShell("phone");
+
+            var wizard = findChild(shell, "wizard");
+            compare(wizard.active, true);
+            compare(Wizard.System.wizardEnabled, true);
+            compare(unlockAllModemsSpy.count, 0);
+
+            wizard.hide();
+            tryCompare(wizard, "active", false);
+            compare(Wizard.System.wizardEnabled, false);
+            compare(unlockAllModemsSpy.count, 1);
+        }
+
         function test_tapOnRightEdgeReachesApplicationSurface() {
             loadShell("phone");
             swipeAwayGreeter();