Unity 8

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration	2014-04-25	Needs Fixing on 2014-04-29
Albert Astals Cid (community)		2014-04-25	Needs Fixing on 2014-04-28
Review via email: mp+217282@code.launchpad.net

This proposal supersedes a proposal from 2014-04-25.

This proposal has been superseded by a proposal from 2014-04-30.

Commit message

Adds usage style help to unity8 executable.

Fixes lp:1269282

Description of the change

* Are there any related MPs required for this MP to build/function as expected? Please list.
lp:~aacid/unity8/killqt51

* Did you perform an exploratory manual test run of your code change and any related functionality?
Compiled code, verified new feature works as expected, and ran ctest

* 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?
Dependent branch does. This MP does not

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

Revision history for this message

Albert Astals Cid (aacid) wrote on 2014-04-22: Posted in a previous version of this proposal

Should increase the qt5 version number dependency in debian/control to 5.2 since QCommandLineParser is new for 5.2
Also probably in the find_package of CMakeListst.txt

review: Needs Fixing

Revision history for this message

Albert Astals Cid (aacid) wrote on 2014-04-22: Posted in a previous version of this proposal

And you're missing https://wiki.ubuntu.com/Process/Merges/Checklists/Unity8 as part of your description of the change

Revision history for this message

Albert Astals Cid (aacid) wrote on 2014-04-22: Posted in a previous version of this proposal

And finally, we don't support Qt translation system (we are using gettext) so for now i think we can just pretend we don't need to translate the commandline options and just drop the QCoreApplication::translate() and just pass the English text as second param of QCommandLineOption.

Saviq, what's your opinion?

Revision history for this message

Michał Sawicz (saviq) wrote on 2014-04-22: Posted in a previous version of this proposal

Yeah, let's leave the help in English. It's not like they're general-use.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-23: Posted in a previous version of this proposal

FAILED: Continuous integration, rev:858
http://jenkins.qa.ubuntu.com/job/unity8-ci/2862/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-trusty-touch/391/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-trusty/1726/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity8-trusty-amd64-ci/1383/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity8-trusty-armhf-ci/1387/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity8-trusty-i386-ci/1383/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/4628/console

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Albert Astals Cid (aacid) wrote on 2014-04-24: Posted in a previous version of this proposal

In case you're confused by all the failings → whitespace issues

review: Needs Fixing

Revision history for this message

Michał Sawicz (saviq) wrote on 2014-04-24: Posted in a previous version of this proposal

src/main.cpp: bad whitespace in lines 67, 84

Revision history for this message

Albert Astals Cid (aacid) wrote on 2014-04-24: Posted in a previous version of this proposal

