Code review comment for lp:~paulliu/unity-mir/logout

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.