qtubuntu

Merge lp:~dandrader/qtubuntu/betterSessionName into lp:qtubuntu

betterSessionName
Merge into trunk

Proposed by Daniel d'Andrada on 2016-06-01

Status:	Merged
Approved by:	Gerry Boland on 2016-06-01
Approved revision:	no longer in the source branch.
Merged at revision:	322
Proposed branch:	lp:~dandrader/qtubuntu/betterSessionName
Merge into:	lp:qtubuntu
Diff against target:	104 lines (+44/-8) 2 files modified src/ubuntumirclient/integration.cpp (+40/-6) src/ubuntumirclient/integration.h (+4/-2)
To merge this branch:	bzr merge lp:~dandrader/qtubuntu/betterSessionName
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Unity8 CI Bot	continuous-integration		Approve on 2016-06-04
Gerry Boland		2016-06-01	Approve on 2016-06-01
Review via email: mp+296198@code.launchpad.net

Commit Message

Generate better mir session names

Try to come up with some meaningful session name to uniquely identify this session,
helping with shell debugging

Description of the Change

Immensely helpful to help make sense of unity8.log when debugging multiple/nested prompt sessions use cases such as in bug 1586219.

Unity8 CI Bot (unity8-ci-bot) wrote on 2016-06-01:

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtubuntu-ci/63/rebuild

review: Approve (continuous-integration)

Nick Dedekind (nick-dedekind) on 2016-06-01:

Daniel d'Andrada (dandrader) wrote on 2016-06-01:

