Merge lp:~dpniel/reminders-app/improve_binary into lp:reminders-app

[Dan Chapman ]

removed -q option from src/app/main.cpp and autopilot/reminders/tests/__init__.py

Now looks for reminders.qml in relation to the path of the application executable

Improvements to app/main.cpp,

  * For command line options switched to using QCommandLineParser
  * Fixed build warnings when setting phone/tablet context properties, (Required QVariant(true/false) and not bool true/false)
  * Removed -q option and instead search for reminders.qml using the path to the application binary and not need to be passing around relative file paths
    which could be a security issue and dumb things could happen when called from inside an attacker-controlled directory.

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

FAILED: Continuous integration, rev:94
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~dpniel/reminders-app/improve_binary/+merge/213621/+edit-commit-message

http://91.189.93.70:8080/job/reminders-app-ci/242/
Executed test runs:
    UNSTABLE: http://91.189.93.70:8080/job/generic-mediumtests-trusty/1930
    FAILURE: http://91.189.93.70:8080/job/reminders-app-saucy-amd64-ci/242/console
    SUCCESS: http://91.189.93.70:8080/job/reminders-app-trusty-amd64-ci/242

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/reminders-app-ci/242/rebuild

review: Needs Fixing (continuous-integration)

[lp:~dpniel/reminders-app/improve_binary]

95. By Dan Chapman  on 2014-04-01

removed -q option in autopilot test

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

FAILED: Continuous integration, rev:95
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~dpniel/reminders-app/improve_binary/+merge/213621/+edit-commit-message

http://91.189.93.70:8080/job/reminders-app-ci/243/
Executed test runs:
    UNSTABLE: http://91.189.93.70:8080/job/generic-mediumtests-trusty/1931
    FAILURE: http://91.189.93.70:8080/job/reminders-app-saucy-amd64-ci/243/console
    SUCCESS: http://91.189.93.70:8080/job/reminders-app-trusty-amd64-ci/243

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/reminders-app-ci/243/rebuild

review: Needs Fixing (continuous-integration)

[Michael Zanetti (mzanetti)]

> which could be a security issue and dumb things could happen when called from inside an attacker-controlled directory.

I don't really agree with this one... If an attacker can create and inject a qml file, he just as well might call qmlscene with that qml file, or place the "bad" qml file relative to the app binary to inject it in your bianary... I don't see any more harm being done by neither the original approach nor this one. Anyways... dropping the -q parameter is fine with me.

===

please clean up the -q option in the autopilot scripts then (If its already in there).

==

please change "DO NOT USE: autopilot sets this automatically" to "Enable loading of testability driver". Note that this would also load any other's platform testability driver and is not related to autopilot in any way (except that on ubuntu we inject our self-made testability driver coming with autopilot).

review: Needs Fixing

[lp:~dpniel/reminders-app/improve_binary]

96. By Dan Chapman  on 2014-04-01

revert changes

[Dan Chapman  (dpniel)]

Ok, i have reverted the changes on this anyway as QCommandLineParser is new in Qt5.2 I didn't realise that builds were still done against saucy as well.

I'll change looking up the qmlfile and not pass the relative path with autopilot to clean that up a bit

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

FAILED: Continuous integration, rev:96
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~dpniel/reminders-app/improve_binary/+merge/213621/+edit-commit-message

http://91.189.93.70:8080/job/reminders-app-ci/244/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-trusty/1932
    SUCCESS: http://91.189.93.70:8080/job/reminders-app-saucy-amd64-ci/244
    SUCCESS: http://91.189.93.70:8080/job/reminders-app-trusty-amd64-ci/244

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/reminders-app-ci/244/rebuild

review: Needs Fixing (continuous-integration)

[Michael Zanetti (mzanetti)]

> Ok, i have reverted the changes on this anyway as QCommandLineParser is new in
> Qt5.2 I didn't realise that builds were still done against saucy as well.
>
> I'll change looking up the qmlfile and not pass the relative path with
> autopilot to clean that up a bit

Too bad. I would have loved to see QCommandLineParser coming in. Also, in another branch of mine I'm using the new QMessageLogger already. Do you know how long we have to keep compatibility with saucy?

[lp:~dpniel/reminders-app/improve_binary]

97. By Dan Chapman  on 2014-04-01

removed -q option from src/app/main.cpp and autopilot/reminders/tests/__init__.py

Now looks for reminders.qml in relation to the path of the application executable

[Dan Chapman  (dpniel)]

> Too bad. I would have loved to see QCommandLineParser coming in. Also, in
> another branch of mine I'm using the new QMessageLogger already. Do you know
> how long we have to keep compatibility with saucy?

I don't atm :-(, i'd presume it would be after this release, but i'm going to find out to make sure. Once it's changed over i'll re-propose using QCommandLineParser

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

FAILED: Continuous integration, rev:97
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~dpniel/reminders-app/improve_binary/+merge/213621/+edit-commit-message

http://91.189.93.70:8080/job/reminders-app-ci/245/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-trusty/1933
    SUCCESS: http://91.189.93.70:8080/job/reminders-app-saucy-amd64-ci/245
    SUCCESS: http://91.189.93.70:8080/job/reminders-app-trusty-amd64-ci/245

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/reminders-app-ci/245/rebuild

review: Needs Fixing (continuous-integration)

[Dan Chapman  (dpniel)]

so those changes seem to pass, i've updated the commit message so that should pass next time around.

[Michael Zanetti (mzanetti)]

lgtm.

review: Approve

[David Planella (dpm)]

If it makes sense to use them, we can set a separate series for 13.10 and
switch to 5.2 features for trunk already. In fact, I'd suggest that and do
the switch already in this branch.