you probably want to merge lp:~aacid/unity8/killqt51 into here (you'll have to resubmit the MR setting a prerequisite branch)

Revision history for this message

Josh Arenson (josharenson) wrote on 2014-04-24: Posted in a previous version of this proposal

> you probably want to merge lp:~aacid/unity8/killqt51 into here (you'll have
> to resubmit the MR setting a prerequisite branch)

So if I clean up my white space, depend on _your_ MP, and resubmit _this_ MP in the proper format, we should be good to go?

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-25: Posted in a previous version of this proposal

FAILED: Continuous integration, rev:859
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/~josharenson/unity8/doc_args/+merge/217158/+edit-commit-message

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Michał Sawicz (saviq) wrote on 2014-04-25: Posted in a previous version of this proposal

You didn't merge Albert's branch into this one, and you didn't put it in the "prerequisite" field when resubmitting.

Revision history for this message

Michał Sawicz (saviq) wrote on 2014-04-25: Posted in a previous version of this proposal

I've resubmitted this for you, you can merge Albert's branch, but that's not necessary, since the other one will be merged before anyway.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-25: Posted in a previous version of this proposal

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-25: Posted in a previous version of this proposal

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-25:

FAILED: Continuous integration, rev:860
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/~josharenson/unity8/doc_args/+merge/217282/+edit-commit-message

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Albert Astals Cid (aacid) wrote on 2014-04-28:

This breaks the -windowgeometry param, i.e.

./builddir/src/unity8 -windowgeometry 400x400

Doesn't work anymore

review: Needs Fixing

lp:~josharenson/unity8/doc_args updated on 2014-04-29

861. By Josh Arenson on 2014-04-29: Added windowgeometry to argument list

Revision history for this message

Josh Arenson (josharenson) wrote on 2014-04-29:

> This breaks the -windowgeometry param, i.e.
>
> ./builddir/src/unity8 -windowgeometry 400x400
>
> Doesn't work anymore
Pushed a small hack to WAR this. Let me know if you have an idea for something more elegant.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-29:

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Albert Astals Cid (aacid) wrote on 2014-04-29:

> > This breaks the -windowgeometry param, i.e.
> >
> > ./builddir/src/unity8 -windowgeometry 400x400
> >
> > Doesn't work anymore
> Pushed a small hack to WAR this. Let me know if you have an idea for something
> more elegant.

I think i'd prefer if you move the parsing of the argument to main.cpp and only pass to ApplicationArguments the already parsed size instead of the list of args

lp:~josharenson/unity8/doc_args updated on 2014-04-29

862. By Josh Arenson on 2014-04-29

Handle windowgeometry argument with new argument parsing scheme.

This should retain backwards compatibility.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-29:

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

review: Needs Fixing (continuous-integration)

lp:~josharenson/unity8/doc_args updated on 2014-05-05

863. By Josh Arenson on 2014-05-05: Refactored ApplicationArguments constructor(s)

Unmerged revisions

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Josh Arenson

Mohammad Rakibul Hasan

Unity Team

 === modified file 'src/ApplicationArguments.h'
 --- src/ApplicationArguments.h	2014-03-19 15:29:35 +0000
 +++ src/ApplicationArguments.h	2014-04-29 17:49:14 +0000
@@ -28,14 +28,13 @@
+ {
      Q_OBJECT
  public:
--    ApplicationArguments(const QStringList& args) {
--        if (args.contains(QLatin1String("-windowgeometry")) && args.size() > args.indexOf(QLatin1String("-windowgeometry")) + 1) {
--            QStringList geometryArg = args.at(args.indexOf(QLatin1String("-windowgeometry")) + 1).split('x');
--            if (geometryArg.size() == 2) {
--                m_size.rwidth() = geometryArg.at(0).toInt();
--                m_size.rheight() = geometryArg.at(1).toInt();
--            }
--        }
++    ApplicationArguments()
++    {
++    }
++
++    ApplicationArguments(int width, int height) {
++        m_size.rwidth()  = width;
++        m_size.rheight() = height;
+     }
      Q_INVOKABLE bool hasGeometry() const { return m_size.isValid(); }
 === modified file 'src/main.cpp'
 --- src/main.cpp	2014-04-02 11:19:54 +0000
 +++ src/main.cpp	2014-04-29 17:49:14 +0000
@@ -18,6 +18,7 @@
   */
  // Qt
++#include <QCommandLineParser>
  #include <QtQuick/QQuickView>
  #include <QtGui/QGuiApplication>
  #include <QtQml/QQmlEngine>
@@ -43,6 +44,32 @@
      QGuiApplication::setApplicationName("Unity 8");
      QGuiApplication *application;
++
++    QCommandLineParser parser;
++    parser.setApplicationDescription("Description: Unity 8 Shell");
++    parser.addHelpOption();
++
++    QCommandLineOption fullscreenOption("fullscreen",
++        "Run in fullscreen");
++    parser.addOption(fullscreenOption);
++
++    QCommandLineOption framelessOption("frameless",
++        "Run without window borders");
++    parser.addOption(framelessOption);
++
++    QCommandLineOption mousetouchOption("mousetouch",
++        "Allow the mouse to provide touch input");
++    parser.addOption(mousetouchOption);
++
++    QCommandLineOption windowGeometryOption(QStringList() << "windowgeometry",
++            "Specify the window geometry as [<width>x<height>]", "windowgeometry", "1");
++    parser.addOption(windowGeometryOption);
++
++    QCommandLineOption testabilityOption("testability",
++        "DISCOURAGED: Please set QT_LOAD_TESTABILITY instead. \n \
++Load the testability driver");
++    parser.addOption(testabilityOption);
++
      if (isUbuntuMirServer) {
          QLibrary unityMir("unity-mir", 1);
          unityMir.load();
@@ -59,6 +86,11 @@
          application = new QGuiApplication(argc, (char**)argv);
+     }
++    // Treat args with single dashes the same as arguments with two dashes
++    // Ex: -fullscreen == --fullscreen
++    parser.setSingleDashWordOptionMode(QCommandLineParser::ParseAsLongOptions);
++    parser.process(*application);
++
      QString indicatorProfile = qgetenv("UNITY_INDICATOR_PROFILE");
      if (indicatorProfile.isEmpty()) {
          indicatorProfile = "phone";
@@ -66,12 +98,20 @@
      resolveIconTheme();
--    QStringList args = application->arguments();
--    ApplicationArguments qmlArgs(args);
++    std::auto_ptr<ApplicationArguments> appArgs;
++    if (parser.isSet(windowGeometryOption) &&
++        parser.value(windowGeometryOption).split('x').size() == 2) {
++            QStringList geom =
++            parser.value(windowGeometryOption).split('x');
++            appArgs.reset(new ApplicationArguments(geom.at(0).toInt(), geom.at(1).toInt()));
++    } else {
++        appArgs.reset(new ApplicationArguments());
++    }
++    ApplicationArguments& qmlArgs = *appArgs;
      // 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 (args.contains(QLatin1String("-testability")) || getenv("QT_LOAD_TESTABILITY")) {
++    if (parser.isSet(testabilityOption) || getenv("QT_LOAD_TESTABILITY")) {
          QLibrary testLib(QLatin1String("qttestability"));
          if (testLib.load()) {
              typedef void (*TasInitialize)(void);
@@ -94,14 +134,14 @@
      view->engine()->setBaseUrl(QUrl::fromLocalFile(::qmlDirectory()));
      view->rootContext()->setContextProperty("applicationArguments", &qmlArgs);
      view->rootContext()->setContextProperty("indicatorProfile", indicatorProfile);
--    if (args.contains(QLatin1String("-frameless"))) {
++    if (parser.isSet(framelessOption)) {
          view->setFlags(Qt::FramelessWindowHint);
+     }
      // You will need this if you want to interact with touch-only components using a mouse
      // Needed only when manually testing on a desktop.
      MouseTouchAdaptor *mouseTouchAdaptor = 0;
--    if (args.contains(QLatin1String("-mousetouch"))) {
++    if (parser.isSet(mousetouchOption)) {
          mouseTouchAdaptor = new MouseTouchAdaptor;
          application->installNativeEventFilter(mouseTouchAdaptor);
+     }
@@ -125,7 +165,7 @@
      view->setSource(source);
      view->setColor("transparent");
--    if (qgetenv("QT_QPA_PLATFORM") == "ubuntu" || isUbuntuMirServer || args.contains(QLatin1String("-fullscreen"))) {
++    if (qgetenv("QT_QPA_PLATFORM") == "ubuntu" || isUbuntuMirServer || parser.isSet(fullscreenOption)) {
          view->showFullScreen();
      } else {
          view->show();