On 01/06/2016 09:48, Nick Dedekind wrote:
>> +QByteArray UbuntuClientIntegration::generateSessionName(QStringList &args)
>> >+{
> Perhaps use QApplication::applicationName first? I actually had a branch for this about a thousand years ago, but it never went anywhere.
>

QApplication::applicationName might not be set yet when
QPlatformIntegration is created (which is really early). It can be set
or even changed at any time by calling
QCoreApplicaiton::setApplicationName, and we would have not way of
updating your session id to follow those changes. So the only thing sure
to exist at QPlatformIntegration creation is the application arguments.

Gerry Boland (gerboland) wrote on 2016-06-01:

+for (int i = 0; i < args.count(); ++i) {
+ QString arg = args[i];
We have Q_FOREACH

Do you really need to read the FileInfo object? Why not just drop the ".qml" from the file name?

+ u_application_id_new_from_stringn(sessionName.data(), sessionName.count());
Does the api expect sessionName to always exist, or will it make a copy itself?

review: Needs Fixing

Daniel d'Andrada (dandrader) wrote on 2016-06-01:

On 01/06/2016 12:01, Gerry Boland wrote:
> Review: Needs Fixing
>
> +for (int i = 0; i < args.count(); ++i) {
> + QString arg = args[i];
> We have Q_FOREACH

I personally find this macro ugly, but if you fancy it...
But it is less code and thus less error prone. Done.

> Do you really need to read the FileInfo object? Why not just drop the ".qml" from the file name?
Yes. The argument could be a full file path like this:

/foo/bar/whatever/zumba.qml

Which would make for a cumbersome session name. QFileInfo neatly and
safely parses that filepath for me.

I like the ".qml" suffix as it tells us it's a plain qml app run via
qmlscene.

> + u_application_id_new_from_stringn(sessionName.data(), sessionName.count());
> Does the api expect sessionName to always exist, or will it make a copy itself?

It makes a copy. What a horrible API it would be otherwise.

Unity8 CI Bot (unity8-ci-bot) wrote on 2016-06-01:

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtubuntu-ci/64/rebuild

review: Approve (continuous-integration)

Gerry Boland (gerboland) wrote on 2016-06-01:

> > Do you really need to read the FileInfo object? Why not just drop the ".qml"
> from the file name?
> Yes. The argument could be a full file path like this:
>
> /foo/bar/whatever/zumba.qml
>
> Which would make for a cumbersome session name. QFileInfo neatly and
> safely parses that filepath for me.
>
> I like the ".qml" suffix as it tells us it's a plain qml app run via
> qmlscene.

Yeah, that's ok. I just want to keep app startup light, using QFileInfo to parse a file name just strikes me as heavy for something that only those reading file logs will appreciate. But I won't block on that.

> > + u_application_id_new_from_stringn(sessionName.data(),
> sessionName.count());
> > Does the api expect sessionName to always exist, or will it make a copy
> itself?
>
> It makes a copy. What a horrible API it would be otherwise.

You never know :)

Daniel d'Andrada (dandrader) wrote on 2016-06-01:

On 01/06/2016 12:58, Gerry Boland wrote:
> Yeah, that's ok. I just want to keep app startup light, using QFileInfo to parse a file name just strikes me as heavy for something that only those reading file logs will appreciate. But I won't block on that.

QFileInfo creation looks pretty tame and lightweight. It pretty much
just stores the given filepath in a internal string.

The only real code we execute happens when we call QFileInfo::fileName,
where it essentially finds the last separator char in the string and
then "return m_filePath.mid(m_lastSeparator + 1);".

Gerry Boland (gerboland) wrote on 2016-06-01:

Ok, thanks for checking that. Change looks good to me!

review: Approve

lp:~dandrader/qtubuntu/betterSessionName updated on 2016-06-03

322. By Daniel d'Andrada on 2016-06-03

Generate better mir session names

Try to come up with some meaningful session name to uniquely identify this session,
helping with shell debugging

Unity8 CI Bot (unity8-ci-bot) wrote on 2016-06-04:

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtubuntu-ci/69/rebuild

review: Approve (continuous-integration)

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:

Daniel d'Andrada

Ubuntu Phablet Team

 === modified file 'src/ubuntumirclient/integration.cpp'
 --- src/ubuntumirclient/integration.cpp	2016-04-28 14:44:18 +0000
 +++ src/ubuntumirclient/integration.cpp	2016-06-03 13:56:35 +0000
@@ -28,6 +28,7 @@
  #include "window.h"
  // Qt
++#include <QFileInfo>
  #include <QGuiApplication>
  #include <private/qguiapplication_p.h>
  #include <qpa/qplatformnativeinterface.h>
@@ -77,8 +78,12 @@
      , mClipboard(new UbuntuClipboard)
      , mScaleFactor(1.0)
+ {
--    setupOptions();
--    setupDescription();
++    {
++        QStringList args = QCoreApplication::arguments();
++        setupOptions(args);
++        QByteArray sessionName = generateSessionName(args);
++        setupDescription(sessionName);
++    }
      // Create new application instance
      mInstance = u_application_instance_new_from_description_with_options(mDesc, mOptions);
@@ -161,9 +166,8 @@
      return mServices;
+ }
--void UbuntuClientIntegration::setupOptions()
++void UbuntuClientIntegration::setupOptions(QStringList &args)
+ {
--    QStringList args = QCoreApplication::arguments();
      int argc = args.size() + 1;
      char **argv = new char*[argc];
      for (int i = 0; i < argc - 1; i++)
@@ -177,10 +181,11 @@
      delete [] argv;
+ }
--void UbuntuClientIntegration::setupDescription()
++void UbuntuClientIntegration::setupDescription(QByteArray &sessionName)
+ {
      mDesc = u_application_description_new();
--    UApplicationId* id = u_application_id_new_from_stringn("QtUbuntu", 8);
++
++    UApplicationId* id = u_application_id_new_from_stringn(sessionName.data(), sessionName.count());
      u_application_description_set_application_id(mDesc, id);
      UApplicationLifecycleDelegate* delegate = u_application_lifecycle_delegate_new();
@@ -190,6 +195,35 @@
      u_application_description_set_application_lifecycle_delegate(mDesc, delegate);
+ }
++QByteArray UbuntuClientIntegration::generateSessionName(QStringList &args)
++{
++    // Try to come up with some meaningful session name to uniquely identify this session,
++    // helping with shell debugging
++
++    if (args.count() == 0) {
++        return QByteArray("QtUbuntu");
++    } if (args[0].contains("qmlscene")) {
++        return generateSessionNameFromQmlFile(args);
++    } else {
++        // use the executable name
++        QFileInfo fileInfo(args[0]);
++        return fileInfo.fileName().toLocal8Bit();
++    }
++}
++
++QByteArray UbuntuClientIntegration::generateSessionNameFromQmlFile(QStringList &args)
++{
++    Q_FOREACH (QString arg, args) {
++        if (arg.endsWith(".qml")) {
++            QFileInfo fileInfo(arg);
++            return fileInfo.fileName().toLocal8Bit();
++        }
++    }
++
++    // give up
++    return "qmlscene";
++}
++
  QPlatformWindow* UbuntuClientIntegration::createPlatformWindow(QWindow* window) const
+ {
      return const_cast<UbuntuClientIntegration*>(this)->createPlatformWindow(window);
 === modified file 'src/ubuntumirclient/integration.h'
 --- src/ubuntumirclient/integration.h	2016-04-28 14:09:54 +0000
 +++ src/ubuntumirclient/integration.h	2016-06-03 13:56:35 +0000
@@ -76,8 +76,10 @@
      void destroyScreen(UbuntuScreen *screen);
  private:
--    void setupOptions();
--    void setupDescription();
++    void setupOptions(QStringList &args);
++    void setupDescription(QByteArray &sessionName);
++    static QByteArray generateSessionName(QStringList &args);
++    static QByteArray generateSessionNameFromQmlFile(QStringList &args);
      UbuntuNativeInterface* mNativeInterface;
      QPlatformFontDatabase* mFontDb;