Merge lp:~dandrader/unity8/fixOrientedShellTests into lp:unity8

[Daniel d'Andrada]

Fix tst_OrientedShell and tst_Tutorial

+ Clear several warnings in Shell.qml
+ Move shellMode into applicationArguments context property

* 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?
Not applicable

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

* Did you have a look at the warnings when running tests? Can they be reduced?
Yes, solved some of them.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1823
http://jenkins.qa.ubuntu.com/job/unity8-ci/5839/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-wily-touch/133
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-amd64-ci/119
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-i386-ci/120
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-wily-mako/86
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-wily-armhf/133
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-wily-armhf/133/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21335

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

review: Needs Fixing (continuous-integration)

[lp:~dandrader/unity8/fixOrientedShellTests]

1830. By CI Train Bot Account on 2015-06-22

Resync trunk.

1831. By Launchpad Translations on behalf of unity-team on 2015-06-23

Launchpad automatic translations update.

[Albert Astals Cid (aacid)]

Text conflict in tests/qmltests/tst_Shell.qml
1 conflicts encountered.

[lp:~dandrader/unity8/fixOrientedShellTests]

1832. By Daniel d'Andrada on 2015-06-23

Fix tst_OrientedShell

+ Clear several warnings in Shell.qml
+ Move shellMode into applicationArguments context property

[Daniel d'Andrada (dandrader)]

> Text conflict in tests/qmltests/tst_Shell.qml
> 1 conflicts encountered.

Fixed.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1832
http://jenkins.qa.ubuntu.com/job/unity8-ci/5855/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-wily-touch/155
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-amd64-ci/135
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-i386-ci/136
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-wily-mako/105
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-wily-armhf/155
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-wily-armhf/155/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21397

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

review: Needs Fixing (continuous-integration)

[lp:~dandrader/unity8/fixOrientedShellTests]

1833. By Daniel d'Andrada on 2015-06-23

Fix tst_Tutorial.qml

[Albert Astals Cid (aacid)]

I do sincerely think the fix for tutorial test is worse than the one in http://bazaar.launchpad.net/~aacid/unity8/fix_test_tutorial/revision/1833 but a fix is better than none so let's have this one.

[Albert Astals Cid (aacid)]

review: Abstain

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1833
http://jenkins.qa.ubuntu.com/job/unity8-ci/5862/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-wily-touch/164/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-amd64-ci/142
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-i386-ci/143
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-wily-mako/112/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-wily-armhf/164
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-wily-armhf/164/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21407

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1833
http://jenkins.qa.ubuntu.com/job/unity8-ci/5871/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-wily-touch/176/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-amd64-ci/151
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-i386-ci/152
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-wily-mako/118/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-wily-armhf/176
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-wily-armhf/176/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21425

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

review: Needs Fixing (continuous-integration)

[Josh Arenson (josharenson)]

Comments inline.

review: Needs Information

[Daniel d'Andrada (dandrader)]

On 23/06/15 20:44, Josh Arenson wrote:
>> @@ -434,7 +432,7 @@
>> > Greeter {
>> >
>> > hides: [launcher, panel.indicators]
>> > - tabletMode: shell.sideStageEnabled
>> > + tabletMode: shell.usageScenario != "phone"
> Do you think naming this property 'phoneMode' would make things more clear? The only place 'tabletMode' is being used is in the greeter, so the amount of refactoring is small. I think the way it is currently is confusing.
>

I think it amounts to the same. Don't see it being any better.

[Josh Arenson (josharenson)]

lgtm.

review: Approve

[lp:~dandrader/unity8/fixOrientedShellTests]

1834. By Daniel d'Andrada on 2015-06-25

Also fix tst_SurfaceContainer

[Lukáš Tinkl (lukas-kde)]

One minor inline nitpick from me :)

review: Needs Fixing (code-review)

[Lukáš Tinkl (lukas-kde)]

See http://stackoverflow.com/questions/2582797/why-pass-by-const-reference-instead-of-by-value for the more elaborate overview of passing by value vs. passing by const ref

review: Needs Fixing (code-review)

[Lukáš Tinkl (lukas-kde)]

Also, https://techbase.kde.org/Development/Tutorials/Common_Programming_Mistakes#Passing_non-POD_types has some more info on the topic

[Daniel d'Andrada (dandrader)]

> See http://stackoverflow.com/questions/2582797/why-pass-by-const-reference-
> instead-of-by-value for the more elaborate overview of passing by value vs.
> passing by const ref

QString does have a move contructor:
http://pastebin.ubuntu.com/11795003/

What problem do you see in this case?

[Daniel d'Andrada (dandrader)]

> Also,
> https://techbase.kde.org/Development/Tutorials/Common_Programming_Mistakes
> #Passing_non-POD_types has some more info on the topic

This applies if you're not using C++11, where you don't have move constructors and move semantics.

[Lukáš Tinkl (lukas-kde)]

Ok, I stand corrected then, I didn't know Qt enables it for QStrings under C++11 :)

review: Approve

=== modified file 'qml/OrientedShell.qml'
--- qml/OrientedShell.qml	2015-06-12 16:07:43 +0000
+++ qml/OrientedShell.qml	2015-06-25 19:25:08 +0000
@@ -135,6 +135,7 @@
         nativeOrientation: root.nativeOrientation
         nativeWidth: root.width
         nativeHeight: root.height
+        mode: applicationArguments.mode
 
         // TODO: Factor in the connected input devices (eg: physical keyboard, mouse, touchscreen),
         //       what's the output device (eg: big TV, desktop monitor, phone display), etc.
 
=== modified file 'qml/Shell.qml'
--- qml/Shell.qml	2015-06-15 22:40:10 +0000
+++ qml/Shell.qml	2015-06-25 19:25:08 +0000
@@ -55,6 +55,7 @@
     property alias indicatorAreaShowProgress: panel.indicatorAreaShowProgress
     property bool beingResized
     property string usageScenario: "phone" // supported values: "phone", "tablet" or "desktop"
+    property string mode: "full-greeter"
     function updateFocusedAppOrientation() {
         applicationsDisplayLoader.item.updateFocusedAppOrientation();
     }
@@ -68,9 +69,9 @@
 
     readonly property bool orientationChangesEnabled: panel.indicators.fullyClosed
             && (applicationsDisplayLoader.item && applicationsDisplayLoader.item.orientationChangesEnabled)
-            && !greeter.animating
+            && (!greeter || !greeter.animating)
 
-    readonly property bool showingGreeter: greeter.shown
+    readonly property bool showingGreeter: greeter && greeter.shown
 
     property bool startingUp: true
     Timer { id: finishStartUpTimer; interval: 500; onTriggered: startingUp = false }
@@ -79,7 +80,7 @@
         if (startingUp) {
             // Ensure we don't rotate during start up
             return Qt.PrimaryOrientation;
-        } else if (greeter.shown) {
+        } else if (greeter && greeter.shown) {
             return Qt.PrimaryOrientation;
         } else if (mainApp) {
             return mainApp.supportedOrientations;
@@ -101,16 +102,13 @@
 
     // Disable everything while greeter is waiting, so that the user can't swipe
     // the greeter or launcher until we know whether the session is locked.
-    enabled: !greeter.waiting
+    enabled: greeter && !greeter.waiting
 
     property real edgeSize: units.gu(2)
     property url defaultBackground: Qt.resolvedUrl(shell.width >= units.gu(60) ? "graphics/tablet_background.jpg" : "graphics/phone_background.jpg")
     property url background: asImageTester.status == Image.Ready ? asImageTester.source
                              : gsImageTester.status == Image.Ready ? gsImageTester.source : defaultBackground
 
-    // This is _only_ used to expose the property to autopilot tests
-    readonly property string testShellMode: shellMode
-
     readonly property alias greeter: greeterLoader.item
 
     function activateApplication(appId) {
@@ -264,7 +262,7 @@
                                            ? "phone"
                                            : shell.usageScenario
             source: {
-                if(shellMode === "greeter") {
+                if(shell.mode === "greeter") {
                     return "Stages/ShimStage.qml"
                 } else if (applicationsDisplayLoader.usageScenario === "phone") {
                     return "Stages/PhoneStage.qml";
@@ -276,7 +274,7 @@
             }
 
             property bool interactive: tutorial.spreadEnabled
-                    && !greeter.shown
+                    && (!greeter || !greeter.shown)
                     && panel.indicators.fullyClosed
                     && launcher.progress == 0
                     && !notifications.useModal
@@ -307,12 +305,12 @@
             Binding {
                 target: applicationsDisplayLoader.item
                 property: "spreadEnabled"
-                value: tutorial.spreadEnabled && !greeter.hasLockedApp
+                value: tutorial.spreadEnabled && (!greeter || !greeter.hasLockedApp)
             }
             Binding {
                 target: applicationsDisplayLoader.item
                 property: "inverseProgress"
-                value: greeter.locked ? 0 : launcher.progress
+                value: greeter && greeter.locked ? 0 : launcher.progress
             }
             Binding {
                 target: applicationsDisplayLoader.item
@@ -422,7 +420,7 @@
         id: greeterLoader
         anchors.fill: parent
         anchors.topMargin: panel.panelHeight
-        sourceComponent: shellMode != "shell" ? integratedGreeter :
+        sourceComponent: shell.mode != "shell" ? integratedGreeter :
             Qt.createComponent(Qt.resolvedUrl("Greeter/ShimGreeter.qml"));
         onLoaded: {
                 item.objectName = "greeter"
@@ -434,7 +432,7 @@
         Greeter {
 
             hides: [launcher, panel.indicators]
-            tabletMode: shell.sideStageEnabled
+            tabletMode: shell.usageScenario != "phone"
             launcherOffset: launcher.progress
             forcedUnlock: tutorial.running
             background: shell.background
@@ -550,8 +548,8 @@
             indicators {
                 hides: [launcher]
                 available: tutorial.panelEnabled
-                        && (!greeter.locked || AccountsService.enableIndicatorsWhileLocked)
-                        && !greeter.hasLockedApp
+                        && ((!greeter || !greeter.locked) || AccountsService.enableIndicatorsWhileLocked)
+                        && (!greeter || !greeter.hasLockedApp)
                 contentEnabled: tutorial.panelContentEnabled
                 width: parent.width > units.gu(60) ? units.gu(40) : parent.width
 
 
=== modified file 'src/ApplicationArguments.h'
--- src/ApplicationArguments.h	2015-06-12 16:07:43 +0000
+++ src/ApplicationArguments.h	2015-06-25 19:25:08 +0000
@@ -27,14 +27,19 @@
 {
     Q_OBJECT
     Q_PROPERTY(QString deviceName READ deviceName CONSTANT)
+    Q_PROPERTY(QString mode READ mode CONSTANT)
 public:
     ApplicationArguments(QObject *parent = nullptr);
 
     void setDeviceName(QString deviceName) { m_deviceName = deviceName; }
     QString deviceName() const { return m_deviceName; }
 
+    void setMode(QString mode) { m_mode = mode; }
+    QString mode() const { return m_mode; }
+
 private:
     QString m_deviceName;
+    QString m_mode;
 };
 
 #endif // APPLICATION_ARGUMENTS_H
 
=== modified file 'src/main.cpp'
--- src/main.cpp	2015-06-12 16:07:43 +0000
+++ src/main.cpp	2015-06-25 19:25:08 +0000
@@ -60,6 +60,8 @@
         qmlArgs.setDeviceName(QString(buffer));
     }
 
+    qmlArgs.setMode(parser.mode());
+
     // The testability driver is only loaded by QApplication but not by QGuiApplication.
     // However, QApplication depends on QWidget which would add some unneeded overhead => Let's load the testability driver on our own.
     if (parser.hasTestability() || getenv("QT_LOAD_TESTABILITY")) {
@@ -92,7 +94,6 @@
 
     view->engine()->setBaseUrl(QUrl::fromLocalFile(::qmlDirectory()));
     view->rootContext()->setContextProperty("applicationArguments", &qmlArgs);
-    view->rootContext()->setContextProperty("shellMode", parser.mode());
     if (parser.hasFrameless()) {
         view->setFlags(Qt::FramelessWindowHint);
     }
 
=== modified file 'tests/autopilot/unity8/greeter/tests/test_args.py'
--- tests/autopilot/unity8/greeter/tests/test_args.py	2015-04-16 14:48:50 +0000
+++ tests/autopilot/unity8/greeter/tests/test_args.py	2015-06-25 19:25:08 +0000
@@ -28,27 +28,27 @@
     def test_full_greeter_mode(self):
         unity_proxy = self.launch_unity(mode='full-greeter')
         shell = self.get_shell(unity_proxy)
-        self.assertTrue(shell.testShellMode == 'full-greeter')
+        self.assertTrue(shell.mode == 'full-greeter')
 
     def test_full_shell_mode(self):
         unity_proxy = self.launch_unity(mode='full-shell')
         shell = self.get_shell(unity_proxy)
-        self.assertTrue(shell.testShellMode == 'full-shell')
+        self.assertTrue(shell.mode == 'full-shell')
 
     def test_greeter_mode(self):
         unity_proxy = self.launch_unity(mode='greeter')
         shell = self.get_shell(unity_proxy)
-        self.assertTrue(shell.testShellMode == 'greeter')
+        self.assertTrue(shell.mode == 'greeter')
 
     def test_nonexistent_mode(self):
         unity_proxy = self.launch_unity(mode=self.NONEXISTENT_MODE)
         shell = self.get_shell(unity_proxy)
-        self.assertTrue(shell.testShellMode == self.DEFAULT_SHELL_MODE,
+        self.assertTrue(shell.mode == self.DEFAULT_SHELL_MODE,
                         "Shell mode was {} but should have been {}"
-                        .format(shell.testShellMode,
+                        .format(shell.mode,
                                 self.DEFAULT_SHELL_MODE))
 
     def test_shell_mode(self):
         unity_proxy = self.launch_unity(mode='shell')
         shell = self.get_shell(unity_proxy)
-        self.assertTrue(shell.testShellMode == 'shell')
+        self.assertTrue(shell.mode == 'shell')
 
=== modified file 'tests/qmltests/Stages/tst_SurfaceContainer.qml'
--- tests/qmltests/Stages/tst_SurfaceContainer.qml	2015-03-12 14:45:44 +0000
+++ tests/qmltests/Stages/tst_SurfaceContainer.qml	2015-06-25 19:25:08 +0000
@@ -32,7 +32,6 @@
         id: surfaceContainerComponent
         SurfaceContainer {
             anchors.fill: parent
-            orientation: Qt.PortraitOrientation
             interactive: interactiveCheckbox.checked
         }
     }
 
=== modified file 'tests/qmltests/Tutorial/tst_Tutorial.qml'
--- tests/qmltests/Tutorial/tst_Tutorial.qml	2015-06-15 22:40:10 +0000
+++ tests/qmltests/Tutorial/tst_Tutorial.qml	2015-06-25 19:25:08 +0000
@@ -17,7 +17,6 @@
 import QtQuick 2.0
 import QtTest 1.0
 import AccountsService 0.1
-import GSettings 1.0
 import LightDM 0.1 as LightDM
 import Ubuntu.Components 1.1
 import Unity.Application 0.1
@@ -46,11 +45,6 @@
         }
     }
 
-    GSettings {
-        id: unity8Settings
-        schema.id: "com.canonical.Unity8"
-    }
-
     Component.onCompleted: {
         // must set the mock mode before loading the Shell
         LightDM.Greeter.mockMode = "single-pin";
@@ -73,7 +67,6 @@
             sourceComponent: Component {
                 Shell {
                     property string indicatorProfile: "phone"
-                    property string shellMode: "full-greeter" /* default */
 
                     Component.onDestruction: {
                         shellLoader.itemDestroyed = true;
@@ -119,7 +112,7 @@
 
         function init() {
             tryCompare(shell, "enabled", true); // enabled by greeter when ready
-            unity8Settings.usageMode = "Staged";
+
             AccountsService.demoEdges = false;
             AccountsService.demoEdges = true;
             swipeAwayGreeter();
@@ -193,7 +186,7 @@
         }
 
         function checkRightEdge() {
-            if (unity8Settings.usageMode === "Staged") {
+            if (shell.usageScenario === "phone") {
                 touchFlick(shell, shell.width, halfHeight, halfWidth, halfHeight);
 
                 var stage = findChild(shell, "stage");
@@ -271,7 +264,7 @@
         }
 
         function test_walkthroughOnDesktop() {
-            unity8Settings.usageMode = "Windowed";
+            shell.usageScenario = "desktop";
             var page = goToPage("tutorialLeftFinish");
             var tick = findChild(page, "tick");
             tap(tick);
 
=== modified file 'tests/qmltests/tst_OrientedShell.qml'
--- tests/qmltests/tst_OrientedShell.qml	2015-05-11 14:36:03 +0000
+++ tests/qmltests/tst_OrientedShell.qml	2015-06-25 19:25:08 +0000
@@ -34,19 +34,8 @@
 
     QtObject {
         id: applicationArguments
-
-        function hasGeometry() {
-            return false;
-        }
-
-        function width() {
-            return 0;
-        }
-
-        function height() {
-            return 0;
-        }
         property string deviceName: "mako"
+        property string mode: "full-greeter"
     }
 
     QtObject {
 
=== modified file 'tests/qmltests/tst_Shell.qml'
--- tests/qmltests/tst_Shell.qml	2015-06-18 18:17:09 +0000
+++ tests/qmltests/tst_Shell.qml	2015-06-25 19:25:08 +0000
@@ -81,8 +81,6 @@
             property bool itemDestroyed: false
             sourceComponent: Component {
                 Shell {
-                    property string shellMode: "full-greeter" /* default */
-
                     usageScenario: usageScenarioSelector.model[usageScenarioSelector.selectedIndex]
                     orientation: Qt.PortraitOrientation
                     primaryOrientation: Qt.PortraitOrientation
 
=== modified file 'tests/qmltests/tst_ShellWithPin.qml'
--- tests/qmltests/tst_ShellWithPin.qml	2015-06-15 22:40:10 +0000
+++ tests/qmltests/tst_ShellWithPin.qml	2015-06-25 19:25:08 +0000
@@ -69,8 +69,6 @@
             property bool itemDestroyed: false
             sourceComponent: Component {
                 Shell {
-                    property string shellMode: "full-greeter" /* default */
-
                     Component.onDestruction: {
                         shellLoader.itemDestroyed = true
                     }