In terms of supporting 13.10, it's still about 4 months to go (non-LTS
releases are supported for 9 months)

Cheers,
David.

On Tue, Apr 1, 2014 at 1:06 PM, Dan Chapman <email address hidden> wrote:

> The proposal to merge lp:~dpniel/reminders-app/improve_binary into
> lp:reminders-app has been updated.
>
> Commit Message changed to:
>
> removed -q option from src/app/main.cpp and
> autopilot/reminders/tests/__init__.py
>
> Now looks for reminders.qml in relation to the path of the application
> executable
>
> For more details, see:
>
> https://code.launchpad.net/~dpniel/reminders-app/improve_binary/+merge/213621
> --
>
> https://code.launchpad.net/~dpniel/reminders-app/improve_binary/+merge/213621
> Your team Ubuntu Reminders app developers is subscribed to branch
> lp:reminders-app.
>

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

review: Approve (continuous-integration)

=== modified file 'src/app/main.cpp'
--- src/app/main.cpp	2014-03-17 22:26:42 +0000
+++ src/app/main.cpp	2014-04-01 10:40:25 +0000
@@ -47,11 +47,10 @@
         qDebug() << "    -t|--tablet   If running on Desktop, start in a tablet sized window.";
         qDebug() << "    -h|--help     Print this help.";
         qDebug() << "    -I <path>     Give a path for an additional QML import directory. May be used multiple times.";
-        qDebug() << "    -q <qmlfile>  Give an alternative location for the main qml file.";
         return 0;
     }
 
-    QString qmlfile;
+
     for (int i = 0; i < args.count(); i++) {
         if (args.at(i) == "-I" && args.count() > i + 1) {
             QString addedPath = args.at(i+1);
@@ -60,8 +59,6 @@
                 addedPath.prepend(QDir::currentPath());
             }
             importPathList.append(addedPath);
-        } else if (args.at(i) == "-q" && args.count() > i + 1) {
-            qmlfile = args.at(i+1);
         }
     }
 
@@ -80,17 +77,17 @@
         }
     }
 
-    view.engine()->rootContext()->setContextProperty("tablet", false);
-    view.engine()->rootContext()->setContextProperty("phone", false);
+    view.engine()->rootContext()->setContextProperty("tablet", QVariant(false));
+    view.engine()->rootContext()->setContextProperty("phone", QVariant(false));
     if (args.contains("-t") || args.contains("--tablet")) {
         qDebug() << "running in tablet mode";
-        view.engine()->rootContext()->setContextProperty("tablet", true);
+        view.engine()->rootContext()->setContextProperty("tablet", QVariant(true));
     } else if (args.contains("-p") || args.contains("--phone")){
         qDebug() << "running in phone mode";
-        view.engine()->rootContext()->setContextProperty("phone", true);
+        view.engine()->rootContext()->setContextProperty("phone", QVariant(true));
     } else if (qgetenv("QT_QPA_PLATFORM") != "ubuntumirclient") {
         // Default to tablet size on X11
-        view.engine()->rootContext()->setContextProperty("tablet", true);
+        view.engine()->rootContext()->setContextProperty("tablet", QVariant(true));
     }
 
     view.engine()->setImportPathList(importPathList);
@@ -103,18 +100,20 @@
     AccountPreference preferences;
     view.engine()->rootContext()->setContextProperty("accountPreference", &preferences);
 
-    // load the qml file
+    QString qmlfile;
+    const QString filePath = QLatin1String("qml/reminders.qml");
+    QStringList paths = QStandardPaths::standardLocations(QStandardPaths::DataLocation);
+    paths.prepend(QCoreApplication::applicationDirPath());
+    Q_FOREACH (const QString &path, paths) {
+        QString myPath = path + QLatin1Char('/') + filePath;
+        if (QFile::exists(myPath)) {
+            qmlfile = myPath;
+            break;
+        }
+    }
+    // sanity check
     if (qmlfile.isEmpty()) {
-        QStringList paths = QStandardPaths::standardLocations(QStandardPaths::DataLocation);
-        paths.prepend(".");
-
-        foreach (const QString &path, paths) {
-            QFileInfo fi(path + "/qml/reminders.qml");
-            if (fi.exists()) {
-                qmlfile = path +  "/qml/reminders.qml";
-                break;
-            }
-        }
+        qFatal("File: %s does not exist at any of the standard paths!", qPrintable(filePath));
     }
     qDebug() << "using main qml file from:" << qmlfile;
     view.setSource(QUrl::fromLocalFile(qmlfile));

=== modified file 'tests/autopilot/reminders/tests/__init__.py'
--- tests/autopilot/reminders/tests/__init__.py	2014-03-17 22:57:31 +0000
+++ tests/autopilot/reminders/tests/__init__.py	2014-04-01 10:40:25 +0000
@@ -43,7 +43,6 @@
         scenarios = [('with touch', dict(input_device_class=Touch))]
 
     local_location_binary = "../../src/app/reminders"
-    local_location_qml = "../../src/app/qml/reminders.qml"
     installed_location_binary= "/usr/bin/reminders"
     installed_location_qml = "/usr/share/reminders/qml/reminders.qml"
 
@@ -62,7 +61,6 @@
         logger.debug("Launching via local")
         self.app = self.launch_test_application(
             self.local_location_binary,
-            "-q " + self.local_location_qml,
             app_type='qt',
             emulator_base=toolkit_emulators.UbuntuUIToolkitEmulatorBase)
 

=== modified file 'tests/autopilot/run' (properties changed: -x to +x)