Merge lp:~mardy/ubuntu-system-settings/plugin-invocation into lp:ubuntu-system-settings

[Alberto Mardegan]

Support launching individual panels, with options

- Let plugin options be specified from the command line
- Quit if the requested plugin does not exist
- Do not open the System Settings page when a plugin is launched

Support launching individual panels, with options

- Let plugin options be specified from the command line
- Quit if the requested plugin does not exist
- Do not open the System Settings page when a plugin is launched

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:185
http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-ci/159/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-saucy-amd64-ci/159

Click here to trigger a rebuild:
http://s-jenkins:8080/job/ubuntu-system-settings-ci/159/rebuild

review: Approve (continuous-integration)

[Iain Lane (laney)]

Thanks. Works fine.

Why did you stop using getopt in favour of custom option parsing? I quite liked the use of a proper API there.

[Sebastien Bacher (seb128)]

looks fine to me ... comment approving but not changing the mp status though since Laney has a good question and it would be nice to sort that detail out before merging

review: Approve

[Alberto Mardegan (mardy)]

> Why did you stop using getopt in favour of custom option parsing? I quite
> liked the use of a proper API there.

The "proper API" in Qt is QCoreApplication::arguments(); using getops doesn't buy us much, and would need converting all the params to QString.

[Iain Lane (laney)]

On Fri, Jul 26, 2013 at 05:58:25AM -0000, Alberto Mardegan wrote:
> > Why did you stop using getopt in favour of custom option parsing? I quite
> > liked the use of a proper API there.
>
> The "proper API" in Qt is QCoreApplication::arguments(); using getops doesn't buy us much, and would need converting all the params to QString.

It's not really an API though. The QApplication page even talks about
using argv so they clearly do recognise that you might want to do this.
Using getopt buys us the fact that it's a standard interface for parsing
which is robust and well-tested. In this case it also means that
--option foo=bar and --option=foo-bar work.

I pushed a branch which uses getopt_long() to
lp:~laney/ubuntu-system-settings/plugin-invocation-getopt-long.

I don't insist though, so if you do insist on your solution then fine.
If you do, there's a segfault if you don't have an = in the option
argument though, so please fix that.

--
Iain Lane [ <email address hidden> ]
Debian Developer [ <email address hidden> ]
Ubuntu Developer [ <email address hidden> ]

[lp:~mardy/ubuntu-system-settings/plugin-invocation]

186. By Alberto Mardegan on 2013-07-26

Don't crash if options are missing the value

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:186
http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-ci/165/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-saucy-amd64-ci/165

Click here to trigger a rebuild:
http://s-jenkins:8080/job/ubuntu-system-settings-ci/165/rebuild

review: Approve (continuous-integration)

[Iain Lane (laney)]

It still segfaults if you pass --option alone (the getopt version just ignores this - are you /sure/ reimplementing its functionality is the best approach?).

review: Needs Fixing

[lp:~mardy/ubuntu-system-settings/plugin-invocation]

187. By Alberto Mardegan on 2013-07-26

Don't crash if --option is missing its argument

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:187
http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-ci/166/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-system-settings-saucy-amd64-ci/166

Click here to trigger a rebuild:
http://s-jenkins:8080/job/ubuntu-system-settings-ci/166/rebuild

review: Approve (continuous-integration)

[Iain Lane (laney)]

review: Approve

=== modified file 'src/main.cpp'
--- src/main.cpp	2013-07-10 17:38:14 +0000
+++ src/main.cpp	2013-07-26 11:54:23 +0000
@@ -22,8 +22,6 @@
 #include "i18n.h"
 #include "plugin-manager.h"
 
-#include <unistd.h>
-
 #include <QGuiApplication>
 #include <QProcessEnvironment>
 #include <QQmlContext>
@@ -51,24 +49,33 @@
     /* HACK: force the theme until lp #1098578 is fixed */
     QIcon::setThemeName("ubuntu-mobile");
 
-    /* Parse the commandline options to see if we've been given a panel to load.
-     * The platform might pass us unknown (to us) options, which we want to just ignore.
+    /* Parse the commandline options to see if we've been given a panel to load,
+     * and other options for the panel.
      */
     QString defaultPlugin;
-    opterr = 0; /* disable errors, i.e. allow unknown arguments */
-    while (getopt(argc, argv, "") != -1); /* skip all option arguments */
-
-    if (optind < argc) { /* we have an argument */
-        defaultPlugin = argv[optind];
+    QVariantMap pluginOptions;
+    QStringList arguments = app.arguments();
+    for (int i = 1; i < arguments.count(); i++) {
+        const QString &argument = arguments.at(i);
+        if (!argument.startsWith('-')) {
+            defaultPlugin = argument;
+        } else if (argument == "--option" && i + 1 < arguments.count()) {
+            QStringList option = arguments.at(++i).split("=");
+            // If no value is given, insert an empty string
+            pluginOptions.insert(option.at(0), option.value(1, ""));
+        }
     }
 
     QQuickView view;
+    QObject::connect(view.engine(), SIGNAL(quit()), &app, SLOT(quit()),
+                     Qt::QueuedConnection);
     qmlRegisterType<QAbstractItemModel>();
     qmlRegisterType<SystemSettings::PluginManager>("SystemSettings", 1, 0, "PluginManager");
     view.setResizeMode(QQuickView::SizeRootObjectToView);
     view.engine()->addImportPath(PLUGIN_PRIVATE_MODULE_DIR);
     view.engine()->addImportPath(PLUGIN_QML_DIR);
     view.rootContext()->setContextProperty("defaultPlugin", defaultPlugin);
+    view.rootContext()->setContextProperty("pluginOptions", pluginOptions);
     view.setSource(QUrl("qrc:/qml/MainWindow.qml"));
     view.show();
 

=== modified file 'src/qml/MainWindow.qml'
--- src/qml/MainWindow.qml	2013-07-18 14:42:22 +0000
+++ src/qml/MainWindow.qml	2013-07-26 11:54:23 +0000
@@ -31,7 +31,6 @@
 
     Component.onCompleted: {
         i18n.domain = "ubuntu-system-settings"
-        pageStack.push(mainPage)
         if (defaultPlugin) {
             var plugin = pluginManager.getByName(defaultPlugin)
             if (plugin) {
@@ -41,8 +40,11 @@
                     pageStack.push(pageComponent, { plugin: plugin })
             } else {
                 // Invalid plugin passed on the commandline
-                console.log("Plugin " + defaultPlugin + " does not exist. Ignoring.")
+                console.log("Plugin " + defaultPlugin + " does not exist.")
+                Qt.quit()
             }
+        } else {
+            pageStack.push(mainPage)
         }
     }