Merge lp:~paulliu/unity-mir/logout into lp:unity-mir

[Ying-Chun Liu]

Adding methods for logout

* Are there any related MPs required for this MP to build/function as expected?
No.

 * Did you perform an exploratory manual test run of your code change and any related functionality?
Yes.Run qdbus on phone and the dbus methods and signals are emitted.

 * If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
N/A

 * If you changed the UI, has there been a design review?
N/A

[lp:~paulliu/unity-mir/logout]

216. By Ying-Chun Liu on 2014-04-17

stop apps

217. By Ying-Chun Liu on 2014-04-17

Fix bug

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:216
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/~paulliu/unity-mir/logout/+merge/216336/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/unity-mir-ci/326/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-amd64-ci/189/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/190/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-i386-ci/189/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-mir-ci/326/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:217
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/~paulliu/unity-mir/logout/+merge/216336/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/unity-mir-ci/327/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-amd64-ci/190
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/191
        deb: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/191/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-i386-ci/190

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-mir-ci/327/rebuild

review: Needs Fixing (continuous-integration)

[lp:~paulliu/unity-mir/logout]

218. By Ying-Chun Liu on 2014-04-17

Fix coding style

219. By Ying-Chun Liu on 2014-04-17

Merged upstream.
[ Ted Gould ]
Ensure the last item of the array is NULL (LP: #1304315)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:219
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/~paulliu/unity-mir/logout/+merge/216336/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/unity-mir-ci/328/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-amd64-ci/191
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/192
        deb: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/192/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-i386-ci/191

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-mir-ci/328/rebuild

review: Needs Fixing (continuous-integration)

[Daniel d'Andrada (dandrader)]

113 + * Copyright (C) 2013 Canonical, Ltd.

s/2013/2014

[Daniel d'Andrada (dandrader)]

213 + Q_SCRIPTABLE void LogoutRequested(bool b);
214 + void LogoutReady();

Signals should not start with a capital letter.

[Daniel d'Andrada (dandrader)]

144 +DBusLogout::DBusLogout(ApplicationManager *parent) : QObject(parent)
145 +{
146 + QDBusConnection connection = QDBusConnection::sessionBus();
147 + m_applicationManager = parent;
148 +
149 + QObject::connect(this, &DBusLogout::LogoutReady,
150 + parent, &ApplicationManager::onLogoutReady);

DBusLogout gets an ApplicationManager pointer just to make a signal/slot connection between the two!?

That's bad design. You're unnecessarily adding a circular dependency between DBusLogout and ApplicationManager. Just make ApplicationManager code create such connection.

Actually I'm not even sure this should be in unity-mir in the first place. As there's nothing "mir" about it.

review: Needs Fixing

[Daniel d'Andrada (dandrader)]

> [...]
> Actually I'm not even sure this should be in unity-mir in the first place. As
> there's nothing "mir" about it.

Meaning that it should probably be in unity8

[Michał Sawicz (saviq)]

Re: Mir or not, there's a lot of non-mir in unity-mir already. But this here is about app management. We talked about this and decided this is as good a place as any.

[lp:~paulliu/unity-mir/logout]

220. By Ying-Chun Liu on 2014-04-18

Move signal connect to application manager.
Fix signal shouldn't start with capital letter.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:220
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/329/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-amd64-ci/192
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/193
        deb: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/193/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-i386-ci/192

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-mir-ci/329/rebuild

review: Approve (continuous-integration)

[Ying-Chun Liu (paulliu)]

> [...]
> Actually I'm not even sure this should be in unity-mir in the first place. As
> there's nothing "mir" about it.

Currently unity7 provides the same dbus interface.
To provide the same for unity8, I think both unity8 and unity-mir have to be patched anyway.
We indeed need to decide which part to put into which branch.

1. For unity-mir (application manager), it needs to receive a signal to kill all the apps.
2. And we need a place to provide the dbus interface for Logout. (either unity8 or unity-mir)
3. And unity8 have to show the dialog and/or kill itself when received a signal.

[Daniel d'Andrada (dandrader)]

In src/modules/Unity/Application/dbuslogout.cpp

"""
140 +#include "application_manager.h"
"""

Should be removed

-----------------------------------------

In src/modules/Unity/Application/dbuslogout.h:

"""
208 +class ApplicationManager;
"""

Should be removed.

"""
227 +private:
228 + ApplicationManager *m_applicationManager;
"""

Should be removed.

------------------------------------------

Is this com.canonical.Unity.Session inteface documented elsewhere? If not I would like to see it documented in src/modules/Unity/Application/dbuslogout.h. It's not clear to me how it's supposed to be used.

"""
Q_SCRIPTABLE void logoutRequested(bool b);
"""

What's that boolean for?

What's the workflow here? RequestLogout() emits logoutRequested() and Logout() emits logoutReady(). Naming is a bit incosistent. What's the meaning and difference between RequestLogout() and Logout()? ApplicationManager is using only of the the two...

---------------------------------------------

ApplicationManager knowing about the logout concept feels out of place. As it name says, it should know only about managing apps. I believe it should be unity8 code doing the connection between the two.

AppliationManger::unityLogout()

Poorly named to say the least (check naming conventions for signals). I believe a clearer separation of tasks and responsibilities would be having something like the following in unity8 code:

DBusLogout {
    onLogoutRequested {
       ApplicationManager.stopAllApplications()
       Qt.quit();
    }
}

Which brings to mind the name of this class: DBusLogout. As it's implementing the com.canonical.Unity.Session service, I would clearer if it was named after it. After all, com.canonical.Unity.Session should be about more than just a Logout method, right? Thus:

UnitySessionDBusService {
    onLogoutRequested {
       ApplicationManager.stopAllApplications()
       Qt.quit();
    }
}

review: Needs Fixing

[Michał Sawicz (saviq)]

The problem with your last suggestion Daniel would be that we'd (have to) block the UI thread during stopAllApplications, as we want to shut them down gracefully and only then shut Unity/Mir down. That's why I think the dual-signal approach will serve us better, even if we think the responsibilities should shift between unity8 and unity-mir.

I'm not entirely sure we want to move it up to the shell, though. If we think unity-mir is meant to facilitate building shells, the more we move up there, the more the shell needs to do. I can be convinced, though.

[lp:~paulliu/unity-mir/logout]

221. By Ying-Chun Liu on 2014-04-23

minor changes

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:221
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/331/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-amd64-ci/194
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/195
        deb: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/195/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-i386-ci/194

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-mir-ci/331/rebuild

review: Approve (continuous-integration)

[lp:~paulliu/unity-mir/logout]

222. By Ying-Chun Liu on 2014-04-23

fix the boolean variable.

[Ying-Chun Liu (paulliu)]

After discussion with some people, I found that we actually don't have any specs or documents about it.
All of it is being defined "while coding" it.
Here's the interface in unity7.
http://pastebin.ubuntu.com/7231097/
It is in shutdown/SessionDBusManager.cpp in lp:unity.

So the thing is "indicator-session" will call the method "RequesetLogout". And the ApplicationManager will see if we have any important apps that inhibits the logout action ( while I don't think there's such app in Phone right now ) and then send out the signal "LogoutRequested" with a boolean to indicate if there's such apps.
And then Unity shell should receive that signal and display a dialog for the user to decide to logout or not.
If the user decide to logout, then unity shell call the method "Logout" and we have to shutdown all the app. After that, we signal the shell to shut itself.

I've fix some unclear naming. Not sure if we have a better name for logoutReady and unityLogout.
In unity7, When Logout is called, it forwards this method to org.gnome.SessionManager and some fallbacks

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:222
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/332/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-amd64-ci/195
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/196
        deb: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/196/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-i386-ci/195

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-mir-ci/332/rebuild

review: Approve (continuous-integration)

[Daniel d'Andrada (dandrader)]

> The problem with your last suggestion Daniel would be that we'd (have to)
> block the UI thread during stopAllApplications, as we want to shut them down
> gracefully and only then shut Unity/Mir down. That's why I think the dual-
> signal approach will serve us better, even if we think the responsibilities
> should shift between unity8 and unity-mir.

Ok.

> I'm not entirely sure we want to move it up to the shell, though. If we think
> unity-mir is meant to facilitate building shells, the more we move up there,
> the more the shell needs to do. I can be convinced, though.

hm!? Come on, unity-mir has "unity" in its name. So it's an integral part of the unity shell. But it seems to be treated like a unity8-cpp (where we stuff all the CPP code of unity8) whereas unity8 proper is like a unity8-qml (i.e., we want unity8 to have only QML code).

Also what's being added here is the com.canonical.Unity.Session service, which I suppose will have other methods and signals in the future that unity8 qml will be interested on and not only ApplicationManager.

[Daniel d'Andrada (dandrader)]

> So the thing is "indicator-session" will call the method "RequesetLogout". And
> the ApplicationManager will see if we have any important apps that inhibits
> the logout action ( while I don't think there's such app in Phone right now )
> and then send out the signal "LogoutRequested" with a boolean to indicate if
> there's such apps.
> And then Unity shell should receive that signal and display a dialog for the
> user to decide to logout or not.
> If the user decide to logout, then unity shell call the method "Logout" and we
> have to shutdown all the app. After that, we signal the shell to shut itself.

1- I asked you to document com.canonical.Unity.Session in its header, not in a merge request comment :D But thanks for the info. Now things are getting clearer.

2- So the D-Bus method Logout() from the com.canonical.Unity.Session service, which is provided by the unity8 process, is called by the unity8 process itself!?

3- What Unity shell is supposed to do with this "have_inhibitors" boolean? Display a different message in the logout dialog? I would refrain from adding such boolean until we actually have use for it. Development might change direction and this boolean could end up just as cruft.

[lp:~paulliu/unity-mir/logout]

223. By Ying-Chun Liu on 2014-04-24

Rename the class.
Add some doxygen.

[Ying-Chun Liu (paulliu)]

> 1- I asked you to document com.canonical.Unity.Session in its header, not in a
> merge request comment :D But thanks for the info. Now things are getting
> clearer.

Added.

>
> 2- So the D-Bus method Logout() from the com.canonical.Unity.Session service,
> which is provided by the unity8 process, is called by the unity8 process
> itself!?
>

Yes. It should be called by unity8 process.
But other apps if they want to do urgent logout, they can call this method too.

> 3- What Unity shell is supposed to do with this "have_inhibitors" boolean?
> Display a different message in the logout dialog? I would refrain from adding
> such boolean until we actually have use for it. Development might change
> direction and this boolean could end up just as cruft.

Yes, a different message in the logout dialog. I remember in unity7 it shows a dialog to tell users the logout is blocked and cancelled. I think we can keep this boolean for a while to be compatible to unity? But seems ok to me to remove it now as we don't have many historical burdens.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:223
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/333/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-amd64-ci/196
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/197
        deb: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/197/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-i386-ci/196

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-mir-ci/333/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:223
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/334/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-amd64-ci/197
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/198
        deb: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/198/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-i386-ci/197

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-mir-ci/334/rebuild

review: Approve (continuous-integration)

[Daniel d'Andrada (dandrader)]

Cosmetics: Please remove the trailing whitespace and the unneeded #include

=== modified file 'src/modules/Unity/Application/dbusunitysessionservice.cpp'
--- src/modules/Unity/Application/dbusunitysessionservice.cpp 2014-04-24 13:38:59 +0000
+++ src/modules/Unity/Application/dbusunitysessionservice.cpp 2014-04-24 17:29:55 +0000
@@ -23,16 +23,13 @@
 #include <QDBusMetaType>
 #include <QCoreApplication>

-// std
-#include <csignal>
-
 namespace unitymir
 {

 DBusUnitySessionService::DBusUnitySessionService(QObject *parent) : QObject(parent)
 {
     QDBusConnection connection = QDBusConnection::sessionBus();
-
+
     connection.registerService("com.canonical.Unity.Session");
     connection.registerObject("/com/canonical/Unity/Session", this, QDBusConnection::ExportAllSignals | QDBusConnection::ExportScriptableSlots | QDBusConnection::ExportScriptableInvokables);
 }

review: Needs Fixing

[Daniel d'Andrada (dandrader)]

Another cosmetic change: That line is waaaaay too long. Please break it.

=== modified file 'src/modules/Unity/Application/dbusunitysessionservice.cpp'
--- src/modules/Unity/Application/dbusunitysessionservice.cpp 2014-04-24 17:31:43 +0000
+++ src/modules/Unity/Application/dbusunitysessionservice.cpp 2014-04-24 17:32:22 +0000
@@ -31,7 +31,10 @@ DBusUnitySessionService::DBusUnitySessio
     QDBusConnection connection = QDBusConnection::sessionBus();

     connection.registerService("com.canonical.Unity.Session");
- connection.registerObject("/com/canonical/Unity/Session", this, QDBusConnection::ExportAllSignals | QDBusConnection::ExportScriptableSlots | QDBusConnection::ExportScriptableInvokables);
+ connection.registerObject("/com/canonical/Unity/Session", this,
+ QDBusConnection::ExportAllSignals
+ | QDBusConnection::ExportScriptableSlots
+ | QDBusConnection::ExportScriptableInvokables);
 }

 DBusUnitySessionService::~DBusUnitySessionService()

review: Needs Fixing

[Daniel d'Andrada (dandrader)]

Thanks for the documentation! Looking good!

[Daniel d'Andrada (dandrader)]

We also don't need those #includes

=== modified file 'src/modules/Unity/Application/dbusunitysessionservice.cpp'
--- src/modules/Unity/Application/dbusunitysessionservice.cpp 2014-04-24 17:34:26 +0000
+++ src/modules/Unity/Application/dbusunitysessionservice.cpp 2014-04-24 17:41:10 +0000
@@ -20,8 +20,6 @@

 // Qt
 #include <QDBusConnection>
-#include <QDBusMetaType>
-#include <QCoreApplication>

 namespace unitymir
 {

=== modified file 'src/modules/Unity/Application/dbusunitysessionservice.h'
--- src/modules/Unity/Application/dbusunitysessionservice.h 2014-04-24 13:38:59 +0000
+++ src/modules/Unity/Application/dbusunitysessionservice.h 2014-04-24 17:40:25 +0000
@@ -18,7 +18,6 @@
 #define DBUSUNITYSESSIONSERVICE_H_1397562297

 #include <QObject>
-#include <QDBusArgument>

 namespace unitymir
 {

review: Needs Fixing

[Daniel d'Andrada (dandrader)]

You're generating all that stuff but you don't seem to even use it. And I don't we needed it at all when using registerService() & registerObject(). It can just be removed.

=== modified file 'src/modules/Unity/Application/CMakeLists.txt'
--- src/modules/Unity/Application/CMakeLists.txt 2014-04-24 13:38:59 +0000
+++ src/modules/Unity/Application/CMakeLists.txt 2014-04-24 18:16:50 +0000
@@ -15,23 +15,9 @@ include_directories(
   ${CMAKE_SOURCE_DIR}/src/unity-mir
 )

-qt5_generate_dbus_interface(
- dbusunitysessionservice.h
- ${CMAKE_CURRENT_BINARY_DIR}/com.canonical.Unity8.Session.xml
- OPTIONS -a
-)
-
-qt5_add_dbus_adaptor(
- dbusunitysessionservice_SRCS
- ${CMAKE_CURRENT_BINARY_DIR}/com.canonical.Unity8.Session.xml
- dbusunitysessionservice.h unitymir::DBusUnitySessionService
- dbusunitysessionserviceadaptor DBusUnitySessionServiceAdaptor
-)
-
 add_library(
   unityapplicationplugin SHARED

- ${CMAKE_CURRENT_BINARY_DIR}/com.canonical.Unity8.Session.xml
   application_manager.cpp
   application.cpp
   desktopfilereader.cpp
@@ -50,7 +36,6 @@ add_library(
   ubuntukeyboardinfo.cpp
   dbusunitysessionservice.cpp
   dbusunitysessionservice.h
- ${dbusunitysessionservice_SRCS}

   upstart/applicationcontroller.h
   upstart/applicationcontroller.cpp

=== modified file 'src/modules/Unity/Application/dbusunitysessionservice.cpp'
--- src/modules/Unity/Application/dbusunitysessionservice.cpp 2014-04-24 17:45:48 +0000
+++ src/modules/Unity/Application/dbusunitysessionservice.cpp 2014-04-24 18:17:23 +0000
@@ -16,7 +16,6 @@

 // local
 #include "dbusunitysessionservice.h"
-#include "dbusunitysessionserviceadaptor.h"

 // Qt
 #include <QDBusConnection>

review: Needs Fixing

[Daniel d'Andrada (dandrader)]

If I understood it correctly the signals logoutRequested and logoutReady are used internally by unity8, right? So no need to expose them in D-Bus.

=== modified file 'src/modules/Unity/Application/dbusunitysessionservice.cpp'
--- src/modules/Unity/Application/dbusunitysessionservice.cpp 2014-04-24 18:27:05 +0000
+++ src/modules/Unity/Application/dbusunitysessionservice.cpp 2014-04-24 18:27:22 +0000
@@ -29,8 +29,7 @@ DBusUnitySessionService::DBusUnitySessio

     connection.registerService("com.canonical.Unity.Session");
     connection.registerObject("/com/canonical/Unity/Session", this,
- QDBusConnection::ExportAllSignals
- | QDBusConnection::ExportScriptableSlots
+ QDBusConnection::ExportScriptableSlots
             | QDBusConnection::ExportScriptableInvokables);
 }

=== modified file 'src/modules/Unity/Application/dbusunitysessionservice.h'
--- src/modules/Unity/Application/dbusunitysessionservice.h 2014-04-24 17:45:48 +0000
+++ src/modules/Unity/Application/dbusunitysessionservice.h 2014-04-24 18:28:40 +0000
@@ -47,7 +47,7 @@ Q_SIGNALS:
      * @param have_inhibitors if there are any special running applications
      * which inhibit the logout.
      */
- Q_SCRIPTABLE void logoutRequested(bool have_inhibitors);
+ void logoutRequested(bool have_inhibitors);

     /**
      * logoutReady signal

review: Needs Fixing

[Daniel d'Andrada (dandrader)]

If I understood it correctly, Q_SCRIPTABLE is used to mark the signals you want exposed on D-Bus. In which case, it makes no sense here.

=== modified file 'src/modules/Unity/Application/application_manager.h'
--- src/modules/Unity/Application/application_manager.h 2014-04-24 13:38:59 +0000
+++ src/modules/Unity/Application/application_manager.h 2014-04-24 18:45:21 +0000
@@ -132,7 +132,7 @@ public Q_SLOTS:

 Q_SIGNALS:
     void focusRequested(const QString &appId);
- Q_SCRIPTABLE void unityLogout();
+ void unityLogout();

 private Q_SLOTS:
     void screenshotUpdated();

review: Needs Fixing

[lp:~paulliu/unity-mir/logout]

224. By Ying-Chun Liu on 2014-04-25

Fix signals/slots publicity.
Remove qdbus tools generating stuff.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:224
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/335/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-amd64-ci/198
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/199
        deb: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/199/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-i386-ci/198

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-mir-ci/335/rebuild

review: Approve (continuous-integration)

[Daniel d'Andrada (dandrader)]

97 +++ src/modules/Unity/Application/dbusunitysessionservice.cpp 2014-04-25 13:37:06 +0000
98 @@ -0,0 +1,52 @@
99 +/* -*- mode: c++; indent-tabs-mode: nil; tab-width: 4 -*- */
100 +/*

I don't think unity-mir or unity8 has this stuff in its coding style. I believe you copy-and-pasted that from some other project?

---------------

Looking at it now, there's a clear discrepancy between what the DBusUnitySessionService API says and what it actually does (or is used for).

"""
    /**
     * logoutReady signal
     *
     * This signal is emitted when all the apps are closed. And the system
     * is safe to logout.
     */
    void logoutReady();
"""

No, this signal is emitted when Logout() is called. That's all. The one that gets emitted when all the apps are closed is ApplicationManager::unityLogout.

Naming it logoutOrdered or logoutCommanded would be more appropriate. Specially considering the existing logoutRequested.

"""
    /**
     * Logout the system.
     *
     * This method directly logout the system without user's confirmation.
     * Ordinary applications should avoid calling this method. Please call
     * RequestLogout() to ask the user to decide logout or not.
     * This method will stop all the running applications and then signal
     * logoutReady when all the apps stopped.
     */
    Q_SCRIPTABLE void Logout();
"""

So the last sentence is wrong (about logoutReady)

Just remembered that as you want to change ApplicationManager, you should have the changes also in its interface: unity::shell::application::ApplicationManagerInterface from unity-api. And don't forget to update its mock in unity8.

I still don't like ApplicationManager::unityLogout. To me it looks more like a new ApplicationManager state.

ApplicationManagerInterface::suspended could be expanded from a boolean to a proper state enum comprising the following values:
 - "running" or "normal" (the equivalent of the current suspended==false behaviour)
 - suspended
 - stopped (all running apps will be stopped/closed and no new apps will be allowed to run).

Because, during logout, you could order AppMan to close all apps but afterwards, still during the logout process, a new app launch might be triggered (you never know) and AppMan should not allow it to run. That's why I think the logout thing, from AppMan's point of view, looks more like a state than a simple, stateless, command.

And I still think DBusUnitySessionService should be moved to unity8.

Would like to hear from mzanetti and Saviq.

review: Needs Fixing

[Daniel d'Andrada (dandrader)]

Results from the discussion with Saviq and mzanetti in #ubuntu-unity:

- This is a "quick & dirty" interim implementation to get things going as currently there's not grand plan or architecture for the logout story. This story need more thought.
- We don't want to pollute or complicate AppMan API with "quick & dirty" changes.

Thus:
Leave ApplicationManager as it is. You can achieve the same result with the existing API. Just move the loop to close all apps to unity8. And move DBusUnitySessionService along with it. Check unity8/plugins sub dir (LightDm and Utils also register dbus services).

review: Disapprove

Unmerged revisions

224. By Ying-Chun Liu on 2014-04-25

Fix signals/slots publicity.
Remove qdbus tools generating stuff.

223. By Ying-Chun Liu on 2014-04-24

Rename the class.
Add some doxygen.

222. By Ying-Chun Liu on 2014-04-23

fix the boolean variable.

221. By Ying-Chun Liu on 2014-04-23

minor changes

220. By Ying-Chun Liu on 2014-04-18

Move signal connect to application manager.
Fix signal shouldn't start with capital letter.

219. By Ying-Chun Liu on 2014-04-17

Merged upstream.
[ Ted Gould ]
Ensure the last item of the array is NULL (LP: #1304315)

218. By Ying-Chun Liu on 2014-04-17

Fix coding style

217. By Ying-Chun Liu on 2014-04-17

Fix bug

216. By Ying-Chun Liu on 2014-04-17

stop apps

215. By Ying-Chun Liu on 2014-04-17

fix bugs

=== modified file 'src/modules/Unity/Application/CMakeLists.txt'
--- src/modules/Unity/Application/CMakeLists.txt	2014-03-28 16:41:02 +0000
+++ src/modules/Unity/Application/CMakeLists.txt	2014-04-25 13:37:06 +0000
@@ -34,6 +34,8 @@
   proc_info.cpp
   shellinputarea.cpp
   ubuntukeyboardinfo.cpp
+  dbusunitysessionservice.cpp
+  dbusunitysessionservice.h
 
   upstart/applicationcontroller.h
   upstart/applicationcontroller.cpp

=== modified file 'src/modules/Unity/Application/application_manager.cpp'
--- src/modules/Unity/Application/application_manager.cpp	2014-04-14 12:56:28 +0000
+++ src/modules/Unity/Application/application_manager.cpp	2014-04-25 13:37:06 +0000
@@ -20,6 +20,7 @@
 #include "application.h"
 #include "desktopfilereader.h"
 #include "dbuswindowstack.h"
+#include "dbusunitysessionservice.h"
 #include "taskcontroller.h"
 #include "upstart/applicationcontroller.h"
 
@@ -183,6 +184,7 @@
     , m_ssApplicationToBeFocused(nullptr)
     , m_lifecycleExceptions(QStringList() << "com.ubuntu.music")
     , m_focusController(controller)
+    , m_dbusUnitySessionService(new DBusUnitySessionService(this))
     , m_dbusWindowStack(new DBusWindowStack(this))
     , m_taskController(taskController)
     , m_desktopFileReaderFactory(desktopFileReaderFactory)
@@ -207,6 +209,11 @@
     int densityPixelPx = qFloor( (float)m_gridUnitPx / 8 );
 
     m_panelHeight = 3 * m_gridUnitPx + 2 * densityPixelPx;
+
+    QObject::connect(m_dbusUnitySessionService,
+                     &unitymir::DBusUnitySessionService::logoutReady,
+                     this, &ApplicationManager::onLogoutReady);
+
 }
 
 ApplicationManager::~ApplicationManager()
@@ -612,6 +619,16 @@
     }
 }
 
+void ApplicationManager::onLogoutReady()
+{
+    // shut all of the apps down here.
+    for (Application *app : m_applications) {
+        stopApplication(app->appId());
+    }
+ 
+    Q_EMIT unityLogout();
+}
+
 void ApplicationManager::screenshotUpdated()
 {
     if (sender()) {

=== modified file 'src/modules/Unity/Application/application_manager.h'
--- src/modules/Unity/Application/application_manager.h	2014-03-12 13:47:18 +0000
+++ src/modules/Unity/Application/application_manager.h	2014-04-25 13:37:06 +0000
@@ -49,6 +49,7 @@
 
 namespace unitymir
 {
+class DBusUnitySessionService;
 class DBusWindowStack;
 class MirSurfaceManager;
 class TaskController;
@@ -127,9 +128,11 @@
     void onProcessStopped(const QString& appId, const bool unexpected);
     void onFocusRequested(const QString& appId);
     void onResumeRequested(const QString& appId);
+    void onLogoutReady();
 
 Q_SIGNALS:
     void focusRequested(const QString &appId);
+    void unityLogout();
 
 private Q_SLOTS:
     void screenshotUpdated();
@@ -154,6 +157,7 @@
     Application* m_ssApplicationToBeFocused; // placeholder store for async focusing
     QStringList m_lifecycleExceptions;
     std::shared_ptr<mir::shell::FocusController> m_focusController;
+    DBusUnitySessionService* m_dbusUnitySessionService;
     DBusWindowStack* m_dbusWindowStack;
     QSharedPointer<TaskController> m_taskController;
     QSharedPointer<DesktopFileReader::Factory> m_desktopFileReaderFactory;

=== added file 'src/modules/Unity/Application/dbusunitysessionservice.cpp'
--- src/modules/Unity/Application/dbusunitysessionservice.cpp	1970-01-01 00:00:00 +0000
+++ src/modules/Unity/Application/dbusunitysessionservice.cpp	2014-04-25 13:37:06 +0000
@@ -0,0 +1,52 @@
+/* -*- mode: c++; indent-tabs-mode: nil; tab-width: 4 -*- */
+/*
+ * Copyright (C) 2014 Canonical, Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it under
+ * the terms of the GNU Lesser General Public License version 3, as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranties of MERCHANTABILITY,
+ * SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+// local
+#include "dbusunitysessionservice.h"
+
+// Qt
+#include <QDBusConnection>
+
+namespace unitymir
+{
+
+DBusUnitySessionService::DBusUnitySessionService(QObject *parent) : QObject(parent)
+{
+    QDBusConnection connection = QDBusConnection::sessionBus();
+
+    connection.registerService("com.canonical.Unity.Session");
+    connection.registerObject("/com/canonical/Unity/Session", this,
+                              QDBusConnection::ExportScriptableSignals
+                              | QDBusConnection::ExportScriptableSlots
+                              | QDBusConnection::ExportScriptableInvokables);
+}
+
+DBusUnitySessionService::~DBusUnitySessionService()
+{
+}
+
+void DBusUnitySessionService::Logout()
+{
+  Q_EMIT logoutReady();
+}
+
+void DBusUnitySessionService::RequestLogout()
+{
+  Q_EMIT logoutRequested(false);
+}
+
+} // namespace unitymir

=== added file 'src/modules/Unity/Application/dbusunitysessionservice.h'
--- src/modules/Unity/Application/dbusunitysessionservice.h	1970-01-01 00:00:00 +0000
+++ src/modules/Unity/Application/dbusunitysessionservice.h	2014-04-25 13:37:06 +0000
@@ -0,0 +1,88 @@
+/* -*- mode: c++; indent-tabs-mode: nil; tab-width: 4 -*- */
+/*
+ * Copyright (C) 2014 Canonical, Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it under
+ * the terms of the GNU Lesser General Public License version 3, as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranties of MERCHANTABILITY,
+ * SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef DBUSUNITYSESSIONSERVICE_H_1397562297
+#define DBUSUNITYSESSIONSERVICE_H_1397562297
+
+#include <QObject>
+
+namespace unitymir
+{
+
+/**
+ * DBusUnitySessionService provides com.canonical.Unity.Session dbus
+ * interface.
+ *
+ * com.canonical.Unity.Session interface provides public methods
+ * and signals to handle Logout/Reboot/Shutdown.
+ */
+class DBusUnitySessionService : public QObject
+{
+    Q_OBJECT
+    Q_CLASSINFO("D-Bus Interface", "com.canonical.Unity.Session")
+
+public:
+    explicit DBusUnitySessionService(QObject* parent);
+    ~DBusUnitySessionService();
+
+Q_SIGNALS:
+    /**
+     * logoutRequested signal
+     *
+     * This signal is emitted when some applications request the system to
+     * logout.
+     * @param have_inhibitors if there are any special running applications
+     *        which inhibit the logout.
+     */
+    void logoutRequested(bool have_inhibitors);
+
+    /**
+     * logoutReady signal
+     *
+     * This signal is emitted when all the apps are closed. And the system
+     * is safe to logout.
+     */
+    void logoutReady();
+
+public Q_SLOTS:
+    /**
+     * Logout the system.
+     *
+     * This method directly logout the system without user's confirmation.
+     * Ordinary applications should avoid calling this method. Please call
+     * RequestLogout() to ask the user to decide logout or not.
+     * This method will stop all the running applications and then signal
+     * logoutReady when all the apps stopped.
+     */
+    Q_SCRIPTABLE void Logout();
+
+    /**
+     * Issue a logout request.
+     *
+     * This method emit the logoutRequested signal to the shell with a boolean
+     * which indicates if there's any inhibitors. The shell should receive
+     * this signal and display a dialog to ask the user to confirm the logout
+     * action. If confirmed, shell can call Logout() method to kill the apps
+     * and then logout
+     */
+    Q_SCRIPTABLE void RequestLogout();
+
+};
+
+} // namespace unitymir
+
+#endif // DBUSUNITYSESSIONSERVICE_H_1397562297